[PATCH] merge: indicate remote tracking branches in merge message

Previously when merging directly from a local tracking
branch like:

  git merge origin/master

The merge message said:

   Merge commit 'origin/master'

     * commit 'origin/master':
       ...

Instead, let's be more explicit about what we are merging:

   Merge remote branch 'origin/master'

     * origin/master:
       ...

We accomplish this by recognizing remote tracking branches
in git-merge when we build the simulated FETCH_HEAD output
that we feed to fmt-merge-msg.

Signed-off-by: Jeff King <redacted>
---
This is a repost of

  http://article.gmane.org/gmane.comp.version-control.git/119909

which got no response from you. I think it is a good idea, but I am not
deeply committed to it. I mainly want a yes or no so I can clean it out
of my patch queue.

 builtin-merge.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index 0b12fb3..c5688e1 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c

@@ -378,6 +378,17 @@ static void merge_name(const char *remote, struct strbuf *msg)
 		goto cleanup;
 	}
 
+	strbuf_setlen(&buf, 0);
+	strbuf_addstr(&buf, "refs/remotes/");
+	strbuf_addstr(&buf, remote);
+	resolve_ref(buf.buf, branch_head, 0, 0);
+
+	if (!hashcmp(remote_head->sha1, branch_head)) {
+		strbuf_addf(msg, "%s\t\tremote branch '%s' of .\n",
+				sha1_to_hex(branch_head), remote);
+		goto cleanup;
+	}
+
 	/* See if remote matches <name>^^^.. or <name>~<number> */
 	for (len = 0, ptr = remote + strlen(remote);
 	     remote < ptr && ptr[-1] == '^';

-- 
1.6.4.72.g50730

On Sun, Aug 09, 2009 at 02:59:36AM -0400, Jeff King wrote:

This is a repost of

  http://article.gmane.org/gmane.comp.version-control.git/119909

which got no response from you. I think it is a good idea, but I am not
deeply committed to it. I mainly want a yes or no so I can clean it out
of my patch queue.

Ah, it may have had to do with the fact that it doesn't actually pass
the tests. There is a trivial text update needed in t3409, which calls
"git merge origin/master" during its setup phase.

Updated, passing-all-tests patch is below. Sorry for the confusion.

-- >8 --
Subject: [PATCH] merge: indicate remote tracking branches in merge message

Previously when merging directly from a local tracking
branch like:

  git merge origin/master

The merge message said:

   Merge commit 'origin/master'

     * commit 'origin/master':
       ...

Instead, let's be more explicit about what we are merging:

   Merge remote branch 'origin/master'

     * origin/master:
       ...

We accomplish this by recognizing remote tracking branches
in git-merge when we build the simulated FETCH_HEAD output
that we feed to fmt-merge-msg.

Signed-off-by: Jeff King <redacted>
---
 builtin-merge.c                   |   11 +++++++++++
 t/t3409-rebase-preserve-merges.sh |    2 +-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index 82b5466..1f59567 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c

@@ -378,6 +378,17 @@ static void merge_name(const char *remote, struct strbuf *msg)
 		goto cleanup;
 	}
 
+	strbuf_setlen(&buf, 0);
+	strbuf_addstr(&buf, "refs/remotes/");
+	strbuf_addstr(&buf, remote);
+	resolve_ref(buf.buf, branch_head, 0, 0);
+
+	if (!hashcmp(remote_head->sha1, branch_head)) {
+		strbuf_addf(msg, "%s\t\tremote branch '%s' of .\n",
+				sha1_to_hex(branch_head), remote);
+		goto cleanup;
+	}
+
 	/* See if remote matches <name>^^^.. or <name>~<number> */
 	for (len = 0, ptr = remote + strlen(remote);
 	     remote < ptr && ptr[-1] == '^';

diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index e6c8327..2ce0236 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh

@@ -71,7 +71,7 @@ test_expect_success 'rebase -p fakes interactive rebase' '
 	git fetch &&
 	git rebase -p origin/topic &&
 	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
-	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge commit" | wc -l)
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge remote branch" | wc -l)
 	)
 '
 

-- 
1.6.4.178.g7a987

Jeff King [off-list ref] writes:

