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

[PATCHv5 18/23] git notes merge: --commit should fail if underlying notes ref has moved

From
Johan Herland <johan@herland.net>
Date
Oct 25, 2010, 00:08 UTC
Message-ID
<1287965333-5099-19-git-send-email-johan@herland.net>
In-Reply-To
<1287965333-5099-1-git-send-email-johan@herland.net>

When manually resolving a notes merge, if the merging ref has moved since the merge started, we should fail to complete the merge, and alert the user to what's going on.

This situation may arise if you start a 'git notes merge' which results in conflicts, and you then update the current notes ref (using for example 'git notes add/copy/amend/edit/remove/prune', 'git update-ref', etc.), before you get around to resolving the notes conflicts and calling 'git notes merge --commit'.

We detect this situation by comparing the first parent of the partial merge commit (which was created when the merge started) to the current value of the merging notes ref (pointed to by the .git/NOTES_MERGE_REF symref).

If we don't fail in this situation, the notes merge commit would overwrite the updated notes ref, thus losing the changes that happened in the meantime.

The patch includes a testcase verifying that we fail correctly in this situation.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin/notes.c                       |   11 ++++-
 t/t3310-notes-merge-manual-resolve.sh |   76 +++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 2 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index b440378..ca09d45 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -786,7 +786,7 @@ static int merge_abort(struct notes_merge_options *o)
 static int merge_commit(struct notes_merge_options *o)
 {
 	struct strbuf msg = STRBUF_INIT;
-	unsigned char sha1[20];
+	unsigned char sha1[20], parent_sha1[20];
 	struct notes_tree *t;
 	struct commit *partial;
 	struct pretty_print_context pretty_ctx;
@@ -803,6 +803,11 @@ static int merge_commit(struct notes_merge_options *o)
 	else if (parse_commit(partial))
 		die("Could not parse commit from NOTES_MERGE_PARTIAL.");
 
+	if (partial->parents)
+		hashcpy(parent_sha1, partial->parents->item->object.sha1);
+	else
+		hashclr(parent_sha1);
+
 	t = xcalloc(1, sizeof(struct notes_tree));
 	init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
 
@@ -818,7 +823,9 @@ static int merge_commit(struct notes_merge_options *o)
 	format_commit_message(partial, "%s", &msg, &pretty_ctx);
 	strbuf_trim(&msg);
 	strbuf_insert(&msg, 0, "notes: ", 7);
-	update_ref(msg.buf, o->local_ref, sha1, NULL, 0, DIE_ON_ERR);
+	update_ref(msg.buf, o->local_ref, sha1,
+		   is_null_sha1(parent_sha1) ? NULL : parent_sha1,
+		   0, DIE_ON_ERR);
 
 	free_notes(t);
 	strbuf_release(&msg);
diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 287fab8..4ec4d11 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -477,4 +477,80 @@ EOF
 	verify_notes z
 '
 
+cp expect_notes_y expect_notes_m
+cp expect_log_y expect_log_m
+
+test_expect_success 'redo merge of z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
+	git update-ref refs/notes/m refs/notes/y &&
+	test_must_fail git notes merge z >output &&
+	# Output should point to where to resolve conflicts
+	grep -q "\\.git/NOTES_MERGE_WORKTREE" output &&
+	# Inspect merge conflicts
+	ls .git/NOTES_MERGE_WORKTREE >output_conflicts &&
+	test_cmp expect_conflicts output_conflicts &&
+	( for f in $(cat expect_conflicts); do
+		test_cmp "expect_conflict_$f" ".git/NOTES_MERGE_WORKTREE/$f" ||
+		exit 1
+	done ) &&
+	# Verify that current notes tree (pre-merge) has not changed (m == y)
+	verify_notes y &&
+	verify_notes m &&
+	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+'
+
+cp expect_notes_w expect_notes_m
+cp expect_log_w expect_log_m
+
+test_expect_success 'reset notes ref m to somewhere else (w)' '
+	git update-ref refs/notes/m refs/notes/w &&
+	verify_notes m &&
+	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+'
+
+test_expect_success 'fail to finalize conflicting merge if underlying ref has moved in the meantime (m != NOTES_MERGE_PARTIAL^1)' '
+	# Resolve conflicts
+	cat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&
+y and z notes on 1st commit
+EOF
+	cat >.git/NOTES_MERGE_WORKTREE/$commit_sha4 <<EOF &&
+y and z notes on 4th commit
+EOF
+	# Fail to finalize merge
+	test_must_fail git notes merge --commit >output 2>&1 &&
+	# .git/NOTES_MERGE_* must remain
+	test -f .git/NOTES_MERGE_PARTIAL &&
+	test -f .git/NOTES_MERGE_REF &&
+	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha1 &&
+	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha2 &&
+	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
+	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
+	# Refs are unchanged
+	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)"
+	test "$(git rev-parse refs/notes/m)" != "$(git rev-parse NOTES_MERGE_PARTIAL^1)"
+	# Mention refs/notes/m, and its current and expected value in output
+	grep -q "refs/notes/m" output &&
+	grep -q "$(git rev-parse refs/notes/m)" output &&
+	grep -q "$(git rev-parse NOTES_MERGE_PARTIAL^1)" output &&
+	# Verify that other notes refs has not changed (w, x, y and z)
+	verify_notes w &&
+	verify_notes x &&
+	verify_notes y &&
+	verify_notes z
+'
+
+test_expect_success 'resolve situation by aborting the notes merge' '
+	git notes merge --abort &&
+	# No .git/NOTES_MERGE_* files left
+	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	test_cmp /dev/null output &&
+	# m has not moved (still == w)
+	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+	# Verify that other notes refs has not changed (w, x, y and z)
+	verify_notes w &&
+	verify_notes x &&
+	verify_notes y &&
+	verify_notes z
+'
+
 test_done

