Re: [PATCH v2 4/4] implement automatic fast forward merge for submodules
- From
Johan Herland <johan@herland.net>
- Date
- Jul 6, 2010, 23:52 UTC
- Message-ID
- <201007070152.17802.johan@herland.net>
- In-Reply-To
- <f17d78656a71558fd290e0b84cad03f22f6fcbd8.1278444110.git.hvoigt@hvoigt.net>
On Tuesday 06 July 2010, Heiko Voigt wrote:
Show 18 quoted lines
> 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?
Show 10 quoted lines
> + 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.
Show 6 quoted lines
> + 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:/
Show 5 quoted lines
> + /* 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?
Show 24 quoted lines
> + 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;
Show 31 quoted lines
> 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
Show 22 quoted lines
> + 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
Show 8 quoted lines
> + 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"
Show 12 quoted lines
> + (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/
Show 7 quoted lines
> + 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...
Show 19 quoted lines
> + 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