Previously when merging directly from a local tracking
branch like:

  git merge origin/master

The merge message said:

   Merge commit 'origin/master'

     * commit 'origin/master':
       ...

Instead, let's be more explicit about what we are merging:

   Merge remote branch 'origin/master'

     * origin/master:
       ...

We accomplish this by recognizing remote tracking branches
in git-merge when we build the simulated FETCH_HEAD output
that we feed to fmt-merge-msg.

Signed-off-by: Jeff King <redacted>
---
This is a repost of

  http://article.gmane.org/gmane.comp.version-control.git/119909

which got no response from you. I think it is a good idea, but I am not
deeply committed to it. I mainly want a yes or no so I can clean it out
of my patch queue.

I somewhat suspect that the patch was not applied because it also lacked
necessary adjustments to tests.  With this patch, I think the tests would
fail.

Nevertheless, I think it is a good thing to do.  But I am unsure about the
implementation.

Shouldn't it instead feed what it got from the end user to the dwim
machinery, and make sure it dwims into refs/remotes/ hierarchy?

In other words, like this.  Note that it would be much clearer to see
what's needed, if you want to extend it to refs/tags hierarchy ;-)

 builtin-merge.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index 82b5466..f4de73f 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c

@@ -358,6 +358,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf bname = STRBUF_INIT;
 	const char *ptr;
+	char *found_ref;
 	int len, early;
 
 	strbuf_branchname(&bname, remote);

@@ -368,14 +369,17 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	if (!remote_head)
 		die("'%s' does not point to a commit", remote);
 
-	strbuf_addstr(&buf, "refs/heads/");
-	strbuf_addstr(&buf, remote);
-	resolve_ref(buf.buf, branch_head, 0, NULL);
-
-	if (!hashcmp(remote_head->sha1, branch_head)) {
-		strbuf_addf(msg, "%s\t\tbranch '%s' of .\n",
-			sha1_to_hex(branch_head), remote);
-		goto cleanup;
+	if (dwim_ref(remote, strlen(remote), branch_head, &found_ref) > 0) {
+		if (!prefixcmp(found_ref, "refs/heads/")) {
+			strbuf_addf(msg, "%s\t\tbranch '%s' of .\n",
+				    sha1_to_hex(branch_head), remote);
+			goto cleanup;
+		}
+		if (!prefixcmp(found_ref, "refs/remotes/")) {
+			strbuf_addf(msg, "%s\t\tremote branch '%s' of .\n",
+				    sha1_to_hex(branch_head), remote);
+			goto cleanup;
+		}
 	}
 
 	/* See if remote matches <name>^^^.. or <name>~<number> */

On Sun, Aug 09, 2009 at 12:31:04AM -0700, Junio C Hamano wrote:

I somewhat suspect that the patch was not applied because it also lacked
necessary adjustments to tests.  With this patch, I think the tests would
fail.

Yeah, see my follow-up patch.

Nevertheless, I think it is a good thing to do.  But I am unsure about the
implementation.

Shouldn't it instead feed what it got from the end user to the dwim
machinery, and make sure it dwims into refs/remotes/ hierarchy?

I'm not sure that is all that different in practice than what is
happening now. Mainly I did it the way I did so that I didn't touch the
code path for detecting local branches.

But assuming they are functionally identical, I think your patch is much
more readable.

In other words, like this.  Note that it would be much clearer to see
what's needed, if you want to extend it to refs/tags hierarchy ;-)

I'm not sure adding "tag foo" will actually work, as it still has to
make it through the bit where we parse FETCH_HEAD. I'm not sure if it
would get mutilated to "commit foo" by that code or not.

-Peff

On Sun, Aug 09, 2009 at 03:40:35AM -0400, Jeff King wrote:

quoted

Shouldn't it instead feed what it got from the end user to the dwim
machinery, and make sure it dwims into refs/remotes/ hierarchy?

I'm not sure that is all that different in practice than what is
happening now. Mainly I did it the way I did so that I didn't touch the
code path for detecting local branches.

But assuming they are functionally identical, I think your patch is much
more readable.

I tested your patch; the two methods aren't identical. In fact, yours
fixes a bug. :)

