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

From: Clemens Buchacher <drizzd@aon.at>
Date: Jan 4, 2009, 20:01 UTC
Message-ID: <20090104200133.GA2202@localhost>
In-Reply-To: <7vtz8fz8yd.fsf@gitster.siamese.dyndns.org>

Hi,

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

Show 9 quoted lines

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

My estimate of this limit is apparently much lower than what is expected by the main contributors to this project. I respect that and I will try not to waste your time any further.

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.

Best regards, Clemens