{"thread":{"id":"46329","subject":"[PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()","startedAt":"2017-07-08T08:59:50Z","lastAt":"2017-07-12T17:59:00Z","messageCount":7,"participants":["René Scharfe","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"323987","messageId":"f59c8256-716b-9305-2a4f-d4fe49f666ff@web.de","threadId":"46329","inReplyTo":null,"subject":"[PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-08T08:59:06Z","receivedAt":"2017-07-08T08:59:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add the slash between loose object subdirectory and file name just once\noutside the loop instead of overwriting it with each readdir call.\nRedefine baselen as the length with that slash, and add dirlen for the\nlength without it.  The result is slightly less wasteful and can use the\nthe cheaper strbuf_addstr instead of strbuf_addf without losing clarity.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n sha1_file.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5862386cd0..6a9deb9e68 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3749,7 +3749,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \t\t\t\teach_loose_subdir_fn subdir_cb,\n \t\t\t\tvoid *data)\n {\n-\tsize_t origlen, baselen;\n+\tsize_t origlen, dirlen, baselen;\n \tDIR *dir;\n \tstruct dirent *de;\n \tint r = 0;\n@@ -3760,7 +3760,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \toriglen = path->len;\n \tstrbuf_complete(path, '/');\n \tstrbuf_addf(path, \"%02x\", subdir_nr);\n-\tbaselen = path->len;\n+\tdirlen = path->len;\n \n \tdir = opendir(path->buf);\n \tif (!dir) {\n@@ -3770,12 +3770,15 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \t\treturn r;\n \t}\n \n+\tstrbuf_addch(path, '/');\n+\tbaselen = path->len;\n+\n \twhile ((de = readdir(dir))) {\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n \n \t\tstrbuf_setlen(path, baselen);\n-\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n+\t\tstrbuf_addstr(path, de->d_name);\n \n \t\tif (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2)  {\n \t\t\tchar hex[GIT_MAX_HEXSZ+1];\n@@ -3801,7 +3804,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \t}\n \tclosedir(dir);\n \n-\tstrbuf_setlen(path, baselen);\n+\tstrbuf_setlen(path, dirlen);\n \tif (!r && subdir_cb)\n \t\tr = subdir_cb(subdir_nr, path->buf, data);\n \n-- \n2.13.2\n"},{"id":"324021","messageId":"20170709110016.nusy62u2morqvwmm@sigill.intra.peff.net","threadId":"46329","inReplyTo":"f59c8256-716b-9305-2a4f-d4fe49f666ff@web.de","subject":"Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-09T11:00:17Z","receivedAt":"2017-07-09T11:00:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 08, 2017 at 10:59:06AM +0200, René Scharfe wrote:\n\n> Add the slash between loose object subdirectory and file name just once\n> outside the loop instead of overwriting it with each readdir call.\n> Redefine baselen as the length with that slash, and add dirlen for the\n> length without it.  The result is slightly less wasteful and can use the\n> the cheaper strbuf_addstr instead of strbuf_addf without losing clarity.\n\nThis patch looks correct to me.\n\nI'm a little lukewarm on it overall, though. I'd be shocked if the\nefficiency change is measurable. What I really care about is whether the\nresult is easier to read or not.\n\nOn the plus side, this moves an invariant out of the loop. On the minus\nside, it has to introduce an extra variable for \"length we add on to\"\nversus \"dir length to pass to the subdir_cb\". That's not rocket science,\nbut it does slightly complicate things (though I note we already have\n\"origlen\", so this is bumping us from 2 to 3 length variables, not 1 to\n2).\n\nSo I dunno. It's fine with me if we take it, and fine if we leave it.\n\n-Peff\n"},{"id":"324028","messageId":"f83adfa6-89f9-9c0e-bd69-eca56d3fcf25@web.de","threadId":"46329","inReplyTo":"20170709110016.nusy62u2morqvwmm@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-09T13:26:03Z","receivedAt":"2017-07-09T13:26:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.07.2017 um 13:00 schrieb Jeff King:\n> On Sat, Jul 08, 2017 at 10:59:06AM +0200, René Scharfe wrote:\n> \n>> Add the slash between loose object subdirectory and file name just once\n>> outside the loop instead of overwriting it with each readdir call.\n>> Redefine baselen as the length with that slash, and add dirlen for the\n>> length without it.  The result is slightly less wasteful and can use the\n>> the cheaper strbuf_addstr instead of strbuf_addf without losing clarity.\n> \n> This patch looks correct to me.\n> \n> I'm a little lukewarm on it overall, though. I'd be shocked if the\n> efficiency change is measurable. What I really care about is whether the\n> result is easier to read or not.\n> \n> On the plus side, this moves an invariant out of the loop. On the minus\n> side, it has to introduce an extra variable for \"length we add on to\"\n> versus \"dir length to pass to the subdir_cb\". That's not rocket science,\n> but it does slightly complicate things (though I note we already have\n> \"origlen\", so this is bumping us from 2 to 3 length variables, not 1 to\n> 2).\n\nAdmittedly this is more of an OCD thing.  Overwriting a character 200\ntimes or parsing a format string don't very long compared to the rest\nof the function, but it's a bit itchy.\n\nRené\n"},{"id":"324033","messageId":"xmqqy3rx8svk.fsf@gitster.mtv.corp.google.com","threadId":"46329","inReplyTo":"20170709110016.nusy62u2morqvwmm@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-09T16:41:51Z","receivedAt":"2017-07-09T16:41:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Jul 08, 2017 at 10:59:06AM +0200, René Scharfe wrote:\n>\n>> Add the slash between loose object subdirectory and file name just once\n>> outside the loop instead of overwriting it with each readdir call.\n>> Redefine baselen as the length with that slash, and add dirlen for the\n>> length without it.  The result is slightly less wasteful and can use the\n>> the cheaper strbuf_addstr instead of strbuf_addf without losing clarity.\n>\n> This patch looks correct to me.\n>\n> I'm a little lukewarm on it overall, though. I'd be shocked if the\n> efficiency change is measurable. What I really care about is whether the\n> result is easier to read or not.\n>\n> On the plus side, this moves an invariant out of the loop. On the minus\n> side, it has to introduce an extra variable for \"length we add on to\"\n> versus \"dir length to pass to the subdir_cb\". That's not rocket science,\n> but it does slightly complicate things (though I note we already have\n> \"origlen\", so this is bumping us from 2 to 3 length variables, not 1 to\n> 2).\n>\n> So I dunno. It's fine with me if we take it, and fine if we leave it.\n\nUnlike origlen, base vs dir lengths are not strictly needed; we\nprepare the base including '/', and we know we always have just one\n'/' at the end, so anybody that uses dirlen to truncate it back to\nthe original before passing it down can truncate to (baselen-1), no?\n\nIn other words, something like this (not an incremental but a\nreplacement) to keep calling \"baselen\" the length of the leading\nconstant part we append to?\n\n sha1_file.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5862386cd0..d277b32bf1 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3760,7 +3760,6 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \toriglen = path->len;\n \tstrbuf_complete(path, '/');\n \tstrbuf_addf(path, \"%02x\", subdir_nr);\n-\tbaselen = path->len;\n \n \tdir = opendir(path->buf);\n \tif (!dir) {\n@@ -3770,12 +3769,15 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \t\treturn r;\n \t}\n \n+\tstrbuf_addch(path, '/');\n+\tbaselen = path->len;\n+\n \twhile ((de = readdir(dir))) {\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n \n \t\tstrbuf_setlen(path, baselen);\n-\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n+\t\tstrbuf_addstr(path, de->d_name);\n \n \t\tif (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2)  {\n \t\t\tchar hex[GIT_MAX_HEXSZ+1];\n@@ -3801,7 +3803,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \t}\n \tclosedir(dir);\n \n-\tstrbuf_setlen(path, baselen);\n+\tstrbuf_setlen(path, baselen - 1); /* chomp the '/' that we added */\n \tif (!r && subdir_cb)\n \t\tr = subdir_cb(subdir_nr, path->buf, data);\n \n\n"},{"id":"324046","messageId":"20170710000638.p7foedomfmdakyww@sigill.intra.peff.net","threadId":"46329","inReplyTo":"xmqqy3rx8svk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T00:06:38Z","receivedAt":"2017-07-10T00:06:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 09, 2017 at 09:41:51AM -0700, Junio C Hamano wrote:\n\n> > On the plus side, this moves an invariant out of the loop. On the minus\n> > side, it has to introduce an extra variable for \"length we add on to\"\n> > versus \"dir length to pass to the subdir_cb\". That's not rocket science,\n> > but it does slightly complicate things (though I note we already have\n> > \"origlen\", so this is bumping us from 2 to 3 length variables, not 1 to\n> > 2).\n> >\n> > So I dunno. It's fine with me if we take it, and fine if we leave it.\n> \n> Unlike origlen, base vs dir lengths are not strictly needed; we\n> prepare the base including '/', and we know we always have just one\n> '/' at the end, so anybody that uses dirlen to truncate it back to\n> the original before passing it down can truncate to (baselen-1), no?\n> \n> In other words, something like this (not an incremental but a\n> replacement) to keep calling \"baselen\" the length of the leading\n> constant part we append to?\n\nYeah, I think that is correct. And you could even drop origlen by\nreplacing it with \"baselen - 3\" at the end. But somehow doing the\ncomputation on the fly actually seems more complicated to me (from the\nperspective of a reader who is trying to make sure all is correct).\n\n-Peff\n"},{"id":"324047","messageId":"xmqq8tjx82kf.fsf@gitster.mtv.corp.google.com","threadId":"46329","inReplyTo":"20170710000638.p7foedomfmdakyww@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-10T02:10:08Z","receivedAt":"2017-07-10T02:10:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... And you could even drop origlen by\n> replacing it with \"baselen - 3\" at the end. But somehow doing the\n> computation on the fly actually seems more complicated to me (from the\n> perspective of a reader who is trying to make sure all is correct).\n\nTrue enough.  I personally do not mind any of the three variants\n(including the original) and would rather not spend too much time\nmicro-optimizing this codepath, lest the next clever person starts\nto turn the loop in for_each_loose_file_in_objdir() to a nested loop\nthat runs 16 times each to avoid copying the same leading byte again\nand again ;-)\n\n"},{"id":"324276","messageId":"0465f03f-3d60-15f5-6d67-80b46b6c8c5f@web.de","threadId":"46329","inReplyTo":"xmqq8tjx82kf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-12T17:58:37Z","receivedAt":"2017-07-12T17:59:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.07.2017 um 04:10 schrieb Junio C Hamano:\n> Jeff King <peff@peff.net> writes:\n> \n>> ... And you could even drop origlen by\n>> replacing it with \"baselen - 3\" at the end. But somehow doing the\n>> computation on the fly actually seems more complicated to me (from the\n>> perspective of a reader who is trying to make sure all is correct).\n> \n> True enough.  I personally do not mind any of the three variants\n> (including the original) and would rather not spend too much time\n> micro-optimizing this codepath, lest the next clever person starts\n> to turn the loop in for_each_loose_file_in_objdir() to a nested loop\n> that runs 16 times each to avoid copying the same leading byte again\n> and again ;-)\n\n*evil grin*\n\nAnyway, I agree that this patch is not worth spending much time on.  I\nstill think the result is cleaner (perhaps due to my strbuf_addf\nallergy), though it's longer and perhaps adding a twelfth variable makes\nthe function too complicated.  The magic chomping variant is a clever\ncompromise -- but needs a comment to explain itself.  So if you are\nindifferent please just drop it.\n\nThanks,\nRené\n"}]}