Mailing-List: contact dev-help@forrest.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@forrest.apache.org
Received-SPF: neutral (herse.apache.org: local policy)
Date: Thu, 7 Jun 2007 10:05:07 +1000
From: David Crossley <crossley@apache.org>
To: dev@forrest.apache.org
Subject: Re: Commit then review revisited.
Message-ID: <20070607000507.GE540@igg.indexgeo.com.au>
References: <008001c7a890$c2773de0$650fa8c0@developer>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <008001c7a890$c2773de0$650fa8c0@developer>
User-Agent: Mutt/1.4.2.2i

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