Re* git merge <tag> behavior

Subsystems: the rest

7 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re* git merge <tag> behavior

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:26

Junio C Hamano [off-list ref] writes:

quoted

2) git merge <tag> VS git merge <object-id>
...
It would be great to have Git using the tag message and check the
signature.

Perhaps, but if you feed the $(git rev-parse v1.12.2) to merge, your
subject will not be able to say "Merge tag 'v1.12.2'" in the first
place, so I do not think you would want to encourage such usage in
the first place.

A patch to do so may look like this.  You would probably want to
also do read_sha1_file(desc->obj->sha1) here and parse out the
header line "tag " to replace the "remote" to recover the symbolic
tag name, but I didn't bother in this illustration.

diff --git a/builtin/merge.c b/builtin/merge.c
index 0ec8f0d..990e90c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -516,6 +516,19 @@ static void merge_name(const char *remote, struct strbuf *msg)
 		strbuf_release(&line);
 		goto cleanup;
 	}
+
+	if (remote_head->util) {
+		struct merge_remote_desc *desc;
+		desc = merge_remote_util(remote_head);
+		if (desc && desc->obj && desc->obj->type == OBJ_TAG) {
+			strbuf_addf(msg, "%s\t\t%s '%s'\n",
+				    sha1_to_hex(desc->obj->sha1),
+				    typename(desc->obj->type),
+				    remote);
+			goto cleanup;
+		}
+	}
+
 	strbuf_addf(msg, "%s\t\tcommit '%s'\n",
 		sha1_to_hex(remote_head->object.sha1), remote);
 cleanup:

[PATCH 1/3] merge: a random object may not necssarily be a commit

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:35

The user could have said "git merge $(git rev-parse v1.0.0)"; we
shouldn't mark it as "Merge commit '15999998fb...'" as the merge
name, even though such an invocation might be crazy.

We could even read the "tag " header from the tag object and replace
the object name the user gave us, but let's not lose the information
by doing so, at least not yet.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/merge.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 0ec8f0d..990e90c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -516,6 +516,19 @@ static void merge_name(const char *remote, struct strbuf *msg)
 		strbuf_release(&line);
 		goto cleanup;
 	}
+
+	if (remote_head->util) {
+		struct merge_remote_desc *desc;
+		desc = merge_remote_util(remote_head);
+		if (desc && desc->obj && desc->obj->type == OBJ_TAG) {
+			strbuf_addf(msg, "%s\t\t%s '%s'\n",
+				    sha1_to_hex(desc->obj->sha1),
+				    typename(desc->obj->type),
+				    remote);
+			goto cleanup;
+		}
+	}
+
 	strbuf_addf(msg, "%s\t\tcommit '%s'\n",
 		sha1_to_hex(remote_head->object.sha1), remote);
 cleanup:

-- 
1.8.2-480-g064f421

[PATCH 2/3] t6200: use test_config/test_unconfig

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:35

The tests were already well protected from previous ones by running
"git config --unset" on variables early they do not want to see, but
it is easier to make sure they start from a clean state by using
more modern test_config/test_unconfig helper functions.

It turns out that the last test depended on the merge.summary
configuration previous one leaves behind.  Set it explicitly in it.

Signed-off-by: Junio C Hamano <redacted>
---
 t/t6200-fmt-merge-msg.sh | 61 +++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 992c2a0..f84bb0c 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh

@@ -112,8 +112,8 @@ test_expect_success '[merge] summary/log configuration' '
 	  Common #1
 	EOF
 
-	git config merge.log true &&
-	test_might_fail git config --unset-all merge.summary &&
+	test_config merge.log true &&
+	test_unconfig merge.summary &&
 
 	git checkout master &&
 	test_tick &&

@@ -121,8 +121,8 @@ test_expect_success '[merge] summary/log configuration' '
 
 	git fmt-merge-msg <.git/FETCH_HEAD >actual1 &&
 
-	test_might_fail git config --unset-all merge.log &&
-	git config merge.summary true &&
+	test_unconfig merge.log &&
+	test_config merge.summary true &&
 
 	git checkout master &&
 	test_tick &&

@@ -134,11 +134,6 @@ test_expect_success '[merge] summary/log configuration' '
 	test_cmp expected actual2
 '
 
-test_expect_success 'setup: clear [merge] configuration' '
-	test_might_fail git config --unset-all merge.log &&
-	test_might_fail git config --unset-all merge.summary
-'
-
 test_expect_success 'setup FETCH_HEAD' '
 	git checkout master &&
 	test_tick &&

@@ -248,14 +243,14 @@ test_expect_success 'fmt-merge-msg -m' '
 	  Common #1
 	EOF
 
