From: Jonathon Mah <me@jonathonmah.com>
Date: Mon, 02 Feb 2015 18:37:57 GMT
Subject: Re: [PATCH 2/2] sha1_file: fix iterating loose alternate objects
Message-ID: <9215E7CF-4A6C-48ED-8A63-76085D5B151A@jonathonmah.com>
In-Reply-To: <20150202175259.GA24025@peff.net>

> On 2015-02-02, at 09:53, Jeff King <peff@peff.net> wrote:
> 
> I think this is probably the best fix, and is the pattern we use
> elsewhere when touching alt->base.
> 
> We _could_ further change this to have for_each_loose_file_in_objdir
> actually use alt->base as its scratch buffer, writing the object
> filenames into the end of it (i.e., what it was designed for). But:
> 
>  1. We still need a strbuf scratch-buffer for the non-alternate object
>     directory. So we'd have to push more code there to over-allocate
>     the buffer, and then for_each_loose_file_in_objdir would assume
>     we always feed it a buffer with the extra slop. That would work,
>     but I find the strbuf approach a little safer; there's not an
>     implicit over-allocation far away in the code preventing us from
>     overflowing a buffer.
> 
>  2. The reason for the existing alt->base behavior is that the
>     sha1_file code gets fed objects one at a time, and don't want to
>     pay strbuf overhead for each. With the iterator, we know we are
>     going to hit a bunch of objects, so we only have to pay the strbuf
>     overhead once for the iteration. So there's not the same
>     performance penalty, and we can stick with the strbuf if we prefer
>     it.

Thanks for your feedback. I considered the same, and came to a similar conclusion. The strbuf cost is only once per alternate, so I feel on balance it's more robust to use alt->base consistently inside each function, rather than have this a more fragile special case to save allocation of only one path.

Updated the test patch.


Jonathon Mah
me@JonathonMah.com