From: René Scharfe <l.s.r@web.de>
Date: Sun, 09 Jul 2017 13:26:03 GMT
Subject: Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()
Message-ID: <f83adfa6-89f9-9c0e-bd69-eca56d3fcf25@web.de>
In-Reply-To: <20170709110016.nusy62u2morqvwmm@sigill.intra.peff.net>

Am 09.07.2017 um 13:00 schrieb Jeff King:
> On Sat, Jul 08, 2017 at 10:59:06AM +0200, René Scharfe wrote:
> 
>> 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).

Admittedly this is more of an OCD thing.  Overwriting a character 200
times or parsing a format string don't very long compared to the rest
of the function, but it's a bit itchy.

René