-	test_might_fail git config --unset merge.log &&
-	test_might_fail git config --unset merge.summary &&
+	test_unconfig merge.log &&
+	test_unconfig merge.summary &&
 	git checkout master &&
 	git fetch "$(pwd)" left &&
 	git fmt-merge-msg -m "Sync with left" <.git/FETCH_HEAD >actual &&
 	git fmt-merge-msg --log -m "Sync with left" \
 					<.git/FETCH_HEAD >actual.log &&
-	git config merge.log true &&
+	test_config merge.log true &&
 	git fmt-merge-msg -m "Sync with left" \
 					<.git/FETCH_HEAD >actual.log-config &&
 	git fmt-merge-msg --no-log -m "Sync with left" \

@@ -290,29 +285,29 @@ test_expect_success 'setup: expected shortlog for two branches' '
 '
 
 test_expect_success 'shortlog for two branches' '
-	git config merge.log true &&
-	test_might_fail git config --unset-all merge.summary &&
+	test_config merge.log true &&
+	test_unconfig merge.summary &&
 	git checkout master &&
 	test_tick &&
 	git fetch . left right &&
 	git fmt-merge-msg <.git/FETCH_HEAD >actual1 &&
 
-	test_might_fail git config --unset-all merge.log &&
-	git config merge.summary true &&
+	test_unconfig merge.log &&
+	test_config merge.summary true &&
 	git checkout master &&
 	test_tick &&
 	git fetch . left right &&
 	git fmt-merge-msg <.git/FETCH_HEAD >actual2 &&
 
-	git config merge.log yes &&
-	test_might_fail git config --unset-all merge.summary &&
+	test_config merge.log yes &&
+	test_unconfig merge.summary &&
 	git checkout master &&
 	test_tick &&
 	git fetch . left right &&
 	git fmt-merge-msg <.git/FETCH_HEAD >actual3 &&
 
-	test_might_fail git config --unset-all merge.log &&
-	git config merge.summary yes &&
+	test_unconfig merge.log &&
+	test_config merge.summary yes &&
 	git checkout master &&
 	test_tick &&
 	git fetch . left right &&

@@ -325,8 +320,8 @@ test_expect_success 'shortlog for two branches' '
 '
 
 test_expect_success 'merge-msg -F' '
-	test_might_fail git config --unset-all merge.log &&
-	git config merge.summary yes &&
+	test_unconfig merge.log &&
+	test_config merge.summary yes &&
 	git checkout master &&
 	test_tick &&
 	git fetch . left right &&

@@ -335,8 +330,8 @@ test_expect_success 'merge-msg -F' '
 '
 
 test_expect_success 'merge-msg -F in subdirectory' '
-	test_might_fail git config --unset-all merge.log &&
-	git config merge.summary yes &&
+	test_unconfig merge.log &&
+	test_config merge.summary yes &&
 	git checkout master &&
 	test_tick &&
 	git fetch . left right &&

@@ -350,8 +345,8 @@ test_expect_success 'merge-msg -F in subdirectory' '
 '
 
 test_expect_success 'merge-msg with nothing to merge' '
-	test_might_fail git config --unset-all merge.log &&
-	git config merge.summary yes &&
+	test_unconfig merge.log &&
+	test_config merge.summary yes &&
 
 	>empty &&

@@ -376,8 +371,8 @@ test_expect_success 'merge-msg tag' '
 	  Common #1
 	EOF
 
-	test_might_fail git config --unset-all merge.log &&
-	git config merge.summary yes &&
+	test_unconfig merge.log &&
+	test_config merge.summary yes &&
 
 	git checkout master &&
 	test_tick &&

@@ -406,8 +401,8 @@ test_expect_success 'merge-msg two tags' '
 	  Common #1
 	EOF
 
-	test_might_fail git config --unset-all merge.log &&
-	git config merge.summary yes &&
+	test_unconfig merge.log &&
+	test_config merge.summary yes &&
 
 	git checkout master &&
 	test_tick &&

@@ -436,8 +431,8 @@ test_expect_success 'merge-msg tag and branch' '
 	  Common #1
 	EOF
 
-	test_might_fail git config --unset-all merge.log &&
-	git config merge.summary yes &&
+	test_unconfig merge.log &&
+	test_config merge.summary yes &&
 
 	git checkout master &&
 	test_tick &&

@@ -464,6 +459,8 @@ test_expect_success 'merge-msg lots of commits' '
 		echo "  ..."
 	} >expected &&
 
+	test_config merge.summary yes &&
+
 	git checkout master &&
 	test_tick &&
 	git fetch . long &&

-- 
1.8.2-480-g064f421

