git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] merge: indicate remote tracking branches in merge message
@ 2009-08-09  6:59 Jeff King
  2009-08-09  7:14 ` Jeff King
  2009-08-09  7:31 ` Junio C Hamano
  0 siblings, 2 replies; 12+ messages in thread
From: Jeff King @ 2009-08-09  6:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] merge: indicate remote tracking branches in merge message
  2009-08-09  6:59 [PATCH] merge: indicate remote tracking branches in merge message Jeff King
@ 2009-08-09  7:14 ` Jeff King
  2009-08-09  7:31 ` Junio C Hamano
  1 sibling, 0 replies; 12+ messages in thread
From: Jeff King @ 2009-08-09  7:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] merge: indicate remote tracking branches in merge message
  2009-08-09  6:59 [PATCH] merge: indicate remote tracking branches in merge message Jeff King
  2009-08-09  7:14 ` Jeff King
@ 2009-08-09  7:31 ` Junio C Hamano
  2009-08-09  7:40   ` Jeff King
  1 sibling, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2009-08-09  7:31 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> 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 <peff@peff.net>
> ---
> 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> */

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] merge: indicate remote tracking branches in merge message
  2009-08-09  7:31 ` Junio C Hamano
@ 2009-08-09  7:40   ` Jeff King
  2009-08-09  9:14     ` Jeff King
  0 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2009-08-09  7:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] merge: indicate remote tracking branches in merge message
  2009-08-09  7:40   ` Jeff King
@ 2009-08-09  9:14     ` Jeff King
  2009-08-09 10:00       ` Jeff King
  0 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2009-08-09  9:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

> > 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'
 |\

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] merge: indicate remote tracking branches in merge message
  2009-08-09  9:14     ` Jeff King
@ 2009-08-09 10:00       ` Jeff King
  2009-08-09 10:01         ` [PATCH 1/3] add tests for merge message headings Jeff King
                           ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Jeff King @ 2009-08-09 10:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] add tests for merge message headings
  2009-08-09 10:00       ` Jeff King
@ 2009-08-09 10:01         ` Jeff King
  2009-08-09 10:02         ` [PATCH 2/3] merge: fix incorrect merge message for ambiguous tag/branch Jeff King
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2009-08-09 10:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] merge: fix incorrect merge message for ambiguous tag/branch
  2009-08-09 10:00       ` Jeff King
  2009-08-09 10:01         ` [PATCH 1/3] add tests for merge message headings Jeff King
@ 2009-08-09 10:02         ` Jeff King
  2009-08-09 10:02         ` [PATCH 3/3] merge: indicate remote tracking branches in merge message Jeff King
  2009-08-09 10:07         ` [PATCH] " Jeff King
  3 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2009-08-09 10:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] merge: indicate remote tracking branches in merge message
  2009-08-09 10:00       ` Jeff King
  2009-08-09 10:01         ` [PATCH 1/3] add tests for merge message headings Jeff King
  2009-08-09 10:02         ` [PATCH 2/3] merge: fix incorrect merge message for ambiguous tag/branch Jeff King
@ 2009-08-09 10:02         ` Jeff King
  2009-08-09 10:07         ` [PATCH] " Jeff King
  3 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2009-08-09 10:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] merge: indicate remote tracking branches in merge message
  2009-08-09 10:00       ` Jeff King
                           ` (2 preceding siblings ...)
  2009-08-09 10:02         ` [PATCH 3/3] merge: indicate remote tracking branches in merge message Jeff King
@ 2009-08-09 10:07         ` Jeff King
  2009-08-09 19:36           ` Junio C Hamano
  3 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2009-08-09 10:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] merge: indicate remote tracking branches in merge message
  2009-08-09 10:07         ` [PATCH] " Jeff King
@ 2009-08-09 19:36           ` Junio C Hamano
  2009-08-09 21:49             ` Jeff King
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2009-08-09 19:36 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> writes:

> 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'").

Maybe "Merge version 'v1.6.3'" or "Merge commit tagged as 'v1.6.3'"? 
I dunno.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] merge: indicate remote tracking branches in merge message
  2009-08-09 19:36           ` Junio C Hamano
@ 2009-08-09 21:49             ` Jeff King
  0 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2009-08-09 21:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Sun, Aug 09, 2009 at 12:36:17PM -0700, Junio C Hamano wrote:

> > 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'").
> 
> Maybe "Merge version 'v1.6.3'" or "Merge commit tagged as 'v1.6.3'"? 
> I dunno.

Definitely "Merge version" is bad. Doing "git tag feature-x-working" and
merging it would lead to "Merge version 'feature-x-working'", which
doesn't really make sense. I don't think the developer would refer to
such a tag as a "version".

Your "Merge commit tagged as..." version is reasonable to me. I'm
dubious whether the additional information that it was a tag is actually
worth anything. That is, given

  (1) Merge commit '1234abcd'
  (2) Merge commit 'v1.6.3'
  (3) Merge tag 'v1.6.3'

where (1) is what we do now and will do in the future for arbitrary
commits, (2) is what we do now for tags, and (3) is the proposed change.
I am not sure that (3) really tells the reader anything useful over the
other two. And your "Merge commit tagged as" is just the same
information written slightly differently.

So maybe this patch should simply be dropped. I don't feel strongly
either way.

-Peff

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2009-08-09 21:49 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-08-09  6:59 [PATCH] merge: indicate remote tracking branches in merge message Jeff King
2009-08-09  7:14 ` Jeff King
2009-08-09  7:31 ` Junio C Hamano
2009-08-09  7:40   ` Jeff King
2009-08-09  9:14     ` Jeff King
2009-08-09 10:00       ` Jeff King
2009-08-09 10:01         ` [PATCH 1/3] add tests for merge message headings Jeff King
2009-08-09 10:02         ` [PATCH 2/3] merge: fix incorrect merge message for ambiguous tag/branch Jeff King
2009-08-09 10:02         ` [PATCH 3/3] merge: indicate remote tracking branches in merge message Jeff King
2009-08-09 10:07         ` [PATCH] " Jeff King
2009-08-09 19:36           ` Junio C Hamano
2009-08-09 21:49             ` Jeff King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).