git notes merge: Manual conflict resolution, part 2/2

When the notes merge conflicts in .git/NOTES_MERGE_WORKTREE have been
resolved, we need to record a new notes commit on the appropriate notes
ref with the resolved notes.

This patch implements 'git notes merge --commit' which the user should
run after resolving conflicts in the notes merge worktree. This command
finalizes the notes merge by recombining the partial notes tree from
part 1 with the now-resolved conflicts in the notes merge worktree in a
merge commit, and updating the appropriate ref to this merge commit.

In order to correctly finalize the merge, we need to keep track of three
things:

- The partial merge result from part 1, containing the auto-merged notes.
  This is now stored into a ref called .git/NOTES_MERGE_PARTIAL.
- The unmerged notes. These are already stored in
  .git/NOTES_MERGE_WORKTREE, thanks to part 1.
- The notes ref to be updated by the finalized merge result. This is now
  stored in a symref called .git/NOTES_MERGE_REF.

In addition to "git notes merge --commit", which uses the above details
to create the finalized notes merge commit, this patch also implements
"git notes merge --reset", which aborts the ongoing notes merge by simply
removing the files/directory described above.

FTR, "git notes merge --commit" reuses "git notes merge --reset" to remove
the information described above (.git/NOTES_MERGE_*) after the notes merge
have been successfully finalized.

The patch also contains documentation and testcases for the two new options.

This patch has been improved by the following contributions:
- Ævar Arnfjörð Bjarmason: Fix nonsense sentence in --commit description
- Sverre Rabbelier: Rename --reset to --abort

Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Thanks-to: Sverre Rabbelier <srabbelier@gmail.com>
Signed-off-by: Johan Herland <johan@herland.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Documentation/git-notes.txt

@@ -15,6 +15,8 @@ SYNOPSIS
 'git notes' edit [<object>]
 'git notes' show [<object>]
 'git notes' merge [-v | -q] [-s <strategy> ] <notes_ref>
+'git notes' merge --commit [-v | -q]
+'git notes' merge --abort [-v | -q]
 'git notes' remove [<object>]
 'git notes' prune [-n | -v]
 
@@ -95,6 +97,9 @@ conflicting notes (see the -s/--strategy option) is not given,
 the "manual" resolver is used. This resolver checks out the
 conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
 and instructs the user to manually resolve the conflicts there.
+When done, the user can either finalize the merge with
+'git notes merge --commit', or abort the merge with
+'git notes merge --abort'.
 
 remove::
 	Remove the notes for a given object (defaults to HEAD).
@@ -154,6 +159,20 @@ OPTIONS
 	See the "NOTES MERGE STRATEGIES" section below for more
 	information on each notes merge strategy.
 
+--commit::
+	Finalize an in-progress 'git notes merge'. Use this option
+	when you have resolved the conflicts that 'git notes merge'
+	stored in .git/NOTES_MERGE_WORKTREE. This amends the partial
+	merge commit created by 'git notes merge' (stored in
+	.git/NOTES_MERGE_PARTIAL) by adding the notes in
+	.git/NOTES_MERGE_WORKTREE. The notes ref stored in the
+	.git/NOTES_MERGE_REF symref is updated to the resulting commit.
+
+--abort::
+	Abort/reset a in-progress 'git notes merge', i.e. a notes merge
+	with conflicts. This simply removes all files related to the
+	notes merge.
+
 -q::
 --quiet::
 	When merging notes, operate quietly.
@@ -197,6 +216,9 @@ The default notes merge strategy is "manual", which checks out
 conflicting notes in a special work tree for resolving notes conflicts
 (`.git/NOTES_MERGE_WORKTREE`), and instructs the user to resolve the
 conflicts in that work tree.
+When done, the user can either finalize the merge with
+'git notes merge --commit', or abort the merge with
+'git notes merge --abort'.
 
 "ours" automatically resolves conflicting notes in favor of the local
 version (i.e. the current notes ref).

builtin/notes.c

