rebase -i: fix re-reading the todo list when newly created objects are referenced

[dscho]

In the ever-green.sh script, we make use of an obscure, yet powerful feature of the interactive rebase: we use an exec command to augment the todo list (to allow for a 2-phase rebase to be executed in a single one, so that the second phase will be executed even if a merge conflict had to be resolved in the first phase).

However, this poses a challenge with the new loose object cache, as we might reference newly-created commits in that new todo list part, and the loose object cache might pretend that those commits do not exist.

Let's fix that.

[dscho]

Cc @peff (because there might very well be more use cases where the loose object cache goes stale and would need to either be blown away, or simply disabled).

[peff]

and the loose object cache might pretend that those commits do not exist.

Can you clarify the symptoms of the bug a little more? Specifically, which use of the cache is having problems?

Since the cache is otherwise only used for OBJECT_INFO_QUICK queries, my guess is that this has to do with the abbrev code? We'd normally drop-and-reload the cache when we hit a missing object, the same way we trigger reprepare_packed_git(). I'm wondering if we could/should do the same in the abbreviation code. It's looking for near-miss objects, but in doing so it should always come across the actual object name we're abbreviating (either as loose or packed). If not, should we consider doing the same reprepare process to pick up new objects?

[peff]

By the way, if the symptom is in the abbrev code, I suspect the problem you are seeing would pre-date the recent loose-object-cache work. That had its own cache since cc817ca (which couldn't even be invalidated!). I'm guessing it's coming up now because of moving rebase -i into C?

[dscho]

This is the symptom after appending some lines to the todo list:

error: invalid line 515: fixup 7fbdafc914 fixup! ident: add the ability to provide a "fallback identity"
error: invalid line 523: fixup e48378074f fixup! stash: add tests for `git stash show` config
error: invalid line 529: fixup dc857a9f9f fixup! stash: convert drop and clear to builtin
[...]

I did single-step through one of those, and it fails at https://github.com/git-for-windows/git/blob/v2.21.0.windows.1/sequencer.c#L2132:

	status = get_oid(bol, &commit_oid);

I had not actually debugged further, but instead I came up with the fix in this PR, on the hunch that it might be the loose object cache. And it seems to have fixed the problem...

I guess I really should add a regression test ;-)

[peff]

Hmm. That get_oid call would not be using the loose object cache at all. So I'd guess that the problem is that the abbreviated hash is ambiguous, because it was generated without the newly-mentioned commits. But at 10 characters, it seems unlikely to collide (and to collide three times). So I'm quite confused about how the loose object cache is even at play here.

[dscho]

That get_oid call would not be using the loose object cache at all.

But it would... this is the backtrace:

#0  odb_loose_cache (odb=0x1e2e3d0, oid=0x7ffffd957025) at sha1-file.c:2227
#1  0x0000000000603658 in find_short_object_filename (ds=0x7ffffd956fe0) at sha1-name.c:98
#2  0x0000000000604371 in get_short_oid (name=0x3b121d4 "6392f5b2fd", len=10, oid=0x7ffffd957280, flags=0) at sha1-name.c:441
#3  0x0000000000605e03 in get_oid_1 (name=0x3b121d4 "6392f5b2fd", len=10, oid=0x7ffffd957280, lookup_flags=0) at sha1-name.c:1127
#4  0x000000000060745a in get_oid_with_context_1 (repo=0x97e440 <the_repo>, name=0x3b121d4 "6392f5b2fd", flags=0, prefix=0x0, oid=0x7ffffd957280,
    oc=0x7ffffd957200) at sha1-name.c:1693
#5  0x0000000000607a37 in get_oid_with_context (repo=0x97e440 <the_repo>, str=0x3b121d4 "6392f5b2fd", flags=0, oid=0x7ffffd957280, oc=0x7ffffd957200)
    at sha1-name.c:1831
#6  0x0000000000606d87 in get_oid (name=0x3b121d4 "6392f5b2fd", oid=0x7ffffd957280) at sha1-name.c:1518
[...]

... and a patch to demonstrate the problem (which I will submit to the Git mailing list Real Soon Now):

diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
index b9292dfc2a..d78f057f35 100755
--- a/t/t3429-rebase-edit-todo.sh
+++ b/t/t3429-rebase-edit-todo.sh
@@ -11,4 +11,28 @@ test_expect_success 'rebase exec modifies rebase-todo' '
 	test -e F
 '
 
+test_expect_success 'loose cache does not interfere with re-reading todo list' '
+	GIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&
+	export GIT_REBASE_TODO &&
+	write_script append-todo.sh <<-\EOS &&
+	# It was determined manually that the following fast-import generates
+	# SHA-1s with the same first two digits for the command-line arguments
+	# 5 and 6:
+
+	git fast-import --quiet <<-EOF
+	commit refs/tags/ref$1
+	committer A U Thor <author@example.org> $1 +0000
+	data <<EOM
+	$1
+	EOM
+	EOF
+
+	echo "pick $(git rev-parse --short ref$1)" >>$GIT_REBASE_TODO
+	test $1 -gt 6 ||
+	echo "exec $0 $(($1+1))" >>$GIT_REBASE_TODO
+	EOS
+
+	git rebase HEAD -x "./append-todo.sh 5"
+'
+
 test_done

[peff]

That get_oid call would not be using the loose object cache at all.

But it would... this is the backtrace:

Ah, right, of course. It triggers the abbrev code for a lookup. So I do think this was probably broken for some time due to the old cache. And has probably been broken forever in a racy way if somebody was simultaneously repacking. We'd probably want something like this:

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;
 

That fixes your test, though I'm not sure if it's entirely safe to call those find_* functions again without reinitializing the ds struct.

[dscho]

But wasn't part of the idea behind the loose object cache to accelerate the awful performance on NFS when trying to find the same object over and over again, even if we already know that it does not exist?

That fix would regress on that, unless I am mistaken.

[dscho]

FYI my current (unsubmitted) iteration is here: gitgitgadget#161

[dscho]

I'm not sure if it's entirely safe to call those find_* functions again without reinitializing the ds struct.

I am fairly certain that it is safe.

But I still think that we should do this only if we also introduce a flag that is set by any loose object-writing (or -deleting) code path, and we should only then blow away the loose object cache.

[peff]

But wasn't part of the idea behind the loose object cache to accelerate the awful performance on NFS when trying to find the same object over and over again, even if we already know that it does not exist?

That fix would regress on that, unless I am mistaken.

No, the caller has to opt into the "quick" mode explicitly, with the intent that we would not introduce these kinds of cache-invalidation bugs. So has_object_file_with_flags(&oid, OBJECT_INFO_QUICK) will do it, but a normal has_object_file will not.

And I'd say the same thing should apply here. We do not currently have a GET_OID_QUICK flag, but if there was a caller who did want "faster, but maybe racily inaccurate", we could add one and respect it here.

[dscho]

Fair enough!

[dscho]

I still think that we should do this only if we also introduce a flag that is set by any loose object-writing (or -deleting) code path, and we should only then blow away the loose object cache.

That would actually not work in the regression test that I added, as the objects are written in a different process. D'oh.

[dscho]

FYI my current (unsubmitted) iteration is here: gitgitgadget#161

FWIW I updated that PR, and the actual fix is this one: gitgitgadget@3be9170

[dscho]

This was accepted into master of core Git; Let's have it in Git for Windows' master, too.