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