From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Sat, 14 Feb 2009 21:24:03 GMT
Subject: Re: [PATCH] config: Use parseopt.
Message-ID: <94a0d4530902141324w1cea462ex99a698e5a702d85a@mail.gmail.com>
In-Reply-To: <7vtz6wrahg.fsf@gitster.siamese.dyndns.org>

On Sat, Feb 14, 2009 at 11:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> Unfortunately, not many patch authors write such a summary.  Sometimes we
>>> see summaries on things that were discussed but nobody has followed
>>> through posted by third parties (including myself), but we do not seem to
>>> have enough helpers to do that either.  This does not take much technical
>>> skills but is a good "trust point" earner.
>>
>> For me it's easier, and more fun to write a separate patch that fixes
>> the issues than writing a summary,...
>
> That certainly is something we should take into consideration.
>
> I however think an unwritten assumption around here so far has been that
> the patch author who gets review comments is expected to keep track of the
> issues raised, both about the patch itself and about the similar breakages
> in the existing code pointed out during the review process, if only
> because the patch author is the focal point of the discussion.
>
> We probably need to break that.
>
> Because it is very likely that the reviewer does not even realize that
> such similar breakages in the existing code when a review is made, we
> cannot ask reviewers to always start a separate discussion.  Some reviews
> do say "Admittedly, we already have the same pattern in here and there,
> but this in your patch is wrong," but the way how we collectively realize
> an existing breakage is often by hearing the patch author respond with
> "but there already are this and that breakages in the existing code."
>
> We do not want such knowledge of existing breakages go to waste in either
> case.  Perhaps it would be a good start to make it the responsibility of
> the first person who mentions an existing breakage (either the reviewer's
> "Admittedly", or the patch author's "but there already are") to begin a
> separate thread, so that mail archive would remember it.  It shouldn't
> take more than 3 minutes.

That is true, however I propose something a bit different. At least in
this review there has been a number of issues brought up, it would be
overkill to create a separate thread for each one of these issues as
they where found, and if the patch submitter is new, he probably
wouldn't know about this rule.

So, I propose that at the end of the patch review process the ack
person (or somebody else) asks the patch submitter (possibly cc'ing
the reviewers) to start a new thread mentioning the pending issues
brought up in the review.

-- 
Felipe Contreras