Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 8, 2011, 18:41 UTC
Message-ID: <7vpqfyj47m.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20111207081734.GG11737@elie.hsd1.il.comcast.net>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 17 quoted lines

> Ramkumar Ramachandra wrote:
>
>> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
>> ($gname/186365) implemented.
>
> Thanks for this.  I am worried that some of my comments in previous
> reviews (especially about change descriptions) were not addressed,
> which could mean one of a few things:
>
>  - you didn't notice them
>  - they were wrong
>  - you noticed them and they were describing real problems, but it
>    wasn't worth the time to fix them
>
> Fine.  But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
> time is well spent.

Another thing I often notice after raising an issue during a review (this is not limited to Ram) is that I do so in the form of a question "why is this function done this way?" expecting either "ah, doing it that way instead is simpler and easier to read" or "because of such and such, which you may have missed. after all this is a tricky codepath with this and that constraints" as a response. The former should and often does result in an update of the code in the re-rolled series, but the latter often results in a frustrating nothing in a re-roll, when the fact that a reviewer needed to ask the question was a sure sign that the code needs explanation either in the log message or in-code comment.

> The series makes various changes, all essentially good, which leaves
> me all the more motivated to figure out how to get this sort of thing
> in smoothly without making life difficult for people in the future
> tracking down regressions.

I looked the series over with your comments and tend to agree with many of them. Perhaps I should wait for another round before picking it up again (I've already dropped the old series I kept before 1.7.8).