Review then commit? (Was: [jira] Commented: (JCR-1331) Inproper deprecation of Locked class)

[Jukka Zitting <ju...@gmail.com>]

Hi,

[Branching the general issue from Jira]

On Jan 22, 2008 12:54 PM, angela (JIRA) <ji...@apache.org> wrote:
> but i expect that we have a minimal response time to give everybody
> the time to look at a patch. apart from that i expect that patches and
> suggestions are really looked at, before they are commited.

-1 You're proposing a switch from CTR to RTC.

There are many cases where having a patch reviewed is good, and I
think we are already doing a good job of evaluating case-by-case
whether such a review is needed. A good percentage of our issues go
through a patch/review/commit cycle even if everyone involved could
just commit the changes directly.

However, there are many cases where a fix is obvious and there is no
real need for a review. It should be OK to just commit such changes
(to trunk) without waiting for other opinions. We have commits
notifications for reviewing such changes.

The decision whether to wait for review or just go ahead and commit is
of course a judgment call and people make mistakes like I did with
JCR-1331, but such mistakes are easily fixed by reverting the changes.
I think we have at most one such revert per month on average, so I
don't think this is a problem.

BR,

Jukka Zitting

Re: Review then commit? (Was: [jira] Commented: (JCR-1331) Inproper deprecation of Locked class)

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Jan 22, 2008 3:07 PM, Angela Schreiber <an...@day.com> wrote:
> > -1 You're proposing a switch from CTR to RTC.
>
> i'm not proposing this. you are mistaken.

ACK, understood. Your original comment seemed like a general
observation about all changes.

> but i expect that we are careful regarding code
> that is more than an internal detail. and that
> we don't make changes forth and back....

+1 Agreed.

> and in this case waiting a couple of days would
> not have done any harm.

You're right. However, since the JCR-1331 change was relatively small
and I was quite convinced that it was the right thing to do I rather
made the change right away instead of waiting and introducing another
"context switch" a few days later.

That was a mistake but I don't think it's that big a deal, since
there's no harm in reverting a change if people object or want more
time for review.

I think you were perfectly right in calling for the changes to be
reverted, but I don't see the issue as a symptom of a more widespread
hastiness. IMHO our current workflow with voluntary reviews works
pretty well.

BR,

Jukka Zitting

Re: Review then commit? (Was: [jira] Commented: (JCR-1331) Inproper deprecation of Locked class)

[Angela Schreiber <an...@day.com>]

hi jukka

> -1 You're proposing a switch from CTR to RTC.

i'm not proposing this. you are mistaken.

but i expect that we are careful regarding code
that is more than an internal detail. and that
we don't make changes forth and back....and in this
case waiting a couple of
days would not have done any harm.

that's all.
angela