Re: [PATCH v2 4/4] implement automatic fast forward merge for submodules

On Tuesday 06 July 2010, Heiko Voigt wrote:

This implements a simple merge strategy for submodule hashes. We check
whether one side of the merge candidates is already contained in the
other and then merge automatically.

If both sides contain changes we search for a merge in the submodule.
In case a single one exists we check that out and suggest it as the
merge resolution. A list of candidates is returned when we find multiple
merges that contain both sides of the changes.

This is useful for a workflow in which the developers can publish topic
branches in submodules and a seperate maintainer merges them. In case
the developers always wait until their branch gets merged before tracking
them in the superproject all merges of branches that contain submodule
changes will be resolved automatically. If developers choose to track
their feature branch the maintainer might get a conflict but git will
search the submodule for a merge and suggest it/them as a resolution.

Signed-off-by: Heiko Voigt <redacted>

In the commit subject: s/fast forward/fast-forward/

+static int find_first_merges(struct object_array *result, const char *path,
+		struct commit *a, struct commit *b)

[...]

+	/* get all revisions that merge commit a */
+	snprintf(merged_revision, sizeof(merged_revision), "^%s",
+			find_unique_abbrev(a->object.sha1, 40));

Why do you call find_unique_abbrev(..., 40) here?
Isn't sha1_to_hex(a->object.sha1) a better solution?