In t4202, we have a branch name and a tag name that are the same
(octopus-a), and we "git merge octopus-a". This actually merges the tag,
but because the branch name existed, we write "Merge branch 'octopus-a'"
in the log, which is not true. With your patch, it does the right thing
and says "Merge commit 'octopus-a'".

The simple thing is to just update the "expect" text. Though the current
behavior does show off the ability to collape the two branches and say

  Merge branches 'octopus-a' and 'octopus-b'

instead of

  Merge commit 'octopus-a'; commit 'octopus-b'

which we could preserve that by renaming the tags, as below.

---

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 48e0088..9d0db2a 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh

@@ -306,13 +306,13 @@ test_expect_success 'set up more tangled history' '
 	git checkout master &&
 	git merge tangle &&
 	git checkout -b reach &&
-	test_commit reach &&
+	test_commit reach-1 &&
 	git checkout master &&
 	git checkout -b octopus-a &&
-	test_commit octopus-a &&
+	test_commit octopus-a-1 &&
 	git checkout master &&
 	git checkout -b octopus-b &&
-	test_commit octopus-b &&
+	test_commit octopus-b-1 &&
 	git checkout master &&
 	test_commit seventh &&
 	git merge octopus-a octopus-b

@@ -327,12 +327,12 @@ cat > expect <<\EOF
 *-. \   Merge branches 'octopus-a' and 'octopus-b'
 |\ \ \
 * | | | seventh
-| | * | octopus-b
+| | * | octopus-b-1
 | |/ /
 |/| |
-| * | octopus-a
+| * | octopus-a-1
 |/ /
-| * reach
+| * reach-1
 |/
 *   Merge branch 'tangle'
 |\

On Sun, Aug 09, 2009 at 05:14:43AM -0400, Jeff King wrote:

In t4202, we have a branch name and a tag name that are the same
(octopus-a), and we "git merge octopus-a". This actually merges the tag,
but because the branch name existed, we write "Merge branch 'octopus-a'"
in the log, which is not true. With your patch, it does the right thing
and says "Merge commit 'octopus-a'".

The simple thing is to just update the "expect" text. Though the current
behavior does show off the ability to collape the two branches and say

  Merge branches 'octopus-a' and 'octopus-b'

instead of

  Merge commit 'octopus-a'; commit 'octopus-b'

Thinking about it for a few seconds, it's silly to try to test something
that happens to occur in a totally unrelated test. The right thing to do
is to write actual tests for this area, fix the bug, and then add the
new feature. So how about this series:

  [1/3] add tests for merge message headings
  [2/3] merge: fix incorrect merge message for ambiguous tag/branch
  [3/3] merge: indicate remote tracking branches in merge message

-Peff

When calling "git merge $X", we automatically generate a
commit message containing something like "Merge branch
'$X'". This test script checks that those messages say what
they should, and exposes a failure when merging a refname
that is ambiguous between a tag and a branch.

Signed-off-by: Jeff King <redacted>
---
 t/t7608-merge-messages.sh |   50 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)
 create mode 100755 t/t7608-merge-messages.sh

diff --git a/t/t7608-merge-messages.sh b/t/t7608-merge-messages.sh
new file mode 100755
index 0000000..9d10583
--- /dev/null
+++ b/t/t7608-merge-messages.sh