@@ -27,6 +27,8 @@ static const char * const git_notes_usage[] = {
 	"git notes [--ref <notes_ref>] edit [<object>]",
 	"git notes [--ref <notes_ref>] show [<object>]",
 	"git notes [--ref <notes_ref>] merge [-v | -q] [-s <strategy> ] <notes_ref>",
+	"git notes merge --commit [-v | -q]",
+	"git notes merge --abort [-v | -q]",
 	"git notes [--ref <notes_ref>] remove [<object>]",
 	"git notes [--ref <notes_ref>] prune [-n | -v]",
 	NULL
@@ -65,6 +67,8 @@ static const char * const git_notes_show_usage[] = {
 
 static const char * const git_notes_merge_usage[] = {
 	"git notes merge [<options>] <notes_ref>",
+	"git notes merge --commit [<options>]",
+	"git notes merge --abort [<options>]",
 	NULL
 };
 
@@ -761,33 +765,119 @@ static int show(int argc, const char **argv, const char *prefix)
 	return retval;
 }
 
+static int merge_abort(struct notes_merge_options *o)
+{
+	int ret = 0;
+
+	/*
+	 * Remove .git/NOTES_MERGE_PARTIAL and .git/NOTES_MERGE_REF, and call
+	 * notes_merge_abort() to remove .git/NOTES_MERGE_WORKTREE.
+	 */
+
+	if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0))
+		ret += error("Failed to delete ref NOTES_MERGE_PARTIAL");
+	if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF))
+		ret += error("Failed to delete ref NOTES_MERGE_REF");
+	if (notes_merge_abort(o))
+		ret += error("Failed to remove 'git notes merge' worktree");
+	return ret;
+}
+
+static int merge_commit(struct notes_merge_options *o)
+{
+	struct strbuf msg = STRBUF_INIT;
+	unsigned char sha1[20];
+	struct notes_tree *t;
+	struct commit *partial;
+	struct pretty_print_context pretty_ctx;
+
+	/*
+	 * Read partial merge result from .git/NOTES_MERGE_PARTIAL,
+	 * and target notes ref from .git/NOTES_MERGE_REF.
+	 */
+
+	if (get_sha1("NOTES_MERGE_PARTIAL", sha1))
+		die("Failed to read ref NOTES_MERGE_PARTIAL");
+	else if (!(partial = lookup_commit_reference(sha1)))
+		die("Could not find commit from NOTES_MERGE_PARTIAL.");
+	else if (parse_commit(partial))
+		die("Could not parse commit from NOTES_MERGE_PARTIAL.");
+
+	t = xcalloc(1, sizeof(struct notes_tree));
+	init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
+
+	o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, 0);
+	if (!o->local_ref)
+		die("Failed to resolve NOTES_MERGE_REF");
+
+	if (notes_merge_commit(o, t, partial, sha1))
+		die("Failed to finalize notes merge");
+
+	/* Reuse existing commit message in reflog message */
+	memset(&pretty_ctx, 0, sizeof(pretty_ctx));
+	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);
+
+	free_notes(t);
+	strbuf_release(&msg);
+	return merge_abort(o);
+}
+
 static int merge(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
 	unsigned char result_sha1[20];
 	struct notes_tree *t;
 	struct notes_merge_options o;
+	int do_merge = 0, do_commit = 0, do_abort = 0;
 	int verbosity = 0, result;
 	const char *strategy = NULL;
 	struct option options[] = {
+		OPT_GROUP("General options"),
 		OPT__VERBOSITY(&verbosity),
+		OPT_GROUP("Merge options"),
 		OPT_STRING('s', "strategy", &strategy, "strategy",
 			   "resolve notes conflicts using the given "
 			   "strategy (manual/ours/theirs/union)"),
+		OPT_GROUP("Committing unmerged notes"),
+		{ OPTION_BOOLEAN, 0, "commit", &do_commit, NULL,
+			"finalize notes merge by committing unmerged notes",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG },
+		OPT_GROUP("Aborting notes merge resolution"),
+		{ OPTION_BOOLEAN, 0, "abort", &do_abort, NULL,
+			"abort notes merge",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG },
 		OPT_END()
 	};
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_notes_merge_usage, 0);
 
-	if (argc != 1) {
+	if (strategy || do_commit + do_abort == 0)
+		do_merge = 1;
+	if (do_merge + do_commit + do_abort != 1) {
+		error("cannot mix --commit, --abort or -s/--strategy");
+		usage_with_options(git_notes_merge_usage, options);
+	}
+
+	if (do_merge && argc != 1) {
 		error("Must specify a notes ref to merge");
 		usage_with_options(git_notes_merge_usage, options);
+	} else if (!do_merge && argc) {
+		error("too many parameters");
+		usage_with_options(git_notes_merge_usage, options);
 	}
 
 	init_notes_merge_options(&o);
 	o.verbosity = verbosity + NOTES_MERGE_VERBOSITY_DEFAULT;
 
+	if (do_abort)
+		return merge_abort(&o);
+	if (do_commit)
+		return merge_commit(&o);
+
 	o.local_ref = default_notes_ref();
 	strbuf_addstr(&remote_ref, argv[0]);
 	expand_notes_ref(&remote_ref);
@@ -820,9 +910,19 @@ static int merge(int argc, const char **argv, const char *prefix)
 		/* Update default notes ref with new commit */
 		update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
 			   0, DIE_ON_ERR);
