On Tue, Oct 4, 2016 at 6:53 AM, Jeff King <peff@xxxxxxxx> 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