@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='test auto-generated merge messages'
+. ./test-lib.sh
+
+check_oneline() {
+	echo "$1" | sed "s/Q/'/g" >expect &&
+	git log -1 --pretty=tformat:%s >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success 'merge local branch' '
+	test_commit master-1 &&
+	git checkout -b local-branch &&
+	test_commit branch-1 &&
+	git checkout master &&
+	test_commit master-2 &&
+	git merge local-branch &&
+	check_oneline "Merge branch Qlocal-branchQ"
+'
+
+test_expect_success 'merge octopus branches' '
+	git checkout -b octopus-a master &&
+	test_commit octopus-1 &&
+	git checkout -b octopus-b master &&
+	test_commit octopus-2 &&
+	git checkout master &&
+	git merge octopus-a octopus-b &&
+	check_oneline "Merge branches Qoctopus-aQ and Qoctopus-bQ"
+'
+
+test_expect_success 'merge tag' '
+	git checkout -b tag-branch master &&
+	test_commit tag-1 &&
+	git checkout master &&
+	test_commit master-3 &&
+	git merge tag-1 &&
+	check_oneline "Merge commit Qtag-1Q"
+'
+
+test_expect_failure 'ambiguous tag' '
+	git checkout -b ambiguous master &&
+	test_commit ambiguous &&
+	git checkout master &&
+	test_commit master-4 &&
+	git merge ambiguous &&
+	check_oneline "Merge commit QambiguousQ"
+'
+
+test_done

-- 
1.6.4.178.g7a987

If we have both a tag and a branch named "foo", then calling
"git merge foo" will warn about the ambiguous ref, but merge
the tag.

When generating the commit message, though, we simply
checked whether "refs/heads/foo" existed, and if it did,
assumed it was a branch. This led to the statement "Merge
branch 'foo'" in the commit message, which is quite wrong.

Instead, we should use dwim_ref to find the actual ref used,
and describe it appropriately.

In addition to the test in t7608, we must also tweak the
expected output of t4202, which was accidentally triggering
this bug.

Signed-off-by: Jeff King <redacted>
---
 builtin-merge.c           |   15 +++++++--------
 t/t4202-log.sh            |    4 ++--
 t/t7608-merge-messages.sh |    2 +-
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index 82b5466..f7db148 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c

@@ -358,6 +358,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf bname = STRBUF_INIT;
 	const char *ptr;
+	char *found_ref;
 	int len, early;
 
 	strbuf_branchname(&bname, remote);

@@ -368,14 +369,12 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	if (!remote_head)
 		die("'%s' does not point to a commit", remote);
 
-	strbuf_addstr(&buf, "refs/heads/");
-	strbuf_addstr(&buf, remote);
-	resolve_ref(buf.buf, branch_head, 0, NULL);
-
-	if (!hashcmp(remote_head->sha1, branch_head)) {
-		strbuf_addf(msg, "%s\t\tbranch '%s' of .\n",
-			sha1_to_hex(branch_head), remote);
-		goto cleanup;
+	if (dwim_ref(remote, strlen(remote), branch_head, &found_ref) > 0) {
+		if (!prefixcmp(found_ref, "refs/heads/")) {
+			strbuf_addf(msg, "%s\t\tbranch '%s' of .\n",
+				    sha1_to_hex(branch_head), remote);
+			goto cleanup;
+		}
 	}
 
 	/* See if remote matches <name>^^^.. or <name>~<number> */

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 48e0088..1e952ca 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh

@@ -320,11 +320,11 @@ test_expect_success 'set up more tangled history' '
 '
 
 cat > expect <<\EOF
-*   Merge branch 'reach'
+*   Merge commit 'reach'
 |\
 | \
 |  \
-*-. \   Merge branches 'octopus-a' and 'octopus-b'
+*-. \   Merge commit 'octopus-a'; commit 'octopus-b'
 |\ \ \
 * | | | seventh
 | | * | octopus-b

diff --git a/t/t7608-merge-messages.sh b/t/t7608-merge-messages.sh
index 9d10583..81ced8a 100755
--- a/t/t7608-merge-messages.sh
+++ b/t/t7608-merge-messages.sh

@@ -38,7 +38,7 @@ test_expect_success 'merge tag' '
 	check_oneline "Merge commit Qtag-1Q"
 '
 
-test_expect_failure 'ambiguous tag' '
+test_expect_success 'ambiguous tag' '
 	git checkout -b ambiguous master &&
 	test_commit ambiguous &&
 	git checkout master &&

-- 
1.6.4.178.g7a987

Previously when merging directly from a local tracking
branch like:

  git merge origin/master

The merge message said:

   Merge commit 'origin/master'

     * commit 'origin/master':
       ...

Instead, let's be more explicit about what we are merging:

   Merge remote branch 'origin/master'

     * origin/master:
       ...

We accomplish this by recognizing remote tracking branches
in git-merge when we build the simulated FETCH_HEAD output
that we feed to fmt-merge-msg.

In addition to a new test in t7608, we have to tweak the
expected output of t3409, which does such a merge.

Signed-off-by: Jeff King <redacted>
---
 builtin-merge.c                   |    5 +++++
 t/t3409-rebase-preserve-merges.sh |    2 +-
 t/t7608-merge-messages.sh         |   10 ++++++++++
 3 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index f7db148..f4de73f 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c

@@ -375,6 +375,11 @@ static void merge_name(const char *remote, struct strbuf *msg)
 				    sha1_to_hex(branch_head), remote);
 			goto cleanup;
 		}