-	else /* Merge has unresolved conflicts */
-		printf("Automatic notes merge failed. Fix conflicts in %s.\n",
+	else { /* Merge has unresolved conflicts */
+		/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */
+		update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL,
+			   0, DIE_ON_ERR);
+		/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
+		if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
+			die("Failed to store link to current notes ref (%s)",
+			    default_notes_ref());
+		printf("Automatic notes merge failed. Fix conflicts in %s and "
+		       "commit the result with 'git notes merge --commit', or "
+		       "abort the merge with 'git notes merge --abort'.\n",
 		       git_path(NOTES_MERGE_WORKTREE));
+	}
 
 	free_notes(t);
 	strbuf_release(&remote_ref);

notes-merge.c

@@ -278,7 +278,7 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
 				die("You have not concluded your previous "
 				    "notes merge (%s exists).\nPlease, use "
 				    "'git notes merge --commit' or 'git notes "
-				    "merge --reset' to commit/abort the "
+				    "merge --abort' to commit/abort the "
 				    "previous merge before you start a new "
 				    "notes merge.", git_path("NOTES_MERGE_*"));
 			else
@@ -650,3 +650,72 @@ found_result:
 	       result, sha1_to_hex(result_sha1));
 	return result;
 }
+
+int notes_merge_commit(struct notes_merge_options *o,
+		       struct notes_tree *partial_tree,
+		       struct commit *partial_commit,
+		       unsigned char *result_sha1)
+{
+	/*
+	 * Iterate through files in .git/NOTES_MERGE_WORKTREE and add all
+	 * found notes to 'partial_tree'. Write the updates notes tree to
+	 * the DB, and commit the resulting tree object while reusing the
+	 * commit message and parents from 'partial_commit'.
+	 * Finally store the new commit object SHA1 into 'result_sha1'.
+	 */
+	struct dir_struct dir;
+	const char *path = git_path(NOTES_MERGE_WORKTREE "/");
+	int path_len = strlen(path), i;
+	const char *msg = strstr(partial_commit->buffer, "\n\n");
+
+	OUTPUT(o, 3, "Committing notes in notes merge worktree at %.*s",
+	       path_len - 1, path);
+
+	if (!msg || msg[2] == '\0')
+		die("partial notes commit has empty message");
+	msg += 2;
+
+	memset(&dir, 0, sizeof(dir));
+	read_directory(&dir, path, path_len, NULL);
+	for (i = 0; i < dir.nr; i++) {
+		struct dir_entry *ent = dir.entries[i];
+		struct stat st;
+		const char *relpath = ent->name + path_len;
+		unsigned char obj_sha1[20], blob_sha1[20];
+
+		if (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {
+			OUTPUT(o, 3, "Skipping non-SHA1 entry '%s'", ent->name);
+			continue;
+		}
+
+		/* write file as blob, and add to partial_tree */
+		if (stat(ent->name, &st))
+			die_errno("Failed to stat '%s'", ent->name);
+		if (index_path(blob_sha1, ent->name, &st, 1))
+			die("Failed to write blob object from '%s'", ent->name);
+		if (add_note(partial_tree, obj_sha1, blob_sha1, NULL))
+			die("Failed to add resolved note '%s' to notes tree",
+			    ent->name);
+		OUTPUT(o, 4, "Added resolved note for object %s: %s",
+		       sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
+	}
+
+	create_notes_commit(partial_tree, partial_commit->parents, msg,
+			    result_sha1);
+	OUTPUT(o, 4, "Finalized notes merge commit: %s",
+	       sha1_to_hex(result_sha1));
+	return 0;
+}
+
+int notes_merge_abort(struct notes_merge_options *o)
+{
+	/* Remove .git/NOTES_MERGE_WORKTREE directory and all files within */
+	struct strbuf buf = STRBUF_INIT;
+	int ret;
+
+	strbuf_addstr(&buf, git_path(NOTES_MERGE_WORKTREE));
+	OUTPUT(o, 3, "Removing notes merge worktree at %s", buf.buf);
+	ret = remove_dir_recursively(&buf, 0);
+	strbuf_release(&buf);
+	return ret;
+}

notes-merge.h

@@ -71,4 +71,27 @@ int notes_merge(struct notes_merge_options *o,
 		struct notes_tree *local_tree,
 		unsigned char *result_sha1);
 
+/*
+ * Finalize conflict resolution from an earlier notes_merge()
+ *
+ * The given notes tree 'partial_tree' must be the notes_tree corresponding to
+ * the given 'partial_commit', the partial result commit created by a previous
+ * call to notes_merge().
+ *
+ * This function will add the (now resolved) notes in .git/NOTES_MERGE_WORKTREE
+ * to 'partial_tree', and create a final notes merge commit, the SHA1 of which
+ * will be stored in 'result_sha1'.
+ */
+int notes_merge_commit(struct notes_merge_options *o,
+		       struct notes_tree *partial_tree,
+		       struct commit *partial_commit,
+		       unsigned char *result_sha1);
+
+/*
+ * Abort conflict resolution from an earlier notes_merge()
+ *
+ * Removes the notes merge worktree in .git/NOTES_MERGE_WORKTREE.
+ */
+int notes_merge_abort(struct notes_merge_options *o);
+
 #endif

t/t3310-notes-merge-manual-resolve.sh

@@ -171,6 +171,7 @@ cp expect_notes_y expect_notes_m
 cp expect_log_y expect_log_m
 
 git rev-parse refs/notes/y > pre_merge_y
+git rev-parse refs/notes/z > pre_merge_z
 
 test_expect_success 'merge z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
 	git update-ref refs/notes/m refs/notes/y &&
@@ -289,4 +290,179 @@ test_expect_success 'can do merge without conflicts even if previous merge is un
 	verify_notes y
 '
 
+cat <<EOF | sort >expect_notes_m
+021faa20e931fb48986ffc6282b4bb05553ac946 $commit_sha4
+5772f42408c0dd6f097a7ca2d24de0e78d1c46b1 $commit_sha3
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+0a59e787e6d688aa6309e56e8c1b89431a0fc1c1 $commit_sha1
+EOF
+
+cat >expect_log_m <<EOF
+$commit_sha5 5th
+
+$commit_sha4 4th
+y and z notes on 4th commit
+
+$commit_sha3 3rd
+y notes on 3rd commit
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+y and z notes on 1st commit
+
+EOF
+
+test_expect_success 'finalize conflicting merge (z => m)' '
+	# Resolve conflicts and finalize merge
+	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
+	git notes merge --commit &&
+	# No .git/NOTES_MERGE_* files left
+	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	test_cmp /dev/null output &&
+	# Merge commit has pre-merge y and pre-merge z as parents
+	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
+	test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
+	# Merge commit mentions the notes refs merged
+	git log -1 --format=%B refs/notes/m > merge_commit_msg &&
+	grep -q refs/notes/m merge_commit_msg &&
+	grep -q refs/notes/z merge_commit_msg &&
+	# Verify contents of merge result
+	verify_notes m &&
+	# 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
+'
+
+cat >expect_conflict_$commit_sha4 <<EOF
+<<<<<<< refs/notes/m
+y notes on 4th commit
+=======
+z notes on 4th commit
+
+More z notes on 4th commit
+>>>>>>> refs/notes/z
+EOF
+
+cp expect_notes_y expect_notes_m
+cp expect_log_y expect_log_m
+
+git rev-parse refs/notes/y > pre_merge_y
+git rev-parse refs/notes/z > pre_merge_z
+
+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 &&
+	git config core.notesRef refs/notes/m &&
+	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)"
+'
+
+test_expect_success 'abort 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 == y)
+	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	# 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
+'
+
+git rev-parse refs/notes/y > pre_merge_y
+git rev-parse refs/notes/z > pre_merge_z
+
+test_expect_success 'redo merge of z into m (== y) with default ("manual") resolver => Conflicting 3-way merge' '
+	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)"
+'
+
+cat <<EOF | sort >expect_notes_m
+304dfb4325cf243025b9957486eb605a9b51c199 $commit_sha5
+283b48219aee9a4105f6cab337e789065c82c2b9 $commit_sha2
+0a59e787e6d688aa6309e56e8c1b89431a0fc1c1 $commit_sha1
+EOF
+
+cat >expect_log_m <<EOF
+$commit_sha5 5th
+new note on 5th commit
+
+$commit_sha4 4th
+
+$commit_sha3 3rd
+
+$commit_sha2 2nd
+z notes on 2nd commit
+
+$commit_sha1 1st
+y and z notes on 1st commit
+
+EOF
+
+test_expect_success 'add + remove notes in finalized merge (z => m)' '
+	# Resolve one conflict
+	cat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&
+y and z notes on 1st commit
+EOF
+	# Remove another conflict
+	rm .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
+	# Remove a D/F conflict
+	rm .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
+	# Add a new note
+	echo "new note on 5th commit" > .git/NOTES_MERGE_WORKTREE/$commit_sha5 &&
+	# Finalize merge
+	git notes merge --commit &&
+	# No .git/NOTES_MERGE_* files left
+	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
+	test_cmp /dev/null output &&
+	# Merge commit has pre-merge y and pre-merge z as parents
+	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
+	test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
+	# Merge commit mentions the notes refs merged
+	git log -1 --format=%B refs/notes/m > merge_commit_msg &&
+	grep -q refs/notes/m merge_commit_msg &&
+	grep -q refs/notes/z merge_commit_msg &&
+	# Verify contents of merge result
+	verify_notes m &&
+	# 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