From: Jacob Keller <jacob.keller@gmail.com>
Date: Tue, 04 Oct 2016 20:46:08 GMT
Subject: Re: [PATCH 08/18] link_alt_odb_entry: refactor string handling
Message-ID: <CA+P7+xqSzm7S4-Mc+keJ1JWioYUmF76es0A3xN+Hwq+EJ6dJSA@mail.gmail.com>
In-Reply-To: <20161004135353.6ywgoxutjcbaali5@sigill.intra.peff.net>

On Tue, Oct 4, 2016 at 6:53 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Oct 03, 2016 at 11:05:42PM -0700, Jacob Keller wrote:
>
>> This definitely makes reading the following function much easier,
>> though the diff is a bit funky. I think the end result is much
>> clearer.
>
> Yeah, it's really hard to see that all of the "ent" setup is kept,
> because it moves _and_ changes its content (from pfxlen to pathbuf.len).
>
> I actually tried to split this into two patches to make the diff easier
> to read, but there are two mutually dependent changes: moving to
> pathbuf.len everywhere requires not-freeing pathbuf in the early code
> path. But if you do that and don't move all of "is it usable" checks up,
> then you have to add a bunch of new error-handling code that would just
> get ripped out in the next patch.
>
> There's definitely _some_ of that in this series already (e.g., the
> counting logic in alt_sha1_path() added by patch 14 that just gets
> ripped out in patch 15 when fill_sha1_path() learns to use a strbuf). I
> tried to balance "show each individual obvious step" with "don't make
> people review a bunch of scaffolding that's not going to be in the final
> product".
>
> -Peff

Mostly the diff is funky because of how the diff selected which chunks
moved vs how your patch described what chunks moved.

Thanks,
Jake