{"thread":{"id":"47255","subject":"[PATCH 0/5] avoiding pointless pack-directory re-scans","startedAt":"2017-11-20T20:26:20Z","lastAt":"2017-11-25T03:20:16Z","messageCount":25,"participants":["Jeff King","Stefan Beller","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"332935","messageId":"20171120202607.tf2pvegqe35mhxjs@sigill.intra.peff.net","threadId":"47255","inReplyTo":null,"subject":"[PATCH 0/5] avoiding pointless pack-directory re-scans","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-20T20:26:07Z","receivedAt":"2017-11-20T20:26:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I recently dug into a performance problem running \"git fetch\" in a\nrepository with 5000 packs. Now obviously that's a silly number of packs\nto have, but I did find some pretty low-hanging fruit. Most of the time\nwas spent in pointlessly re-scanning the objects/pack directory.\n\nThis series has two fixes, along with a perf test that covers this case.\nI think the perf test is especially important here because we actually\nfixed one of these cases (patch 4) already, but later regressed it\nwithout noticing. The perf suite could have caught that (and would after\nthis series).\n\nThere are numbers in the individual commits, but here's the before/after\nfor the whole series:\n\nTest origin HEAD\n--------------------------------------------------------\n5551.4: fetch 5.48(4.99+0.50) 0.14(0.09+0.05) -97.4%\n\nThat's on a somewhat-contrived setup meant to maximize us noticing a\nregression. But I do think these re-scans are probably kicking in for\nnormal situations, but just aren't quite expensive enough for anybody to\nhave noticed and dug into it.\n\nPatches:\n\n [1/5]: p5550: factor our nonsense-pack creation\n [2/5]: t/perf/lib-pack: use fast-import checkpoint to create packs\n [3/5]: p5551: add a script to test fetch pack-dir rescans\n [4/5]: everything_local: use \"quick\" object existence check\n [5/5]: sha1_file: don't re-scan pack directory for null sha1\n\n fetch-pack.c | 3 ++-\n sha1_file.c | 3 +++\n t/perf/lib-pack.sh | 25 ++++++++++++++++++++\n t/perf/p5550-fetch-tags.sh | 25 ++------------------\n t/perf/p5551-fetch-rescan.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 87 insertions(+), 24 deletions(-)\n create mode 100644 t/perf/lib-pack.sh\n create mode 100755 t/perf/p5551-fetch-rescan.sh\n\n-Peff\n"},{"id":"332936","messageId":"20171120202643.s2cywlqykayq5qdb@sigill.intra.peff.net","threadId":"47255","inReplyTo":"20171120202607.tf2pvegqe35mhxjs@sigill.intra.peff.net","subject":"[PATCH 1/5] p5550: factor our nonsense-pack creation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-20T20:26:43Z","receivedAt":"2017-11-20T20:26:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a function to create a bunch of irrelevant packs to\nmeasure the expense of reprepare_packed_git(). Let's make\nthat available to other perf scripts.\n\nSigned-off-by: Jeff King \n---\n t/perf/lib-pack.sh | 29 +++++++++++++++++++++++++++++\n t/perf/p5550-fetch-tags.sh | 25 ++-----------------------\n 2 files changed, 31 insertions(+), 23 deletions(-)\n create mode 100644 t/perf/lib-pack.sh\n\ndiff --git a/t/perf/lib-pack.sh b/t/perf/lib-pack.sh\nnew file mode 100644\nindex 0000000000..501bb7b272\n--- /dev/null\n+++ b/t/perf/lib-pack.sh\n@@ -0,0 +1,29 @@\n+# Helpers for dealing with large numbers of packs.\n+\n+# create $1 nonsense packs, each with a single blob\n+create_packs () {\n+\tperl -le '\n+\t\tmy ($n) = @ARGV;\n+\t\tfor (1..$n) {\n+\t\t\tprint \"blob\";\n+\t\t\tprint \"data <\n---\n t/perf/lib-pack.sh | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/t/perf/lib-pack.sh b/t/perf/lib-pack.sh\nindex 501bb7b272..d3865db286 100644\n--- a/t/perf/lib-pack.sh\n+++ b/t/perf/lib-pack.sh\n@@ -9,15 +9,10 @@ create_packs () {\n \t\t\tprint \"data <\n---\n t/perf/p5551-fetch-rescan.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 55 insertions(+)\n create mode 100755 t/perf/p5551-fetch-rescan.sh\n\ndiff --git a/t/perf/p5551-fetch-rescan.sh b/t/perf/p5551-fetch-rescan.sh\nnew file mode 100755\nindex 0000000000..b99dc23e32\n--- /dev/null\n+++ b/t/perf/p5551-fetch-rescan.sh\n@@ -0,0 +1,55 @@\n+#!/bin/sh\n+\n+test_description='fetch performance with many packs\n+\n+It is common for fetch to consider objects that we might not have, and it is an\n+easy mistake for the code to use a function like `parse_object` that might\n+give the correct _answer_ on such an object, but do so slowly (due to\n+re-scanning the pack directory for lookup failures).\n+\n+The resulting performance drop can be hard to notice in a real repository, but\n+becomes quite large in a repository with a large number of packs. So this\n+test creates a more pathological case, since any mistakes would produce a more\n+noticeable slowdown.\n+'\n+. ./perf-lib.sh\n+. \"$TEST_DIRECTORY\"/perf/lib-pack.sh\n+\n+test_expect_success 'create parent and child' '\n+\tgit init parent &&\n+\tgit clone parent child\n+'\n+\n+\n+test_expect_success 'create refs in the parent' '\n+\t(\n+\t\tcd parent &&\n+\t\tgit commit --allow-empty -m foo &&\n+\t\thead=$(git rev-parse HEAD) &&\n+\t\ttest_seq 1000 |\n+\t\tsed \"s,.*,update refs/heads/& $head,\" |\n+\t\t$MODERN_GIT update-ref --stdin\n+\t)\n+'\n+\n+test_expect_success 'create many packs in the child' '\n+\t(\n+\t\tcd child &&\n+\t\tsetup_many_packs\n+\t)\n+'\n+\n+test_perf 'fetch' '\n+\t# start at the same state for each iteration\n+\tobj=$($MODERN_GIT -C parent rev-parse HEAD) &&\n+\t(\n+\t\tcd child &&\n+\t\t$MODERN_GIT for-each-ref --format=\"delete %(refname)\" refs/remotes |\n+\t\t$MODERN_GIT update-ref --stdin &&\n+\t\trm -vf .git/objects/$(echo $obj | sed \"s|^..|&/|\") &&\n+\n+\t\tgit fetch\n+\t)\n+'\n+\n+test_done\n-- \n2.15.0.494.g79a8547723\n\n"},{"id":"332941","messageId":"20171120202920.7ppcwmzkxifywtoj@sigill.intra.peff.net","threadId":"47255","inReplyTo":"20171120202607.tf2pvegqe35mhxjs@sigill.intra.peff.net","subject":"[PATCH 4/5] everything_local: use \"quick\" object existence check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-20T20:29:21Z","receivedAt":"2017-11-20T20:29:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In b495697b82 (fetch-pack: avoid repeatedly re-scanning pack\ndirectory, 2013-01-26), we noticed that everything_local()\ncould waste time trying to find and parse objects which we\n_expect_ to be missing. The solution was to put\nhas_sha1_file() in front of parse_object() to skip the\nmore-expensive parse attempt.\n\nThat optimization was negated later when has_sha1_file()\nlearned to do the same re-scan in 45e8a74873 (has_sha1_file:\nre-check pack directory before giving up, 2013-08-30).\n\nWe can restore it by using the \"quick\" flag to tell\nhas_sha1_file (actually has_object_file these days) that we\nprefer speed to thoroughness for this call. See also the\nfixes in 5827a0354 and 0eeb077be7 for prior art and\ndiscussion on using the \"quick\" flag for these cases.\n\nThe recently-added performance regression test in p5551\ndemonstrates the problem. You can see the original fix:\n\n Test b495697b82^ b495697b82\n --------------------------------------------------------\n 5551.4: fetch 1.68(1.33+0.35) 0.87(0.69+0.18) -48.2%\n\nand then the regression:\n\n Test 45e8a74873^ 45e8a74873\n ---------------------------------------------------------\n 5551.4: fetch 0.96(0.77+0.19) 2.55(2.04+0.50) +165.6%\n\nand now our fix:\n\n Test HEAD^ HEAD\n --------------------------------------------------------\n 5551.4: fetch 7.21(6.58+0.63) 5.47(5.04+0.43) -24.1%\n\nYou can also see that other things have gotten a lot slower\nsince 2013. We'll deal with those in separate patches.\n\nSigned-off-by: Jeff King \n---\n fetch-pack.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 008b25d3db..9f6b07ad91 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -716,7 +716,8 @@ static int everything_local(struct fetch_pack_args *args,\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct object *o;\n \n-\t\tif (!has_object_file(&ref->old_oid))\n+\t\tif (!has_object_file_with_flags(&ref->old_oid,\n+\t\t\t\t\t\tOBJECT_INFO_QUICK))\n \t\t\tcontinue;\n \n \t\to = parse_object(&ref->old_oid);\n-- \n2.15.0.494.g79a8547723\n\n"},{"id":"332942","messageId":"20171120203523.c3pt5qi43e24ttqq@sigill.intra.peff.net","threadId":"47255","inReplyTo":"20171120202607.tf2pvegqe35mhxjs@sigill.intra.peff.net","subject":"[PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-20T20:35:23Z","receivedAt":"2017-11-20T20:35:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In theory nobody should ever ask the low-level object code\nfor a null sha1. It's used as a sentinel for \"no such\nobject\" in lots of places, so leaking through to this level\nis a sign that the higher-level code is not being careful\nabout its error-checking. In practice, though, quite a few\ncode paths seem to rely on the null sha1 lookup failing as a\nway to quietly propagate non-existence (e.g., by feeding it\nto lookup_commit_reference_gently(), which then returns\nNULL).\n\nWhen this happens, we do two inefficient things:\n\n 1. We actually search for the null sha1 in packs and in\n the loose object directory.\n\n 2. When we fail to find it, we re-scan the pack directory\n in case a simultaneous repack happened to move it from\n loose to packed.\n\nIt's debatable whether (1) is a good idea or not. The\nperformance drop is at least linear in the number of\nlookups, so it's not too bad. And if by some 2^-160th chance\nsomebody does have such an object, we probably ought to\naccess it. On the other hand, enough code paths treat the\nnull sha1 specially that the object probably isn't usable,\nanyway.\n\nProblem (2), on the other hand, is a pretty severe\nperformance issue. If you have a large number of packs,\nrescanning the pack directory can be very expensive. And it\nonly helps in the case that you have an object with the null\nsha1 _and_ somebody was simultaneously repacking it. The\ntradeoff is certainly a bad one there.\n\nIn an ideal world, we'd simply fix all of the callers to\nnotice the null sha1 and avoid passing it to us. But a\nsimple experiment to catch this with a BUG() shows that\nthere are a large number of code paths.\n\nIn the meantime, let's address problem (2). That's the\nminimal fix we can do to make the performance problems go\naway. p5551 shows this off (when a fetched ref is new, the\n\"old\" sha1 is 0{40}, which ends up being passed for\nfast-forward checks, the status table abbreviations, etc):\n\n Test HEAD^ HEAD\n --------------------------------------------------------\n 5551.4: fetch 5.51(5.03+0.48) 0.17(0.10+0.06) -96.9%\n\nWe could address (1), too, but there's not much performance\nimprovement left to make.\n\nSigned-off-by: Jeff King \n---\nThis is the minimal fix that addresses the performance issues.\nI'd actually have no problem at all declaring that looking up a null\nsha1 is insane, and having the object-lookup routines simply return \"no\nsuch object\" without even doing the loose/pack lookup first.\n\nI'm also fine with going down the BUG() route and fixing the\nhigher-level callers, but it's a big enough task (with little enough\nreal-world impact) that I think it would be worth applying this in the\nmeantime.\n\n sha1_file.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8a7c6b7eba..dde0ad101d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1180,6 +1180,9 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \t\tif (!sha1_loose_object_info(real, oi, flags))\n \t\t\treturn 0;\n \n+\t\tif (is_null_sha1(sha1))\n+\t\t\treturn -1;\n+\n \t\t/* Not a loose object; someone else may have just packed it. */\n \t\tif (flags & OBJECT_INFO_QUICK) {\n \t\t\treturn -1;\n-- \n2.15.0.494.g79a8547723\n"},{"id":"332945","messageId":"CAGZ79kZWJhZ-6-1MZiWK4O-uWJG+T5QU29a5fRnw6R26J4fuCA@mail.gmail.com","threadId":"47255","inReplyTo":"20171120203523.c3pt5qi43e24ttqq@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-20T20:47:53Z","receivedAt":"2017-11-20T20:48:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 20, 2017 at 12:35 PM, Jeff King wrote:\n> In theory nobody should ever ask the low-level object code\n> for a null sha1. It's used as a sentinel for \"no such\n> object\" in lots of places, so leaking through to this level\n> is a sign that the higher-level code is not being careful\n> about its error-checking. In practice, though, quite a few\n> code paths seem to rely on the null sha1 lookup failing as a\n> way to quietly propagate non-existence (e.g., by feeding it\n> to lookup_commit_reference_gently(), which then returns\n> NULL).\n>\n> When this happens, we do two inefficient things:\n>\n> 1. We actually search for the null sha1 in packs and in\n> the loose object directory.\n>\n> 2. When we fail to find it, we re-scan the pack directory\n> in case a simultaneous repack happened to move it from\n> loose to packed.\n>\n> It's debatable whether (1) is a good idea or not. The\n> performance drop is at least linear in the number of\n> lookups, so it's not too bad. And if by some 2^-160th chance\n> somebody does have such an object, we probably ought to\n> access it. On the other hand, enough code paths treat the\n> null sha1 specially that the object probably isn't usable,\n> anyway.\n>\n> Problem (2), on the other hand, is a pretty severe\n> performance issue. If you have a large number of packs,\n> rescanning the pack directory can be very expensive. And it\n> only helps in the case that you have an object with the null\n> sha1 _and_ somebody was simultaneously repacking it. The\n> tradeoff is certainly a bad one there.\n>\n> In an ideal world, we'd simply fix all of the callers to\n> notice the null sha1 and avoid passing it to us. But a\n> simple experiment to catch this with a BUG() shows that\n> there are a large number of code paths.\n>\n> In the meantime, let's address problem (2). That's the\n> minimal fix we can do to make the performance problems go\n> away. p5551 shows this off (when a fetched ref is new, the\n> \"old\" sha1 is 0{40}, which ends up being passed for\n> fast-forward checks, the status table abbreviations, etc):\n>\n> Test HEAD^ HEAD\n> --------------------------------------------------------\n> 5551.4: fetch 5.51(5.03+0.48) 0.17(0.10+0.06) -96.9%\n>\n> We could address (1), too, but there's not much performance\n> improvement left to make.\n>\n> Signed-off-by: Jeff King \n> ---\n> This is the minimal fix that addresses the performance issues.\n> I'd actually have no problem at all declaring that looking up a null\n> sha1 is insane, and having the object-lookup routines simply return \"no\n> such object\" without even doing the loose/pack lookup first.\n>\n> I'm also fine with going down the BUG() route and fixing the\n> higher-level callers, but it's a big enough task (with little enough\n> real-world impact) that I think it would be worth applying this in the\n> meantime.\n\nIt would have a lot of impact in the future, when new developers\nare hindered mis-using the API. The (unwritten) series with introducing\nBUG() would help a lot in 'holding it right' and I would expect fewer\nperformance\nregressions over time.\n\nThe patch is impressively small for such a performance gain.\nPersonally I think (1) (which essentially means \"making null sha1\nwork like a regular sha1\") is quite an endeavor at this point in time\nfor this code base.\n\nAs a tangent, a similar but solved problem in the diff code is how\nNUL in user data is treated in xdiff for example, as there we kept\nbeing careful since the beginning (though I think we don't have tests\nfor it, so it might be broken)\n\nStefan\n"},{"id":"332946","messageId":"20171120205843.chcpp5qryhta4ctt@sigill.intra.peff.net","threadId":"47255","inReplyTo":"CAGZ79kZWJhZ-6-1MZiWK4O-uWJG+T5QU29a5fRnw6R26J4fuCA@mail.gmail.com","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-20T20:58:43Z","receivedAt":"2017-11-20T20:58:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 20, 2017 at 12:47:53PM -0800, Stefan Beller wrote:\n\n> > This is the minimal fix that addresses the performance issues.\n> > I'd actually have no problem at all declaring that looking up a null\n> > sha1 is insane, and having the object-lookup routines simply return \"no\n> > such object\" without even doing the loose/pack lookup first.\n> >\n> > I'm also fine with going down the BUG() route and fixing the\n> > higher-level callers, but it's a big enough task (with little enough\n> > real-world impact) that I think it would be worth applying this in the\n> > meantime.\n> \n> It would have a lot of impact in the future, when new developers\n> are hindered mis-using the API. The (unwritten) series with introducing\n> BUG() would help a lot in 'holding it right' and I would expect fewer\n> performance\n> regressions over time.\n\nYeah, I agree the BUG() route is the nicest. I started trying to hunt\ndown every code path causing a test failure, though, and soon got\noverwhelmed.\n\nI actually think it's not the worst thing in the world to say \"it's OK\nto look up the null sha1; the result is well-defined, and it does not\nexist\". It's perhaps philosophically a little ugly, but I think it's\nquite practical.\n\n> The patch is impressively small for such a performance gain.\n> Personally I think (1) (which essentially means \"making null sha1\n> work like a regular sha1\") is quite an endeavor at this point in time\n> for this code base.\n\nYeah, it would be a crazy amount of effort to try to support that, for\nvery little gain. If we did want to just declare that the null sha1 does\nnot exist (even if you happen to have objects/00/00... in your\nrepository), the patch is similarly easy:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8a7c6b7eba..754fe7f03e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1152,6 +1152,9 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \t\t\t\t lookup_replace_object(sha1) :\n \t\t\t\t sha1;\n \n+\tif (is_null_sha1(sha1))\n+\t\treturn -1;\n+\n \tif (!oi)\n \t\toi = &blank_oi;\n \n\n> As a tangent, a similar but solved problem in the diff code is how\n> NUL in user data is treated in xdiff for example, as there we kept\n> being careful since the beginning (though I think we don't have tests\n> for it, so it might be broken)\n\nThere I think it's nice if we can be tolerant of NULs, because they're a\nthing that might actually happen in user input. A real null sha1 has a\n2^-160 chance of happening, and the design of sha1 is such that it's\nhard for somebody to do it intentionally.\n\nYou might be able to get up to some mischief with replace objects,\nthough.\n\n-Peff\n"},{"id":"333023","messageId":"CAPig+cRDNmjmM2Ed+mwqf7JZ76CxA3Eh7-UV6k=LFngZ3YE_sQ@mail.gmail.com","threadId":"47255","inReplyTo":"20171120202643.s2cywlqykayq5qdb@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] p5550: factor our nonsense-pack creation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-20T23:55:51Z","receivedAt":"2017-11-20T23:56:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 20, 2017 at 3:26 PM, Jeff King wrote:\n> p5550: factor our nonsense-pack creation\n\ns/our/out/, I guess.\n\n> We have a function to create a bunch of irrelevant packs to\n> measure the expense of reprepare_packed_git(). Let's make\n> that available to other perf scripts.\n>\n> Signed-off-by: Jeff King \n"},{"id":"333043","messageId":"xmqqd14cjr13.fsf@gitster.mtv.corp.google.com","threadId":"47255","inReplyTo":"20171120203523.c3pt5qi43e24ttqq@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-21T02:37:28Z","receivedAt":"2017-11-21T02:37:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In an ideal world, we'd simply fix all of the callers to\n> notice the null sha1 and avoid passing it to us. But a\n> simple experiment to catch this with a BUG() shows that\n> there are a large number of code paths.\n\nWell, we can view this (or the alternative you sent later that does\nthe same a bit earlier in the function) as \"fixing the caller\" but\nhas already refactord the common logic to a helper function that all\nof these callers call into ;-).\n\n"},{"id":"333047","messageId":"xmqqwp2ki4x8.fsf@gitster.mtv.corp.google.com","threadId":"47255","inReplyTo":"20171120203523.c3pt5qi43e24ttqq@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-21T05:20:19Z","receivedAt":"2017-11-21T05:20:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is the minimal fix that addresses the performance issues.\n> I'd actually have no problem at all declaring that looking up a null\n> sha1 is insane, and having the object-lookup routines simply return \"no\n> such object\" without even doing the loose/pack lookup first.\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 8a7c6b7eba..dde0ad101d 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1180,6 +1180,9 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n> \t\tif (!sha1_loose_object_info(real, oi, flags))\n> \t\t\treturn 0;\n> \n> +\t\tif (is_null_sha1(sha1))\n> +\t\t\treturn -1;\n> +\n> \t\t/* Not a loose object; someone else may have just packed it. */\n> \t\tif (flags & OBJECT_INFO_QUICK) {\n> \t\t\treturn -1;\n\nAfter queuing this series to an earlier part of 'pu' and resolving a\nconflict with jh/fsck-promisors topic, I ended up with a code that\nrejects 0{40} a lot earlier, before we try to see if a pack entry\nfor 0{40} exists, even though the patch that is queued on this topic\nis more conservative (i.e. the above one).\n\nPerhaps we would want to use the alternate version that declares the\n0{40} is a sentinel that signals that there is no such object in\nthis topic---that would give us a consistent semantics without\nhaving to adjust jh/fsck-promisors when it becomes ready to be\nmerged.\n\n\n"},{"id":"333103","messageId":"20171121155815.46ih4ld4cp7n544l@sigill.intra.peff.net","threadId":"47255","inReplyTo":"CAPig+cRDNmjmM2Ed+mwqf7JZ76CxA3Eh7-UV6k=LFngZ3YE_sQ@mail.gmail.com","subject":"Re: [PATCH 1/5] p5550: factor our nonsense-pack creation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-21T15:58:16Z","receivedAt":"2017-11-21T15:58:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 20, 2017 at 06:55:51PM -0500, Eric Sunshine wrote:\n\n> On Mon, Nov 20, 2017 at 3:26 PM, Jeff King wrote:\n> > p5550: factor our nonsense-pack creation\n> \n> s/our/out/, I guess.\n\nHeh, yes. I even fixed it once, but I have the funny habit of noticing\nsuch typos while reading the \"todo\" list of \"rebase -i\" and fixing them\nthere. Which of course has no impact whatsoever on the commit. :-/\n\n-Peff\n"},{"id":"333182","messageId":"20171121225744.GA21197@sigill","threadId":"47255","inReplyTo":"xmqqd14cjr13.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-21T22:57:44Z","receivedAt":"2017-11-21T22:57:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 21, 2017 at 11:37:28AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > In an ideal world, we'd simply fix all of the callers to\n> > notice the null sha1 and avoid passing it to us. But a\n> > simple experiment to catch this with a BUG() shows that\n> > there are a large number of code paths.\n> \n> Well, we can view this (or the alternative you sent later that does\n> the same a bit earlier in the function) as \"fixing the caller\" but\n> has already refactord the common logic to a helper function that all\n> of these callers call into ;-).\n\nYes, I'm definitely tempted by that view. :)\n\nWhat worries me, though, is that callers who lazily propagate the null\nsha1 to the lookup functions cannot reasonably tell the difference\nbetween \"this object was corrupt or missing\" and \"we passed the null\nsha1, and a missing object is expected\".\n\nFor example, look at how fetch.c:update_local_ref() looks up objects.\nIt feeds the old and new sha1 to lookup_commit_reference_gently(), and\nif either is NULL, it skips the fast-forward check. That makes sense if\nwe expect the null sha1 to get translated into a NULL commit. But it\nalso triggers for a broken ref, and we'd overwrite it (when the right\nthing is probably refusing to update).\n\nHere's a runnable example:\n\n-- >8 --\ngit init parent\ngit -C parent commit --allow-empty -m base\n\ngit clone parent child\ngit -C parent commit --allow-empty -m more\n\ncd child\nrm -f .git/objects/??/*\ngit fetch\n-- 8< --\n\nThat final fetch spews a bunch of errors about broken refs, and then\noverwrites the value of origin/master, even though it's broken (in this\ncase it actually is a fast-forward, but the child repo doesn't even know\nthat).\n\nI'm not sure what the right behavior is, but I'm pretty sure that's not\nit. Probably one of:\n\n - skip updating the ref when we see the breakage\n\n - ditto, but terminate the whole operation, since we might be deleting\n other refs and in a broken repo we're probably best to make as few\n changes as possible\n\n - behave as if it was a non-ff, which would allow \"--force\" to\n overwrite the broken ref. Maybe convenient for fixing things, but\n possibly surprising (and it's not that hard to just delete the\n broken refs manually before proceeding).\n\n-Peff\n"},{"id":"333187","messageId":"20171121231739.GB21197@sigill","threadId":"47255","inReplyTo":"xmqqwp2ki4x8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-21T23:17:39Z","receivedAt":"2017-11-21T23:17:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 21, 2017 at 02:20:19PM +0900, Junio C Hamano wrote:\n\n> After queuing this series to an earlier part of 'pu' and resolving a\n> conflict with jh/fsck-promisors topic, I ended up with a code that\n> rejects 0{40} a lot earlier, before we try to see if a pack entry\n> for 0{40} exists, even though the patch that is queued on this topic\n> is more conservative (i.e. the above one).\n> \n> Perhaps we would want to use the alternate version that declares the\n> 0{40} is a sentinel that signals that there is no such object in\n> this topic---that would give us a consistent semantics without\n> having to adjust jh/fsck-promisors when it becomes ready to be\n> merged.\n\nI think we could adjust the patches without too much effort. That said,\nI am very on-the-fence about whether to just declare the null sha1 as\n\"not a real object\" and automatically return from the lookup without\ndoing any work. But if you agree that it's not likely to hurt anything,\nit's IMHO a lot easier to reason about.\n\nHere's a re-roll of patch 5 that behaves this way (the patch should be\nunsurprising, but I've updated the commit message to match).\n\nI did notice one other thing: the function looks up replace objects, so\nwe have both the original and the replaced sha1 available. My earlier\npatch used the original sha1, but this one uses the replaced value.\nI'm not sure if that's sane or not. It lets the fast-path kick in if you\npoint a replace ref at 0{40}. And it lets you point refs/replace/0{40}\nto something else. I doubt that's useful, but it could perhaps be for\ndebugging, etc.\n\nIn most cases, of course, it wouldn't matter (since pointing to or from\nthe null sha1 is vaguely crazy in the first place).\n\n-- >8 --\nSubject: [PATCH v2 5/5] sha1_file: fast-path null sha1 as a missing object\n\nIn theory nobody should ever ask the low-level object code\nfor a null sha1. It's used as a sentinel for \"no such\nobject\" in lots of places, so leaking through to this level\nis a sign that the higher-level code is not being careful\nabout its error-checking. In practice, though, quite a few\ncode paths seem to rely on the null sha1 lookup failing as a\nway to quietly propagate non-existence (e.g., by feeding it\nto lookup_commit_reference_gently(), which then returns\nNULL).\n\nWhen this happens, we do two inefficient things:\n\n 1. We actually search for the null sha1 in packs and in\n the loose object directory.\n\n 2. When we fail to find it, we re-scan the pack directory\n in case a simultaneous repack happened to move it from\n loose to packed. This can be very expensive if you have\n a large number of packs.\n\nOnly the second one actually causes noticeable performance\nproblems, so we could treat them independently. But for the\nsake of simplicity (both of code and of reasoning about it),\nit makes sense to just declare that the null sha1 cannot be\na real on-disk object, and looking it up will always return\n\"no such object\".\n\nThere's no real loss of functionality to do so Its use as a\nsentinel value means that anybody who is unlucky enough to\nhit the 2^-160th chance of generating an object with that\nsha1 is already going to find the object largely unusable.\n\nIn an ideal world, we'd simply fix all of the callers to\nnotice the null sha1 and avoid passing it to us. But a\nsimple experiment to catch this with a BUG() shows that\nthere are a large number of code paths that do so.\n\nSo in the meantime, let's fix the performance problem by\ntaking a fast exit from the object lookup when we see a null\nsha1. p5551 shows off the improvement (when a fetched ref is\nnew, the \"old\" sha1 is 0{40}, which ends up being passed for\nfast-forward checks, the status table abbreviations, etc):\n\n Test HEAD^ HEAD\n --------------------------------------------------------\n 5551.4: fetch 5.51(5.03+0.48) 0.17(0.10+0.06) -96.9%\n\nSigned-off-by: Jeff King \n---\n sha1_file.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8a7c6b7eba..ae4b71f445 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1152,6 +1152,9 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,\n \t\t\t\t lookup_replace_object(sha1) :\n \t\t\t\t sha1;\n \n+\tif (is_null_sha1(real))\n+\t\treturn -1;\n+\n \tif (!oi)\n \t\toi = &blank_oi;\n \n-- \n2.15.0.578.g35b419775f\n\n"},{"id":"333203","messageId":"CAGZ79kbNQCzmkQDFY+gxEuZDPziehZdhgReCzpk3oxn=fpv=UQ@mail.gmail.com","threadId":"47255","inReplyTo":"20171121155815.46ih4ld4cp7n544l@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] p5550: factor our nonsense-pack creation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-22T00:32:42Z","receivedAt":"2017-11-22T00:32:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 21, 2017 at 7:58 AM, Jeff King wrote:\n> On Mon, Nov 20, 2017 at 06:55:51PM -0500, Eric Sunshine wrote:\n>\n>> On Mon, Nov 20, 2017 at 3:26 PM, Jeff King wrote:\n>> > p5550: factor our nonsense-pack creation\n>>\n>> s/our/out/, I guess.\n>\n> Heh, yes. I even fixed it once, but I have the funny habit of noticing\n> such typos while reading the \"todo\" list of \"rebase -i\" and fixing them\n> there. Which of course has no impact whatsoever on the commit. :-/\n>\n\nThat happened to me a couple of times as well before.\nThis sounds like a UX bug on first thought.\n\nOn second thought the text after the abbreviated hash can be\nuser dependent IIRC, by setting some format option how to populate the\nrebase instruction sheet using log, so it would not be easy to take\nfixes from that line into the commit for a fixup.\n"},{"id":"333216","messageId":"xmqqwp2jxf5l.fsf@gitster.mtv.corp.google.com","threadId":"47255","inReplyTo":"20171121225744.GA21197@sigill","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T01:42:30Z","receivedAt":"2017-11-22T01:42:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not sure what the right behavior is, but I'm pretty sure that's not\n> it. Probably one of:\n>\n> - skip updating the ref when we see the breakage\n>\n> - ditto, but terminate the whole operation, since we might be deleting\n> other refs and in a broken repo we're probably best to make as few\n> changes as possible\n>\n> - behave as if it was a non-ff, which would allow \"--force\" to\n> overwrite the broken ref. Maybe convenient for fixing things, but\n> possibly surprising (and it's not that hard to just delete the\n> broken refs manually before proceeding).\n\nPerhaps the last one would be the ideal endgame, but the second one\nmay be a good stopping point in the shorter term.\n\n"},{"id":"333217","messageId":"xmqqr2srxeu2.fsf@gitster.mtv.corp.google.com","threadId":"47255","inReplyTo":"20171121231739.GB21197@sigill","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T01:49:25Z","receivedAt":"2017-11-22T01:49:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here's a re-roll of patch 5 that behaves this way (the patch should be\n> unsurprising, but I've updated the commit message to match).\n>\n> I did notice one other thing: the function looks up replace objects, so\n> we have both the original and the replaced sha1 available. My earlier\n> patch used the original sha1, but this one uses the replaced value.\n> I'm not sure if that's sane or not. It lets the fast-path kick in if you\n> point a replace ref at 0{40}. And it lets you point refs/replace/0{40}\n> to something else. I doubt that's useful, but it could perhaps be for\n> debugging, etc.\n>\n> In most cases, of course, it wouldn't matter (since pointing to or from\n> the null sha1 is vaguely crazy in the first place).\n\nI tend to agree that those who go crazy/fancy with replace mechanism\ncan keep both halves when it breaks.\n\nWRT existing codepaths that pass 0{40} and refuses to notice a\npotential repository corruption (from getting a NULL for a non null\nobject name), I think we would need a sweep of the codebase and fix\nthem in the longer term. As long as that will happen someday, either\napproach between \"we know 'no loose object? let's redo the packs' is\nthe part that matters performance-wise, so let's do a short-cut only\nfor that\" and \"we know that callers that comes with 0{40} want to get\nNULL back\" should be OK, I would think.\n\n\n\n"},{"id":"333228","messageId":"20171122031701.GA16789@sigill","threadId":"47255","inReplyTo":"xmqqr2srxeu2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-22T03:17:01Z","receivedAt":"2017-11-22T03:17:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 22, 2017 at 10:49:25AM +0900, Junio C Hamano wrote:\n\n> WRT existing codepaths that pass 0{40} and refuses to notice a\n> potential repository corruption (from getting a NULL for a non null\n> object name), I think we would need a sweep of the codebase and fix\n> them in the longer term. As long as that will happen someday, either\n> approach between \"we know 'no loose object? let's redo the packs' is\n> the part that matters performance-wise, so let's do a short-cut only\n> for that\" and \"we know that callers that comes with 0{40} want to get\n> NULL back\" should be OK, I would think.\n\nI agree. Let's go with the \"v2 5/5\" I posted then.\n\nI'll try to work up a patch for the fetch.c case I found tomorrow, but I\nsuspect there are many more. But that's largely orthogonal to the\nseries.\n\n-Peff\n"},{"id":"333337","messageId":"20171122223613.GA1405@sigill","threadId":"47255","inReplyTo":"xmqqwp2jxf5l.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-22T22:36:14Z","receivedAt":"2017-11-22T22:36:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 22, 2017 at 10:42:30AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm not sure what the right behavior is, but I'm pretty sure that's not\n> > it. Probably one of:\n> >\n> > - skip updating the ref when we see the breakage\n> >\n> > - ditto, but terminate the whole operation, since we might be deleting\n> > other refs and in a broken repo we're probably best to make as few\n> > changes as possible\n> >\n> > - behave as if it was a non-ff, which would allow \"--force\" to\n> > overwrite the broken ref. Maybe convenient for fixing things, but\n> > possibly surprising (and it's not that hard to just delete the\n> > broken refs manually before proceeding).\n> \n> Perhaps the last one would be the ideal endgame, but the second one\n> may be a good stopping point in the shorter term.\n\nThis turns out to be a lot trickier than I expected. The crux of the\nmatter is that the case we care about is hidden inside\nlookup_commit_reference_gently(), which doesn't distinguish between\ncorruption and \"not a commit\".\n\nSo there are four cases we care about for this call in fetch:\n\n 1. We fed a real sha1 and got a commit (or peeled to one).\n\n 2. We fed a real sha1 which resolved to a non-commit, and we got NULL.\n\n 3. We fed a real sha1 and the object was missing or corrupted, and we\n got NULL.\n\n 4. We fed a null sha1 and got NULL.\n\nRight now we lump cases 2-4 together as \"do not do a fast-forward\ncheck\". That's fine for 2 and 4, but probably not for 3. We can easily\ncatch case 4 ourselves (if we care to), but distinguishing case 3 from\nthe others is hard. How should lookup_commit_reference_gently() signal\nit to us?\n\nOr should lookup_commit_reference_gently() die on corruption? That's not\nvery \"gentle\", but I think the \"gently\" here is really about \"it might\nnot be a commit\", not \"the repo might be corrupted\". But I think even\nthat may be the tip of the iceberg. The next thing we do is feed the\ncommits to in_merge_bases(), which will happily return \"nope\" if the old\ncommit cannot be parsed (because it has only a boolean return value).\n\nSo I dunno. Maybe it is a losing battle to try to pass this kind of\ncorruption information up the stack. I'm tempted to say that there\nshould just be a \"paranoid\" flag to globally die() whenever we see a\ncorruption (and you could run with it normally, but relax it whenever\nyou're investigating a broken repo). But I doubt even that works. Not\nhaving the \"old_oid\" object at all would be a repo corruption here, but\nhow are the low-level routines supposed to know when a missing object is\na corruption and when it is not?\n\n-Peff\n"},{"id":"333338","messageId":"20171122223817.GB1405@sigill","threadId":"47255","inReplyTo":"CAGZ79kbNQCzmkQDFY+gxEuZDPziehZdhgReCzpk3oxn=fpv=UQ@mail.gmail.com","subject":"Re: [PATCH 1/5] p5550: factor our nonsense-pack creation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-22T22:38:17Z","receivedAt":"2017-11-22T22:38:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 21, 2017 at 04:32:42PM -0800, Stefan Beller wrote:\n\n> > Heh, yes. I even fixed it once, but I have the funny habit of noticing\n> > such typos while reading the \"todo\" list of \"rebase -i\" and fixing them\n> > there. Which of course has no impact whatsoever on the commit. :-/\n> \n> That happened to me a couple of times as well before.\n> This sounds like a UX bug on first thought.\n> \n> On second thought the text after the abbreviated hash can be\n> user dependent IIRC, by setting some format option how to populate the\n> rebase instruction sheet using log, so it would not be easy to take\n> fixes from that line into the commit for a fixup.\n\nRight, I came to the same conclusion (we may even have discussed this on\nthe list, I don't remember). The current \"todo\" format says that only\nthe command and sha1 matter, and we'd be changing that. Maybe that's not\nso bad if the user has to enable the feature themselves (and clearly it\nwould be incompatible with a custom format option). But I think in the\nend it probably just makes sense to retrain my expectation, and remember\nto \"reword\" instead of \"pick\".\n\n-Peff\n"},{"id":"333368","messageId":"xmqqpo89rac6.fsf@gitster.mtv.corp.google.com","threadId":"47255","inReplyTo":"20171122223613.GA1405@sigill","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-23T02:35:21Z","receivedAt":"2017-11-23T02:35:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So there are four cases we care about for this call in fetch:\n>\n> 1. We fed a real sha1 and got a commit (or peeled to one).\n>\n> 2. We fed a real sha1 which resolved to a non-commit, and we got NULL.\n>\n> 3. We fed a real sha1 and the object was missing or corrupted, and we\n> got NULL.\n>\n> 4. We fed a null sha1 and got NULL.\n>\n> Right now we lump cases 2-4 together as \"do not do a fast-forward\n> check\". That's fine for 2 and 4, but probably not for 3. We can easily\n> catch case 4 ourselves (if we care to), but distinguishing case 3 from\n> the others is hard. How should lookup_commit_reference_gently() signal\n> it to us?\n\nNot limiting us to the caller in the \"fetch\" codepath, I think the\nexpectation by callers of lookup_commit_reference_gently() in the\nideal world would be:\n\n - It has an object name, and wants to use it as point in the commit\n DAG to define the traversal over the DAG, if it refers to a\n commit known to us.\n\n - It does not know if these object names represent a tag object, a\n commit object, or some other object. It does not know if the\n local repository actually has them (e.g. we received a \"have\"\n from the other side---missing is expected).\n\n - Hence, it would happily accept a NULL as \"we do not have it\" and\n \"we do have it, but it is not a commit-ish\".\n\nAnd from that point of view, 2, 3a (missing), and 4 (0{40}) to yield\nNULL is perfectly fine. 3b (exists but broken) may be a noteworthy\nevent, but for the purpose of the caller, it may want to proceed as\nif the object is missing from our end, so it might deserve warning()\nbut not die(), at least as the default behaviour.\n"},{"id":"333369","messageId":"xmqqlgixra22.fsf@gitster.mtv.corp.google.com","threadId":"47255","inReplyTo":"20171122223817.GB1405@sigill","subject":"Re: [PATCH 1/5] p5550: factor our nonsense-pack creation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-23T02:41:25Z","receivedAt":"2017-11-23T02:41:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Right, I came to the same conclusion (we may even have discussed this on\n> the list, I don't remember). The current \"todo\" format says that only\n> the command and sha1 matter, and we'd be changing that. Maybe that's not\n> so bad if the user has to enable the feature themselves (and clearly it\n> would be incompatible with a custom format option).\n\nIf you are in the habit of always writing 4 or more lines, the\nchance that you would make a typo on the line you can correct in the\n\"todo\" list with such a feature is at most 25% ;-).\n\nI think one downside is that a mere presence of such an option hints\nthat we somehow encourage people to commit with a title-only message.\n"},{"id":"333373","messageId":"20171123050214.GB22218@sigill","threadId":"47255","inReplyTo":"xmqqlgixra22.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/5] p5550: factor our nonsense-pack creation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-23T05:02:15Z","receivedAt":"2017-11-23T05:02:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 23, 2017 at 11:41:25AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Right, I came to the same conclusion (we may even have discussed this on\n> > the list, I don't remember). The current \"todo\" format says that only\n> > the command and sha1 matter, and we'd be changing that. Maybe that's not\n> > so bad if the user has to enable the feature themselves (and clearly it\n> > would be incompatible with a custom format option).\n> \n> If you are in the habit of always writing 4 or more lines, the\n> chance that you would make a typo on the line you can correct in the\n> \"todo\" list with such a feature is at most 25% ;-).\n\nYou'd think, but the distribution of typos doesn't seem to be uniform.\n:)\n\nI think what actually happens is that I quite often do my final\nproofread in my MUA, where the subject is displayed apart from the rest\nof the message. So I tend to gloss over it, and any errors there are\nmore likely to make it to the list.\n\n> I think one downside is that a mere presence of such an option hints\n> that we somehow encourage people to commit with a title-only message.\n\nYeah, though it might be handy for me I think it just opens up too many\nfunny questions like this.\n\n-Peff\n"},{"id":"333447","messageId":"20171124173201.GA29190@sigill","threadId":"47255","inReplyTo":"xmqqpo89rac6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-24T17:32:01Z","receivedAt":"2017-11-24T17:32:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 23, 2017 at 11:35:21AM +0900, Junio C Hamano wrote:\n\n> Not limiting us to the caller in the \"fetch\" codepath, I think the\n> expectation by callers of lookup_commit_reference_gently() in the\n> ideal world would be:\n> \n> - It has an object name, and wants to use it as point in the commit\n> DAG to define the traversal over the DAG, if it refers to a\n> commit known to us.\n> \n> - It does not know if these object names represent a tag object, a\n> commit object, or some other object. It does not know if the\n> local repository actually has them (e.g. we received a \"have\"\n> from the other side---missing is expected).\n> \n> - Hence, it would happily accept a NULL as \"we do not have it\" and\n> \"we do have it, but it is not a commit-ish\".\n> \n> And from that point of view, 2, 3a (missing), and 4 (0{40}) to yield\n> NULL is perfectly fine. 3b (exists but broken) may be a noteworthy\n> event, but for the purpose of the caller, it may want to proceed as\n> if the object is missing from our end, so it might deserve warning()\n> but not die(), at least as the default behaviour.\n\nHmm, yeah, I did not differentiate 3a and 3b in my analysis. How we'd\nwant to handle \"missing\" would vary from caller to caller, I think.\nCertainly for this case in \"fetch\" where it's the \"old\" value for a ref,\nit would be a corruption not to have it. Just grepping around a few of\nthe other callers, I see:\n\n - archive.c:parse_treeish_arg: fine not to have it (we'd complain soon\n after that it doesn't point to a tree either). But also fine to\n complain hard.\n\n - blame.c:dwim_reverse_initial, and checkout_paths and switch_branches\n in checkout.c: missing object would mean a broken HEAD\n\n - show-branch.c:append_ref: missing would mean a broken ref\n\n - clear_commit_marks_for_object_array: conceptually OK to have a\n missing object, though I suspect practically impossible since we\n examined and marked the objects earlier\n\n - ref-filter's --merged, --contains, etc: the low-level code quietly\n ignores a missing object or non-commit, but the command-line parser\n enforces that we find a commit. So probably impossible to trigger,\n but arguably the second call should be a BUG().\n\nSo I dunno. That is far from exhaustive, but it does seem like most\ncases should assume the presence of the file.\n\nAs for your proposed behavior:\n\n> And from that point of view, 2, 3a (missing), and 4 (0{40}) to yield\n> NULL is perfectly fine. 3b (exists but broken) may be a noteworthy\n> event, but for the purpose of the caller, it may want to proceed as\n> if the object is missing from our end, so it might deserve warning()\n> but not die(), at least as the default behaviour.\n\nThat's actually not far from what happens now, with the only difference\nbeing that we _do_ actually die() on a corruption (really any error\nexcept ENOENT). I forgot that when I wrote my earlier message. You can\nsee it by updating the \"fetch\" reproduction I sent earlier to corrupt:\n\n-- >8 --\ngit init parent\ngit -C parent commit --allow-empty -m base\n\ngit clone parent child\ngit -C parent commit --allow-empty -m more\n\ncd child\nfor i in .git/objects/??/*\ndo\n\tchmod +w $i\n\techo corrupt >$i\ndone\ngit fetch\n-- 8< --\n\nwhich gives something like:\n\nerror: inflate: data stream error (incorrect header check)\nerror: unable to unpack 55c66a401fd4190382f9cd8b70c11f9f5adb044e header\nfatal: loose object 55c66a401fd4190382f9cd8b70c11f9f5adb044e (stored in .git/objects/55/c66a401fd4190382f9cd8b70c11f9f5adb044e) is corrupt\n\nSo that's good. I do still think that many of the callers of\nlookup_commit_reference_gently() probably ought to die() in the\n\"missing\" case rather than continue (and produce subtly wrong answers).\nBut it may not be that big a deal. For the most part, all bets are off\nin a corrupt repo. My main concern is just that we do not want the\ncorruption to spread or to make it harder for us to recover from (and\nthat includes allowing us to delete or overwrite other data that would\notherwise be forbidden, which is what's happening in the fetch case).\nMost of the callers probably don't fall into that situation, but it\nmight be nice to err on the side of caution.\n\n-Peff\n"},{"id":"333469","messageId":"xmqqh8tjkpsl.fsf@gitster.mtv.corp.google.com","threadId":"47255","inReplyTo":"20171124173201.GA29190@sigill","subject":"Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-25T03:20:10Z","receivedAt":"2017-11-25T03:20:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I dunno. That is far from exhaustive, but it does seem like most\n> cases should assume the presence of the file.\n\nYou are right. I should have considered that \"we got a random\nobject-name looking thing and we do not care if it does not exist\"\nis a very narrow minority case. Most of the object names we deal\nwith come from local repository traversal and unless things like the\n\"fsck-promisor\" comes into the picture, we should always have them\navailable locally.\n\n> But it may not be that big a deal. For the most part, all bets are off\n> in a corrupt repo. My main concern is just that we do not want the\n> corruption to spread or to make it harder for us to recover from (and\n> that includes allowing us to delete or overwrite other data that would\n> otherwise be forbidden, which is what's happening in the fetch case).\n\nAbsolutely.\n\nThanks.\n"}]}