Commit then review revisited.

["Gav...." <br...@brightontown.com.au>]

In the FOAF thread Ross says :-

"... (and to be fair Gav made a mistake in committing it without reviewing
it properly - that's our job as committers)...."

First , I apologise for not having the time yesterday to review it properly
before committing. (Wrists still sore from the slapping :) ) 
However, a conversation while back stuck in my mind [1] and so I adopted the
approach of 'commit then review' - as long as it is sure not to break
something else.

The patch I applied yesterday was actually the first patch file that I had
successfully managed to unpatch back into code (?)  -- I finally got
patch.exe installed into Cygwin and used Davids command line code [2] of '
patch -p0 < ~/pathToFile/filename.patch '. In fact I copied the patch file
to 'trunk' first and just did 'patch -p0 filename.patch' and was pleased to
see that it all expanded into the correct place in whiteboard -- so I now
see properly the reasons for correctly creating patches relative from trunk
root.

Anyway, just wanted to nip this in the bud now, should I from now on revert
my thinking and review the patches before committing, even though this will
mean the patch just waits there a little longer ?

Thanks

Gav...



[1] - http://marc.info/?l=forrest-dev&m=114579093419890&w=2
[2] - http://marc.info/?l=forrest-dev&m=115931572618302&w=2

Re: Commit then review revisited.

[Ross Gardler <rg...@apache.org>]

On 07/06/07, David Crossley <cr...@apache.org> wrote:
> Gav.... wrote:

...

> > Anyway, just wanted to nip this in the bud now, should I from now on revert
> > my thinking and review the patches before committing, even though this will
> > mean the patch just waits there a little longer ?

...

> No, stick with "commit-then-review".

I agree.

[...]

> Doing "commit-then-review" does not mean "don't look at it
> beforehand".

Yes, in this case a quick review of the patch would have revealed that
the patch itself was faulty and so we could have asked for a new
patch. This would have been much faster than the time it took me to
figure out what had gone wrong and to edit each file individually by
hand.

How much reviewing would you do of your own work before committing? I
guess you would at least check that it worked and then commit. In this
case just trying to run the code locally would have indicated a
problem.

> It is up to the patch applier to consider how much pre-commit
> review that they feel like doing.

Agreed, but at the very least, read it over to make sure it is
correctly put together. Especially when it is an early patch since
creating patches correctly is a fine art, I don't know anyone who has
not made a few errors at first.

Finally, as ever, no-one should feel that this is a problem in an open
source community. We all make mistakes and all pick up the pieces for
one another. I only highlight the issue in order to fine tune the
process.

So, thanks for taking the time to figure out how to apply the patch in
the first place.

Ross

Re: Commit then review revisited.

[David Crossley <cr...@apache.org>]

Gav.... wrote:
> In the FOAF thread Ross says :-
> 
> "... (and to be fair Gav made a mistake in committing it without reviewing
> it properly - that's our job as committers)...."
> 
> First , I apologise for not having the time yesterday to review it properly
> before committing. (Wrists still sore from the slapping :) ) 
> However, a conversation while back stuck in my mind [1] and so I adopted the
> approach of 'commit then review' - as long as it is sure not to break
> something else.
> 
> The patch I applied yesterday was actually the first patch file that I had
> successfully managed to unpatch back into code (?)  -- I finally got
> patch.exe installed into Cygwin and used Davids command line code [2] of '
> patch -p0 < ~/pathToFile/filename.patch '. In fact I copied the patch file
> to 'trunk' first and just did 'patch -p0 filename.patch' and was pleased to
> see that it all expanded into the correct place in whiteboard -- so I now
> see properly the reasons for correctly creating patches relative from trunk
> root.
> 
> Anyway, just wanted to nip this in the bud now, should I from now on revert
> my thinking and review the patches before committing, even though this will
> mean the patch just waits there a little longer ?
> 
> Thanks
> 
> Gav...
> 
> [1] - http://marc.info/?l=forrest-dev&m=114579093419890&w=2
> [2] - http://marc.info/?l=forrest-dev&m=115931572618302&w=2

No, stick with "commit-then-review". Your reference [1] has
lost the thread context, so i searched the archives for the
previous stuff [3].

[3] http://marc.info/?l=forrest-dev&m=114576320102618

There are good notes about this topic in our Guidelines [4].

[4] http://forrest.apache.org/guidelines.html#code

Doing "commit-then-review" does not mean "don't look at it
beforehand". It is fine to do some minor review first.
It is also fine to just commit it, then follow up with
questions or commits to address the problems, either by you,
by another committer, or via patches from any other developer.

It is up to the patch applier to consider how much pre-commit
review that they feel like doing.

Whatever, our policy is still "commit-then-review". As said
in [4] "Note that it does not preclude the committer from
making changes to patches prior to their commit, nor mean
that the committer can be careless. Rather it is a policy
for decision-making".

-David