[PATCH] merge: deprecate 'git merge <message> HEAD <commit>' syntax

[PATCH] merge: deprecate 'git merge <message> HEAD <commit>' syntax

[Junio C Hamano]

We had this in "git merge" manual for eternity:

    'git merge' <msg> HEAD <commit>...

    [This] syntax (<msg> `HEAD` <commit>...) 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 <msg> <commit>...`.

I wanted to see how much damage we would incur to people by checking
what the damage to _our_ system, including the test scripts, if we
dropped the support for the syntax.  The last time I tried this, I
thought that replacing the use of this syntax in "git pull" with its
obvious and trivial rewrite 'git merge -m <msg> <commit>' broke some
output, and that is the primary reason why I stayed away from trying
this again, but it no longer seems to be the case with today's code
for some reason.

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 <gitster@pobox.com>
---
 builtin/merge.c               | 1 +
 git-cvsimport.perl            | 2 +-
 git-pull.sh                   | 2 +-
 t/t3402-rebase-merge.sh       | 2 +-
 t/t6021-merge-criss-cross.sh  | 6 +++---
 t/t9402-git-cvsserver-refs.sh | 2 +-
 6 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 3b0f8f9..0795358 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1182,6 +1182,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	if (!have_message && head_commit &&
 	    is_old_style_invocation(argc, argv, head_commit->object.sha1)) {
+		warning("old-style 'git merge <msg> HEAD <commit>' 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 73d367c..82ecb03 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 4d4fc77..252969e 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -334,7 +334,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 -m \"\$merge_name\" $merge_head"
 	;;
 esac
 eval "exec $eval"
diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
index 5a27ec9..8f64505 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/t6021-merge-criss-cross.sh b/t/t6021-merge-criss-cross.sh
index d15b313..213deec 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 <<EOF
 1
diff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh
index 1e266ef..d00df08 100755
--- a/t/t9402-git-cvsserver-refs.sh
+++ b/t/t9402-git-cvsserver-refs.sh
@@ -496,7 +496,7 @@ test_expect_success 'check [cvswork3] diff' '
 '
 
 test_expect_success 'merge early [cvswork3] b3 with b1' '
-	( cd gitwork3 && git merge "message" HEAD b1 ) &&
+	( cd gitwork3 && git merge -m "message" b1 ) &&
 	git fetch gitwork3 b3:b3 &&
 	git tag v3merged b3 &&
 	git push --tags gitcvs.git b3:b3
-- 
2.3.4-475-g3180e2e

[PATCH] merge: drop 'git merge <message> HEAD <commit>' syntax

[Junio C Hamano]

And then if we and our users survived the previous "start warning if
the old syntax is used" patch for a few years, we could apply this
to actually drop the support for the ancient syntax.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-merge.txt |  7 +------
 builtin/merge.c             | 38 +-------------------------------------
 2 files changed, 2 insertions(+), 43 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 1f94908..4a2f519 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -12,7 +12,6 @@ SYNOPSIS
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
 	[-s <strategy>] [-X <strategy-option>] [-S[<key-id>]]
 	[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
-'git merge' <msg> HEAD <commit>...
 'git merge' --abort
 
 DESCRIPTION
@@ -44,11 +43,7 @@ a log message from the user describing the changes.
     D---E---F---G---H master
 ------------
 
-The second syntax (<msg> `HEAD` <commit>...) 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 <msg> <commit>...`.
-
-The third syntax ("`git merge --abort`") can only be run after the
+The second syntax ("`git merge --abort`") can only be run after the
 merge has resulted in conflicts. 'git merge --abort' will abort the
 merge process and try to reconstruct the pre-merge state. However,
 if there were uncommitted changes when the merge started (and
diff --git a/builtin/merge.c b/builtin/merge.c
index 0795358..a4ddd31 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -43,7 +43,6 @@ struct strategy {
 
 static const char * const builtin_merge_usage[] = {
 	N_("git merge [<options>] [<commit>...]"),
-	N_("git merge [<options>] <msg> HEAD <commit>"),
 	N_("git merge --abort"),
 	NULL
 };
@@ -902,24 +901,6 @@ static int suggest_conflicts(void)
 	return 1;
 }
 
-static struct commit *is_old_style_invocation(int argc, const char **argv,
-					      const unsigned char *head)
-{
-	struct commit *second_token = NULL;
-	if (argc > 2) {
-		unsigned char second_sha1[20];
-
-		if (get_sha1(argv[1], second_sha1))
-			return NULL;
-		second_token = lookup_commit_reference_gently(second_sha1, 0);
-		if (!second_token)
-			die(_("'%s' is not a commit"), argv[1]);
-		if (hashcmp(second_token->object.sha1, head))
-			return NULL;
-	}
-	return second_token;
-}
-
 static int evaluate_result(void)
 {
 	int cnt = 0;
@@ -1171,24 +1152,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 <msg> HEAD <commit>..."  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)) {
-		warning("old-style 'git merge <msg> HEAD <commit>' is deprecated.");
-		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
-- 
2.3.4-475-g3180e2e

Re: [PATCH] merge: deprecate 'git merge <message> HEAD <commit>' syntax

[Jeff King]

On Wed, Mar 25, 2015 at 09:58:45PM -0700, Junio C Hamano wrote:

> We had this in "git merge" manual for eternity:
> 
>     'git merge' <msg> HEAD <commit>...
> 
>     [This] syntax (<msg> `HEAD` <commit>...) 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 <msg> <commit>...`.

It would be nice to see this ancient wart ago. I agree we need a
deprecation period, though, as you've outlined here.

>  builtin/merge.c               | 1 +
>  git-cvsimport.perl            | 2 +-
>  git-pull.sh                   | 2 +-
>  t/t3402-rebase-merge.sh       | 2 +-
>  t/t6021-merge-criss-cross.sh  | 6 +++---
>  t/t9402-git-cvsserver-refs.sh | 2 +-
>  6 files changed, 8 insertions(+), 7 deletions(-)

Maybe squash in:

diff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh
index 27c3d73..2af1bee 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' '


We do not call it HEAD here, but the setup test just before has put us
on branch B, so I think it is equivalent.

-Peff

Re: [PATCH] merge: deprecate 'git merge <message> HEAD <commit>' syntax

[Eric Sunshine]

On Thu, Mar 26, 2015 at 12:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> We had this in "git merge" manual for eternity:
>
>     'git merge' <msg> HEAD <commit>...
>
>     [This] syntax (<msg> `HEAD` <commit>...) 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 <msg> <commit>...`.
>
> I wanted to see how much damage we would incur to people by checking
> what the damage to _our_ system, including the test scripts, if we
> dropped the support for the syntax.  The last time I tried this, I
> thought that replacing the use of this syntax in "git pull" with its
> obvious and trivial rewrite 'git merge -m <msg> <commit>' broke some
> output, and that is the primary reason why I stayed away from trying
> this again, but it no longer seems to be the case with today's code
> for some reason.
>
> 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 <gitster@pobox.com>
> ---
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 3b0f8f9..0795358 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1182,6 +1182,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>
>         if (!have_message && head_commit &&
>             is_old_style_invocation(argc, argv, head_commit->object.sha1)) {
> +               warning("old-style 'git merge <msg> HEAD <commit>' is deprecated.");

To be a bit more helpful, perhaps point the user at "git merge -m
<msg> <commit>..." as the recommended replacement?

>                 strbuf_addstr(&merge_msg, argv[0]);
>                 head_arg = argv[1];
>                 argv += 2;