Re: [PATCH 08/18] link_alt_odb_entry: refactor string handling

From: Jeff King <peff@peff.net>
Date: Oct 4, 2016, 13:53 UTC
Message-ID: <20161004135353.6ywgoxutjcbaali5@sigill.intra.peff.net>
In-Reply-To: <CA+P7+xrUOnDebwZnfu-xv-GuTJka4-eNUAfBudQf5ZhnkczU6w@mail.gmail.com>

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