git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 1/4] rebase -i: demonstrate obscure loose object cache bug

From
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Date
Mar 14, 2019, 15:33 UTC
Message-ID
<63ddb1dd040ab2a0faf3a516b97775a220ca5145.1552577586.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.161.v2.git.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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..83e5bb5eba 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 <author@example.org> $1 +0000" \
+			"committer A U Thor <author@example.org> $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
-- 
gitgitgadget
Previous: Johannes Schindelin via GitGitGadgetNext: Johannes Schindelin via GitGitGadget
Message 29 of 32 in “get_oid: cope with a possibly stale loose object cache”
  1. 0/4 get_oid: cope with a possibly stale loose object cacheJohannes Schindelin via GitGitGadget, Mar 13, 2019
  2. 1/4 rebase -i: demonstrate obscure loose object cache bugJohannes Schindelin via GitGitGadget, Mar 13, 2019
  3. Ævar Arnfjörð BjarmasonMar 13, 2019
  4. Jeff KingMar 13, 2019
  5. Jeff KingMar 13, 2019
  6. Johannes SchindelinMar 13, 2019
  7. Jeff KingMar 14, 2019
  8. Johannes SchindelinMar 13, 2019
  9. Jeff KingMar 14, 2019
  10. Johannes SixtMar 14, 2019
  11. Johannes SchindelinMar 14, 2019
  12. Junio C HamanoMar 14, 2019
  13. Johannes SchindelinMar 14, 2019
  14. 2/4 sequencer: improve error message when an OID could not be parsedJohannes Schindelin via GitGitGadget, Mar 13, 2019
  15. Junio C HamanoMar 14, 2019
  16. 4/4 get_oid(): when an object was not found, try harderJohannes Schindelin via GitGitGadget, Mar 13, 2019
  17. Junio C HamanoMar 14, 2019
  18. Jeff KingMar 14, 2019
  19. Jeff KingMar 14, 2019
  20. Junio C HamanoMar 14, 2019
  21. Jeff KingMar 14, 2019
  22. Junio C HamanoMar 14, 2019
  23. Johannes SchindelinMar 14, 2019
  24. Jeff KingMar 14, 2019
  25. Johannes SchindelinMar 14, 2019
  26. 3/4 sequencer: move stale comment into correct locationJohannes Schindelin via GitGitGadget, Mar 13, 2019
  27. Jeff KingMar 13, 2019
  28. 0/4 get_short_oid: cope with a possibly stale loose object cacheJohannes Schindelin via GitGitGadget, Mar 14, 2019
  29. 1/4 rebase -i: demonstrate obscure loose object cache bugJohannes Schindelin via GitGitGadget, Mar 14, 2019
  30. 2/4 sequencer: improve error message when an OID could not be parsedJohannes Schindelin via GitGitGadget, Mar 14, 2019
  31. 3/4 sequencer: move stale comment into correct locationJohannes Schindelin via GitGitGadget, Mar 14, 2019
  32. 4/4 get_oid(): when an object was not found, try harderJohannes Schindelin via GitGitGadget, Mar 14, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.