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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 6, 2009, 19:35 UTC
Message-ID: <alpine.DEB.1.00.0901062031101.30769@pacific.mpi-cbg.de>
In-Reply-To: <20090104200133.GA2202@localhost>

Hi,

On Sun, 4 Jan 2009, Clemens Buchacher wrote:

Show 14 quoted lines

> On Sun, Jan 04, 2009 at 02:01:14AM -0800, Junio C Hamano wrote:
> >     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