From 9e62316df7f918eabc879f97da76543cc5f0b3fc Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 29 Apr 2015 13:14:50 -0700 Subject: [PATCH 01/15] merge: test the top-level merge driver We seem to have tests for specific merge strategy backends (e.g. recursive), but not much test coverage for the "git merge" itself. As I am planning to update the semantics of merging "FETCH_HEAD" in such a way that these two git pull . topic_a topic_b... vs. git fetch . topic_a topic_b... git merge FETCH_HEAD are truly equivalent, let me add a few test cases to cover the tricky ones. Signed-off-by: Junio C Hamano --- t/t3033-merge-toplevel.sh | 136 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100755 t/t3033-merge-toplevel.sh diff --git a/t/t3033-merge-toplevel.sh b/t/t3033-merge-toplevel.sh new file mode 100755 index 0000000000..9d92d3c1a2 --- /dev/null +++ b/t/t3033-merge-toplevel.sh @@ -0,0 +1,136 @@ +#!/bin/sh + +test_description='"git merge" top-level frontend' + +. ./test-lib.sh + +t3033_reset () { + git checkout -B master two && + git branch -f left three && + git branch -f right four +} + +test_expect_success setup ' + test_commit one && + git branch left && + git branch right && + test_commit two && + git checkout left && + test_commit three && + git checkout right && + test_commit four && + git checkout master +' + +# Local branches + +test_expect_success 'merge an octopus into void' ' + t3033_reset && + git checkout --orphan test && + git rm -fr . && + test_must_fail git merge left right && + test_must_fail git rev-parse --verify HEAD && + git diff --quiet && + test_must_fail git rev-parse HEAD +' + +test_expect_success 'merge an octopus, fast-forward (ff)' ' + t3033_reset && + git reset --hard one && + git merge left right && + # one is ancestor of three (left) and four (right) + test_must_fail git rev-parse --verify HEAD^3 && + git rev-parse HEAD^1 HEAD^2 | sort >actual && + git rev-parse three four | sort >expect && + test_cmp expect actual +' + +test_expect_success 'merge octopus, non-fast-forward (ff)' ' + t3033_reset && + git reset --hard one && + git merge --no-ff left right && + # one is ancestor of three (left) and four (right) + test_must_fail git rev-parse --verify HEAD^4 && + git rev-parse HEAD^1 HEAD^2 HEAD^3 | sort >actual && + git rev-parse one three four | sort >expect && + test_cmp expect actual +' + +test_expect_success 'merge octopus, fast-forward (does not ff)' ' + t3033_reset && + git merge left right && + # two (master) is not an ancestor of three (left) and four (right) + test_must_fail git rev-parse --verify HEAD^4 && + git rev-parse HEAD^1 HEAD^2 HEAD^3 | sort >actual && + git rev-parse two three four | sort >expect && + test_cmp expect actual +' + +test_expect_success 'merge octopus, non-fast-forward' ' + t3033_reset && + git merge --no-ff left right && + test_must_fail git rev-parse --verify HEAD^4 && + git rev-parse HEAD^1 HEAD^2 HEAD^3 | sort >actual && + git rev-parse two three four | sort >expect && + test_cmp expect actual +' + +# The same set with FETCH_HEAD + +test_expect_failure 'merge FETCH_HEAD octopus into void' ' + t3033_reset && + git checkout --orphan test && + git rm -fr . && + git fetch . left right && + test_must_fail git merge FETCH_HEAD && + test_must_fail git rev-parse --verify HEAD && + git diff --quiet && + test_must_fail git rev-parse HEAD +' + +test_expect_failure 'merge FETCH_HEAD octopus fast-forward (ff)' ' + t3033_reset && + git reset --hard one && + git fetch . left right && + git merge FETCH_HEAD && + # one is ancestor of three (left) and four (right) + test_must_fail git rev-parse --verify HEAD^3 && + git rev-parse HEAD^1 HEAD^2 | sort >actual && + git rev-parse three four | sort >expect && + test_cmp expect actual +' + +test_expect_failure 'merge FETCH_HEAD octopus non-fast-forward (ff)' ' + t3033_reset && + git reset --hard one && + git fetch . left right && + git merge --no-ff FETCH_HEAD && + # one is ancestor of three (left) and four (right) + test_must_fail git rev-parse --verify HEAD^4 && + git rev-parse HEAD^1 HEAD^2 HEAD^3 | sort >actual && + git rev-parse one three four | sort >expect && + test_cmp expect actual +' + +test_expect_failure 'merge FETCH_HEAD octopus fast-forward (does not ff)' ' + t3033_reset && + git fetch . left right && + git merge FETCH_HEAD && + # two (master) is not an ancestor of three (left) and four (right) + test_must_fail git rev-parse --verify HEAD^4 && + git rev-parse HEAD^1 HEAD^2 HEAD^3 | sort >actual && + git rev-parse two three four | sort >expect && + test_cmp expect actual +' + +test_expect_failure 'merge FETCH_HEAD octopus non-fast-forward' ' + t3033_reset && + git fetch . left right && + git merge --no-ff FETCH_HEAD && + test_must_fail git rev-parse --verify HEAD^4 && + git rev-parse HEAD^1 HEAD^2 HEAD^3 | sort >actual && + git rev-parse two three four | sort >expect && + test_cmp expect actual +' + +test_done From 00c7e7e7e8a2aa07b2cd3a09c8f9e11872727d86 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 23 Apr 2015 13:01:44 -0700 Subject: [PATCH 02/15] merge: simplify code flow One of the first things cmd_merge() does is to see if the "--abort" option is given and run "reset --merge" and exit. When the control reaches this point, we know "--abort" was not given. Signed-off-by: Junio C Hamano --- builtin/merge.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index bebbe5b308..8477878442 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1165,15 +1165,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix) option_commit = 0; } - if (!abort_current_merge) { - if (!argc) { - if (default_to_upstream) - argc = setup_with_upstream(&argv); - else - die(_("No commit specified and merge.defaultToUpstream not set.")); - } else if (argc == 1 && !strcmp(argv[0], "-")) - argv[0] = "@{-1}"; + if (!argc) { + if (default_to_upstream) + argc = setup_with_upstream(&argv); + else + die(_("No commit specified and merge.defaultToUpstream not set.")); + } else if (argc == 1 && !strcmp(argv[0], "-")) { + argv[0] = "@{-1}"; } + if (!argc) usage_with_options(builtin_merge_usage, builtin_merge_options); From 55691133291560eacaf03003528e007a3534213d Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 23 Apr 2015 13:29:14 -0700 Subject: [PATCH 03/15] t5520: style fixes Fix style funnies in early part of this test script that checks "git pull" into an unborn branch. The primary change is that 'chdir' to a newly created empty test repository is now protected by being done in a subshell to make it more robust without having to chdir back to the original place. Signed-off-by: Junio C Hamano --- t/t5520-pull.sh | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 227d293350..5195a218a2 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -9,36 +9,27 @@ modify () { mv "$2.x" "$2" } -D=`pwd` - test_expect_success setup ' - echo file >file && git add file && git commit -a -m original - ' test_expect_success 'pulling into void' ' - mkdir cloned && - cd cloned && - git init && - git pull .. -' - -cd "$D" - -test_expect_success 'checking the results' ' + git init cloned && + ( + cd cloned && + git pull .. + ) && test -f file && test -f cloned/file && test_cmp file cloned/file ' test_expect_success 'pulling into void using master:master' ' - mkdir cloned-uho && + git init cloned-uho && ( cd cloned-uho && - git init && git pull .. master:master ) && test -f file && @@ -71,7 +62,6 @@ test_expect_success 'pulling into void does not overwrite staged files' ' ) ' - test_expect_success 'pulling into void does not remove new staged files' ' git init cloned-staged-new && ( From 7ad39a2784fd2d2ad6538e687404cf7f35a1e771 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 23 Apr 2015 13:34:08 -0700 Subject: [PATCH 04/15] t5520: test pulling an octopus into an unborn branch The code comment for "git merge" in builtin/merge.c, we say If the merged head is a valid one there is no reason to forbid "git merge" into a branch yet to be born. We do the same for "git pull". and t5520 does have an existing test for that behaviour. However, there was no test to make sure that 'git pull' to pull multiple branches into an unborn branch must fail. Signed-off-by: Junio C Hamano --- t/t5520-pull.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 5195a218a2..7efd45bc27 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -76,6 +76,15 @@ test_expect_success 'pulling into void does not remove new staged files' ' ) ' +test_expect_success 'pulling into void must not create an octopus' ' + git init cloned-octopus && + ( + cd cloned-octopus && + test_must_fail git pull .. master master && + ! test -f file + ) +' + test_expect_success 'test . as a remote' ' git branch copy master && From 1faac1cedca475a364c72b063c79da08caee0fe9 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 23 Apr 2015 13:46:44 -0700 Subject: [PATCH 05/15] merge: clarify "pulling into void" special case Instead of having it as one of the three if/elseif/.. case arms, test the condition and handle this special case upfront. This makes it easier to follow the flow of logic. Signed-off-by: Junio C Hamano --- builtin/merge.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 8477878442..878f82a410 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1178,23 +1178,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) usage_with_options(builtin_merge_usage, builtin_merge_options); - /* - * This could be traditional "merge HEAD ..." and - * the way we can tell it is to see if the second token is HEAD, - * but some people might have misused the interface and used a - * commit-ish that is the same as HEAD there instead. - * Traditional format never would have "-m" so it is an - * additional safety measure to check for it. - */ - - if (!have_message && head_commit && - is_old_style_invocation(argc, argv, head_commit->object.sha1)) { - strbuf_addstr(&merge_msg, argv[0]); - head_arg = argv[1]; - argv += 2; - argc -= 2; - remoteheads = collect_parents(head_commit, &head_subsumed, argc, argv); - } else if (!head_commit) { + if (!head_commit) { struct commit *remote_head; /* * If the merged head is a valid one there is no reason @@ -1217,6 +1201,23 @@ int cmd_merge(int argc, const char **argv, const char *prefix) update_ref("initial pull", "HEAD", remote_head->object.sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); goto done; + } + + /* + * This could be traditional "merge HEAD ..." and + * the way we can tell it is to see if the second token is HEAD, + * but some people might have misused the interface and used a + * commit-ish that is the same as HEAD there instead. + * Traditional format never would have "-m" so it is an + * additional safety measure to check for it. + */ + if (!have_message && + is_old_style_invocation(argc, argv, head_commit->object.sha1)) { + strbuf_addstr(&merge_msg, argv[0]); + head_arg = argv[1]; + argv += 2; + argc -= 2; + remoteheads = collect_parents(head_commit, &head_subsumed, argc, argv); } else { struct strbuf merge_names = STRBUF_INIT; From eaa4e59c8545f61c6e61559df33dc4792e455d5a Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 23 Apr 2015 13:56:34 -0700 Subject: [PATCH 06/15] merge: do not check argc to determine number of remote heads To reject merging multiple commits into an unborn branch, we check argc, thinking that collect_parents() that reads the remaining command line arguments from will give us the same number of commits as its input, i.e. argc. Because what we really care about is the number of commits, let the function run and then make sure it returns only one commit instead. Signed-off-by: Junio C Hamano --- builtin/merge.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 878f82a410..1d4fbd3288 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1185,9 +1185,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * to forbid "git merge" into a branch yet to be born. * We do the same for "git pull". */ - if (argc != 1) - die(_("Can merge only exactly one commit into " - "empty head")); if (squash) die(_("Squash commit into empty head not supported yet")); if (fast_forward == FF_NO) @@ -1197,6 +1194,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) remote_head = remoteheads->item; if (!remote_head) die(_("%s - not something we can merge"), argv[0]); + if (remoteheads->next) + die(_("Can merge only exactly one commit into empty head")); read_empty(remote_head->object.sha1, 0); update_ref("initial pull", "HEAD", remote_head->object.sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); From 1016658de3fe213eedebdb478cca0324ac8fbe87 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 23 Apr 2015 14:37:13 -0700 Subject: [PATCH 07/15] merge: small leakfix and code simplification When parsing a merged object name like "foo~20" to formulate a merge summary "Merge branch foo (early part)", a temporary strbuf is used, but we forgot to deallocate it when we failed to find the named branch. Signed-off-by: Junio C Hamano --- builtin/merge.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 1d4fbd3288..b2d03323cb 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -491,8 +491,7 @@ static void merge_name(const char *remote, struct strbuf *msg) } if (len) { struct strbuf truname = STRBUF_INIT; - strbuf_addstr(&truname, "refs/heads/"); - strbuf_addstr(&truname, remote); + strbuf_addf(&truname, "refs/heads/%s", remote); strbuf_setlen(&truname, truname.len - len); if (ref_exists(truname.buf)) { strbuf_addf(msg, @@ -503,6 +502,7 @@ static void merge_name(const char *remote, struct strbuf *msg) strbuf_release(&truname); goto cleanup; } + strbuf_release(&truname); } if (!strcmp(remote, "FETCH_HEAD") && From 0b10b8a3d53c7aaf42b8f14da1021ea59ea4e0ec Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 25 Apr 2015 10:25:43 -0700 Subject: [PATCH 08/15] merge: clarify collect_parents() logic Clarify this small function in three ways. - The function initially collects all commits to be merged into a commit_list "remoteheads"; the "remotes" pointer always points at the tail of this list (either the remoteheads variable itself, or the ->next slot of the element at the end of the list) to help elongate the list by repeated calls to commit_list_insert(). Because the new element appended by commit_list_insert() will always have its ->next slot NULLed out, there is no need for us to assign NULL to *remotes to terminate the list at the end. - The variable "head_subsumed" always confused me every time I read this code. What is happening here is that we inspect what the caller told us to merge (including the current HEAD) and come up with the list of parents to be recorded for the resulting merge commit, omitting commits that are ancestor of other commits. This filtering may remove the current HEAD from the resulting parent list---and we signal that fact with this variable, so that we can later record it as the first parent when "--no-ff" is in effect. - The "parents" list is created for this function by reduce_heads() and was not deallocated after its use, even though the loop control was written in such a way to allow us to do so by taking the "next" element in a separate variable so that it can be used in the next-step part of the loop control. Signed-off-by: Junio C Hamano --- builtin/merge.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index b2d03323cb..d2e36e2051 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1061,11 +1061,19 @@ static struct commit_list *collect_parents(struct commit *head_commit, "not something we can merge"); remotes = &commit_list_insert(commit, remotes)->next; } - *remotes = NULL; + /* + * Is the current HEAD reachable from another commit being + * merged? If so we do not want to record it as a parent of + * the resulting merge, unless --no-ff is given. We will flip + * this variable to 0 when we find HEAD among the independent + * tips being merged. + */ + *head_subsumed = 1; + + /* Find what parents to record by checking independent ones. */ parents = reduce_heads(remoteheads); - *head_subsumed = 1; /* we will flip this to 0 when we find it */ for (remoteheads = NULL, remotes = &remoteheads; parents; parents = next) { @@ -1075,6 +1083,7 @@ static struct commit_list *collect_parents(struct commit *head_commit, *head_subsumed = 0; else remotes = &commit_list_insert(commit, remotes)->next; + free(parents); } return remoteheads; } From 34349dbff8632c353f083959881e38e1c853abf8 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 25 Apr 2015 12:00:14 -0700 Subject: [PATCH 09/15] merge: split reduce_parents() out of collect_parents() The latter does two separate things: - Parse the list of commits on the command line, and formulate the list of commits to be merged (including the current HEAD); - Compute the list of parents to be recorded in the resulting merge commit. Split the latter into a separate helper function, so that we can later supply the list commits to be merged from a different source (namely, FETCH_HEAD). Signed-off-by: Junio C Hamano --- builtin/merge.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index d2e36e2051..054f0521d8 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1044,23 +1044,11 @@ static int default_edit_option(void) st_stdin.st_mode == st_stdout.st_mode); } -static struct commit_list *collect_parents(struct commit *head_commit, - int *head_subsumed, - int argc, const char **argv) +static struct commit_list *reduce_parents(struct commit *head_commit, + int *head_subsumed, + struct commit_list *remoteheads) { - int i; - struct commit_list *remoteheads = NULL, *parents, *next; - struct commit_list **remotes = &remoteheads; - - if (head_commit) - remotes = &commit_list_insert(head_commit, remotes)->next; - for (i = 0; i < argc; i++) { - struct commit *commit = get_merge_parent(argv[i]); - if (!commit) - help_unknown_ref(argv[i], "merge", - "not something we can merge"); - remotes = &commit_list_insert(commit, remotes)->next; - } + struct commit_list *parents, *next, **remotes = &remoteheads; /* * Is the current HEAD reachable from another commit being @@ -1088,6 +1076,27 @@ static struct commit_list *collect_parents(struct commit *head_commit, return remoteheads; } +static struct commit_list *collect_parents(struct commit *head_commit, + int *head_subsumed, + int argc, const char **argv) +{ + int i; + struct commit_list *remoteheads = NULL; + struct commit_list **remotes = &remoteheads; + + if (head_commit) + remotes = &commit_list_insert(head_commit, remotes)->next; + for (i = 0; i < argc; i++) { + struct commit *commit = get_merge_parent(argv[i]); + if (!commit) + help_unknown_ref(argv[i], "merge", + "not something we can merge"); + remotes = &commit_list_insert(commit, remotes)->next; + } + + return reduce_parents(head_commit, head_subsumed, remoteheads); +} + int cmd_merge(int argc, const char **argv, const char *prefix) { unsigned char result_tree[20]; From 018b3fbc7e66439786492662b88cb34d7d000621 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 25 Apr 2015 12:31:57 -0700 Subject: [PATCH 10/15] merge: narrow scope of merge_names In order to pass the list of parents to fmt_merge_msg(), cmd_merge() uses this strbuf to create something that look like FETCH_HEAD that describes commits that are being merged. This is necessary only when we are creating the merge commit message ourselves, but was done unconditionally. Move the variable and the logic to populate it to confine them in a block that needs them. Signed-off-by: Junio C Hamano --- builtin/merge.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 054f0521d8..d853c9d805 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1236,8 +1236,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix) argc -= 2; remoteheads = collect_parents(head_commit, &head_subsumed, argc, argv); } else { - struct strbuf merge_names = STRBUF_INIT; - /* We are invoked directly as the first-class UI. */ head_arg = "HEAD"; @@ -1247,11 +1245,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * to the given message. */ remoteheads = collect_parents(head_commit, &head_subsumed, argc, argv); - for (p = remoteheads; p; p = p->next) - merge_name(merge_remote_util(p->item)->name, &merge_names); if (!have_message || shortlog_len) { + struct strbuf merge_names = STRBUF_INIT; struct fmt_merge_msg_opts opts; + + for (p = remoteheads; p; p = p->next) + merge_name(merge_remote_util(p->item)->name, &merge_names); + memset(&opts, 0, sizeof(opts)); opts.add_title = !have_message; opts.shortlog_len = shortlog_len; @@ -1260,6 +1261,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) fmt_merge_msg(&merge_names, &merge_msg, &opts); if (merge_msg.len) strbuf_setlen(&merge_msg, merge_msg.len - 1); + + strbuf_release(&merge_names); } } From 52fecab20cf28f4b5621e502a265ba551a26877e Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 25 Apr 2015 18:29:44 -0700 Subject: [PATCH 11/15] merge: extract prepare_merge_message() logic out Signed-off-by: Junio C Hamano --- builtin/merge.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index d853c9d805..a972ed6422 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1076,6 +1076,20 @@ static struct commit_list *reduce_parents(struct commit *head_commit, return remoteheads; } +static void prepare_merge_message(struct strbuf *merge_names, struct strbuf *merge_msg) +{ + struct fmt_merge_msg_opts opts; + + memset(&opts, 0, sizeof(opts)); + opts.add_title = !have_message; + opts.shortlog_len = shortlog_len; + opts.credit_people = (0 < option_edit); + + fmt_merge_msg(merge_names, merge_msg, &opts); + if (merge_msg->len) + strbuf_setlen(merge_msg, merge_msg->len - 1); +} + static struct commit_list *collect_parents(struct commit *head_commit, int *head_subsumed, int argc, const char **argv) @@ -1248,20 +1262,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (!have_message || shortlog_len) { struct strbuf merge_names = STRBUF_INIT; - struct fmt_merge_msg_opts opts; for (p = remoteheads; p; p = p->next) merge_name(merge_remote_util(p->item)->name, &merge_names); - - memset(&opts, 0, sizeof(opts)); - opts.add_title = !have_message; - opts.shortlog_len = shortlog_len; - opts.credit_people = (0 < option_edit); - - fmt_merge_msg(&merge_names, &merge_msg, &opts); - if (merge_msg.len) - strbuf_setlen(&merge_msg, merge_msg.len - 1); - + prepare_merge_message(&merge_names, &merge_msg); strbuf_release(&merge_names); } } From 1cf32f4d54d7db0da9b2fb35cdd277f9ab7ae0af Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 25 Apr 2015 18:34:22 -0700 Subject: [PATCH 12/15] merge: make collect_parents() auto-generate the merge message Signed-off-by: Junio C Hamano --- builtin/merge.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index a972ed6422..9f9853876b 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1092,7 +1092,8 @@ static void prepare_merge_message(struct strbuf *merge_names, struct strbuf *mer static struct commit_list *collect_parents(struct commit *head_commit, int *head_subsumed, - int argc, const char **argv) + int argc, const char **argv, + struct strbuf *merge_msg) { int i; struct commit_list *remoteheads = NULL; @@ -1108,7 +1109,20 @@ static struct commit_list *collect_parents(struct commit *head_commit, remotes = &commit_list_insert(commit, remotes)->next; } - return reduce_parents(head_commit, head_subsumed, remoteheads); + remoteheads = reduce_parents(head_commit, head_subsumed, remoteheads); + + if (merge_msg && + (!have_message || shortlog_len)) { + struct strbuf merge_names = STRBUF_INIT; + struct commit_list *p; + + for (p = remoteheads; p; p = p->next) + merge_name(merge_remote_util(p->item)->name, &merge_names); + prepare_merge_message(&merge_names, merge_msg); + strbuf_release(&merge_names); + } + + return remoteheads; } int cmd_merge(int argc, const char **argv, const char *prefix) @@ -1222,7 +1236,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (fast_forward == FF_NO) die(_("Non-fast-forward commit does not make sense into " "an empty head")); - remoteheads = collect_parents(head_commit, &head_subsumed, argc, argv); + remoteheads = collect_parents(head_commit, &head_subsumed, + argc, argv, NULL); remote_head = remoteheads->item; if (!remote_head) die(_("%s - not something we can merge"), argv[0]); @@ -1248,7 +1263,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) head_arg = argv[1]; argv += 2; argc -= 2; - remoteheads = collect_parents(head_commit, &head_subsumed, argc, argv); + remoteheads = collect_parents(head_commit, &head_subsumed, + argc, argv, NULL); } else { /* We are invoked directly as the first-class UI. */ head_arg = "HEAD"; @@ -1258,16 +1274,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * the standard merge summary message to be appended * to the given message. */ - remoteheads = collect_parents(head_commit, &head_subsumed, argc, argv); - - if (!have_message || shortlog_len) { - struct strbuf merge_names = STRBUF_INIT; - - for (p = remoteheads; p; p = p->next) - merge_name(merge_remote_util(p->item)->name, &merge_names); - prepare_merge_message(&merge_names, &merge_msg); - strbuf_release(&merge_names); - } + remoteheads = collect_parents(head_commit, &head_subsumed, + argc, argv, &merge_msg); } if (!head_commit || !argc) From 770380156dd06cd03d83957d55484f4a98ad284f Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 25 Apr 2015 18:39:43 -0700 Subject: [PATCH 13/15] merge: decide if we auto-generate the message early in collect_parents() Signed-off-by: Junio C Hamano --- builtin/merge.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 9f9853876b..eb3be6817e 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1098,6 +1098,10 @@ static struct commit_list *collect_parents(struct commit *head_commit, int i; struct commit_list *remoteheads = NULL; struct commit_list **remotes = &remoteheads; + struct strbuf merge_names = STRBUF_INIT, *autogen = NULL; + + if (merge_msg && (!have_message || shortlog_len)) + autogen = &merge_names; if (head_commit) remotes = &commit_list_insert(head_commit, remotes)->next; @@ -1111,15 +1115,13 @@ static struct commit_list *collect_parents(struct commit *head_commit, remoteheads = reduce_parents(head_commit, head_subsumed, remoteheads); - if (merge_msg && - (!have_message || shortlog_len)) { - struct strbuf merge_names = STRBUF_INIT; + if (autogen) { struct commit_list *p; - for (p = remoteheads; p; p = p->next) - merge_name(merge_remote_util(p->item)->name, &merge_names); - prepare_merge_message(&merge_names, merge_msg); - strbuf_release(&merge_names); + merge_name(merge_remote_util(p->item)->name, autogen); + + prepare_merge_message(autogen, merge_msg); + strbuf_release(autogen); } return remoteheads; From 74e8bc59cb324d2d7a55c90195db004219770eec Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 25 Apr 2015 18:47:21 -0700 Subject: [PATCH 14/15] merge: handle FETCH_HEAD internally The collect_parents() function now is responsible for 1. parsing the commits given on the command line into a list of commits to be merged; 2. filtering these parents into independent ones; and 3. optionally calling fmt_merge_msg() via prepare_merge_message() to prepare an auto-generated merge log message, using fake contents that FETCH_HEAD would have had if these commits were fetched from the current repository with "git pull . $args..." Make "git merge FETCH_HEAD" to be the same as the traditional git merge "$(git fmt-merge-msg <.git/FETCH_HEAD)" $commits invocation of the command in "git pull", where $commits are the ones that appear in FETCH_HEAD that are not marked as not-for-merge, by making it do a bit more, specifically: - noticing "FETCH_HEAD" is the only "commit" on the command line and picking the commits that are not marked as not-for-merge as the list of commits to be merged (substitute for step #1 above); - letting the resulting list fed to step #2 above; - doing the step #3 above, using the contents of the FETCH_HEAD instead of fake contents crafted from the list of commits parsed in the step #1 above. Note that this changes the semantics. "git merge FETCH_HEAD" has always behaved as if the first commit in the FETCH_HEAD file were directly specified on the command line, creating a two-way merge whose auto-generated merge log said "merge commit xyz". With this change, if the previous fetch was to grab multiple branches (e.g. "git fetch $there topic-a topic-b"), the new world order is to create an octopus, behaving as if "git pull $there topic-a topic-b" were run. This is a deliberate change to make that happen, and can be seen in the changes to t3033 tests. Signed-off-by: Junio C Hamano --- Documentation/git-merge.txt | 4 ++ builtin/merge.c | 106 ++++++++++++++++++++++++------------ t/t3033-merge-toplevel.sh | 10 ++-- 3 files changed, 81 insertions(+), 39 deletions(-) diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt index cf2c374b71..d9aa6b6f11 100644 --- a/Documentation/git-merge.txt +++ b/Documentation/git-merge.txt @@ -104,6 +104,10 @@ commit or stash your changes before running 'git merge'. If no commit is given from the command line, merge the remote-tracking branches that the current branch is configured to use as its upstream. See also the configuration section of this manual page. ++ +When `FETCH_HEAD` (and no other commit) is specified, the branches +recorded in the `.git/FETCH_HEAD` file by the previous invocation +of `git fetch` for merging are merged to the current branch. PRE-MERGE CHECKS diff --git a/builtin/merge.c b/builtin/merge.c index eb3be6817e..42f9fcc174 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -505,28 +505,6 @@ static void merge_name(const char *remote, struct strbuf *msg) strbuf_release(&truname); } - if (!strcmp(remote, "FETCH_HEAD") && - !access(git_path("FETCH_HEAD"), R_OK)) { - const char *filename; - FILE *fp; - struct strbuf line = STRBUF_INIT; - char *ptr; - - filename = git_path("FETCH_HEAD"); - fp = fopen(filename, "r"); - if (!fp) - die_errno(_("could not open '%s' for reading"), - filename); - strbuf_getline(&line, fp, '\n'); - fclose(fp); - ptr = strstr(line.buf, "\tnot-for-merge\t"); - if (ptr) - strbuf_remove(&line, ptr-line.buf+1, 13); - strbuf_addbuf(msg, &line); - strbuf_release(&line); - goto cleanup; - } - if (remote_head->util) { struct merge_remote_desc *desc; desc = merge_remote_util(remote_head); @@ -1090,6 +1068,60 @@ static void prepare_merge_message(struct strbuf *merge_names, struct strbuf *mer strbuf_setlen(merge_msg, merge_msg->len - 1); } +static void handle_fetch_head(struct commit_list **remotes, struct strbuf *merge_names) +{ + const char *filename; + int fd, pos, npos; + struct strbuf fetch_head_file = STRBUF_INIT; + + if (!merge_names) + merge_names = &fetch_head_file; + + filename = git_path("FETCH_HEAD"); + fd = open(filename, O_RDONLY); + if (fd < 0) + die_errno(_("could not open '%s' for reading"), filename); + + if (strbuf_read(merge_names, fd, 0) < 0) + die_errno(_("could not read '%s'"), filename); + if (close(fd) < 0) + die_errno(_("could not close '%s'"), filename); + + for (pos = 0; pos < merge_names->len; pos = npos) { + unsigned char sha1[20]; + char *ptr; + struct commit *commit; + + ptr = strchr(merge_names->buf + pos, '\n'); + if (ptr) + npos = ptr - merge_names->buf + 1; + else + npos = merge_names->len; + + if (npos - pos < 40 + 2 || + get_sha1_hex(merge_names->buf + pos, sha1)) + commit = NULL; /* bad */ + else if (memcmp(merge_names->buf + pos + 40, "\t\t", 2)) + continue; /* not-for-merge */ + else { + char saved = merge_names->buf[pos + 40]; + merge_names->buf[pos + 40] = '\0'; + commit = get_merge_parent(merge_names->buf + pos); + merge_names->buf[pos + 40] = saved; + } + if (!commit) { + if (ptr) + *ptr = '\0'; + die("not something we can merge in %s: %s", + filename, merge_names->buf + pos); + } + remotes = &commit_list_insert(commit, remotes)->next; + } + + if (merge_names == &fetch_head_file) + strbuf_release(&fetch_head_file); +} + static struct commit_list *collect_parents(struct commit *head_commit, int *head_subsumed, int argc, const char **argv, @@ -1105,21 +1137,27 @@ static struct commit_list *collect_parents(struct commit *head_commit, if (head_commit) remotes = &commit_list_insert(head_commit, remotes)->next; - for (i = 0; i < argc; i++) { - struct commit *commit = get_merge_parent(argv[i]); - if (!commit) - help_unknown_ref(argv[i], "merge", - "not something we can merge"); - remotes = &commit_list_insert(commit, remotes)->next; + + if (argc == 1 && !strcmp(argv[0], "FETCH_HEAD")) { + handle_fetch_head(remotes, autogen); + remoteheads = reduce_parents(head_commit, head_subsumed, remoteheads); + } else { + for (i = 0; i < argc; i++) { + struct commit *commit = get_merge_parent(argv[i]); + if (!commit) + help_unknown_ref(argv[i], "merge", + "not something we can merge"); + remotes = &commit_list_insert(commit, remotes)->next; + } + remoteheads = reduce_parents(head_commit, head_subsumed, remoteheads); + if (autogen) { + struct commit_list *p; + for (p = remoteheads; p; p = p->next) + merge_name(merge_remote_util(p->item)->name, autogen); + } } - remoteheads = reduce_parents(head_commit, head_subsumed, remoteheads); - if (autogen) { - struct commit_list *p; - for (p = remoteheads; p; p = p->next) - merge_name(merge_remote_util(p->item)->name, autogen); - prepare_merge_message(autogen, merge_msg); strbuf_release(autogen); } diff --git a/t/t3033-merge-toplevel.sh b/t/t3033-merge-toplevel.sh index 9d92d3c1a2..46aadc410b 100755 --- a/t/t3033-merge-toplevel.sh +++ b/t/t3033-merge-toplevel.sh @@ -77,7 +77,7 @@ test_expect_success 'merge octopus, non-fast-forward' ' # The same set with FETCH_HEAD -test_expect_failure 'merge FETCH_HEAD octopus into void' ' +test_expect_success 'merge FETCH_HEAD octopus into void' ' t3033_reset && git checkout --orphan test && git rm -fr . && @@ -88,7 +88,7 @@ test_expect_failure 'merge FETCH_HEAD octopus into void' ' test_must_fail git rev-parse HEAD ' -test_expect_failure 'merge FETCH_HEAD octopus fast-forward (ff)' ' +test_expect_success 'merge FETCH_HEAD octopus fast-forward (ff)' ' t3033_reset && git reset --hard one && git fetch . left right && @@ -100,7 +100,7 @@ test_expect_failure 'merge FETCH_HEAD octopus fast-forward (ff)' ' test_cmp expect actual ' -test_expect_failure 'merge FETCH_HEAD octopus non-fast-forward (ff)' ' +test_expect_success 'merge FETCH_HEAD octopus non-fast-forward (ff)' ' t3033_reset && git reset --hard one && git fetch . left right && @@ -112,7 +112,7 @@ test_expect_failure 'merge FETCH_HEAD octopus non-fast-forward (ff)' ' test_cmp expect actual ' -test_expect_failure 'merge FETCH_HEAD octopus fast-forward (does not ff)' ' +test_expect_success 'merge FETCH_HEAD octopus fast-forward (does not ff)' ' t3033_reset && git fetch . left right && git merge FETCH_HEAD && @@ -123,7 +123,7 @@ test_expect_failure 'merge FETCH_HEAD octopus fast-forward (does not ff)' ' test_cmp expect actual ' -test_expect_failure 'merge FETCH_HEAD octopus non-fast-forward' ' +test_expect_success 'merge FETCH_HEAD octopus non-fast-forward' ' t3033_reset && git fetch . left right && git merge --no-ff FETCH_HEAD && From d45366e8aa922037e7e84c3f35924d2b1399a453 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 25 Mar 2015 21:58:45 -0700 Subject: [PATCH 15/15] merge: deprecate 'git merge HEAD ' syntax We had this in "git merge" manual for eternity: 'git merge' HEAD ... [This] syntax ( `HEAD` ...) is supported for historical reasons. Do not use it from the command line or in new scripts. It is the same as `git merge -m ...`. With the update to "git merge" to make it understand what is recorded in FETCH_HEAD directly, including Octopus merge cases, we now can rewrite the use of this syntax in "git pull" with a simple "git merge FETCH_HEAD". Also there are quite a few fallouts in the test scripts, and it turns out that "git cvsimport" also uses this old syntax to record a merge. Judging from this result, I would not be surprised if dropping the support of the old syntax broke scripts people have written and been relying on for the past ten years. But at least we can start the deprecation process by throwing a warning message when the syntax is used. With luck, we might be able to drop the support in a few years. Signed-off-by: Junio C Hamano --- builtin/merge.c | 1 + git-cvsimport.perl | 2 +- git-pull.sh | 3 +-- t/t3402-rebase-merge.sh | 2 +- t/t6020-merge-df.sh | 2 +- t/t6021-merge-criss-cross.sh | 6 +++--- t/t9402-git-cvsserver-refs.sh | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 42f9fcc174..67fbfafa02 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1299,6 +1299,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) */ if (!have_message && is_old_style_invocation(argc, argv, head_commit->object.sha1)) { + warning("old-style 'git merge HEAD ' is deprecated."); strbuf_addstr(&merge_msg, argv[0]); head_arg = argv[1]; argv += 2; diff --git a/git-cvsimport.perl b/git-cvsimport.perl index 73d367cea8..82ecb0343a 100755 --- a/git-cvsimport.perl +++ b/git-cvsimport.perl @@ -1162,7 +1162,7 @@ sub commit { die "Fast-forward update failed: $?\n" if $?; } else { - system(qw(git merge cvsimport HEAD), "$remote/$opt_o"); + system(qw(git merge -m cvsimport), "$remote/$opt_o"); die "Could not merge $opt_o into the current branch.\n" if $?; } } else { diff --git a/git-pull.sh b/git-pull.sh index 4d4fc77b05..15d943190c 100755 --- a/git-pull.sh +++ b/git-pull.sh @@ -323,7 +323,6 @@ then fi fi -merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit case "$rebase" in true) eval="git-rebase $diffstat $strategy_args $merge_args $rebase_args $verbosity" @@ -334,7 +333,7 @@ true) eval="git-merge $diffstat $no_commit $verify_signatures $edit $squash $no_ff $ff_only" eval="$eval $log_arg $strategy_args $merge_args $verbosity $progress" eval="$eval $gpg_sign_args" - eval="$eval \"\$merge_name\" HEAD $merge_head" + eval="$eval FETCH_HEAD" ;; esac eval "exec $eval" diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh index 5a27ec9b5e..8f64505e4f 100755 --- a/t/t3402-rebase-merge.sh +++ b/t/t3402-rebase-merge.sh @@ -47,7 +47,7 @@ test_expect_success setup ' ' test_expect_success 'reference merge' ' - git merge -s recursive "reference merge" HEAD master + git merge -s recursive -m "reference merge" master ' PRE_REBASE=$(git rev-parse test-rebase) diff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh index 27c3d73961..2af1beec5f 100755 --- a/t/t6020-merge-df.sh +++ b/t/t6020-merge-df.sh @@ -24,7 +24,7 @@ test_expect_success 'prepare repository' ' ' test_expect_success 'Merge with d/f conflicts' ' - test_expect_code 1 git merge "merge msg" B master + test_expect_code 1 git merge -m "merge msg" master ' test_expect_success 'F/D conflict' ' diff --git a/t/t6021-merge-criss-cross.sh b/t/t6021-merge-criss-cross.sh index d15b313d4b..213deecab1 100755 --- a/t/t6021-merge-criss-cross.sh +++ b/t/t6021-merge-criss-cross.sh @@ -48,7 +48,7 @@ echo "1 " > file && git commit -m "C3" file && git branch C3 && -git merge "pre E3 merge" B A && +git merge -m "pre E3 merge" A && echo "1 2 3 changed in E3, branch B. New file size @@ -61,7 +61,7 @@ echo "1 " > file && git commit -m "E3" file && git checkout A && -git merge "pre D8 merge" A C3 && +git merge -m "pre D8 merge" C3 && echo "1 2 3 changed in C3, branch B @@ -73,7 +73,7 @@ echo "1 9" > file && git commit -m D8 file' -test_expect_success 'Criss-cross merge' 'git merge "final merge" A B' +test_expect_success 'Criss-cross merge' 'git merge -m "final merge" B' cat > file-expect <