{"thread":{"id":"47362","subject":"[PATCH] sha1_file: use strbuf_add() instead of strbuf_addf()","startedAt":"2017-12-01T17:50:22Z","lastAt":"2017-12-04T17:12:36Z","messageCount":7,"participants":["Derrick Stolee","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"333926","messageId":"20171201174956.143245-1-dstolee@microsoft.com","threadId":"47362","inReplyTo":null,"subject":"[PATCH] sha1_file: use strbuf_add() instead of strbuf_addf()","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2017-12-01T17:49:56Z","receivedAt":"2017-12-01T17:50:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Replace use of strbuf_addf() with strbuf_add() when enumerating\nloose objects in for_each_file_in_obj_subdir(). Since we already\ncheck the length and hex-values of the string before consuming\nthe path, we can prevent extra computation by using the lower-\nlevel method.\n\nOne consumer of for_each_file_in_obj_subdir() is the abbreviation\ncode. OID abbreviations use a cached list of loose objects (per\nobject subdirectory) to make repeated queries fast, but there is\nsignificant cache load time when there are many loose objects.\n\nMost repositories do not have many loose objects before repacking,\nbut in the GVFS case the repos can grow to have millions of loose\nobjects. Profiling 'git log' performance in GitForWindows on a\nGVFS-enabled repo with ~2.5 million loose objects revealed 12% of\nthe CPU time was spent in strbuf_addf().\n\nAdd a new performance test to p4211-line-log.sh that is more\nsensitive to this cache-loading. By limiting to 1000 commits, we\nmore closely resemble user wait time when reading history into a\npager.\n\nFor a copy of the Linux repo with two ~512 MB packfiles and ~572K\nloose objects, running 'git log --oneline --raw --parents -1000'\nhad the following performance:\n\n HEAD~1            HEAD\n----------------------------------------\n 7.70(7.15+0.54)   7.44(7.09+0.29) -3.4%\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n sha1_file.c              | 7 ++++---\n t/perf/p4211-line-log.sh | 4 ++++\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8ae6cb6285..2160323c4a 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1914,17 +1914,18 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \t}\n \n \toid.hash[0] = subdir_nr;\n+\tstrbuf_add(path, \"/\", 1);\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-\n \t\tif (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&\n \t\t    !hex_to_bytes(oid.hash + 1, de->d_name,\n \t\t\t\t  GIT_SHA1_RAWSZ - 1)) {\n+\t\t\tstrbuf_setlen(path, baselen);\n+\t\t\tstrbuf_add(path, de->d_name, GIT_SHA1_HEXSZ - 2);\n \t\t\tif (obj_cb) {\n \t\t\t\tr = obj_cb(&oid, path->buf, data);\n \t\t\t\tif (r)\ndiff --git a/t/perf/p4211-line-log.sh b/t/perf/p4211-line-log.sh\nindex e0ed05907c..392bcc0e51 100755\n--- a/t/perf/p4211-line-log.sh\n+++ b/t/perf/p4211-line-log.sh\n@@ -35,4 +35,8 @@ test_perf 'git log --oneline --raw --parents' '\n \tgit log --oneline --raw --parents >/dev/null\n '\n \n+test_perf 'git log --oneline --raw --parents -1000' '\n+\tgit log --oneline --raw --parents -1000 >/dev/null\n+'\n+\n test_done\n-- \n2.15.0\n\n"},{"id":"333930","messageId":"20171201182238.GA27688@sigill.intra.peff.net","threadId":"47362","inReplyTo":"20171201174956.143245-1-dstolee@microsoft.com","subject":"Re: [PATCH] sha1_file: use strbuf_add() instead of strbuf_addf()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-01T18:22:38Z","receivedAt":"2017-12-01T18:23:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 01, 2017 at 12:49:56PM -0500, Derrick Stolee wrote:\n\n> Replace use of strbuf_addf() with strbuf_add() when enumerating\n> loose objects in for_each_file_in_obj_subdir(). Since we already\n> check the length and hex-values of the string before consuming\n> the path, we can prevent extra computation by using the lower-\n> level method.\n\nMakes sense. It's a shame that \"addf\" is turning out to be so slow (I'm\nstill mildly curious if that differs between Windows and glibc, but\nthere are so many other variables between the two platforms it's hard to\ntest).\n\n> One consumer of for_each_file_in_obj_subdir() is the abbreviation\n> code. OID abbreviations use a cached list of loose objects (per\n> object subdirectory) to make repeated queries fast, but there is\n> significant cache load time when there are many loose objects.\n> \n> Most repositories do not have many loose objects before repacking,\n> but in the GVFS case the repos can grow to have millions of loose\n> objects. Profiling 'git log' performance in GitForWindows on a\n> GVFS-enabled repo with ~2.5 million loose objects revealed 12% of\n> the CPU time was spent in strbuf_addf().\n\nYeah, we haven't heavily micro-optimized the case for having lots of\nloose objects. The general philosophy about having lots of loose objects\nis: don't. You're generally going to pay a system call for each access,\nwhich is much heavier-weight than packfiles.\n\nI think abbreviation-finding is the exception there, though, because we\nliterally just readdir() the entries and never do anything else with\nthem.\n\n> Add a new performance test to p4211-line-log.sh that is more\n> sensitive to this cache-loading. By limiting to 1000 commits, we\n> more closely resemble user wait time when reading history into a\n> pager.\n> \n> For a copy of the Linux repo with two ~512 MB packfiles and ~572K\n> loose objects, running 'git log --oneline --raw --parents -1000'\n> had the following performance:\n> \n>  HEAD~1            HEAD\n> ----------------------------------------\n>  7.70(7.15+0.54)   7.44(7.09+0.29) -3.4%\n\nThanks for including numbers. I think the setup here highlights a\nweakness of the perf suite that we've discussed: if there's a\nperformance regression for your case, nobody is likely to notice because\nthey won't test on a repo with 500k loose objects.\n\nProbably \"repo with a bunch of loose objects\" ought to be a stock\nrepository profile for doing regression runs.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 8ae6cb6285..2160323c4a 100644\n\nThis overall looks good, but I noticed one bug and a few cosmetic\nimprovements.\n\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1914,17 +1914,18 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n>  \t}\n>  \n>  \toid.hash[0] = subdir_nr;\n> +\tstrbuf_add(path, \"/\", 1);\n\nMaybe:\n\n  strbuf_addch(path, '/');\n\nwould be a bit more readable (it's also a tiny bit faster, but this\nisn't part of the tight loop).\n\n> +\tbaselen = path->len;\n\nWe set this here so that the '/' is included as part of the base. Makes\nsense, but can we now drop the earlier setting of baselen before the\nopendir() call?\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> -\n>  \t\tif (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&\n>  \t\t    !hex_to_bytes(oid.hash + 1, de->d_name,\n>  \t\t\t\t  GIT_SHA1_RAWSZ - 1)) {\n> +\t\t\tstrbuf_setlen(path, baselen);\n> +\t\t\tstrbuf_add(path, de->d_name, GIT_SHA1_HEXSZ - 2);\n\nMoving this code into the conditional makes sense here, since that's\nwhere we know the actual size. But what about the case when we _don't_\nhit this conditional. We still do:\n\n  if (cruft_cb)\n        cruft_cb(path->buf);\n\nwhich is now broken (it will just get the directory name without the\nentry appended).\n\nI think the optimized versions probably needs to be something more like:\n\n  while (de = readdir(...)) {\n      strbuf_setlen(path, baselen);\n      if (size is HEXSZ - 2) {\n          strbuf_add(path, de->d_name, HEXSZ - 2);\n\t  obj_cb(path->buf);\n      } else if (cruft_cb) {\n          strbuf_addstr(path, de->d_name);\n\t  cruft_cb(path->buf);\n      }\n  }\n\nTwo other thoughts:\n\n  - from past discussions, I suspect most of your performance\n    improvement actually comes from avoiding addf(), and the difference\n    between addstr() and add() may be negligible here. It might be worth\n    timing strbuf_addstr(). If that's similarly fast, that means we\n    could keep the logic out of the conditional entirely.\n\n  - there's an extra micro-optimization there, which is that if there's\n    no obj_cb, we have no need to assemble the full path at all. I doubt\n    it makes much of a difference, as most callers would pass an object\n    callback (I'd be surprised if we even have one that doesn't).\n\n-Peff\n"},{"id":"333936","messageId":"54da89a5-bf20-cb20-9884-6285033f0d95@gmail.com","threadId":"47362","inReplyTo":"20171201182238.GA27688@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_file: use strbuf_add() instead of strbuf_addf()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2017-12-01T19:50:05Z","receivedAt":"2017-12-01T19:50:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/1/2017 1:22 PM, Jeff King wrote:\n> On Fri, Dec 01, 2017 at 12:49:56PM -0500, Derrick Stolee wrote:\n[snip]\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 8ae6cb6285..2160323c4a 100644\n> This overall looks good, but I noticed one bug and a few cosmetic\n> improvements.\n\nThanks for finding quality improvements to this patch. I'll let it sit \nover the weekend for more feedback before sending a v2.\n\n>\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -1914,17 +1914,18 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n>>   \t}\n>>   \n>>   \toid.hash[0] = subdir_nr;\n>> +\tstrbuf_add(path, \"/\", 1);\n> Maybe:\n>\n>    strbuf_addch(path, '/');\n>\n> would be a bit more readable (it's also a tiny bit faster, but this\n> isn't part of the tight loop).\n>\n>> +\tbaselen = path->len;\n> We set this here so that the '/' is included as part of the base. Makes\n> sense, but can we now drop the earlier setting of baselen before the\n> opendir() call?\n\nYeah, probably. I had briefly considered just adding the '/' before the \nfirst assignment of \"baselen\", but didn't want to change the error \noutput. I also don't know if there are side effects for other platforms \nby calling opendir() with a '/'-terminated path.\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>> -\n>>   \t\tif (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&\n>>   \t\t    !hex_to_bytes(oid.hash + 1, de->d_name,\n>>   \t\t\t\t  GIT_SHA1_RAWSZ - 1)) {\n>> +\t\t\tstrbuf_setlen(path, baselen);\n>> +\t\t\tstrbuf_add(path, de->d_name, GIT_SHA1_HEXSZ - 2);\n> Moving this code into the conditional makes sense here, since that's\n> where we know the actual size. But what about the case when we _don't_\n> hit this conditional. We still do:\n>\n>    if (cruft_cb)\n>          cruft_cb(path->buf);\n>\n> which is now broken (it will just get the directory name without the\n> entry appended).\n\nGood catch! A big reason to pull it inside and use strbuf_add over \nstrbuf_addstr is to avoid a duplicate strlen() calculation. However, I \ncan store the length before the conditional.\n\n> I think the optimized versions probably needs to be something more like:\n>\n>    while (de = readdir(...)) {\n>        strbuf_setlen(path, baselen);\n>        if (size is HEXSZ - 2) {\n>            strbuf_add(path, de->d_name, HEXSZ - 2);\n> \t  obj_cb(path->buf);\n>        } else if (cruft_cb) {\n>            strbuf_addstr(path, de->d_name);\n> \t  cruft_cb(path->buf);\n>        }\n>    }\n\nSmall change by storing the length in advance of the conditional:\n\nwhile (de = readdir(...)) {\n     int namelen = strlen(de->d_name);\n     strbuf_setlen(path, baselen);\n     strbuf_add(path, de->d_name, namelen);\n\n     if (namelen == HEXSZ - 2)\n         obj_cb(path->buf)\n     else\n         cruft_cb(path->buf);\n}\n\n> Two other thoughts:\n>\n>    - from past discussions, I suspect most of your performance\n>      improvement actually comes from avoiding addf(), and the difference\n>      between addstr() and add() may be negligible here. It might be worth\n>      timing strbuf_addstr(). If that's similarly fast, that means we\n>      could keep the logic out of the conditional entirely.\n\naddstr() duplicates the strlen(), which isn't much but we might as well \nsave cycles where we can if it isn't too hard.\n\n>    - there's an extra micro-optimization there, which is that if there's\n>      no obj_cb, we have no need to assemble the full path at all. I doubt\n>      it makes much of a difference, as most callers would pass an object\n>      callback (I'd be surprised if we even have one that doesn't).\n\nAfter doing a few 'git grep' commands, I found several that include a \nNULL cruft_cb but none that have a NULL obj_cb.\n\nThanks,\n-Stolee\n"},{"id":"333941","messageId":"20171201223945.GB17334@sigill.intra.peff.net","threadId":"47362","inReplyTo":"54da89a5-bf20-cb20-9884-6285033f0d95@gmail.com","subject":"Re: [PATCH] sha1_file: use strbuf_add() instead of strbuf_addf()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-01T22:39:45Z","receivedAt":"2017-12-01T22:39:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 01, 2017 at 02:50:05PM -0500, Derrick Stolee wrote:\n\n> > > +\tbaselen = path->len;\n> > We set this here so that the '/' is included as part of the base. Makes\n> > sense, but can we now drop the earlier setting of baselen before the\n> > opendir() call?\n> \n> Yeah, probably. I had briefly considered just adding the '/' before the\n> first assignment of \"baselen\", but didn't want to change the error output. I\n> also don't know if there are side effects for other platforms by calling\n> opendir() with a '/'-terminated path.\n\nI noticed that, too. Since it's so easy to keep doing the opendir\nwithout the slash, I'd prefer to avoid finding out if there are such\nplatforms. :)\n\n> Good catch! A big reason to pull it inside and use strbuf_add over\n> strbuf_addstr is to avoid a duplicate strlen() calculation. However, I can\n> store the length before the conditional.\n\nI'd give 50/50 odds no whether a compiler could optimize out that\nstrlen. We inline addstr exactly so that callsites can see that strlen\n(it's primarily for string literals, where it can become a compile-time\nconstant, but I think it could apply here). But sometimes C's pointer\naliasing rules can be surprising in blocking \"obviously correct\"\noptimizations like that.\n\nThe generated asm is a little dense, but I _think_ \"gcc -O2\" does in\nfact do this with a single strlen based on the following tweak on top of\nyour patch:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 2160323c4a..f234519744 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1921,11 +1921,12 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n \n+\t\tstrbuf_setlen(path, baselen);\n+\t\tstrbuf_addstr(path, de->d_name);\n+\n \t\tif (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&\n \t\t    !hex_to_bytes(oid.hash + 1, de->d_name,\n \t\t\t\t  GIT_SHA1_RAWSZ - 1)) {\n-\t\t\tstrbuf_setlen(path, baselen);\n-\t\t\tstrbuf_add(path, de->d_name, GIT_SHA1_HEXSZ - 2);\n \t\t\tif (obj_cb) {\n \t\t\t\tr = obj_cb(&oid, path->buf, data);\n \t\t\t\tif (r)\n\nNot that I overly mind the manual assignment of the strlen result in\nthis particular case. But I'm a curious fellow by nature, and knowing\nthese kinds of answers helps us build up an accurate gut instinct for\nfuture cases.\n\n> Small change by storing the length in advance of the conditional:\n> \n> while (de = readdir(...)) {\n>     int namelen = strlen(de->d_name);\n>     strbuf_setlen(path, baselen);\n>     strbuf_add(path, de->d_name, namelen);\n> \n>     if (namelen == HEXSZ - 2)\n>         obj_cb(path->buf)\n>     else\n>         cruft_cb(path->buf);\n> }\n\nYup, I don't mind that approach either, but do please use size_t to\nstore the result of strlen (I know it's nearly impossible to overflow in\nthis case, but I've been trying to push the codebase in that direction\nslowly over time).\n\n> >    - there's an extra micro-optimization there, which is that if there's\n> >      no obj_cb, we have no need to assemble the full path at all. I doubt\n> >      it makes much of a difference, as most callers would pass an object\n> >      callback (I'd be surprised if we even have one that doesn't).\n> \n> After doing a few 'git grep' commands, I found several that include a NULL\n> cruft_cb but none that have a NULL obj_cb.\n\nYeah, that agrees with my cursory look.\n\nThanks!\n\n-Peff\n"},{"id":"334047","messageId":"20171204140603.180079-1-dstolee@microsoft.com","threadId":"47362","inReplyTo":"20171201223945.GB17334@sigill.intra.peff.net","subject":"[PATCH v2] sha1_file: use strbuf_add() instead of strbuf_addf()","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2017-12-04T14:06:03Z","receivedAt":"2017-12-04T14:06:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Thanks for the feedback on v1. This version fixes the cruft_cb\nbug and streamlines the strlen(). I would include an inter-diff\nbut it was the same size as the patch.\n\nThanks,\n-Stolee\n\n---\n\nReplace use of strbuf_addf() with strbuf_add() when enumerating\nloose objects in for_each_file_in_obj_subdir(). Since we already\ncheck the length and hex-values of the string before consuming\nthe path, we can prevent extra computation by using the lower-\nlevel method.\n\nOne consumer of for_each_file_in_obj_subdir() is the abbreviation\ncode. OID abbreviations use a cached list of loose objects (per\nobject subdirectory) to make repeated queries fast, but there is\nsignificant cache load time when there are many loose objects.\n\nMost repositories do not have many loose objects before repacking,\nbut in the GVFS case the repos can grow to have millions of loose\nobjects. Profiling 'git log' performance in GitForWindows on a\nGVFS-enabled repo with ~2.5 million loose objects revealed 12% of\nthe CPU time was spent in strbuf_addf().\n\nAdd a new performance test to p4211-line-log.sh that is more\nsensitive to this cache-loading. By limiting to 1000 commits, we\nmore closely resemble user wait time when reading history into a\npager.\n\nFor a copy of the Linux repo with two ~512 MB packfiles and ~572K\nloose objects, running 'git log --oneline --parents --raw -1000'\nhad the following performance:\n\n HEAD~1            HEAD\n----------------------------------------\n 7.70(7.15+0.54)   7.44(7.09+0.29) -3.4%\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n sha1_file.c              | 12 +++++++-----\n t/perf/p4211-line-log.sh |  4 ++++\n 2 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8ae6cb6285..2fc8fa93b4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1903,7 +1903,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@@ -1914,15 +1913,18 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,\n \t}\n \n \toid.hash[0] = subdir_nr;\n+\tstrbuf_addch(path, '/');\n+\tbaselen = path->len;\n \n \twhile ((de = readdir(dir))) {\n+\t\tsize_t namelen;\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n \n+\t\tnamelen = strlen(de->d_name);\n \t\tstrbuf_setlen(path, baselen);\n-\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n-\n-\t\tif (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&\n+\t\tstrbuf_add(path, de->d_name, namelen);\n+\t\tif (namelen == GIT_SHA1_HEXSZ - 2 &&\n \t\t    !hex_to_bytes(oid.hash + 1, de->d_name,\n \t\t\t\t  GIT_SHA1_RAWSZ - 1)) {\n \t\t\tif (obj_cb) {\n@@ -1941,7 +1943,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);\n \tif (!r && subdir_cb)\n \t\tr = subdir_cb(subdir_nr, path->buf, data);\n \ndiff --git a/t/perf/p4211-line-log.sh b/t/perf/p4211-line-log.sh\nindex e0ed05907c..392bcc0e51 100755\n--- a/t/perf/p4211-line-log.sh\n+++ b/t/perf/p4211-line-log.sh\n@@ -35,4 +35,8 @@ test_perf 'git log --oneline --raw --parents' '\n \tgit log --oneline --raw --parents >/dev/null\n '\n \n+test_perf 'git log --oneline --raw --parents -1000' '\n+\tgit log --oneline --raw --parents -1000 >/dev/null\n+'\n+\n test_done\n-- \n2.15.0\n\n"},{"id":"334065","messageId":"20171204165652.GA12701@sigill.intra.peff.net","threadId":"47362","inReplyTo":"20171204140603.180079-1-dstolee@microsoft.com","subject":"Re: [PATCH v2] sha1_file: use strbuf_add() instead of strbuf_addf()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-04T16:56:53Z","receivedAt":"2017-12-04T16:57:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 04, 2017 at 09:06:03AM -0500, Derrick Stolee wrote:\n\n> Thanks for the feedback on v1. This version fixes the cruft_cb\n> bug and streamlines the strlen(). I would include an inter-diff\n> but it was the same size as the patch.\n\nThanks, this version looks good to me. One procedural nit:\n\n> Thanks,\n> -Stolee\n> \n> ---\n\nWhen you put your cover letter first, you need to use \"scissors\" like:\n\n-- >8 --\n\nto separate it from the commit message. Using three-dashes means \"git\nam\" will include your cover letter as the commit message, and omit your\nreal commit message entirely.\n\n-Peff\n"},{"id":"334066","messageId":"f539b5e3-6bb2-4970-4205-242a2a3cc6ce@gmail.com","threadId":"47362","inReplyTo":"20171204165652.GA12701@sigill.intra.peff.net","subject":"Re: [PATCH v2] sha1_file: use strbuf_add() instead of strbuf_addf()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2017-12-04T17:12:29Z","receivedAt":"2017-12-04T17:12:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/4/2017 11:56 AM, Jeff King wrote:\n> When you put your cover letter first, you need to use \"scissors\" like:\n> -- >8 --\n>\n> to separate it from the commit message. Using three-dashes means \"git\n> am\" will include your cover letter as the commit message, and omit your\n> real commit message entirely.\n\nThanks. I updated our internal best-practices accordingly.\n\n-Stolee\n"}]}