[PATCH 0/3] Merging an annotated tag object

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:35

This is a patch I posted as an illustration, and then have been
carrying in my tree a while, with some tests.

Junio C Hamano (3):
  merge: a random object may not necssarily be a commit
  t6200: use test_config/test_unconfig
  t6200: test message for merging of an annotated tag

 builtin/merge.c          |  13 ++++++
 t/t6200-fmt-merge-msg.sh | 100 ++++++++++++++++++++++++++++++++---------------
 2 files changed, 81 insertions(+), 32 deletions(-)

-- 
1.8.2-480-g064f421

[PATCH 3/3] t6200: test message for merging of an annotated tag

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:35

Signed-off-by: Junio C Hamano <redacted>
---
 t/t6200-fmt-merge-msg.sh | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index f84bb0c..f73ecea 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh

@@ -469,4 +469,43 @@ test_expect_success 'merge-msg lots of commits' '
 	test_cmp expected actual
 '
 
+test_expect_success 'merge-msg with "merging" an annotated tag' '
+	test_config merge.log true &&
+
+	git checkout master^0 &&
+	git commit --allow-empty -m "One step ahead" &&
+	git tag -a -m "An annotated one" annote HEAD &&
+
+	git checkout master &&
+	git fetch . annote &&
+
+	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
+	{
+		cat <<-\EOF
+		Merge tag '\''annote'\''
+
+		An annotated one
+
+		* tag '\''annote'\'':
+		  One step ahead
+		EOF
+	} >expected &&
+	test_cmp expected actual &&
+
+	test_when_finished "git reset --hard" &&
+	annote=$(git rev-parse annote) &&
+	git merge --no-commit $annote &&
+	{
+		cat <<-EOF
+		Merge tag '\''$annote'\''
+
+		An annotated one
+
+		* tag '\''$annote'\'':
+		  One step ahead
+		EOF
+	} >expected &&
+	test_cmp expected .git/MERGE_MSG
+'
+
 test_done

-- 
1.8.2-480-g064f421

Re: [PATCH 1/3] merge: a random object may not necssarily be a commit

From: Yann Droneaud <hidden>
Date: 2016-06-15 22:56:35

Hi,

Le lundi 01 avril 2013 à 12:57 -0700, Junio C Hamano a écrit :

The user could have said "git merge $(git rev-parse v1.0.0)"; we
shouldn't mark it as "Merge commit '15999998fb...'" as the merge
name, even though such an invocation might be crazy.

We could even read the "tag " header from the tag object and replace
the object name the user gave us, but let's not lose the information
by doing so, at least not yet.

Signed-off-by: Junio C Hamano <redacted>

Thanks for the patch.

I gave it a try and found the behavior rather good.

Merging a tag object by its name or by its object-id are now using the
same behavor: it is more consistent. 

Tested-by: Yann Droneaud <redacted>

PS: there's a typo in the commit title :)

Regards.

-- 
Yann Droneaud
OPTEYA

Re: [PATCH 1/3] merge: a random object may not necssarily be a commit

From: Jeff King <hidden>
Date: 2016-06-15 22:56:36

On Mon, Apr 01, 2013 at 12:57:17PM -0700, Junio C Hamano wrote:

quoted hunk

The user could have said "git merge $(git rev-parse v1.0.0)"; we
shouldn't mark it as "Merge commit '15999998fb...'" as the merge
name, even though such an invocation might be crazy.

We could even read the "tag " header from the tag object and replace
the object name the user gave us, but let's not lose the information
by doing so, at least not yet.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/merge.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 0ec8f0d..990e90c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -516,6 +516,19 @@ static void merge_name(const char *remote, struct strbuf *msg)
 		strbuf_release(&line);
 		goto cleanup;
 	}
+
+	if (remote_head->util) {
+		struct merge_remote_desc *desc;
+		desc = merge_remote_util(remote_head);
+		if (desc && desc->obj && desc->obj->type == OBJ_TAG) {
+			strbuf_addf(msg, "%s\t\t%s '%s'\n",
+				    sha1_to_hex(desc->obj->sha1),
+				    typename(desc->obj->type),
+				    remote);
+			goto cleanup;
+		}
+	}
+
 	strbuf_addf(msg, "%s\t\tcommit '%s'\n",
 		sha1_to_hex(remote_head->object.sha1), remote);

I guess there is no other object type besides OBJ_TAG and OBJ_COMMIT
that would yield something we could merge, but it feels weird that you
check only for OBJ_TAG here, and otherwise still say "commit". Would the
intent be more clear if it just said:

  if (desc && desc->obj && desc->obj->type != OBJ_COMMIT) {
          ...

?

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help