+	init_revisions(&revs, NULL);
+	rev_opts.submodule = path;
+	setup_revisions(sizeof(rev_args)/sizeof(char *)-1, rev_args, &revs, &rev_opts);
+
+	/* save all revisions from the above list that contain b */
+	if (prepare_revision_walk(&revs))
+		die("revision walk setup failed");
+	while ((commit = get_revision(&revs)) != NULL) {
+		struct object *o = &(commit->object);
+		if (in_merge_bases(b, (struct commit **) &o, 1)) {

Why not s/(struct commit **) &o/&commit/ ?

+static void print_commit(struct commit *commit)
+{
+	static const char *format = " %h: %m %s";

You don't need this 'format' variable; instead use the literal string below.

+	struct strbuf sb = STRBUF_INIT;
+	struct pretty_print_context ctx = {0};
+	ctx.date_mode = DATE_NORMAL;
+	format_commit_message(commit, format, &sb, &ctx);
+	strbuf_addstr(&sb, "\n");
+	fprintf(stderr, "%s", sb.buf);

Drop the strbuf_addstr() and add the newline directly in the fprintf()
format:

fprintf(stderr, "%s\n", sb.buf);

+int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
+		    const unsigned char a[20], const unsigned char b[20])

Watch your line wrapping. Max 80 chars.

+		warning("Submodule rewound can not merge");

s/Submodule rewound can not merge/
  Failed to merge submodule %s (commits don't follow merge-base)/
	

+	/* 1. case a is contained in b or vice versa */

s/1. case/Case #1:/

+	/* 2. case there are one ore more merges that contain a and b in
+	 * the submodule. If there is a single one present it as
+	 * suggestion to the user but leave it marked unmerged so the
+	 * user needs to confirm the resolution.
+	 */

s/2. case/Case #2:/

s/one ore more/one or more/

s/If there is a single one present it as suggestion to the user but/
  If there is only one, then present it a suggestion to the user, but/

+	/* find commit which merges them */
+	parent_count = find_first_merges(&merges, path, commit_a, commit_b);
+	if (!parent_count) {
+		warning("Failed to merge submodule %s (merge not found)", path);

s/merge not found/merge following commits not found/

BTW, what about a

#define MERGE_WARNING(path, msg) \
	warning("Failed to merge submodule %s (%s)", path, msg);

towards the top of merge_submodule, so you don't have to repeat
the same string over and over?

+		goto finish;
+	}
+
+	if (parent_count != 1) {
+		warning("Failed to merge submodule %s (multiple merges found):", path);
+		for (i = 0; i < merges.nr; i++) {
+			print_commit((struct commit *) merges.objects[i].item);
+		}
+		goto finish;
+	}
+
+	warning("Failed to merge submodule %s (not fast-forward):\n", path);
+	fprintf(stderr, "Found a possible merge resolution for the submodule:\n");
+	print_commit((struct commit *) merges.objects[0].item);
+	fprintf(stderr, "If this is correct simply add it to the index for example\n"
+			"by using:\n\n"
+			"  git update-index --cacheinfo 160000 %s \"%s\"\n\n"
+			"which will accept this suggestion.\n",
+		sha1_to_hex(merges.objects[0].item->sha1), path);
+
+finish:
+	free(merges.objects);
+	return 0;
+}

Hmm, you can get rid of the 'goto finish' by restructuring the code like this:

parent_count = find_first_merges(&merges, path, commit_a, commit_b);
switch (parent_count) {
case 0:
	MERGE_WARNING(path, "merge following commits not found");
	break;
case 1:
	MERGE_WARNING(path, "not fast-forward");
	fprintf(...
	...
	break;
default:
	MERGE_WARNING(path, "multiple merges found");
	for (i = 0; i < merges.nr; i++)
		print_commit((struct commit *) merges.objects[i].item);
}

free(merges.objects);
return 0;

If you just need to create a new branch, but not necessarily check it out,
then use 'git branch <name>' instead of 'git checkout -b <name>'.

+	git add sub &&
+	git commit -m "a" &&
+	git checkout -b a &&

Same as above

+	git checkout -b b &&
+	(cd sub &&
+	 git checkout -b sub-b &&
+	 echo "file-b" > file-b &&
+	 git add file-b &&
+	 git commit -m "sub-b") &&
+	git commit -a -m "b" &&
+
+	git checkout -b c a &&
+	(cd sub &&
+	 git checkout -b sub-c sub-a &&
+	 echo "file-c" > file-c &&
+	 git add file-c &&
+	 git commit -m "sub-c") &&
+	git commit -a -m "c" &&
+
+	git checkout -b d a &&
+	(cd sub &&
+	 git checkout -b sub-d sub-b &&
+	 git merge sub-c) &&
+	git commit -a -m "d" &&
+	git checkout -b test b &&

same as above

+	cd ..
+'
+
+test_expect_success 'merge with one side as a fast-forward of the other' '
+	(cd merge-search &&
+	 git checkout -b test-forward b &&
+	 git merge d &&
+	 git ls-tree test-forward | grep sub | cut -f1 | cut -f3 -d" " > actual &&

Use "git ls-tree test-forward sub" instead of "git ls-tree test-forward | grep sub"

+	 (cd sub &&
+	  git rev-parse sub-d > ../expect) &&
+	 test_cmp actual expect)
+'
+
+test_expect_success 'merging should conflict for non fast-forward' '
+	(cd merge-search &&
+	 git checkout -b test-nonforward b &&
+	 (cd sub &&
+	  git rev-parse sub-d > ../expect) &&
+	 test_must_fail 	git merge c 2> actual  &&
+	 grep $(<expect) actual > /dev/null &&

I cannot find the "$(<expect)" construct anywhere else in out test scripts.
Is it portable? Should we maybe use "$(cat expect)" instead?

+	 git reset --hard)
+'
+
+test_expect_success 'merging should fail for ambigous common parent' '

s/ambigous/ambiguous/

+	cd merge-search &&
+	git checkout -b test-ambigous b &&
+	(cd sub &&
+	 git checkout -b ambigous sub-b &&
+	 git merge sub-c &&
+	 git rev-parse sub-d > ../expect1 &&
+	 git rev-parse ambigous > ../expect2) &&

s/ambigous/ambiguous/

+	test_must_fail git merge c 2> actual &&
+	grep $(<expect1) actual > /dev/null &&
+	grep $(<expect2) actual > /dev/null &&

Same $(<foo) construct...

+	git reset --hard &&
+	cd ..
+'
+
+# in a situation like this
+#
+# submodule tree:
+#
+#    sub-a --- sub-b --- sub-d
+#
+# main tree:
+#
+#    e (sub-a)
+#   /
+#  bb (sub-b)
+#   \
+#    f (sub-d)
+#
+# A merge should fail because one change points backwards.

Instead: A merge between e and f should fail because one of the submodule
commits (sub-a) don't descend from the submodule merge-base (sub-b).


Otherwise, it looks good to me. Thanks for the effort!


Have fun! :)

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net