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

[Johan Herland <johan@herland.net>]

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 <hvoigt@hvoigt.net>

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

> diff --git a/submodule.c b/submodule.c

> +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;


> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh
> index 4a7b893..0f568ab 100755
> --- a/t/t7405-submodule-merge.sh
> +++ b/t/t7405-submodule-merge.sh
> @@ -54,13 +54,128 @@ test_expect_success setup '
>  	git merge -s ours a
>  '
> 
> -test_expect_success 'merging with modify/modify conflict' '
> +# History setup
> +#
> +#      b
> +#    /   \
> +#   a     d
> +#    \   /
> +#      c
> +#
> +# a in the main repository records to sub-a in the submodule and
> +# analogous b and c. d should be automatically found by merging c into
> +# b in the main repository.
> +test_expect_success 'setup for merge search' '
> +	mkdir merge-search &&
> +	cd merge-search &&
> +	git init &&
> +	mkdir sub &&
> +	(cd sub &&
> +	 git init &&
> +	 echo "file-a" > file-a &&
> +	 git add file-a &&
> +	 git commit -m "sub-a" &&
> +	 git checkout -b sub-a) &&

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, <johan@herland.net>
www.herland.net