-- 
1.7.3.98.g5ad7d9
Previous: Johan HerlandNext: Johan Herland
Message 19 of 31 in “[PATCHv5 00/23] git notes merge”
  1. Johan HerlandOct 25, 2010
  2. 01/23 notes.c: Hexify SHA1 in die() message from init_notes()Johan Herland, Oct 25, 2010
  3. 02/23 (trivial) notes.h: Minor documentation fixes to copy_notes()Johan Herland, Oct 25, 2010
  4. 03/23 notes.h: Make default_notes_ref() available in notes APIJohan Herland, Oct 25, 2010
  5. 04/23 notes.c: Reorder functions in preparation for next commitJohan Herland, Oct 25, 2010
  6. 05/23 notes.h/c: Allow combine_notes functions to remove notesJohan Herland, Oct 25, 2010
  7. 06/23 notes.h/c: Propagate combine_notes_fn return value to add_note() and beyondJohan Herland, Oct 25, 2010
  8. 07/23 (trivial) t3303: Indent with tabs instead of spaces for consistencyJohan Herland, Oct 25, 2010
  9. 08/23 notes.c: Use two newlines (instead of one) when concatenating notesJohan Herland, Oct 25, 2010
  10. 09/23 builtin/notes.c: Split notes ref DWIMmery into a separate functionJohan Herland, Oct 25, 2010
  11. 10/23 git notes merge: Initial implementation handling trivial merges onlyJohan Herland, Oct 25, 2010
  12. 11/23 builtin/notes.c: Refactor creation of notes commits.Johan Herland, Oct 25, 2010
  13. 12/23 git notes merge: Handle real, non-conflicting notes mergesJohan Herland, Oct 25, 2010
  14. 13/23 git notes merge: Add automatic conflict resolvers (ours, theirs, union)Johan Herland, Oct 25, 2010
  15. 14/23 Documentation: Preliminary docs on 'git notes merge'Johan Herland, Oct 25, 2010
  16. 15/23 git notes merge: Manual conflict resolution, part 1/2Johan Herland, Oct 25, 2010
  17. 16/23 git notes merge: Manual conflict resolution, part 2/2Johan Herland, Oct 25, 2010
  18. 17/23 git notes merge: List conflicting notes in notes merge commit messageJohan Herland, Oct 25, 2010
  19. 18/23 git notes merge: --commit should fail if underlying notes ref has movedJohan Herland, Oct 25, 2010
  20. 19/23 git notes merge: Add another auto-resolving strategy: "cat_sort_uniq"Johan Herland, Oct 25, 2010
  21. 20/23 git notes merge: Add testcases for merging notes trees at different fanoutsJohan Herland, Oct 25, 2010
  22. Junio C HamanoOct 29, 2010
  23. Johan HerlandNov 1, 2010
  24. 21/23 Provide 'git notes get-ref' to easily retrieve current notes refJohan Herland, Oct 25, 2010
  25. 22/23 cmd_merge(): Parse options before checking MERGE_HEADJohan Herland, Oct 25, 2010
  26. 23/23 Provide 'git merge --abort' as a synonym to 'git reset --merge'Johan Herland, Oct 25, 2010
  27. Jonathan NiederOct 25, 2010
  28. Johan HerlandOct 25, 2010
  29. Johan HerlandOct 26, 2010
  30. 22/23 cmd_merge(): Parse options before checking MERGE_HEADJohan Herland, Oct 26, 2010
  31. 23/23 Provide 'git merge --abort' as a synonym to 'git reset --merge'Johan Herland, Oct 26, 2010

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.