Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 9, 2017, 16:41 UTC
- Message-ID
- <xmqqy3rx8svk.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170709110016.nusy62u2morqvwmm@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> 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). > > So I dunno. It's fine with me if we take it, and fine if we leave it.
Unlike origlen, base vs dir lengths are not strictly needed; we prepare the base including '/', and we know we always have just one '/' at the end, so anybody that uses dirlen to truncate it back to the original before passing it down can truncate to (baselen-1), no?
In other words, something like this (not an incremental but a replacement) to keep calling "baselen" the length of the leading constant part we append to?
sha1_file.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c index 5862386cd0..d277b32bf1 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -3760,7 +3760,6 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr, origlen = path->len; strbuf_complete(path, '/'); strbuf_addf(path, "%02x", subdir_nr); - baselen = path->len; dir = opendir(path->buf); if (!dir) { @@ -3770,12 +3769,15 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr, return r; } + strbuf_addch(path, '/'); + baselen = path->len; + while ((de = readdir(dir))) { if (is_dot_or_dotdot(de->d_name)) continue; strbuf_setlen(path, baselen); - strbuf_addf(path, "/%s", de->d_name); + strbuf_addstr(path, de->d_name); if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2) { char hex[GIT_MAX_HEXSZ+1]; @@ -3801,7 +3803,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr, } closedir(dir); - strbuf_setlen(path, baselen); + strbuf_setlen(path, baselen - 1); /* chomp the '/' that we added */ if (!r && subdir_cb) r = subdir_cb(subdir_nr, path->buf, data);