From 14f0773faf97ca8b0afd8f4e3ba067e280f64bf0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 12 Mar 2019 22:00:07 +0100 Subject: [PATCH 1/4] rebase -i: demonstrate obscure loose object cache bug We specifically support `exec` commands in `git rebase -i`'s todo lists to rewrite the very same todo list. Of course, we need to validate that todo list when re-reading it. It is also totally legitimate to extend the todo list by `pick` lines using short names of commits that were created only after the rebase started. And this is where the loose object cache interferes with this feature: if *some* loose object was read whose hash shares the same first two digits with a commit that was not yet created when that loose object was created, then we fail to find that new commit by its short name in `get_oid()`, and the interactive rebase fails with an obscure error message like: error: invalid line 1: pick 6568fef error: please fix this using 'git rebase --edit-todo'. Let's first demonstrate that this is actually a bug in a new regression test, in a separate commit so that other developers who do not believe me can cherry-pick it to confirm the problem. This new regression test generates two commits whose hashes share the first two hex digits (so that their corresponding loose objects live in the same subdirectory of .git/objects/, and are therefore supposed to be in the same loose object cache bin). It then picks the first, to make sure that the loose object cache is initialized and cached that object directory, then generates the second commit and picks it, too. Since the commit was generated in a different process than the sequencer that wants to pick it, the loose object cache had no chance of being updated in the meantime. Technically, we would need only one `exec` command in this regression test case, but for ease of implementation, it uses a pseudo-recursive call to the same script. Signed-off-by: Johannes Schindelin --- t/t3429-rebase-edit-todo.sh | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh index b9292dfc2a..862f229c87 100755 --- a/t/t3429-rebase-edit-todo.sh +++ b/t/t3429-rebase-edit-todo.sh @@ -11,4 +11,26 @@ test_expect_success 'rebase exec modifies rebase-todo' ' test -e F ' +test_expect_failure SHA1 'loose object cache vs re-reading todo list' ' + GIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo && + export GIT_REBASE_TODO && + write_script append-todo.sh <<-\EOS && + # For values 5 and 6, this yields SHA-1s with the same first two digits + echo "pick $(git rev-parse --short \ + $(printf "%s\\n" \ + "tree $EMPTY_TREE" \ + "author A U Thor $1 +0000" \ + "committer A U Thor $1 +0000" \ + "" \ + "$1" | + git hash-object -t commit -w --stdin))" >>$GIT_REBASE_TODO + + shift + test -z "$*" || + echo "exec $0 $*" >>$GIT_REBASE_TODO + EOS + + git rebase HEAD -x "./append-todo.sh 5 6" +' + test_done From 37edef57b97ac8efcad59788011d976f42bdf4f9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 12 Mar 2019 22:15:05 +0100 Subject: [PATCH 2/4] sequencer: improve error message when an OID could not be parsed The interactive rebase simply complains about an "invalid line" when the object hash of, say, a `pick` line could not be parsed. Let's tell the user what happened in a little more detail. Signed-off-by: Johannes Schindelin --- sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 0db410d590..97e93d2338 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2136,7 +2136,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, item->arg_len = (int)(eol - item->arg); if (status < 0) - return -1; + return error(_("could not parse '%.*s'"), + (int)(end_of_object_name - bol), bol); item->commit = lookup_commit_reference(r, &commit_oid); return !item->commit; From 3f45f7580c35bc9454128ed06ef0465b2ea8d943 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 11 Mar 2019 22:53:00 +0100 Subject: [PATCH 3/4] sequencer: move stale comment into correct location Signed-off-by: Johannes Schindelin --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 97e93d2338..6a44a0b93d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3640,7 +3640,6 @@ static int pick_commits(struct repository *r, res = do_exec(r, item->arg); *end_of_arg = saved; - /* Reread the todo file if it has changed. */ if (res) { if (opts->reschedule_failed_exec) reschedule = 1; @@ -3648,6 +3647,7 @@ static int pick_commits(struct repository *r, res = error_errno(_("could not stat '%s'"), get_todo_path(opts)); else if (match_stat_data(&todo_list->stat, &st)) { + /* Reread the todo file if it has changed. */ todo_list_release(todo_list); if (read_populate_todo(r, todo_list, opts)) res = -1; /* message was printed */ From b435d7c8701e06b83b1f85adf7506caf547eb0bd Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 11 Mar 2019 22:53:06 +0100 Subject: [PATCH 4/4] get_oid(): when an object was not found, try harder It is quite possible that the loose object cache gets stale when new objects are written. In that case, get_oid() would potentially say that it cannot find a given object, even if it should find it. Let's blow away the loose object cache as well as the read packs and try again in that case. Note: this does *not* affect the code path that was introduced to help avoid looking for the same non-existing objects (which made some operations really expensive via NFS): that code path is handled by the `OBJECT_INFO_QUICK` flag (which does not even apply to `get_oid()`, which has no equivalent flag, at least at the time this patch was written). This incidentally fixes the problem identified earlier where an interactive rebase wanted to re-read (and validate) the todo list after an `exec` command modified it. Helped-by: Jeff King Signed-off-by: Johannes Schindelin --- sha1-name.c | 12 ++++++++++++ t/t3429-rebase-edit-todo.sh | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/sha1-name.c b/sha1-name.c index 6dda2c16df..cfe5c874b6 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -442,6 +442,18 @@ static enum get_oid_result get_short_oid(const char *name, int len, find_short_packed_object(&ds); status = finish_object_disambiguation(&ds, oid); + /* + * If we didn't find it, do the usual reprepare() slow-path, + * since the object may have recently been added to the repository + * or migrated from loose to packed. + */ + if (status == MISSING_OBJECT) { + reprepare_packed_git(the_repository); + find_short_object_filename(&ds); + find_short_packed_object(&ds); + status = finish_object_disambiguation(&ds, oid); + } + if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) { struct oid_array collect = OID_ARRAY_INIT; diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh index 862f229c87..76f6d306ea 100755 --- a/t/t3429-rebase-edit-todo.sh +++ b/t/t3429-rebase-edit-todo.sh @@ -11,7 +11,7 @@ test_expect_success 'rebase exec modifies rebase-todo' ' test -e F ' -test_expect_failure SHA1 'loose object cache vs re-reading todo list' ' +test_expect_success SHA1 'loose object cache vs re-reading todo list' ' GIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo && export GIT_REBASE_TODO && write_script append-todo.sh <<-\EOS &&