Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent

Hi,

On Sun, 4 Jan 2009, Clemens Buchacher wrote:

On Sun, Jan 04, 2009 at 02:01:14AM -0800, Junio C Hamano wrote:

quoted

    The function's purpose is ....  Before entering the loop to count 
    the number of entries to skip, this check to detect if we do not 
    even have to count appears.  When this check triggers, we know we 
    do not want to skip anything, and returning constant 0 is much 
    clearer than returning a variable cnt that was initialized to 0 
    near the beginning of the function; we haven't even started using 
    it to count yet.

But the point is, if that is the reason the author thinks it is an 
improvement, that probably needs to be stated.

If you want to check the validity of the patch you have to view it in
context anyways.

Umm.

You can make reviewing your patch attractive and easy, and you can make it 
unattractive and difficult.

If you explain in the commit message that you replaced "cnt" by "0" 
because it is initialized to 0 at that point anyway, it is a _much bigger_ 
pleasure to review your patch.

Let alone a much bigger pleasure for you, 6 months from now, when somebody 
says "why does this silly function return 0, when it should return cnt?"

BTW exactly for that reason, I'd like to leave it as "cnt".  Because code 
_will_ change, and it's quite possible that cnt will not be 0 at that 
point in the future.

Compared to understanding the change to the code, it takes much longer 
to parse and understand the above paragraph _plus_ verify its agreement 
with the code. I think you will agree that there is a limit to the 
amount of documentation that's still useful.

Just look at a concrete case: me.  I saw that part of the patch, even 
before coming to the real meat of it.  And that head-scratching already 
removed all the enthusiasm I had to look at unpack-trees.c again, so you 
lost a reviewer.

What's sad, however, is that we are now discussing style and commenting 
issues of a line of code, which, as by my analysis of [PATCH 3/3] never 
actually gets executed in the first place. I would have been much more 
curious about your comments on that.

Exactly,
Dscho