Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()

From: Jeff King <peff@peff.net>
Date: Jul 9, 2017, 11:00 UTC
Message-ID: <20170709110016.nusy62u2morqvwmm@sigill.intra.peff.net>
In-Reply-To: <f59c8256-716b-9305-2a4f-d4fe49f666ff@web.de>

On Sat, Jul 08, 2017 at 10:59:06AM +0200, René Scharfe wrote:

Show 5 quoted lines

> Add the slash between loose object subdirectory and file name just once
> outside the loop instead of overwriting it with each readdir call.
> Redefine baselen as the length with that slash, and add dirlen for the
> length without it.  The result is slightly less wasteful and can use the
> the cheaper strbuf_addstr instead of strbuf_addf without losing clarity.

This patch looks correct to me.

I'm a little lukewarm on it overall, though. I'd be shocked if the efficiency change is measurable. What I really care about is whether the result is easier to read or not.

On the plus side, this moves an invariant out of the loop. On the minus side, it has to introduce an extra variable for "length we add on to" versus "dir length to pass to the subdir_cb". That's not rocket science, but it does slightly complicate things (though I note we already have "origlen", so this is bumping us from 2 to 3 length variables, not 1 to 2).

So I dunno. It's fine with me if we take it, and fine if we leave it.

-Peff