+		if (!prefixcmp(found_ref, "refs/remotes/")) {
+			strbuf_addf(msg, "%s\t\tremote branch '%s' of .\n",
+				    sha1_to_hex(branch_head), remote);
+			goto cleanup;
+		}
 	}
 
 	/* See if remote matches <name>^^^.. or <name>~<number> */

diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index e6c8327..297d165 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh

@@ -71,7 +71,7 @@ test_expect_success 'rebase -p fakes interactive rebase' '
 	git fetch &&
 	git rebase -p origin/topic &&
 	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
-	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge commit" | wc -l)
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge remote branch " | wc -l)
 	)
 '
 

diff --git a/t/t7608-merge-messages.sh b/t/t7608-merge-messages.sh
index 81ced8a..28d5679 100755
--- a/t/t7608-merge-messages.sh
+++ b/t/t7608-merge-messages.sh

@@ -47,4 +47,14 @@ test_expect_success 'ambiguous tag' '
 	check_oneline "Merge commit QambiguousQ"
 '
 
+test_expect_success 'remote branch' '
+	git checkout -b remote master &&
+	test_commit remote-1 &&
+	git update-ref refs/remotes/origin/master remote &&
+	git checkout master &&
+	test_commit master-5 &&
+	git merge origin/master &&
+	check_oneline "Merge remote branch Qorigin/masterQ"
+'
+
 test_done

-- 
1.6.4.178.g7a987

On Sun, Aug 09, 2009 at 06:00:45AM -0400, Jeff King wrote:

  [1/3] add tests for merge message headings
  [2/3] merge: fix incorrect merge message for ambiguous tag/branch
  [3/3] merge: indicate remote tracking branches in merge message

And here is the 4/3 you mentioned earlier:

-- >8 --
Subject: [PATCH] merge: describe tags as such in merge message

Previously, merging a tag directly via "git merge tag" would
get you the message "Merge commit 'tag'". It is a little
more descriptive to note that it was actually a tag (i.e.,
"Merge tag 'tag'").

Signed-off-by: Jeff King <redacted>
---
 builtin-merge.c           |    5 +++++
 t/t7608-merge-messages.sh |    4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index f4de73f..db74901 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c

@@ -380,6 +380,11 @@ static void merge_name(const char *remote, struct strbuf *msg)
 				    sha1_to_hex(branch_head), remote);
 			goto cleanup;
 		}
+		if (!prefixcmp(found_ref, "refs/tags/")) {
+			strbuf_addf(msg, "%s\t\ttag '%s' of .\n",
+				    sha1_to_hex(branch_head), remote);
+			goto cleanup;
+		}
 	}
 
 	/* See if remote matches <name>^^^.. or <name>~<number> */

diff --git a/t/t7608-merge-messages.sh b/t/t7608-merge-messages.sh
index 28d5679..3ee0983 100755
--- a/t/t7608-merge-messages.sh
+++ b/t/t7608-merge-messages.sh

@@ -35,7 +35,7 @@ test_expect_success 'merge tag' '
 	git checkout master &&
 	test_commit master-3 &&
 	git merge tag-1 &&
-	check_oneline "Merge commit Qtag-1Q"
+	check_oneline "Merge tag Qtag-1Q"
 '
 
 test_expect_success 'ambiguous tag' '

@@ -44,7 +44,7 @@ test_expect_success 'ambiguous tag' '
 	git checkout master &&
 	test_commit master-4 &&
 	git merge ambiguous &&
-	check_oneline "Merge commit QambiguousQ"
+	check_oneline "Merge tag QambiguousQ"
 '
 
 test_expect_success 'remote branch' '

-- 
1.6.4.178.g7a987