{"thread":{"id":"50726","subject":"[PATCH 0/4] get_oid: cope with a possibly stale loose object cache","startedAt":"2019-03-13T10:16:35Z","lastAt":"2019-03-14T19:55:46Z","messageCount":32,"participants":["Johannes Schindelin via GitGitGadget","Jeff King","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"371341","messageId":"pull.161.git.gitgitgadget@gmail.com","threadId":"50726","inReplyTo":null,"subject":"[PATCH 0/4] get_oid: cope with a possibly stale loose object cache","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-13T10:16:30Z","receivedAt":"2019-03-13T10:16:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Being rather a power user of the interactive rebase, I discovered recently\nthat one of my scripts ran into an odd problem where an interactive rebase\nappended new commands to the todo list, and the interactive rebase failed to\nvalidate the (short) OIDs. But when I tried to fix things via git rebase\n--edit-todo, the interactive rebase had no longer any problem to validate\nthem.\n\nTurns out that I generated the objects during the interactive rebase (yes, I\nimplemented a 2-phase rebase\n[https://github.com/git-for-windows/build-extra/blob/master/ever-green.sh]),\nand that their hashes happened to share the first two hex digits with some\nloose object that had already been read in the same interactive rebase run,\ncausing a now-stale loose object cache to be initialized for that \n.git/objects/?? subdirectory.\n\nIt was quite the, let's say, adventure, to track that one down.\n\nOver at the companion PR for Git for Windows\n[https://github.com/git-for-windows/git/pull/2121], I discussed this with\nPeff (who introduced the loose object cache), and he pointed out that my\noriginal solution was a bit too specific for the interactive rebase. He\nsuggested the current method of re-reading upon a missing object instead,\nand explained patiently that this does not affect the code path for which\nthe loose object cache was introduced originally: to help with performance\nissues on NFS when Git tries to find the same missing objects over and over\nagain.\n\nThe regression test still reflects what I need this fix for, and I would\nrather keep it as-is (even if the fix is not about the interactive rebase\nper se), as it really tests for the functionality that I really need to\ncontinue to work.\n\nMy only concern is that this might cause some performance regressions that\nneither Peff nor I could think of, where get_oid() may run repeatedly into\nmissing objects by design, and where we should not blow away and recreate\nthe loose object cache all the time.\n\nDoes anybody share this concern? Or even better: can anybody think of such a\nscenario?\n\nJohannes Schindelin (4):\n rebase -i: demonstrate obscure loose object cache bug\n sequencer: improve error message when an OID could not be parsed\n sequencer: move stale comment into correct location\n get_oid(): when an object was not found, try harder\n\n sequencer.c | 5 +++--\n sha1-name.c | 12 ++++++++++++\n t/t3429-rebase-edit-todo.sh | 22 ++++++++++++++++++++++\n 3 files changed, 37 insertions(+), 2 deletions(-)\n\n\nbase-commit: e902e9bcae2010bc42648c80ab6adc6c5a16a4a5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-161%2Fdscho%2Frereading-todo-list-and-loose-object-cache-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-161/dscho/rereading-todo-list-and-loose-object-cache-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/161\n-- \ngitgitgadget\n"},{"id":"371342","messageId":"b3fcd377652103584b6f307c6ee209980b44529f.1552472189.git.gitgitgadget@gmail.com","threadId":"50726","inReplyTo":"pull.161.git.gitgitgadget@gmail.com","subject":"[PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-13T10:16:31Z","receivedAt":"2019-03-13T10:16:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe specifically support `exec` commands in `git rebase -i`'s todo lists\nto rewrite the very same todo list. Of course, we need to validate that\ntodo list when re-reading it.\n\nIt is also totally legitimate to extend the todo list by `pick` lines\nusing short names of commits that were created only after the rebase\nstarted.\n\nAnd this is where the loose object cache interferes with this feature:\nif *some* loose object was read whose hash shares the same first two\ndigits with a commit that was not yet created when that loose object was\ncreated, then we fail to find that new commit by its short name in\n`get_oid()`, and the interactive rebase fails with an obscure error\nmessage like:\n\n\terror: invalid line 1: pick 6568fef\n\terror: please fix this using 'git rebase --edit-todo'.\n\nLet's first demonstrate that this is actually a bug in a new regression\ntest, in a separate commit so that other developers who do not believe\nme can cherry-pick it to confirm the problem.\n\nThis new regression test generates two commits whose hashes share the\nfirst two hex digits (so that their corresponding loose objects live in\nthe same subdirectory of .git/objects/, and are therefore supposed to be\nin the same loose object cache bin).\n\nIt then picks the first, to make sure that the loose object cache is\ninitialized and cached that object directory, then generates the second\ncommit and picks it, too. Since the commit was generated in a different\nprocess than the sequencer that wants to pick it, the loose object cache\nhad no chance of being updated in the meantime.\n\nTechnically, we would need only one `exec` command in this regression\ntest case, but for ease of implementation, it uses a pseudo-recursive\ncall to the same script.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3429-rebase-edit-todo.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh\nindex b9292dfc2a..862f229c87 100755\n--- a/t/t3429-rebase-edit-todo.sh\n+++ b/t/t3429-rebase-edit-todo.sh\n@@ -11,4 +11,26 @@ test_expect_success 'rebase exec modifies rebase-todo' '\n \ttest -e F\n '\n \n+test_expect_failure SHA1 'loose object cache vs re-reading todo list' '\n+\tGIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&\n+\texport GIT_REBASE_TODO &&\n+\twrite_script append-todo.sh <<-\\EOS &&\n+\t# For values 5 and 6, this yields SHA-1s with the same first two digits\n+\techo \"pick $(git rev-parse --short \\\n+\t\t$(printf \"%s\\\\n\" \\\n+\t\t\t\"tree $EMPTY_TREE\" \\\n+\t\t\t\"author A U Thor $1 +0000\" \\\n+\t\t\t\"committer A U Thor $1 +0000\" \\\n+\t\t\t\"\" \\\n+\t\t\t\"$1\" |\n+\t\t git hash-object -t commit -w --stdin))\" >>$GIT_REBASE_TODO\n+\n+\tshift\n+\ttest -z \"$*\" ||\n+\techo \"exec $0 $*\" >>$GIT_REBASE_TODO\n+\tEOS\n+\n+\tgit rebase HEAD -x \"./append-todo.sh 5 6\"\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"371343","messageId":"d8c4a3dde5bed492bcdb6399fd8a73ebf4798320.1552472189.git.gitgitgadget@gmail.com","threadId":"50726","inReplyTo":"pull.161.git.gitgitgadget@gmail.com","subject":"[PATCH 2/4] sequencer: improve error message when an OID could not be parsed","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-13T10:16:32Z","receivedAt":"2019-03-13T10:16:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe interactive rebase simply complains about an \"invalid line\" when the\nobject hash of, say, a `pick` line could not be parsed.\n\nLet's tell the user what happened in a little more detail.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 95dda23eee..f91062718d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2137,7 +2137,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \titem->arg_len = (int)(eol - item->arg);\n \n \tif (status < 0)\n-\t\treturn -1;\n+\t\treturn error(_(\"could not parse '%.*s'\"),\n+\t\t\t (int)(end_of_object_name - bol), bol);\n \n \titem->commit = lookup_commit_reference(r, &commit_oid);\n \treturn !item->commit;\n-- \ngitgitgadget\n\n"},{"id":"371344","messageId":"994446236d05d9d014e12a5102bcf9be222e3b57.1552472189.git.gitgitgadget@gmail.com","threadId":"50726","inReplyTo":"pull.161.git.gitgitgadget@gmail.com","subject":"[PATCH 4/4] get_oid(): when an object was not found, try harder","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-13T10:16:34Z","receivedAt":"2019-03-13T10:16:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt is quite possible that the loose object cache gets stale when new\nobjects are written. In that case, get_oid() would potentially say that\nit cannot find a given object, even if it should find it.\n\nLet's blow away the loose object cache as well as the read packs and try\nagain in that case.\n\nNote: this does *not* affect the code path that was introduced to help\navoid looking for the same non-existing objects (which made some\noperations really expensive via NFS): that code path is handled by the\n`OBJECT_INFO_QUICK` flag (which does not even apply to `get_oid()`,\nwhich has no equivalent flag, at least at the time this patch was\nwritten).\n\nThis incidentally fixes the problem identified earlier where an\ninteractive rebase wanted to re-read (and validate) the todo list after\nan `exec` command modified it.\n\nHelped-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n sha1-name.c | 12 ++++++++++++\n t/t3429-rebase-edit-todo.sh | 2 +-\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 6dda2c16df..cfe5c874b6 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -442,6 +442,18 @@ static enum get_oid_result get_short_oid(const char *name, int len,\n \tfind_short_packed_object(&ds);\n \tstatus = finish_object_disambiguation(&ds, oid);\n \n+\t/*\n+\t * If we didn't find it, do the usual reprepare() slow-path,\n+\t * since the object may have recently been added to the repository\n+\t * or migrated from loose to packed.\n+\t */\n+\tif (status == MISSING_OBJECT) {\n+\t\treprepare_packed_git(the_repository);\n+\t\tfind_short_object_filename(&ds);\n+\t\tfind_short_packed_object(&ds);\n+\t\tstatus = finish_object_disambiguation(&ds, oid);\n+\t}\n+\n \tif (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {\n \t\tstruct oid_array collect = OID_ARRAY_INIT;\n \ndiff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh\nindex 862f229c87..76f6d306ea 100755\n--- a/t/t3429-rebase-edit-todo.sh\n+++ b/t/t3429-rebase-edit-todo.sh\n@@ -11,7 +11,7 @@ test_expect_success 'rebase exec modifies rebase-todo' '\n \ttest -e F\n '\n \n-test_expect_failure SHA1 'loose object cache vs re-reading todo list' '\n+test_expect_success SHA1 'loose object cache vs re-reading todo list' '\n \tGIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&\n \texport GIT_REBASE_TODO &&\n \twrite_script append-todo.sh <<-\\EOS &&\n-- \ngitgitgadget\n"},{"id":"371345","messageId":"d749c63170ca5f23f80f8f98816188b237eb648b.1552472189.git.gitgitgadget@gmail.com","threadId":"50726","inReplyTo":"pull.161.git.gitgitgadget@gmail.com","subject":"[PATCH 3/4] sequencer: move stale comment into correct location","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-13T10:16:33Z","receivedAt":"2019-03-13T10:16:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f91062718d..79a046d748 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3641,7 +3641,6 @@ static int pick_commits(struct repository *r,\n \t\t\tres = do_exec(r, item->arg);\n \t\t\t*end_of_arg = saved;\n \n-\t\t\t/* Reread the todo file if it has changed. */\n \t\t\tif (res) {\n \t\t\t\tif (opts->reschedule_failed_exec)\n \t\t\t\t\treschedule = 1;\n@@ -3649,6 +3648,7 @@ static int pick_commits(struct repository *r,\n \t\t\t\tres = error_errno(_(\"could not stat '%s'\"),\n \t\t\t\t\t\t get_todo_path(opts));\n \t\t\telse if (match_stat_data(&todo_list->stat, &st)) {\n+\t\t\t\t/* Reread the todo file if it has changed. */\n \t\t\t\ttodo_list_release(todo_list);\n \t\t\t\tif (read_populate_todo(r, todo_list, opts))\n \t\t\t\t\tres = -1; /* message was printed */\n-- \ngitgitgadget\n\n"},{"id":"371371","messageId":"20190313153317.GB24101@sigill.intra.peff.net","threadId":"50726","inReplyTo":"pull.161.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] get_oid: cope with a possibly stale loose object cache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-13T15:33:17Z","receivedAt":"2019-03-13T15:34:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 03:16:30AM -0700, Johannes Schindelin via GitGitGadget wrote:\n\n> Over at the companion PR for Git for Windows\n> [https://github.com/git-for-windows/git/pull/2121], I discussed this with\n> Peff (who introduced the loose object cache), and he pointed out that my\n> original solution was a bit too specific for the interactive rebase. He\n> suggested the current method of re-reading upon a missing object instead,\n> and explained patiently that this does not affect the code path for which\n> the loose object cache was introduced originally: to help with performance\n> issues on NFS when Git tries to find the same missing objects over and over\n> again.\n> \n> The regression test still reflects what I need this fix for, and I would\n> rather keep it as-is (even if the fix is not about the interactive rebase\n> per se), as it really tests for the functionality that I really need to\n> continue to work.\n\nThe case you found is the only way I can think of to trigger this\ndeterministically. So I think it is quite a good test case. :)\n\nJust to rehash a bit from that PR: I believe that this bug has been\npresent in the way the test checks for it since we started caching in\nthe sha1-abbreviation code in cc817ca3ef (sha1_name: cache readdir(3)\nresults in find_short_object_filename(), 2017-06-22).\n\nBut the more fundamental issue, that we do not do the usual\nreprepare_packed_git() thing upon failing to find an object, goes back\nforever. It's just much less common to hit that race than the one with\nnormal object access, because we tend to resolve objects quickly at the\nbeginning of a program and then spend a long time walking the graph. But\nit is still possible, especially in a long-running program that resolves\nnames not just at the beginning. Like rebase.\n\n Actually, I guess one could probably construct a similar case with\n a long-running \"cat-file --batch\", feeding it one name, then doing a\n complete repack, and then feeding it another name. I guess I could\n think of another case. ;)\n\n> My only concern is that this might cause some performance regressions that\n> neither Peff nor I could think of, where get_oid() may run repeatedly into\n> missing objects by design, and where we should not blow away and recreate\n> the loose object cache all the time.\n\nThis only affects get_oid_short(), which is already pretty expensive,\nbecause it has to disambiguate them. The usual path we care about being\nfast is the 40-hex sha1 parser, which should be able to do its job\nwithout doing anything but parsing the string. We've already run into\nspeed issues there with the object/refname ambiguity checks, and callers\ncan disable that with a flag.\n\nSo I'd say:\n\n - if you want to think about callers which might be sensitive to this\n change, the ones setting warn_on_object_refname_ambiguity might be\n good candidates.\n\n - anything except 40-hex sha1 is already pretty expensive (ref\n lookups, disambiguation, etc), so as long as we're not touching that\n path, I'm not incredibly worried.\n\n I did wonder for a minute whether the 40-hex sha1 resolution would\n want this similar reprepare_packed_git() handling. But it doesn't\n verify the object at all, and just passes back the oid whether it\n exists or not (and it's up to the caller to then use\n OBJECT_INFO_QUICK to access the object data if it chooses).\n\nThat's my opinion, of course. I think your intent was to solicit other\nthoughts, so I'd be curious to hear if anybody disagrees. :)\n\n> Johannes Schindelin (4):\n> rebase -i: demonstrate obscure loose object cache bug\n> sequencer: improve error message when an OID could not be parsed\n> sequencer: move stale comment into correct location\n> get_oid(): when an object was not found, try harder\n\nThe patches themselves look good to me. Thanks for pushing our\ndiscussion forward.\n\n-Peff\n"},{"id":"371375","messageId":"87k1h2bvpb.fsf@evledraar.gmail.com","threadId":"50726","inReplyTo":"b3fcd377652103584b6f307c6ee209980b44529f.1552472189.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-13T16:11:44Z","receivedAt":"2019-03-13T16:11:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 13 2019, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> We specifically support `exec` commands in `git rebase -i`'s todo lists\n> to rewrite the very same todo list. Of course, we need to validate that\n> todo list when re-reading it.\n>\n> It is also totally legitimate to extend the todo list by `pick` lines\n> using short names of commits that were created only after the rebase\n> started.\n>\n> And this is where the loose object cache interferes with this feature:\n> if *some* loose object was read whose hash shares the same first two\n> digits with a commit that was not yet created when that loose object was\n> created, then we fail to find that new commit by its short name in\n> `get_oid()`, and the interactive rebase fails with an obscure error\n> message like:\n>\n> \terror: invalid line 1: pick 6568fef\n> \terror: please fix this using 'git rebase --edit-todo'.\n\nAs a further improvement, is there a good reason for why we wouldn't\npass something down to the oid machinery to say \"we're only interested\nin commits\". I have a WIP series somewhere to generalize that more, but\ne.g. here locally:\n\n $ git rev-parse 80b06\n error: short SHA1 80b06 is ambiguous\n hint: The candidates are:\n hint: 80b06b942e commit 2019-03-13 - Revert \"this patch fail whales\"\n hint: 80b063bb9b blob\n hint: 80b06f0714 blob\n 80b06\n $ git rev-parse 80b06^{commit}\n 80b06b942ed33e597a0152b3e6ba45b7d8ead94b\n\nI can't remember if there's some caveat with that particular peel syntax\nI meant to fix, but I mean if we could pass something down to say \"no\nblobs or trees\" shouldn't we?\n\nThen stuff like this wouldn't die:\n \n $ git rebase -i\n hint: Waiting for your editor to close the file... Waiting for Emacs...\n error: short SHA1 80b06 is ambiguous\n hint: The candidates are:\n hint: 80b06b942e commit 2019-03-13 - Revert \"this patch fail whales\"\n hint: 80b063bb9b blob\n hint: 80b06f0714 blob\n error: invalid line 2: p 80b06 Revert \"this patch fail whales\"\n You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.\n\n> [...]\n> +test_expect_failure SHA1 'loose object cache vs re-reading todo list' '\n> +\tGIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&\n> +\texport GIT_REBASE_TODO &&\n> +\twrite_script append-todo.sh <<-\\EOS &&\n> +\t# For values 5 and 6, this yields SHA-1s with the same first two digits\n> +\techo \"pick $(git rev-parse --short \\\n> +\t\t$(printf \"%s\\\\n\" \\\n> +\t\t\t\"tree $EMPTY_TREE\" \\\n> +\t\t\t\"author A U Thor $1 +0000\" \\\n> +\t\t\t\"committer A U Thor $1 +0000\" \\\n> +\t\t\t\"\" \\\n> +\t\t\t\"$1\" |\n> +\t\t git hash-object -t commit -w --stdin))\" >>$GIT_REBASE_TODO\n> +\n> +\tshift\n> +\ttest -z \"$*\" ||\n> +\techo \"exec $0 $*\" >>$GIT_REBASE_TODO\n> +\tEOS\n> +\n> +\tgit rebase HEAD -x \"./append-todo.sh 5 6\"\n> +'\n> +\n> test_done\n\nThis is a test for rebase, but perhaps it would be best put in\nt1512-rev-parse-disambiguation.sh. Then when we finally port that\nsomehow to SHA256 we'll have all this SHA-1 golfing in the same file &\ncan fix it at once. Just a thought...\n"},{"id":"371377","messageId":"20190313163516.GA26045@sigill.intra.peff.net","threadId":"50726","inReplyTo":"87k1h2bvpb.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-13T16:35:16Z","receivedAt":"2019-03-13T16:36:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 05:11:44PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > And this is where the loose object cache interferes with this feature:\n> > if *some* loose object was read whose hash shares the same first two\n> > digits with a commit that was not yet created when that loose object was\n> > created, then we fail to find that new commit by its short name in\n> > `get_oid()`, and the interactive rebase fails with an obscure error\n> > message like:\n> >\n> > \terror: invalid line 1: pick 6568fef\n> > \terror: please fix this using 'git rebase --edit-todo'.\n\nAre we 100% sure this part is necessary? From my understanding of the\nproblem, even without any ambiguity get_oid() could fail due to just\nplain not finding the object in question.\n\n> As a further improvement, is there a good reason for why we wouldn't\n> pass something down to the oid machinery to say \"we're only interested\n> in commits\". I have a WIP series somewhere to generalize that more, but\n> e.g. here locally:\n\nWe have get_oid_commit() and get_oid_committish() already. Should rebase\njust be using those? (I think we probably want \"commit()\", because we do\nnot expect a \"pick\" line to have a tag, for example.\n\n-Peff\n"},{"id":"371378","messageId":"20190313165320.GA717@sigill.intra.peff.net","threadId":"50726","inReplyTo":"20190313163516.GA26045@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-13T16:53:21Z","receivedAt":"2019-03-13T16:54:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 12:35:16PM -0400, Jeff King wrote:\n\n> On Wed, Mar 13, 2019 at 05:11:44PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> > > And this is where the loose object cache interferes with this feature:\n> > > if *some* loose object was read whose hash shares the same first two\n> > > digits with a commit that was not yet created when that loose object was\n> > > created, then we fail to find that new commit by its short name in\n> > > `get_oid()`, and the interactive rebase fails with an obscure error\n> > > message like:\n> > >\n> > > \terror: invalid line 1: pick 6568fef\n> > > \terror: please fix this using 'git rebase --edit-todo'.\n> \n> Are we 100% sure this part is necessary? From my understanding of the\n> problem, even without any ambiguity get_oid() could fail due to just\n> plain not finding the object in question.\n\nSorry, I was being dumb. We do need a two-digit collision, not because\nwe need an ambiguity, but because the loose-object cache is filled in\none directory at a time. So we must be unlucky enough to hit the same\ndirectory twice, and using these objects ensures that unluckiness. And\nthe commit message does describe this.\n\nIf we didn't want to depend on a particular hash, we could simply do it\nN times, where N is enough to get us any two entries which collide in\nthe first byte. By the birthday paradox, that's 50% at only 2^4. But 50%\nis not very good odds for the test working. You'd need 257 iterations to\nensure a collision by the pigeon-hole principle. That's enough that I\nkind of prefer the found collision here, even if we'll have to update it\nfor a new hash (it is correctly marked with SHA1, so I don't think\nfinding it later will be a problem).\n\nBy the way, while reading the test more carefully, I did notice two\nfunny things:\n\n> +test_expect_failure SHA1 'loose object cache vs re-reading todo list' '\n> +\tGIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&\n> +\texport GIT_REBASE_TODO &&\n> +\twrite_script append-todo.sh <<-\\EOS &&\n> +\t# For values 5 and 6, this yields SHA-1s with the same first two digits\n> +\techo \"pick $(git rev-parse --short \\\n> +\t\t$(printf \"%s\\\\n\" \\\n> +\t\t\t\"tree $EMPTY_TREE\" \\\n> +\t\t\t\"author A U Thor $1 +0000\" \\\n> +\t\t\t\"committer A U Thor $1 +0000\" \\\n> +\t\t\t\"\" \\\n> +\t\t\t\"$1\" |\n> +\t\t git hash-object -t commit -w --stdin))\" >>$GIT_REBASE_TODO\n\nHere we redirect the output into $GIT_REBASE_TODO, not stdout.\n\n> +\tshift\n> +\ttest -z \"$*\" ||\n> +\techo \"exec $0 $*\" >>$GIT_REBASE_TODO\n\nAnd here we do the same thing. That second redirection is unnecessary.\n\nI also find it interesting that it iterates over its arguments by\nrecursive processes. Wouldn't:\n\n for i in \"$@\"; do\n\techo \"pick ...\" >>$GIT_REBASE_TODO\n done\n\nbe a bit more efficient (as well as more obvious?).\n\n-Peff\n"},{"id":"371409","messageId":"nycvar.QRO.7.76.6.1903132322420.41@tvgsbejvaqbjf.bet","threadId":"50726","inReplyTo":"20190313163516.GA26045@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-13T22:27:02Z","receivedAt":"2019-03-13T22:27:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar & Peff,\n\nOn Wed, 13 Mar 2019, Jeff King wrote:\n\n> On Wed, Mar 13, 2019 at 05:11:44PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> > > And this is where the loose object cache interferes with this\n> > > feature: if *some* loose object was read whose hash shares the same\n> > > first two digits with a commit that was not yet created when that\n> > > loose object was created, then we fail to find that new commit by\n> > > its short name in `get_oid()`, and the interactive rebase fails with\n> > > an obscure error message like:\n> > >\n> > > \terror: invalid line 1: pick 6568fef\n> > > \terror: please fix this using 'git rebase --edit-todo'.\n> \n> Are we 100% sure this part is necessary? From my understanding of the\n> problem, even without any ambiguity get_oid() could fail due to just\n> plain not finding the object in question.\n\nIndeed. It could be a typo, for example. Which is why that error message\nis so helpful.\n\n> > As a further improvement, is there a good reason for why we wouldn't\n> > pass something down to the oid machinery to say \"we're only interested\n> > in commits\". I have a WIP series somewhere to generalize that more, but\n> > e.g. here locally:\n> \n> We have get_oid_commit() and get_oid_committish() already. Should rebase\n> just be using those? (I think we probably want \"commit()\", because we do\n> not expect a \"pick\" line to have a tag, for example.\n\nI did think about this while developing this patch series, and decided\nagainst conflating concerns.\n\nAnd I was totally right to do so! Because I do have an internal ticket\nthat talks about allowing `reset v2.20.1`, which is a tag, not a commit.\n\nGranted, it is easy to work around: just use `reset v2.20.1^0`, but it is\nquite annoying that we do not allow this at the moment: even if we do\nallow `get_oid()` to resolve the tag, we don't peel it to the commit.\n\n#leftoverbits\n\nCiao,\nDscho"},{"id":"371410","messageId":"nycvar.QRO.7.76.6.1903132327500.41@tvgsbejvaqbjf.bet","threadId":"50726","inReplyTo":"20190313165320.GA717@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-13T22:40:54Z","receivedAt":"2019-03-13T22:41:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 13 Mar 2019, Jeff King wrote:\n\n> \n> By the way, while reading the test more carefully, I did notice two\n> funny things:\n> \n> > +test_expect_failure SHA1 'loose object cache vs re-reading todo list' '\n> > +\tGIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&\n> > +\texport GIT_REBASE_TODO &&\n> > +\twrite_script append-todo.sh <<-\\EOS &&\n> > +\t# For values 5 and 6, this yields SHA-1s with the same first two digits\n> > +\techo \"pick $(git rev-parse --short \\\n> > +\t\t$(printf \"%s\\\\n\" \\\n> > +\t\t\t\"tree $EMPTY_TREE\" \\\n> > +\t\t\t\"author A U Thor $1 +0000\" \\\n> > +\t\t\t\"committer A U Thor $1 +0000\" \\\n> > +\t\t\t\"\" \\\n> > +\t\t\t\"$1\" |\n> > +\t\t git hash-object -t commit -w --stdin))\" >>$GIT_REBASE_TODO\n> \n> Here we redirect the output into $GIT_REBASE_TODO, not stdout.\n\nIndeed, because we want to append a `pick` command to the todo list.\n\n> > +\tshift\n> > +\ttest -z \"$*\" ||\n> > +\techo \"exec $0 $*\" >>$GIT_REBASE_TODO\n> \n> And here we do the same thing. That second redirection is unnecessary.\n\nIt is actually not unnecessary, but to the contrary quite necessary to\nachieve the intended effect: with this command, we append an `exec` line\nto the todo list that is guaranteed to be executed after the `pick`\ncommand that we added earlier.\n\n> I also find it interesting that it iterates over its arguments by\n> recursive processes. Wouldn't:\n> \n> for i in \"$@\"; do\n> \techo \"pick ...\" >>$GIT_REBASE_TODO\n> done\n> \n> be a bit more efficient (as well as more obvious?).\n\nIt would be more efficient, but it would also fail to test for the\nregression.\n\nRemember: it is absolutely crucial for the regression test that the parent\nprocess' loose object cache already has been initialized *before* the new\ncommit is created and then picked. Otherwise the cache would contain that\ncommit object already. The whole point of the regression test is that the\ncache does *not* contain that object.\n\nThe only way we can guarantee that order in this test is if the first\ncommit is created and picked *before* we `exec` to create the second\ncommit and then append the `pick` line for that one.\n\nNow, I could have tried to play some fake editor games because it is not\nstrictly necessary to create the first commit via an `exec` line. Instead,\nI could have generated it before the rebase, and then initialized the todo\nlist with the `pick` of the first commit and then an `exec` of the script\nthat creates the second commit and then appends a `pick` line for that.\n\nBut the reality is that this would have resulted in more code! And not\neven easier-to-read code at that! (I know, because one of my unsent\niterations did exactly that.)\n\nSo instead, I opted for using the `-x` option to modify the initial todo\nlist to begin with (it consists of a single `noop`, obviously). This will\nadd that `exec` line that calls the script that creates the first commit\nand appends the `pick` line.\n\nIt *also* adds an `exec` line to guarantee that the second commit is\ncreated, and a `pick` line for it is appended to the todo list, *after*\nthe sequencer initalized the loose object cache by virtue of picking the\nfirst commit.\n\nSo yes, it is crucial that the `append-todo.sh` script is `exec`ed\n*twice*. Otherwise the first `pick` would initialize the loose object\ncache *after* the second commit was created, which Just Works, even\nwithout this here patch series.\n\nCiao,\nDscho\n"},{"id":"371428","messageId":"20190314000511.GA31968@sigill.intra.peff.net","threadId":"50726","inReplyTo":"nycvar.QRO.7.76.6.1903132322420.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T00:05:12Z","receivedAt":"2019-03-14T00:06:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 11:27:02PM +0100, Johannes Schindelin wrote:\n\n> > We have get_oid_commit() and get_oid_committish() already. Should rebase\n> > just be using those? (I think we probably want \"commit()\", because we do\n> > not expect a \"pick\" line to have a tag, for example.\n> \n> I did think about this while developing this patch series, and decided\n> against conflating concerns.\n\nYeah, I'd definitely agree it is a separate topic.\n\n> And I was totally right to do so! Because I do have an internal ticket\n> that talks about allowing `reset v2.20.1`, which is a tag, not a commit.\n\nThanks for that example. I agree it's plausible for somebody to be using\na tag there, so committish() is probably the better option.\n\n(It also doesn't really hurt disambiguation much; in any given repo,\ncommits tend to be dwarfed by trees and blobs by an order of magnitude,\nand tags are even an order of magnitude smaller than commits).\n\n-Peff\n"},{"id":"371429","messageId":"20190314000759.GB31968@sigill.intra.peff.net","threadId":"50726","inReplyTo":"nycvar.QRO.7.76.6.1903132327500.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T00:07:59Z","receivedAt":"2019-03-14T00:09:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 11:40:54PM +0100, Johannes Schindelin wrote:\n\n> > > +\tshift\n> > > +\ttest -z \"$*\" ||\n> > > +\techo \"exec $0 $*\" >>$GIT_REBASE_TODO\n> > \n> > And here we do the same thing. That second redirection is unnecessary.\n> \n> It is actually not unnecessary, but to the contrary quite necessary to\n> achieve the intended effect: with this command, we append an `exec` line\n> to the todo list that is guaranteed to be executed after the `pick`\n> command that we added earlier.\n\nAh, right, I was totally misreading it. We are not execing _now_, but\nrather appending an exec. And that explains my confusion about\nrecursion.\n\nSo your nice explanation below has gone to waste, because I have just\nnow understood what is going on. ;)\n\nI agree that what you have here is probably the simplest way of\ndemonstrating the issue.\n\n-Peff\n"},{"id":"371439","messageId":"xmqqimwmcl93.fsf@gitster-ct.c.googlers.com","threadId":"50726","inReplyTo":"b3fcd377652103584b6f307c6ee209980b44529f.1552472189.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T01:12:08Z","receivedAt":"2019-03-14T01:12:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> +test_expect_failure SHA1 'loose object cache vs re-reading todo list' '\n> +\tGIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&\n> +\texport GIT_REBASE_TODO &&\n> +\twrite_script append-todo.sh <<-\\EOS &&\n> +\t# For values 5 and 6, this yields SHA-1s with the same first two digits\n> +\techo \"pick $(git rev-parse --short \\\n> +\t\t$(printf \"%s\\\\n\" \\\n> +\t\t\t\"tree $EMPTY_TREE\" \\\n> +\t\t\t\"author A U Thor $1 +0000\" \\\n> +\t\t\t\"committer A U Thor $1 +0000\" \\\n> +\t\t\t\"\" \\\n> +\t\t\t\"$1\" |\n> +\t\t git hash-object -t commit -w --stdin))\" >>$GIT_REBASE_TODO\n\nIn the generated append-todo.sh, as the <>$GIT_REBASE_TODO\" literally (not the actual pathname\nthat begins with .git, but a reference to the variable). test-lint\nmay or may not catch it, but redirecting into variable reference\nwould trigger a(n arguably misguided) warning when run with some\nversions of bash. Quoting\n\n\t\techo ... >>\"$GIT_REBASE_TODO\"\n\nwould work it around, of course.\n\n> +\tshift\n> +\ttest -z \"$*\" ||\n> +\techo \"exec $0 $*\" >>$GIT_REBASE_TODO\n\nLikewise.\n\n> +\tEOS\n> +\n> +\tgit rebase HEAD -x \"./append-todo.sh 5 6\"\n> +'\n> +\n> test_done\n"},{"id":"371440","messageId":"xmqqef7acl5c.fsf@gitster-ct.c.googlers.com","threadId":"50726","inReplyTo":"d8c4a3dde5bed492bcdb6399fd8a73ebf4798320.1552472189.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] sequencer: improve error message when an OID could not be parsed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T01:14:23Z","receivedAt":"2019-03-14T01:14:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> The interactive rebase simply complains about an \"invalid line\" when the\n> object hash of, say, a `pick` line could not be parsed.\n>\n> Let's tell the user what happened in a little more detail.\n\nMakes sense.\n\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> sequencer.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 95dda23eee..f91062718d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2137,7 +2137,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n> \titem->arg_len = (int)(eol - item->arg);\n> \n> \tif (status < 0)\n> -\t\treturn -1;\n> +\t\treturn error(_(\"could not parse '%.*s'\"),\n> +\t\t\t (int)(end_of_object_name - bol), bol);\n> \n> \titem->commit = lookup_commit_reference(r, &commit_oid);\n> \treturn !item->commit;\n"},{"id":"371441","messageId":"xmqqa7hyckfm.fsf@gitster-ct.c.googlers.com","threadId":"50726","inReplyTo":"994446236d05d9d014e12a5102bcf9be222e3b57.1552472189.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] get_oid(): when an object was not found, try harder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T01:29:49Z","receivedAt":"2019-03-14T01:29:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> @@ -442,6 +442,18 @@ static enum get_oid_result get_short_oid(const char *name, int len,\n> \tfind_short_packed_object(&ds);\n> \tstatus = finish_object_disambiguation(&ds, oid);\n> \n> +\t/*\n> +\t * If we didn't find it, do the usual reprepare() slow-path,\n> +\t * since the object may have recently been added to the repository\n> +\t * or migrated from loose to packed.\n> +\t */\n> +\tif (status == MISSING_OBJECT) {\n> +\t\treprepare_packed_git(the_repository);\n> +\t\tfind_short_object_filename(&ds);\n> +\t\tfind_short_packed_object(&ds);\n> +\t\tstatus = finish_object_disambiguation(&ds, oid);\n> +\t}\n> +\n\nThis looks obviously correct, but two things that made me wonder\nbriefly were:\n\n 1. is reprepare_packed_git() a bit too heavy-weight, if the only\n thing we are addressing is the loose-object cache going stale?\n\n 2. is there a way to cleanly avoid the three-line duplicate?\n\nMy tentative answers are (1) even if it is, but get_short_oid() is\nalready heavy-weight enough; it won't be worth restructuring the\ncode to make it possible to clear only the loose-object cache, and\n(2) a loop that runs twice when the first result is MISSING_OBJECT\nand otherwise leaves after once would need an extra variable, its\niniialization, check and increment, which is more than what we might\nsave with such a restructuring, so it won't be worth pursuing.\n\nBut others may have better ideas, as always ;-)\n"},{"id":"371444","messageId":"20190314022245.GA1414@sigill.intra.peff.net","threadId":"50726","inReplyTo":"xmqqa7hyckfm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/4] get_oid(): when an object was not found, try harder","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T02:22:46Z","receivedAt":"2019-03-14T02:24:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 10:29:49AM +0900, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > @@ -442,6 +442,18 @@ static enum get_oid_result get_short_oid(const char *name, int len,\n> > \tfind_short_packed_object(&ds);\n> > \tstatus = finish_object_disambiguation(&ds, oid);\n> > \n> > +\t/*\n> > +\t * If we didn't find it, do the usual reprepare() slow-path,\n> > +\t * since the object may have recently been added to the repository\n> > +\t * or migrated from loose to packed.\n> > +\t */\n> > +\tif (status == MISSING_OBJECT) {\n> > +\t\treprepare_packed_git(the_repository);\n> > +\t\tfind_short_object_filename(&ds);\n> > +\t\tfind_short_packed_object(&ds);\n> > +\t\tstatus = finish_object_disambiguation(&ds, oid);\n> > +\t}\n> > +\n> \n> This looks obviously correct, but two things that made me wonder\n> briefly were:\n> \n> 1. is reprepare_packed_git() a bit too heavy-weight, if the only\n> thing we are addressing is the loose-object cache going stale?\n\nIt's not the only thing we are addressing. :)\n\nTry this:\n\n-- >8 --\nmkfifo in\n(git cat-file --batch-check in\n\ngit commit --allow-empty -m one\ngit commit --allow-empty -m two\n\ngit rev-parse --short HEAD^ >&9\ngit repack -adk\ngit rev-parse --short HEAD >&9\n-- >8 --\n\nThe second object will (usually) be reported as \"missing\", even though\nby calling reprepare_packed_git(), we would then find it in the\npackfile.\n\nI say \"usually\" because if the two commits have the same first-byte to\ntheir sha1, then we'd load both into the loose cache during the first\nrequest, and use that during the second request.\n\nAfter this patch, it works consistently (we flush both the loose cache\nand the cached set of packs, and find the packed version).\n\n> 2. is there a way to cleanly avoid the three-line duplicate?\n\nYeah, as you noted, I think the boilerplate is worse than the\nduplication. The most readable alternative to me is a separate function,\nlike:\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex cfe5c874b6..441666993b 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -411,6 +411,14 @@ static int sort_ambiguous(const void *a, const void *b)\n \treturn a_type_sort > b_type_sort ? 1 : -1;\n }\n \n+static enum get_oid_result(struct disambiguate_state *ds,\n+\t\t\t struct object_id *oid)\n+{\n+\tfind_short_object_filename(&ds);\n+\tfind_short_packed_object(&ds);\n+\treturn finish_object_disambiguation(&ds, oid);\n+}\n+\n static enum get_oid_result get_short_oid(const char *name, int len,\n \t\t\t\t\t struct object_id *oid,\n \t\t\t\t\t unsigned flags)\n@@ -438,20 +446,16 @@ static enum get_oid_result get_short_oid(const char *name, int len,\n \telse\n \t\tds.fn = default_disambiguate_hint;\n \n-\tfind_short_object_filename(&ds);\n-\tfind_short_packed_object(&ds);\n-\tstatus = finish_object_disambiguation(&ds, oid);\n+\tstatus = do_get_short_oid(&ds, oid);\n \n \t/*\n \t * If we didn't find it, do the usual reprepare() slow-path,\n \t * since the object may have recently been added to the repository\n \t * or migrated from loose to packed.\n \t */\n \tif (status == MISSING_OBJECT) {\n \t\treprepare_packed_git(the_repository);\n-\t\tfind_short_object_filename(&ds);\n-\t\tfind_short_packed_object(&ds);\n-\t\tstatus = finish_object_disambiguation(&ds, oid);\n+\t\tstatus = do_get_short_oid(&ds, oid);\n \t}\n \n \tif (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {\n\nBut what I find particularly ugly is not just that it's more lines, but\nthat the assumptions and outputs of do_get_short_oid() aren't\nparticularly clear.\n\n-Peff\n"},{"id":"371446","messageId":"20190314024023.GA3889@sigill.intra.peff.net","threadId":"50726","inReplyTo":"20190314022245.GA1414@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] get_oid(): when an object was not found, try harder","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T02:40:23Z","receivedAt":"2019-03-14T02:41:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 10:22:46PM -0400, Jeff King wrote:\n\n> Try this:\n> \n> -- >8 --\n> mkfifo in\n> (git cat-file --batch-check exec 9>in\n> \n> git commit --allow-empty -m one\n> git commit --allow-empty -m two\n> \n> git rev-parse --short HEAD^ >&9\n> git repack -adk\n> git rev-parse --short HEAD >&9\n> -- >8 --\n> \n> The second object will (usually) be reported as \"missing\", even though\n> by calling reprepare_packed_git(), we would then find it in the\n> packfile.\n> \n> I say \"usually\" because if the two commits have the same first-byte to\n> their sha1, then we'd load both into the loose cache during the first\n> request, and use that during the second request.\n> \n> After this patch, it works consistently (we flush both the loose cache\n> and the cached set of packs, and find the packed version).\n\nBy the way, an interesting implication of this is that we're doing the\nre-prepare only when we come up with no object. But we _could_ actually\ncome up with the wrong object when we should say \"ambiguous\". I.e., the\nuser asks for 1234abcd. We have 1234abcde in a pack, and 1234abcdf\nloose. A simultaneous process repacks, and we see neither copy of\n1234abcdf, reporting that 1234abcde is the right answer.\n\nI'm not sure it's really worth addressing (just because I don't think\nthere's a good way to do it that isn't expensive).\n\n-Peff\n"},{"id":"371451","messageId":"xmqq8sxiaze2.fsf@gitster-ct.c.googlers.com","threadId":"50726","inReplyTo":"20190314022245.GA1414@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] get_oid(): when an object was not found, try harder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T03:49:41Z","receivedAt":"2019-03-14T03:49:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> 1. is reprepare_packed_git() a bit too heavy-weight, if the only\n>> thing we are addressing is the loose-object cache going stale?\n>\n> It's not the only thing we are addressing. :)\n>\n> Try this:\n\nYes, I knew about repacking. I was alluding to the overly heavy\nreference to loose-object-cache in the log message ;-).\n\n>> 2. is there a way to cleanly avoid the three-line duplicate?\n>\n> Yeah, as you noted, I think the boilerplate is worse than the\n> duplication. The most readable alternative to me is a separate function,\n> like:\n> ...\n> But what I find particularly ugly is not just that it's more lines, but\n> that the assumptions and outputs of do_get_short_oid() aren't\n> particularly clear.\n\nYeah, exactly.\n"},{"id":"371452","messageId":"xmqqzhpy9k40.fsf@gitster-ct.c.googlers.com","threadId":"50726","inReplyTo":"20190314024023.GA3889@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] get_oid(): when an object was not found, try harder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-14T04:05:03Z","receivedAt":"2019-03-14T04:08:53Z","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 it's really worth addressing (just because I don't think\n> there's a good way to do it that isn't expensive).\n\nI do not think so, either. Not at this layer, anyway.\n\nIf a \"-x\" command newly created an object whose prefix makes it\nambiguous against an existing object, we would not notice, unless we\nrefresh the loose object cache, that we now have two objects that\nshare the prefix. And we will find an object (the older one) with\nthe prefix in the stale cache without hitting \"oh we do not find it,\nlet's retry\" codepath. So to solve both sides of the coin, we'd\nneed invalidation either way, which obviously defeats the whole\ncaching concept, doesn't it?\n\nBut when we know we are running things like interactive rebase with\n\"-x\" that (1) creates new objects while we are waiting for that\nprocess to finish and (2) not performance critical, the loop that\ndrives these steps can flush any cache after seeing an external\nprocess to return control to us. Such an approach would still not\nsolve the issue a totally unrelated \"repack\" process creates by\nmucking with the object database behind our back, but at least the\ndamage to the code is much better isolated ;-)\n\n\n\n\n"},{"id":"371467","messageId":"0f7870b7-f92a-ad63-7854-300c202cb4a8@kdbg.org","threadId":"50726","inReplyTo":"20190313163516.GA26045@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-03-14T06:40:19Z","receivedAt":"2019-03-14T06:42:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 13.03.19 um 17:35 schrieb Jeff King:\n> On Wed, Mar 13, 2019 at 05:11:44PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> As a further improvement, is there a good reason for why we wouldn't\n>> pass something down to the oid machinery to say \"we're only interested\n>> in commits\". I have a WIP series somewhere to generalize that more, but\n>> e.g. here locally:\n> \n> We have get_oid_commit() and get_oid_committish() already. Should rebase\n> just be using those? (I think we probably want \"commit()\", because we do\n> not expect a \"pick\" line to have a tag, for example.\n\n'pick' needs all the power of revision expressions. Not all too\ninfrequently I do insert a pick line with a rev expression argument.\nAssuming that the resolved object is a commit is too strict.\n\n-- Hannes\n"},{"id":"371492","messageId":"nycvar.QRO.7.76.6.1903141344010.41@tvgsbejvaqbjf.bet","threadId":"50726","inReplyTo":"xmqqimwmcl93.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-14T12:44:31Z","receivedAt":"2019-03-14T12:45:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 14 Mar 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > +test_expect_failure SHA1 'loose object cache vs re-reading todo list' '\n> > +\tGIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&\n> > +\texport GIT_REBASE_TODO &&\n> > +\twrite_script append-todo.sh <<-\\EOS &&\n> > +\t# For values 5 and 6, this yields SHA-1s with the same first two digits\n> > +\techo \"pick $(git rev-parse --short \\\n> > +\t\t$(printf \"%s\\\\n\" \\\n> > +\t\t\t\"tree $EMPTY_TREE\" \\\n> > +\t\t\t\"author A U Thor $1 +0000\" \\\n> > +\t\t\t\"committer A U Thor $1 +0000\" \\\n> > +\t\t\t\"\" \\\n> > +\t\t\t\"$1\" |\n> > +\t\t git hash-object -t commit -w --stdin))\" >>$GIT_REBASE_TODO\n> \n> In the generated append-todo.sh, as the < will see \">>$GIT_REBASE_TODO\" literally (not the actual pathname\n> that begins with .git, but a reference to the variable). test-lint\n> may or may not catch it, but redirecting into variable reference\n> would trigger a(n arguably misguided) warning when run with some\n> versions of bash. Quoting\n> \n> \t\techo ... >>\"$GIT_REBASE_TODO\"\n> \n> would work it around, of course.\n> \n> > +\tshift\n> > +\ttest -z \"$*\" ||\n> > +\techo \"exec $0 $*\" >>$GIT_REBASE_TODO\n> \n> Likewise.\n\nYep. As a rule of thumb, I should always quote variable expansions.\n\nThanks,\nDscho\n\n> \n> > +\tEOS\n> > +\n> > +\tgit rebase HEAD -x \"./append-todo.sh 5 6\"\n> > +'\n> > +\n> > test_done\n> \n"},{"id":"371494","messageId":"nycvar.QRO.7.76.6.1903141403360.41@tvgsbejvaqbjf.bet","threadId":"50726","inReplyTo":"0f7870b7-f92a-ad63-7854-300c202cb4a8@kdbg.org","subject":"Re: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-14T13:06:31Z","receivedAt":"2019-03-14T13:06:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Thu, 14 Mar 2019, Johannes Sixt wrote:\n\n> Am 13.03.19 um 17:35 schrieb Jeff King:\n> > On Wed, Mar 13, 2019 at 05:11:44PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> As a further improvement, is there a good reason for why we wouldn't\n> >> pass something down to the oid machinery to say \"we're only interested\n> >> in commits\". I have a WIP series somewhere to generalize that more, but\n> >> e.g. here locally:\n> > \n> > We have get_oid_commit() and get_oid_committish() already. Should rebase\n> > just be using those? (I think we probably want \"commit()\", because we do\n> > not expect a \"pick\" line to have a tag, for example.\n> \n> 'pick' needs all the power of revision expressions. Not all too\n> infrequently I do insert a pick line with a rev expression argument.\n> Assuming that the resolved object is a commit is too strict.\n\nNo need to worry. `get_oid_committish()` can resolve all kinds of crazy\nexpressions. In a way, it is surprising that it will even resolve\n\"HEAD:Makefile\". The only thing (that I know of) that\n`get_oid_committish()` does that `get_oid()` does not is when you pass a\nshort hash which is unambiguous *only* when you exclude blobs and trees\nfrom the disambiguation.\n\nSo I think `get_oid_committish()` will serve us really well, as we really\nwant to parse the given hash name into the `struct todo_item`'s `commit`\nfield.\n\nCiao,\nDscho"},{"id":"371495","messageId":"nycvar.QRO.7.76.6.1903141410360.41@tvgsbejvaqbjf.bet","threadId":"50726","inReplyTo":"xmqqa7hyckfm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/4] get_oid(): when an object was not found, try harder","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-14T13:17:18Z","receivedAt":"2019-03-14T13:17:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio & Peff,\n\nOn Thu, 14 Mar 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > @@ -442,6 +442,18 @@ static enum get_oid_result get_short_oid(const char *name, int len,\n> > \tfind_short_packed_object(&ds);\n> > \tstatus = finish_object_disambiguation(&ds, oid);\n> > \n> > +\t/*\n> > +\t * If we didn't find it, do the usual reprepare() slow-path,\n> > +\t * since the object may have recently been added to the repository\n> > +\t * or migrated from loose to packed.\n> > +\t */\n> > +\tif (status == MISSING_OBJECT) {\n> > +\t\treprepare_packed_git(the_repository);\n> > +\t\tfind_short_object_filename(&ds);\n> > +\t\tfind_short_packed_object(&ds);\n> > +\t\tstatus = finish_object_disambiguation(&ds, oid);\n> > +\t}\n> > +\n> \n> This looks obviously correct, but two things that made me wonder\n> briefly were:\n> \n> 1. is reprepare_packed_git() a bit too heavy-weight, if the only\n> thing we are addressing is the loose-object cache going stale?\n> \n> 2. is there a way to cleanly avoid the three-line duplicate?\n> \n> My tentative answers are (1) even if it is, but get_short_oid() is\n> already heavy-weight enough; it won't be worth restructuring the\n> code to make it possible to clear only the loose-object cache, and\n> (2) a loop that runs twice when the first result is MISSING_OBJECT\n> and otherwise leaves after once would need an extra variable, its\n> iniialization, check and increment, which is more than what we might\n> save with such a restructuring, so it won't be worth pursuing.\n> \n> But others may have better ideas, as always ;-)\n\nPeff tried with a function, but I think that this would actually be a\nreally appropriate occasion for a well-placed `goto`:\n\n-- snip --\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 6dda2c16df10..36a66026964a 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -415,7 +415,7 @@ static enum get_oid_result get_short_oid(const char *name, int len,\n \t\t\t\t\t struct object_id *oid,\n \t\t\t\t\t unsigned flags)\n {\n-\tint status;\n+\tint status, attempts = 0;\n \tstruct disambiguate_state ds;\n \tint quietly = !!(flags & GET_OID_QUIETLY);\n \n@@ -438,10 +438,21 @@ static enum get_oid_result get_short_oid(const char *name, int len,\n \telse\n \t\tds.fn = default_disambiguate_hint;\n \n+try_again:\n \tfind_short_object_filename(&ds);\n \tfind_short_packed_object(&ds);\n \tstatus = finish_object_disambiguation(&ds, oid);\n \n+\t/*\n+\t * If we did not find it, do the usual reprepare() slow-path, since the\n+\t * object may have recently been added to the repository or migrated\n+\t * from loose to packed.\n+\t */\n+\tif (status == MISSING_OBJECT && !attempts++) {\n+\t\treprepare_packed_git(the_repository);\n+\t\tgoto try_again;\n+\t}\n+\n \tif (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {\n \t\tstruct oid_array collect = OID_ARRAY_INIT;\n \n-- snap --\n\nGranted, it's 11 lines inserted and one changed as opposed to 12 lines\ninserted, but it does make the code DRYer (and therefore slightly safer to\nmodify in the future). I pushed this to the GitGitGadget PR.\n\nThanks,\nDscho\n"},{"id":"371515","messageId":"63ddb1dd040ab2a0faf3a516b97775a220ca5145.1552577586.git.gitgitgadget@gmail.com","threadId":"50726","inReplyTo":"pull.161.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] rebase -i: demonstrate obscure loose object cache bug","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-14T15:33:07Z","receivedAt":"2019-03-14T15:33:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe specifically support `exec` commands in `git rebase -i`'s todo lists\nto rewrite the very same todo list. Of course, we need to validate that\ntodo list when re-reading it.\n\nIt is also totally legitimate to extend the todo list by `pick` lines\nusing short names of commits that were created only after the rebase\nstarted.\n\nAnd this is where the loose object cache interferes with this feature:\nif *some* loose object was read whose hash shares the same first two\ndigits with a commit that was not yet created when that loose object was\ncreated, then we fail to find that new commit by its short name in\n`get_oid()`, and the interactive rebase fails with an obscure error\nmessage like:\n\n\terror: invalid line 1: pick 6568fef\n\terror: please fix this using 'git rebase --edit-todo'.\n\nLet's first demonstrate that this is actually a bug in a new regression\ntest, in a separate commit so that other developers who do not believe\nme can cherry-pick it to confirm the problem.\n\nThis new regression test generates two commits whose hashes share the\nfirst two hex digits (so that their corresponding loose objects live in\nthe same subdirectory of .git/objects/, and are therefore supposed to be\nin the same loose object cache bin).\n\nIt then picks the first, to make sure that the loose object cache is\ninitialized and cached that object directory, then generates the second\ncommit and picks it, too. Since the commit was generated in a different\nprocess than the sequencer that wants to pick it, the loose object cache\nhad no chance of being updated in the meantime.\n\nTechnically, we would need only one `exec` command in this regression\ntest case, but for ease of implementation, it uses a pseudo-recursive\ncall to the same script.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3429-rebase-edit-todo.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh\nindex b9292dfc2a..83e5bb5eba 100755\n--- a/t/t3429-rebase-edit-todo.sh\n+++ b/t/t3429-rebase-edit-todo.sh\n@@ -11,4 +11,26 @@ test_expect_success 'rebase exec modifies rebase-todo' '\n \ttest -e F\n '\n \n+test_expect_failure SHA1 'loose object cache vs re-reading todo list' '\n+\tGIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&\n+\texport GIT_REBASE_TODO &&\n+\twrite_script append-todo.sh <<-\\EOS &&\n+\t# For values 5 and 6, this yields SHA-1s with the same first two digits\n+\techo \"pick $(git rev-parse --short \\\n+\t\t$(printf \"%s\\\\n\" \\\n+\t\t\t\"tree $EMPTY_TREE\" \\\n+\t\t\t\"author A U Thor $1 +0000\" \\\n+\t\t\t\"committer A U Thor $1 +0000\" \\\n+\t\t\t\"\" \\\n+\t\t\t\"$1\" |\n+\t\t git hash-object -t commit -w --stdin))\" >>\"$GIT_REBASE_TODO\"\n+\n+\tshift\n+\ttest -z \"$*\" ||\n+\techo \"exec $0 $*\" >>\"$GIT_REBASE_TODO\"\n+\tEOS\n+\n+\tgit rebase HEAD -x \"./append-todo.sh 5 6\"\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"371516","messageId":"f0cbfacf9ae54b73be58474e8cdf4cb262620b72.1552577586.git.gitgitgadget@gmail.com","threadId":"50726","inReplyTo":"pull.161.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] sequencer: improve error message when an OID could not be parsed","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-14T15:33:08Z","receivedAt":"2019-03-14T15:33:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe interactive rebase simply complains about an \"invalid line\" when the\nobject hash of, say, a `pick` line could not be parsed.\n\nLet's tell the user what happened in a little more detail.\n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 95dda23eee..f91062718d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2137,7 +2137,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \titem->arg_len = (int)(eol - item->arg);\n \n \tif (status < 0)\n-\t\treturn -1;\n+\t\treturn error(_(\"could not parse '%.*s'\"),\n+\t\t\t (int)(end_of_object_name - bol), bol);\n \n \titem->commit = lookup_commit_reference(r, &commit_oid);\n \treturn !item->commit;\n-- \ngitgitgadget\n\n"},{"id":"371517","messageId":"b55e14cd63be82deb4f216074b8d4311364787c7.1552577586.git.gitgitgadget@gmail.com","threadId":"50726","inReplyTo":"pull.161.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] sequencer: move stale comment into correct location","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-14T15:33:09Z","receivedAt":"2019-03-14T15:33:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nSigned-off-by: Johannes Schindelin \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f91062718d..79a046d748 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3641,7 +3641,6 @@ static int pick_commits(struct repository *r,\n \t\t\tres = do_exec(r, item->arg);\n \t\t\t*end_of_arg = saved;\n \n-\t\t\t/* Reread the todo file if it has changed. */\n \t\t\tif (res) {\n \t\t\t\tif (opts->reschedule_failed_exec)\n \t\t\t\t\treschedule = 1;\n@@ -3649,6 +3648,7 @@ static int pick_commits(struct repository *r,\n \t\t\t\tres = error_errno(_(\"could not stat '%s'\"),\n \t\t\t\t\t\t get_todo_path(opts));\n \t\t\telse if (match_stat_data(&todo_list->stat, &st)) {\n+\t\t\t\t/* Reread the todo file if it has changed. */\n \t\t\t\ttodo_list_release(todo_list);\n \t\t\t\tif (read_populate_todo(r, todo_list, opts))\n \t\t\t\t\tres = -1; /* message was printed */\n-- \ngitgitgadget\n\n"},{"id":"371518","messageId":"41fc6eb9baa2ab54e6061c6894a17664209a0e53.1552577586.git.gitgitgadget@gmail.com","threadId":"50726","inReplyTo":"pull.161.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] get_oid(): when an object was not found, try harder","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-14T15:33:10Z","receivedAt":"2019-03-14T15:33:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt is quite possible that the loose object cache gets stale when new\nobjects are written. Or that a new pack was installed. In those cases,\nget_oid() would potentially say that it cannot find a given object, even\nif it should find it.\n\nLet's blow away the loose object cache as well as the read packs and try\nagain in that case.\n\nNote: this does *not* affect the code path that was introduced to help\navoid looking for the same non-existing objects (which made some\noperations really expensive via NFS): that code path is handled by the\n`OBJECT_INFO_QUICK` flag (which does not even apply to `get_oid()`,\nwhich has no equivalent flag, at least at the time this patch was\nwritten).\n\nThis incidentally fixes the problem identified earlier where an\ninteractive rebase wanted to re-read (and validate) the todo list after\nan `exec` command modified it.\n\nHelped-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n sha1-name.c | 13 ++++++++++++-\n t/t3429-rebase-edit-todo.sh | 2 +-\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 6dda2c16df..36a6602696 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -415,7 +415,7 @@ static enum get_oid_result get_short_oid(const char *name, int len,\n \t\t\t\t\t struct object_id *oid,\n \t\t\t\t\t unsigned flags)\n {\n-\tint status;\n+\tint status, attempts = 0;\n \tstruct disambiguate_state ds;\n \tint quietly = !!(flags & GET_OID_QUIETLY);\n \n@@ -438,10 +438,21 @@ static enum get_oid_result get_short_oid(const char *name, int len,\n \telse\n \t\tds.fn = default_disambiguate_hint;\n \n+try_again:\n \tfind_short_object_filename(&ds);\n \tfind_short_packed_object(&ds);\n \tstatus = finish_object_disambiguation(&ds, oid);\n \n+\t/*\n+\t * If we did not find it, do the usual reprepare() slow-path, since the\n+\t * object may have recently been added to the repository or migrated\n+\t * from loose to packed.\n+\t */\n+\tif (status == MISSING_OBJECT && !attempts++) {\n+\t\treprepare_packed_git(the_repository);\n+\t\tgoto try_again;\n+\t}\n+\n \tif (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {\n \t\tstruct oid_array collect = OID_ARRAY_INIT;\n \ndiff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh\nindex 83e5bb5eba..d5d2925a44 100755\n--- a/t/t3429-rebase-edit-todo.sh\n+++ b/t/t3429-rebase-edit-todo.sh\n@@ -11,7 +11,7 @@ test_expect_success 'rebase exec modifies rebase-todo' '\n \ttest -e F\n '\n \n-test_expect_failure SHA1 'loose object cache vs re-reading todo list' '\n+test_expect_success SHA1 'loose object cache vs re-reading todo list' '\n \tGIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&\n \texport GIT_REBASE_TODO &&\n \twrite_script append-todo.sh <<-\\EOS &&\n-- \ngitgitgadget\n"},{"id":"371519","messageId":"pull.161.v2.git.gitgitgadget@gmail.com","threadId":"50726","inReplyTo":"pull.161.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] get_short_oid: cope with a possibly stale loose object cache","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-14T15:33:07Z","receivedAt":"2019-03-14T15:33:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Being rather a power user of the interactive rebase, I discovered recently\nthat one of my scripts ran into an odd problem where an interactive rebase\nappended new commands to the todo list, and the interactive rebase failed to\nvalidate the (short) OIDs. But when I tried to fix things via git rebase\n--edit-todo, the interactive rebase had no longer any problem to validate\nthem.\n\nTurns out that I generated the objects during the interactive rebase (yes, I\nimplemented a 2-phase rebase\n[https://github.com/git-for-windows/build-extra/blob/master/ever-green.sh]),\nand that their hashes happened to share the first two hex digits with some\nloose object that had already been read in the same interactive rebase run,\ncausing a now-stale loose object cache to be initialized for that \n.git/objects/?? subdirectory.\n\nIt was quite the, let's say, adventure, to track that one down.\n\nOver at the companion PR for Git for Windows\n[https://github.com/git-for-windows/git/pull/2121], I discussed this with\nPeff (who introduced the loose object cache), and he pointed out that my\noriginal solution was a bit too specific for the interactive rebase. He\nsuggested the current method of re-reading upon a missing object instead,\nand explained patiently that this does not affect the code path for which\nthe loose object cache was introduced originally: to help with performance\nissues on NFS when Git tries to find the same missing objects over and over\nagain.\n\nThe regression test still reflects what I need this fix for, and I would\nrather keep it as-is (even if the fix is not about the interactive rebase\nper se), as it really tests for the functionality that I really need to\ncontinue to work. Since I (and other contributors touching the rebase code)\nfrequently run only the t34* subset of the test suite while developing git\nrebase patches, I also want to keep the test case there, rather than putting\nit in with other SHA1-specific disambiguation test cases in \nt1512-rev-parse-disambiguation.sh.\n\nMy only concern was that this might cause some performance regressions that\nneither Peff nor I could think of, where get_oid() may run repeatedly into\nmissing objects by design, and where we should not blow away and recreate\nthe loose object cache all the time. But Peff dispelled my fears, pointing\nout that get_oid() is already rather expensive and therefore avoided in hot\nloops.\n\nChanges since v1:\n\n * The added test case now properly quotes $GIT_REBASE_TODO (even if it does\n not contain spaces, it is simply safer to quote variable expansions in\n Bash always [https://mywiki.wooledge.org/Quotes].\n * The three lines to try again to disambiguate are no longer repeated.\n Instead, we use a Beautiful Goto.\n * The commit message of 2/2 does not stress lean on the loose object cache\n so much, but also talks about packs a bit more.\n\nJohannes Schindelin (4):\n rebase -i: demonstrate obscure loose object cache bug\n sequencer: improve error message when an OID could not be parsed\n sequencer: move stale comment into correct location\n get_oid(): when an object was not found, try harder\n\n sequencer.c | 5 +++--\n sha1-name.c | 13 ++++++++++++-\n t/t3429-rebase-edit-todo.sh | 22 ++++++++++++++++++++++\n 3 files changed, 37 insertions(+), 3 deletions(-)\n\n\nbase-commit: e902e9bcae2010bc42648c80ab6adc6c5a16a4a5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-161%2Fdscho%2Frereading-todo-list-and-loose-object-cache-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-161/dscho/rereading-todo-list-and-loose-object-cache-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/161\n\nRange-diff vs v1:\n\n 1: b3fcd37765 ! 1: 63ddb1dd04 rebase -i: demonstrate obscure loose object cache bug\n @@ -60,11 +60,11 @@\n +\t\t\t\"committer A U Thor $1 +0000\" \\\n +\t\t\t\"\" \\\n +\t\t\t\"$1\" |\n -+\t\t git hash-object -t commit -w --stdin))\" >>$GIT_REBASE_TODO\n ++\t\t git hash-object -t commit -w --stdin))\" >>\"$GIT_REBASE_TODO\"\n +\n +\tshift\n +\ttest -z \"$*\" ||\n -+\techo \"exec $0 $*\" >>$GIT_REBASE_TODO\n ++\techo \"exec $0 $*\" >>\"$GIT_REBASE_TODO\"\n +\tEOS\n +\n +\tgit rebase HEAD -x \"./append-todo.sh 5 6\"\n 2: d8c4a3dde5 = 2: f0cbfacf9a sequencer: improve error message when an OID could not be parsed\n 3: d749c63170 = 3: b55e14cd63 sequencer: move stale comment into correct location\n 4: 994446236d ! 4: 41fc6eb9ba get_oid(): when an object was not found, try harder\n @@ -3,8 +3,9 @@\n get_oid(): when an object was not found, try harder\n \n It is quite possible that the loose object cache gets stale when new\n - objects are written. In that case, get_oid() would potentially say that\n - it cannot find a given object, even if it should find it.\n + objects are written. Or that a new pack was installed. In those cases,\n + get_oid() would potentially say that it cannot find a given object, even\n + if it should find it.\n \n Let's blow away the loose object cache as well as the read packs and try\n again in that case.\n @@ -27,19 +28,31 @@\n --- a/sha1-name.c\n +++ b/sha1-name.c\n @@\n + \t\t\t\t\t struct object_id *oid,\n + \t\t\t\t\t unsigned flags)\n + {\n +-\tint status;\n ++\tint status, attempts = 0;\n + \tstruct disambiguate_state ds;\n + \tint quietly = !!(flags & GET_OID_QUIETLY);\n + \n +@@\n + \telse\n + \t\tds.fn = default_disambiguate_hint;\n + \n ++try_again:\n + \tfind_short_object_filename(&ds);\n \tfind_short_packed_object(&ds);\n \tstatus = finish_object_disambiguation(&ds, oid);\n \n +\t/*\n -+\t * If we didn't find it, do the usual reprepare() slow-path,\n -+\t * since the object may have recently been added to the repository\n -+\t * or migrated from loose to packed.\n ++\t * If we did not find it, do the usual reprepare() slow-path, since the\n ++\t * object may have recently been added to the repository or migrated\n ++\t * from loose to packed.\n +\t */\n -+\tif (status == MISSING_OBJECT) {\n ++\tif (status == MISSING_OBJECT && !attempts++) {\n +\t\treprepare_packed_git(the_repository);\n -+\t\tfind_short_object_filename(&ds);\n -+\t\tfind_short_packed_object(&ds);\n -+\t\tstatus = finish_object_disambiguation(&ds, oid);\n ++\t\tgoto try_again;\n +\t}\n +\n \tif (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {\n\n-- \ngitgitgadget\n"},{"id":"371532","messageId":"20190314185556.GA26250@sigill.intra.peff.net","threadId":"50726","inReplyTo":"xmqqzhpy9k40.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/4] get_oid(): when an object was not found, try harder","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T18:55:57Z","receivedAt":"2019-03-14T18:57:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 01:05:03PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm not sure it's really worth addressing (just because I don't think\n> > there's a good way to do it that isn't expensive).\n> \n> I do not think so, either. Not at this layer, anyway.\n> \n> If a \"-x\" command newly created an object whose prefix makes it\n> ambiguous against an existing object, we would not notice, unless we\n> refresh the loose object cache, that we now have two objects that\n> share the prefix. And we will find an object (the older one) with\n> the prefix in the stale cache without hitting \"oh we do not find it,\n> let's retry\" codepath. So to solve both sides of the coin, we'd\n> need invalidation either way, which obviously defeats the whole\n> caching concept, doesn't it?\n\nRight, that was the conclusion I came to. You cannot solve it in all\ncases without checking whether new objects have appeared for every\nlookup. I think that could be as cheap as a stat() on the pack\ndirectory and one on the appropriate loose directory, but I suspect even\nthat might be measurably slower.\n\n-Peff\n"},{"id":"371533","messageId":"20190314185758.GB26250@sigill.intra.peff.net","threadId":"50726","inReplyTo":"nycvar.QRO.7.76.6.1903141410360.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/4] get_oid(): when an object was not found, try harder","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-14T18:57:58Z","receivedAt":"2019-03-14T18:59:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 14, 2019 at 02:17:18PM +0100, Johannes Schindelin wrote:\n\n> > 2. is there a way to cleanly avoid the three-line duplicate?\n> [...]\n> Peff tried with a function, but I think that this would actually be a\n> really appropriate occasion for a well-placed `goto`:\n\nI worried that a goto would be too confusing/ugly. But I am OK with it\nif you are.\n\n-Peff\n"},{"id":"371538","messageId":"nycvar.QRO.7.76.6.1903142054490.41@tvgsbejvaqbjf.bet","threadId":"50726","inReplyTo":"20190314185758.GB26250@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] get_oid(): when an object was not found, try harder","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-14T19:55:16Z","receivedAt":"2019-03-14T19:55:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 14 Mar 2019, Jeff King wrote:\n\n> On Thu, Mar 14, 2019 at 02:17:18PM +0100, Johannes Schindelin wrote:\n> \n> > > 2. is there a way to cleanly avoid the three-line duplicate?\n> > [...]\n> > Peff tried with a function, but I think that this would actually be a\n> > really appropriate occasion for a well-placed `goto`:\n> \n> I worried that a goto would be too confusing/ugly. But I am OK with it\n> if you are.\n\nI hope that after reading the patch, you will agree with me that it is not\ntoo confusing/ugly... ;-)\n\nCiao,\nDscho\n"}]}