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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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).