git merge <tag> behavior

git merge <tag> behavior

[Yann Droneaud]

Hi,

While trying to reproduce/understand the problems[1][2] I was facing
when using Google's Git repo tool[3], I've found minor problems in Git:

1) there's no mention of the git merge <tag> behavior in git-merge.1

When asking Git to merge a tag (such as a signed tag or annotated tag),
it will always create a merge commit even if fast-forward was possible.
It's like having --no-ff present on the command line.

It's a difference from the default behavior described in git-merge.1[4].
It should be documented as an exception of "FAST-FORWARD MERGE" section
and "--ff" option description.

2) git merge <tag> VS git merge <object-id>

If <tag> is an object (not a lightweight/reference tag), git merge <tag>
will by default create a merge commit with the tag message.
Additionally, the signature check will be reported as comment, for
example:

    Merge tag 'v1.12.2' into branch-v1.12.2

    repo 1.12.2

    # gpg: Signature made Fri Mar  1 18:36:42 2013 CET using DSA key ID 920F5C65
    # gpg: Good signature from "Repo Maintainer <repo@android.kernel.org>"
    # gpg: WARNING: This key is not certified with a trusted signature!
    # gpg:          There is no indication that the signature belongs to the owner.
    # Primary key fingerprint: 8BB9 AD79 3E8E 6153 AF0F  9A44 1653 0D5E 920F 5C65

But, if you use the tag object-id instead of its name, for example using
git merge `git show-ref <tag>`, the tag is not recognized and the
signature is not checked. Git still create a merge commit, but doesn't
prepare a commit message with the tag message and the signature:

    Merge commit 'ac22c7ae2e652f63366b65ee23122292d3564fff' into
branch-ac22c7ae2e652f63366b65ee23122292d3564fff

It would be great to have Git using the tag message and check the
signature.

3) Merge options can't be overridden.

If I modify .git/config to set a merge option, for example forcing
fast-forward merge, this option cannot be overridden on command line:

Example 1:

    $ cat .git/config:
    [branch "master"]
            mergeoptions = --ff-only

    $ git merge --no-ff <tag>
    fatal: You cannot combine --no-ff with --ff-only

Example 2:

    $ cat .git/config:
    [merge]
           ff = only

    $ git merge --no-ff <tag>
    fatal: You cannot combine --no-ff with --ff-only

Setting the merge options in config should overridden by command line.

Regards.

[1] issue 135: repo: repo sync should force fast-forward merge
https://code.google.com/p/git-repo/issues/detail?id=135

[2] Issue 136: repo: repo sync should use the tag name instead of object identifier of the tag
https://code.google.com/p/git-repo/issues/detail?id=136

[3] git-repo - repo - The multiple repository tool 
http://code.google.com/p/git-repo/

[4] git-merge(1) Manual Page
https://www.kernel.org/pub/software/scm/git/docs/git-merge.html

-- 
Yann Droneaud
OPTEYA

Re: git merge <tag> behavior

[Junio C Hamano]

Yann Droneaud <ydroneaud@opteya.com> writes:

> 1) there's no mention of the git merge <tag> behavior in git-merge.1
>
> When asking Git to merge a tag (such as a signed tag or annotated tag),
> it will always create a merge commit even if fast-forward was possible.
> It's like having --no-ff present on the command line.
>
> It's a difference from the default behavior described in git-merge.1[4].
> It should be documented as an exception of "FAST-FORWARD MERGE" section
> and "--ff" option description.

Yes; we welcome documentation patches.

> 2) git merge <tag> VS git merge <object-id>
>
> If <tag> is an object (not a lightweight/reference tag), git merge <tag>
> ...
> But, if you use the tag object-id instead of its name, for example using
> git merge `git show-ref <tag>`,

"git show-ref <tag>" gives you something like

    572a535454612a046e7dd7404dcca94d6243c788 refs/tags/v1.8.2

which is an invalid thing to merge with.  Perhaps you meant

	git merge $(git rev-parse v1.12.2)

> signature is not checked. Git still create a merge commit, but doesn't
> prepare a commit message with the tag message and the signature:
>
> 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.

Re* git merge <tag> behavior

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> 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 0/3] Merging an annotated tag object

[Junio C Hamano]

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 1/3] merge: a random object may not necssarily be a commit

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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

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

[Yann Droneaud]

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 <gitster@pobox.com>

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 <ydroneaud@opteya.com>

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

[Jeff King]

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

> 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 <gitster@pobox.com>
> ---
>  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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Apr 01, 2013 at 12:57:17PM -0700, Junio C Hamano wrote:
>
>> 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 <gitster@pobox.com>
>> ---
>>  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) {
>           ...
>
> ?

I suspect not.

The point of the added code is that it knows we want to special case
merging a tag object, and it wants to keep any other case behaving
the same as before.

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

[Jeff King]

On Tue, Apr 02, 2013 at 08:02:13AM -0700, Junio C Hamano wrote:

> >> +	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) {
> >           ...
> >
> > ?
> 
> I suspect not.
> 
> The point of the added code is that it knows we want to special case
> merging a tag object, and it wants to keep any other case behaving
> the same as before.

Ah. I read it as "if we have a random object, we do not want to just say
"commit X", because it is not a commit.

-Peff

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

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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 3/3] t6200: test message for merging of an annotated tag

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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

[PATCH] Documentation: merging a tag is a special case

[Yann Droneaud]

When asking Git to merge a tag (such as a signed tag or annotated tag),
it will always create a merge commit even if fast-forward was possible.
It's like having --no-ff present on the command line.

It's a difference from the default behavior described in git-merge.txt.
It should be documented as an exception of "FAST-FORWARD MERGE" section
and "--ff" option description.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 Documentation/git-merge.txt     | 9 +++++++++
 Documentation/merge-options.txt | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index c852a26..84bc873 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -170,6 +170,15 @@ happens:
 If you tried a merge which resulted in complex conflicts and
 want to start over, you can recover with `git merge --abort`.
 
+MERGING TAG
+-----------
+
+When merging a tag (annotated or signed), Git will create a merge commit
+even if a fast-forward merge is possible (see above).
+The commit message template will be created from the tag message.
+Additionally, the signature check will be reported as a comment
+if the tag was signed. See also linkgit:git-tag[1].
+
 HOW CONFLICTS ARE PRESENTED
 ---------------------------
 
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..70d1ec0 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -26,7 +26,7 @@ set to `no` at the beginning of them.
 --ff::
 	When the merge resolves as a fast-forward, only update the branch
 	pointer, without creating a merge commit.  This is the default
-	behavior.
+	behavior (except when merging a tag).
 
 --no-ff::
 	Create a merge commit even when the merge resolves as a
-- 
1.7.11.7

Re: [PATCH] Documentation: merging a tag is a special case

[Jonathan Nieder]

Yann Droneaud wrote:

> When asking Git to merge a tag (such as a signed tag or annotated tag),
> it will always create a merge commit even if fast-forward was possible.
> It's like having --no-ff present on the command line.

Thanks.  This looks good, modulo some nitpicks.

[...]
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -170,6 +170,15 @@ happens:
>  If you tried a merge which resulted in complex conflicts and
>  want to start over, you can recover with `git merge --abort`.
>  
> +MERGING TAG
> +-----------
> +
> +When merging a tag (annotated or signed), Git will create a merge commit

How about something like "When merging an annotated or signed tag" or
"When merging an annotated (and possibly signed) tag"?  The above text
can be misread as meaning "When merging any tag, no matter whether it
is annotated or signed", which is needlessly confusing for people who
don't know about unannotated tags.

[...]
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -26,7 +26,7 @@ set to `no` at the beginning of them.
>  --ff::
>  	When the merge resolves as a fast-forward, only update the branch
>  	pointer, without creating a merge commit.  This is the default
> -	behavior.
> +	behavior (except when merging a tag).

s/a tag/an annotated tag/ here as well.

By the way, what about the possibility of dropping this implicit
--no-ff?  I think Linus could get used to passing --no-ff explicitly
when responding to pull requests.  I could go either way on it.

It is certainly useful to document the current state before
considering changing it, so with the tweaks mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH] Documentation: merging a tag is a special case

[Junio C Hamano]

Yann Droneaud <ydroneaud@opteya.com> writes:

> When asking Git to merge a tag (such as a signed tag or annotated tag),
> it will always create a merge commit even if fast-forward was possible.
> It's like having --no-ff present on the command line.
>
> It's a difference from the default behavior described in git-merge.txt.
> It should be documented as an exception of "FAST-FORWARD MERGE" section
> and "--ff" option description.
>
> Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
> ---
>  Documentation/git-merge.txt     | 9 +++++++++
>  Documentation/merge-options.txt | 2 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index c852a26..84bc873 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -170,6 +170,15 @@ happens:
>  If you tried a merge which resulted in complex conflicts and
>  want to start over, you can recover with `git merge --abort`.
>  
> +MERGING TAG
> +-----------
> +
> +When merging a tag (annotated or signed), Git will create a merge commit
> +even if a fast-forward merge is possible (see above).
> +The commit message template will be created from the tag message.
> +Additionally, the signature check will be reported as a comment
> +if the tag was signed. See also linkgit:git-tag[1].
> +

It would make it more helpful to readers to describe how _not_ to
create such a merge commit if it is unwanted, and how the request to
merge a tag interacts with --ff-only option.

>  HOW CONFLICTS ARE PRESENTED
>  ---------------------------
>  
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 0bcbe0a..70d1ec0 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -26,7 +26,7 @@ set to `no` at the beginning of them.
>  --ff::
>  	When the merge resolves as a fast-forward, only update the branch
>  	pointer, without creating a merge commit.  This is the default
> -	behavior.
> +	behavior (except when merging a tag).

With this update, the reader will be left wondering what would be
the default when she asks Git to merge a tag, no?

>  --no-ff::
>  	Create a merge commit even when the merge resolves as a

Re: [PATCH] Documentation: merging a tag is a special case

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +MERGING TAG
>> +-----------
>> +
>> +When merging a tag (annotated or signed), Git will create a merge commit
>> +...
>> +if the tag was signed. See also linkgit:git-tag[1].
>> +
>
> It would make it more helpful to readers to describe how _not_ to
> create such a merge commit if it is unwanted, and how the request to
> merge a tag interacts with --ff-only option.
>
>> @@ -26,7 +26,7 @@ set to `no` at the beginning of them.
>>  --ff::
>>  	When the merge resolves as a fast-forward, only update the branch
>>  	pointer, without creating a merge commit.  This is the default
>> -	behavior.
>> +	behavior (except when merging a tag).
>
> With this update, the reader will be left wondering what would be
> the default when she asks Git to merge a tag, no?

Taking Jonathan's input and the above into account, perhaps we can
do something like this on top of the posted patch?

 Documentation/git-merge.txt     | 26 +++++++++++++++++++++-----
 Documentation/merge-options.txt |  5 +++--
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index f7e68e1..75b5ee7 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -173,11 +173,27 @@ want to start over, you can recover with `git merge --abort`.
 MERGING TAG
 -----------
 
-When merging a tag (annotated or signed), Git will create a merge commit
-even if a fast-forward merge is possible (see above).
-The commit message template will be created from the tag message.
-Additionally, the signature check will be reported as a comment
-if the tag was signed. See also linkgit:git-tag[1].
+When merging an annotated (and possibly signed) tag, Git always
+creates a merge commit even if a fast-forward merge is possible, and
+the commit message template is prepared with the tag message.  
+Additionally, the signature check is reported as a comment
+if the tag is signed.  See also linkgit:git-tag[1].
+
+Consequently a request `git merge --ff-only v1.2.3` to merge such a
+tag would fail.
+
+When you want to just integrate with the work leading to the commit
+that happens to be tagged, e.g. synchronizing with an upstream
+release point, you may not want to make an unnecessary merge commit
+especially when you do not have any work on your own.  In such a
+case, you can "unwrap" the tag yourself before feeding it to `git
+merge`, e.g.
+
+---
+git fetch origin
+git merge [--ff-only] v1.2.3^0
+---
+
 
 HOW CONFLICTS ARE PRESENTED
 ---------------------------
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 70d1ec0..34a8445 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -26,11 +26,12 @@ set to `no` at the beginning of them.
 --ff::
 	When the merge resolves as a fast-forward, only update the branch
 	pointer, without creating a merge commit.  This is the default
-	behavior (except when merging a tag).
+	behavior.
 
 --no-ff::
 	Create a merge commit even when the merge resolves as a
-	fast-forward.
+	fast-forward.  This is the default behaviour when merging an
+	annotated (and possibly signed) tag.
 
 --ff-only::
 	Refuse to merge and exit with a non-zero status unless the

Re: [PATCH] Documentation: merging a tag is a special case

[Jonathan Nieder]

Junio C Hamano wrote:

> +Consequently a request `git merge --ff-only v1.2.3` to merge such a
> +tag would fail.
> +
> +When you want to just integrate with the work leading to the commit
> +that happens to be tagged, e.g. synchronizing with an upstream
> +release point, you may not want to make an unnecessary merge commit
> +especially when you do not have any work on your own.  In such a
> +case, you can "unwrap" the tag yourself before feeding it to `git
> +merge`, e.g.
> +
> +---
> +git fetch origin
> +git merge [--ff-only] v1.2.3^0
> +---

Nice and clear, but doesn't this contradict b5c9f1c1b0ed (merge: do
not create a signed tag merge under --ff-only option, 2012-02-05)?

Re: [PATCH] Documentation: merging a tag is a special case

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Nice and clear, but doesn't this contradict b5c9f1c1b0ed (merge: do
> not create a signed tag merge under --ff-only option, 2012-02-05)?

It does X-<.  Here is a replacement.

The "--ff-only v1.2.3 will fail" can be left unsaid because it would
fail (and succeed) under the same condition "-ff-only v1.2.3^0"
would.

 Documentation/git-merge.txt | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index df2d28d..d1f3df9 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -179,19 +179,18 @@ the commit message template is prepared with the tag message.
 Additionally, the signature check is reported as a comment
 if the tag is signed.  See also linkgit:git-tag[1].
 
-Consequently a request `git merge --ff-only v1.2.3` to merge such a
-tag would fail.
-
 When you want to just integrate with the work leading to the commit
 that happens to be tagged, e.g. synchronizing with an upstream
-release point, you may not want to make an unnecessary merge commit
-especially when you do not have any work on your own.  In such a
-case, you can "unwrap" the tag yourself before feeding it to `git
-merge`, e.g.
+release point, you may not want to make an unnecessary merge commit.
+
+In such a case, you can "unwrap" the tag yourself before feeding it
+to `git merge`, or pass `--ff-only` when you do not have any work on
+your own. e.g.
 
 ---
 git fetch origin
-git merge [--ff-only] v1.2.3^0
+git merge v1.2.3^0
+git merge --ff-only v1.2.3
 ---
 
 

Re: [PATCH] Documentation: merging a tag is a special case

[Jonathan Nieder]

Junio C Hamano wrote:

>               Here is a replacement.

Looks good.  Thanks for taking care of this.

Re: [PATCH] Documentation: merging a tag is a special case

[Yann Droneaud]

Hi,

Just a little change I made on my own.
The other part are definitely better than my version, so I propose
to merge all the patches in the thread with you as author,
putting Jonathan Nieder and myself as reviewers.

Regards

 Documentation/git-merge.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index dd90feb..42391f2 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -176,8 +176,8 @@ MERGING TAG
 When merging an annotated (and possibly signed) tag, Git always
 creates a merge commit even if a fast-forward merge is possible, and
 the commit message template is prepared with the tag message.
-Additionally, the signature check is reported as a comment
-if the tag is signed.  See also linkgit:git-tag[1].
+Additionally, if the tag is signed, the signature check is reported
+as a comment in the message template. See also linkgit:git-tag[1].
 
 When you want to just integrate with the work leading to the commit
 that happens to be tagged, e.g. synchronizing with an upstream
-- 
1.7.11.7

[PATCH v2] Documentation: merging a tag is a special case

[Yann Droneaud]

From: Junio C Hamano <gitster@pobox.com>

When asking Git to merge a tag (such as a signed tag or annotated tag),
it will always create a merge commit even if fast-forward was possible.
It's like having --no-ff present on the command line.

It's a difference from the default behavior described in git-merge.txt.
It should be documented as an exception of "FAST-FORWARD MERGE" section
and "--ff" option description.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Reviewed-by: Yann Droneaud <ydroneaud@opteya.com>
---
 Documentation/git-merge.txt     | 24 ++++++++++++++++++++++++
 Documentation/merge-options.txt |  3 ++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index c852a26..42391f2 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -170,6 +170,30 @@ happens:
 If you tried a merge which resulted in complex conflicts and
 want to start over, you can recover with `git merge --abort`.
 
+MERGING TAG
+-----------
+
+When merging an annotated (and possibly signed) tag, Git always
+creates a merge commit even if a fast-forward merge is possible, and
+the commit message template is prepared with the tag message.
+Additionally, if the tag is signed, the signature check is reported
+as a comment in the message template. See also linkgit:git-tag[1].
+
+When you want to just integrate with the work leading to the commit
+that happens to be tagged, e.g. synchronizing with an upstream
+release point, you may not want to make an unnecessary merge commit.
+
+In such a case, you can "unwrap" the tag yourself before feeding it
+to `git merge`, or pass `--ff-only` when you do not have any work on
+your own. e.g.
+
+---
+git fetch origin
+git merge v1.2.3^0
+git merge --ff-only v1.2.3
+---
+
+
 HOW CONFLICTS ARE PRESENTED
 ---------------------------
 
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..34a8445 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -30,7 +30,8 @@ set to `no` at the beginning of them.
 
 --no-ff::
 	Create a merge commit even when the merge resolves as a
-	fast-forward.
+	fast-forward.  This is the default behaviour when merging an
+	annotated (and possibly signed) tag.
 
 --ff-only::
 	Refuse to merge and exit with a non-zero status unless the
-- 
1.7.11.7

Re: [PATCH v2] Documentation: merging a tag is a special case

[Jonathan Nieder]

Yann Droneaud wrote:

> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Yes, I think this is in good shape now.

Re: git merge <tag> behavior

[Yann Droneaud]

Hi,

Le mardi 19 mars 2013 à 09:19 -0700, Junio C Hamano a écrit :
> Yann Droneaud <ydroneaud@opteya.com> writes:
> 
> > 1) there's no mention of the git merge <tag> behavior in git-merge.1

> Yes; we welcome documentation patches.
> 

Sent.

> > 2) git merge <tag> VS git merge <object-id>
> >
> > If <tag> is an object (not a lightweight/reference tag), git merge <tag>
> > ...
> > But, if you use the tag object-id instead of its name, for example using
> > git merge `git parse-rev <tag>`,
[EDIT]
> > signature is not checked. Git still create a merge commit, but doesn't
> > prepare a commit message with the tag message and the signature:
> >
> > 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.

I think if someone want to merge the tag object-id instead of the tag,
the commit subject/message should probably not make a reference to the
tag.

The only use case for such tag merging by commit-id would be to get
consistent behavior in case of tag deletion. The named tag could be
recreated to point to another point in time. So when looking at the
merge commit message and searching for the tag (by name) could be
misleading.

PS: and what about my third issue ?

Regards.

-- 
Yann Droneaud
OPTEYA

Re: git merge <tag> behavior

[Yann Droneaud]

Hi,
 
Le mercredi 20 mars 2013 à 19:04 +0100, Yann Droneaud a écrit :
> > > 2) git merge <tag> VS git merge <object-id>
> > >
> > > If <tag> is an object (not a lightweight/reference tag), git merge <tag>
> > > ...
> > > But, if you use the tag object-id instead of its name, for example using
> > > git merge `git parse-rev <tag>`,
> [EDIT]
> > > signature is not checked. Git still create a merge commit, but doesn't
> > > prepare a commit message with the tag message and the signature:
> > >
> > > 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.
> 
> I think if someone want to merge the tag object-id instead of the tag,
> the commit subject/message should probably not make a reference to the
> tag.
> 
> The only use case for such tag merging by commit-id would be to get
> consistent behavior in case of tag deletion. The named tag could be
> recreated to point to another point in time. So when looking at the
> merge commit message and searching for the tag (by name) could be
> misleading.
> 

But but do not take those remarks as a feature request.
I was just asking for clarification/comment on the behavior difference
between merging tag/tag object-id.

Regards

-- 
Yann Droneaud
OPTEYA

Re: git merge <tag> behavior

[Junio C Hamano]

Yann Droneaud <yann@droneaud.fr> writes:

> But but do not take those remarks as a feature request.
> I was just asking for clarification/comment on the behavior difference
> between merging tag/tag object-id.

If you are asking why things are as they are, the answer is simply
because "git merge $(git rev-parse v1.2.3)" was not even considered
while adding the support to pull signed tags.

We did find the use case for "git merge v1.2.3" interesting and
important enough to give it a proper support with defined semantics.
"git merge $(git rev-parse v1.2.3)" may behave differently but it
was not because we found the use case for it important and designed
a behaviour that is different from merging the tag by name that
suits that use case.

It is just we didn't even think giving the bare object name to name
an annotated or signed tag on the command line is interesting, and
the command does whatever the implementation happens to do to such
an input.

I think I sent out a "how about this" patch.  Have you tried it?

Re: git merge <tag> behavior

[Max Nanasy]

Yann Droneaud <ydroneaud <at> opteya.com> writes:

> 
> 3) Merge options can't be overridden.
> 
> If I modify .git/config to set a merge option, for example forcing
> fast-forward merge, this option cannot be overridden on command line:
> 

(git merge --no-ff-only --no-ff) should work.  The --no-ff-only overrides
the --ff-only configuration setting, and the --no-ff ensures that the merge
is not a fast-forward.

Re: git merge <tag> behavior

[Yann Droneaud]

Hi,

Le 21.03.2013 21:31, Max Nanasy a écrit :
> Yann Droneaud <ydroneaud <at> opteya.com> writes:
>
>>
>> 3) Merge options can't be overridden.
>>
>> If I modify .git/config to set a merge option, for example forcing
>> fast-forward merge, this option cannot be overridden on command 
>> line:
>>
>
> (git merge --no-ff-only --no-ff) should work.  The --no-ff-only 
> overrides
> the --ff-only configuration setting, and the --no-ff ensures that the 
> merge
> is not a fast-forward.
>

Thanks. I wasn't aware of the --no-ff-only option and
thought --no-ff would be the opposite of --ff-only,
or at least disable it given the order of the options.

Please find a patch to document option --no-ff-only

   Documentation/merge-options.txt | 4 ++++
   1 file changed, 4 insertions(+)

diff --git a/Documentation/merge-options.txt
b/Documentation/merge-options.txt
index 0bcbe0a..20a31cf 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -37,6 +37,10 @@ set to `no` at the beginning of them.
  	current `HEAD` is already up-to-date or the merge can be
  	resolved as a fast-forward.

+--no-ff-only::
+	Disable `--ff-only` behavior, eg. allows creation of merge commit.
+	This is the default behavior.
+
   --log[=<n>]::
   --no-log::
  	In addition to branch names, populate the log message with

[PATCH] t7600: test merge configuration override

[Yann Droneaud]

Set the configuration variable 'merge.ff' to either 'only' or 'no'
and check that this configuration can be overridden on command line.

Additionally, test for currently not tested option '--no-ff-only'

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t7600-merge.sh | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 5e19598..b524bdb 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -254,6 +254,32 @@ test_expect_success 'merges with merge.ff=only' '
 	verify_head $c3
 '
 
+test_expect_success 'merges with merge.ff=only and --no-ff-only' '
+	git reset --hard c1 &&
+	test_tick &&
+	test_when_finished "git config --unset merge.ff" &&
+	git config merge.ff only &&
+	test_must_fail git merge --no-ff c2 &&
+	git merge --no-ff-only c2 &&
+
+	git reset --hard c1 &&
+	git merge --no-ff-only --no-ff c2
+'
+
+test_expect_success 'merges with merge.ff=no and --ff' '
+	git reset --hard c0 &&
+	test_tick &&
+	test_when_finished "git config --unset merge.ff" &&
+	git config merge.ff no &&
+	test_must_fail git merge --ff-only c1 &&
+	git merge --ff c1 &&
+	verify_head $c1 &&
+
+	git reset --hard c0 &&
+	git merge --ff --ff-only c1 &&
+	verify_head $c1
+'
+
 test_expect_success 'merge c0 with c1 (no-commit)' '
 	git reset --hard c0 &&
 	git merge --no-commit c1 &&
-- 
1.7.11.7

Re: [PATCH] t7600: test merge configuration override

[Junio C Hamano]

Yann Droneaud <ydroneaud@opteya.com> writes:

> +test_expect_success 'merges with merge.ff=only and --no-ff-only' '
> +	git reset --hard c1 &&
> +	test_tick &&
> +	test_when_finished "git config --unset merge.ff" &&
> +	git config merge.ff only &&

I see this was copied from existing tests, but we should use
"test_config" these days.  It would be a good approach to first do a
preparatory patch to convert the existing ones to use "test_config"
and then to redo this patch using "test_config" on top of it.

[PATCH 00/15] Use test_config

[Yann Droneaud]

Please find some patches to use test_config/test_unconfig

Instead of using construct such as:
   test_when_finished "git config --unset <key>"
   git config <key> <value>
uses
   test_config <key> <value>
The latter takes care of removing <key> at the end of the test.
    
Additionally, instead of
   git config <key> ""
or
   git config --unset <key>
 uses
   test_unconfig <key>
The latter doesn't failed if <key> is not defined.

Patch "t7600: use test_config to set/unset git config variables"
is more important than the other and must be carefully reviewed
regarded to the --no-log --no-ff behavior.

Others patches are fairly simple.

Testsuite results are the same after the patches.
Tested against master, 7b592fadf1e23b10b913e0771b9f711770597266

Yann Droneaud (15):
  t4018: remove test_config implementation
  t7810: remove test_config implementation
  t7811: remove test_config implementation
  t3400: use test_config to set/unset git config variables
  t4304: use test_config to set/unset git config variables
  t4034: use test_config/test_unconfig to set/unset git config
    variables
  t4202: use test_config/test_unconfig to set/unset git config
    variables
  t5520: use test_config to set/unset git config variables
  t5541: use test_config to set/unset git config variables
  t7500: use test_config to set/unset git config variables
  t7502: use test_config to set/unset git config variables
  t7508: use test_config to set/unset git config variables
  t7600: use test_config to set/unset git config variables
  t9500: use test_config to set/unset git config variables
  t7502: remove clear_config

 t/t3400-rebase.sh                      |  3 +-
 t/t3404-rebase-interactive.sh          |  3 +-
 t/t4018-diff-funcname.sh               |  5 ---
 t/t4034-diff-words.sh                  |  7 ++--
 t/t4202-log.sh                         | 28 +++++-----------
 t/t5520-pull.sh                        | 12 +++----
 t/t5541-http-push.sh                   |  3 +-
 t/t7500-commit.sh                      |  6 ++--
 t/t7502-commit.sh                      | 40 +++++------------------
 t/t7508-status.sh                      | 46 +++++++++-----------------
 t/t7600-merge.sh                       | 60 +++++++++++++++-------------------
 t/t7810-grep.sh                        |  5 ---
 t/t7811-grep-open.sh                   |  5 ---
 t/t9500-gitweb-standalone-no-errors.sh |  3 +-
 14 files changed, 72 insertions(+), 154 deletions(-)

-- 
1.7.11.7

[PATCH 01/15] t4018: remove test_config implementation

[Yann Droneaud]

test_config is provided by the test library,
a private version is not needed.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t4018-diff-funcname.sh | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 082d3e8..38a092a 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -93,11 +93,6 @@ sed -e '
 	s/song;/song();/
 ' <Beer.perl >Beer-correct.perl
 
-test_config () {
-	git config "$1" "$2" &&
-	test_when_finished "git config --unset $1"
-}
-
 test_expect_funcname () {
 	lang=${2-java}
 	test_expect_code 1 git diff --no-index -U1 \
-- 
1.7.11.7

[PATCH 02/15] t7810: remove test_config implementation

[Yann Droneaud]

test_config is provided by the test library,
a private version is not needed.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t7810-grep.sh | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index f698001..500eb50 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1084,11 +1084,6 @@ test_expect_success 'grep -E pattern with grep.patternType=fixed' '
 	test_cmp expected actual
 '
 
-test_config() {
-	git config "$1" "$2" &&
-	test_when_finished "git config --unset $1"
-}
-
 cat >expected <<EOF
 hello.c<RED>:<RESET>int main(int argc, const char **argv)
 hello.c<RED>-<RESET>{
-- 
1.7.11.7

[PATCH 03/15] t7811: remove test_config implementation

[Yann Droneaud]

test_config is provided by the test library,
a private version is not needed.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t7811-grep-open.sh | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh
index a895778..e1951a5 100755
--- a/t/t7811-grep-open.sh
+++ b/t/t7811-grep-open.sh
@@ -125,11 +125,6 @@ test_expect_success 'modified file' '
 	test_cmp empty out
 '
 
-test_config() {
-	git config "$1" "$2" &&
-	test_when_finished "git config --unset $1"
-}
-
 test_expect_success 'copes with color settings' '
 	rm -f actual &&
 	echo grep.h >expect &&
-- 
1.7.11.7

[PATCH 04/15] t3400: use test_config to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t3400-rebase.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 1de0ebd..f6cc102 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -138,8 +138,7 @@ test_expect_success 'rebase a single mode change' '
 '
 
 test_expect_success 'rebase is not broken by diff.renames' '
-	git config diff.renames copies &&
-	test_when_finished "git config --unset diff.renames" &&
+	test_config diff.renames copies &&
 	git checkout filemove &&
 	GIT_TRACE=1 git rebase force-3way
 '
-- 
1.7.11.7

[PATCH 05/15] t4304: use test_config to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Tests are modified to assume correct (default) configuration at entry,
and to reset the modified configuration variables at the end.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t3404-rebase-interactive.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 15dcbd4..a58406d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -937,8 +937,7 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '
 test_expect_success 'rebase -i respects core.commentchar' '
 	git reset --hard &&
 	git checkout E^0 &&
-	git config core.commentchar "\\" &&
-	test_when_finished "git config --unset core.commentchar" &&
+	test_config core.commentchar "\\" &&
 	write_script remove-all-but-first.sh <<-\EOF &&
 	sed -e "2,\$s/^/\\\\/" "$1" >"$1.tmp" &&
 	mv "$1.tmp" "$1"
-- 
1.7.11.7

[PATCH 06/15] t4034: use test_config/test_unconfig to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Additionally, instead of
     git config <key> ""
or
     git config --unset <key>
uses
     test_unconfig <key>
The latter doesn't failed if <key> is not defined.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t4034-diff-words.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 40ab333..f2f55fc 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -230,7 +230,7 @@ test_expect_success '.gitattributes override config' '
 '
 
 test_expect_success 'setup: remove diff driver regex' '
-	test_might_fail git config --unset diff.testdriver.wordRegex
+	test_unconfig diff.testdriver.wordRegex
 '
 
 test_expect_success 'use configured regex' '
@@ -335,8 +335,7 @@ test_expect_success 'word-diff with diff.sbe' '
 
 	c
 	EOF
-	test_when_finished "git config --unset diff.suppress-blank-empty" &&
-	git config diff.suppress-blank-empty true &&
+	test_config diff.suppress-blank-empty true &&
 	word_diff --word-diff=plain
 '
 
@@ -368,7 +367,7 @@ test_expect_success 'setup history with two files' '
 
 test_expect_success 'wordRegex for the first file does not apply to the second' '
 	echo "*.tex diff=tex" >.gitattributes &&
-	git config diff.tex.wordRegex "[a-z]+|." &&
+	test_config diff.tex.wordRegex "[a-z]+|." &&
 	cat >expect <<-\EOF &&
 		diff --git a/a.tex b/a.tex
 		--- a/a.tex
-- 
1.7.11.7

[PATCH 07/15] t4202: use test_config/test_unconfig to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Additionally, instead of
     git config <key> ""
or
     git config --unset <key>
uses
     test_unconfig <key>
The latter doesn't failed if <key> is not defined.

Tests are modified to assume correct (default) configuration at entry,
and to reset the modified configuration variables at the end.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t4202-log.sh | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index fa686b8..9243a97 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -419,8 +419,6 @@ test_expect_success 'log --graph with merge' '
 '
 
 test_expect_success 'log.decorate configuration' '
-	test_might_fail git config --unset-all log.decorate &&
-
 	git log --oneline >expect.none &&
 	git log --oneline --decorate >expect.short &&
 	git log --oneline --decorate=full >expect.full &&
@@ -429,8 +427,7 @@ test_expect_success 'log.decorate configuration' '
 	git log --oneline >actual &&
 	test_cmp expect.short actual &&
 
-	git config --unset-all log.decorate &&
-	git config log.decorate true &&
+	test_config log.decorate true &&
 	git log --oneline >actual &&
 	test_cmp expect.short actual &&
 	git log --oneline --decorate=full >actual &&
@@ -438,8 +435,7 @@ test_expect_success 'log.decorate configuration' '
 	git log --oneline --decorate=no >actual &&
 	test_cmp expect.none actual &&
 
-	git config --unset-all log.decorate &&
-	git config log.decorate no &&
+	test_config log.decorate no &&
 	git log --oneline >actual &&
 	test_cmp expect.none actual &&
 	git log --oneline --decorate >actual &&
@@ -447,8 +443,7 @@ test_expect_success 'log.decorate configuration' '
 	git log --oneline --decorate=full >actual &&
 	test_cmp expect.full actual &&
 
-	git config --unset-all log.decorate &&
-	git config log.decorate 1 &&
+	test_config log.decorate 1 &&
 	git log --oneline >actual &&
 	test_cmp expect.short actual &&
 	git log --oneline --decorate=full >actual &&
@@ -456,8 +451,7 @@ test_expect_success 'log.decorate configuration' '
 	git log --oneline --decorate=no >actual &&
 	test_cmp expect.none actual &&
 
-	git config --unset-all log.decorate &&
-	git config log.decorate short &&
+	test_config log.decorate short &&
 	git log --oneline >actual &&
 	test_cmp expect.short actual &&
 	git log --oneline --no-decorate >actual &&
@@ -465,8 +459,7 @@ test_expect_success 'log.decorate configuration' '
 	git log --oneline --decorate=full >actual &&
 	test_cmp expect.full actual &&
 
-	git config --unset-all log.decorate &&
-	git config log.decorate full &&
+	test_config log.decorate full &&
 	git log --oneline >actual &&
 	test_cmp expect.full actual &&
 	git log --oneline --no-decorate >actual &&
@@ -474,16 +467,15 @@ test_expect_success 'log.decorate configuration' '
 	git log --oneline --decorate >actual &&
 	test_cmp expect.short actual
 
-	git config --unset-all log.decorate &&
+	test_unconfig log.decorate &&
 	git log --pretty=raw >expect.raw &&
-	git config log.decorate full &&
+	test_config log.decorate full &&
 	git log --pretty=raw >actual &&
 	test_cmp expect.raw actual
 
 '
 
 test_expect_success 'reflog is expected format' '
-	test_might_fail git config --remove-section log &&
 	git log -g --abbrev-commit --pretty=oneline >expect &&
 	git reflog >actual &&
 	test_cmp expect actual
@@ -496,10 +488,6 @@ test_expect_success 'whatchanged is expected format' '
 '
 
 test_expect_success 'log.abbrevCommit configuration' '
-	test_when_finished "git config --unset log.abbrevCommit" &&
-
-	test_might_fail git config --unset log.abbrevCommit &&
-
 	git log --abbrev-commit >expect.log.abbrev &&
 	git log --no-abbrev-commit >expect.log.full &&
 	git log --pretty=raw >expect.log.raw &&
@@ -508,7 +496,7 @@ test_expect_success 'log.abbrevCommit configuration' '
 	git whatchanged --abbrev-commit >expect.whatchanged.abbrev &&
 	git whatchanged --no-abbrev-commit >expect.whatchanged.full &&
 
-	git config log.abbrevCommit true &&
+	test_config log.abbrevCommit true &&
 
 	git log >actual &&
 	test_cmp expect.log.abbrev actual &&
-- 
1.7.11.7

[PATCH 08/15] t5520: use test_config to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t5520-pull.sh | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 35304b4..cb1a4c5 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -96,8 +96,7 @@ test_expect_success '--rebase' '
 '
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
-	git config --bool pull.rebase true &&
-	test_when_finished "git config --unset pull.rebase" &&
+	test_config pull.rebase true &&
 	git pull . copy &&
 	test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
 	test new = $(git show HEAD:file2)
@@ -105,8 +104,7 @@ test_expect_success 'pull.rebase' '
 
 test_expect_success 'branch.to-rebase.rebase' '
 	git reset --hard before-rebase &&
-	git config --bool branch.to-rebase.rebase true &&
-	test_when_finished "git config --unset branch.to-rebase.rebase" &&
+	test_config branch.to-rebase.rebase true &&
 	git pull . copy &&
 	test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
 	test new = $(git show HEAD:file2)
@@ -114,10 +112,8 @@ test_expect_success 'branch.to-rebase.rebase' '
 
 test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 	git reset --hard before-rebase &&
-	git config --bool pull.rebase true &&
-	test_when_finished "git config --unset pull.rebase" &&
-	git config --bool branch.to-rebase.rebase false &&
-	test_when_finished "git config --unset branch.to-rebase.rebase" &&
+	test_config pull.rebase true &&
+	test_config branch.to-rebase.rebase false &&
 	git pull . copy &&
 	test $(git rev-parse HEAD^) != $(git rev-parse copy) &&
 	test new = $(git show HEAD:file2)
-- 
1.7.11.7

[PATCH 09/15] t5541: use test_config to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t5541-http-push.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 4b4b4a6..4086f02 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -181,8 +181,7 @@ test_expect_success 'push (chunked)' '
 	git checkout master &&
 	test_commit commit path3 &&
 	HEAD=$(git rev-parse --verify HEAD) &&
-	git config http.postbuffer 4 &&
-	test_when_finished "git config --unset http.postbuffer" &&
+	test_config http.postbuffer 4 &&
 	git push -v -v origin $BRANCH 2>err &&
 	grep "POST git-receive-pack (chunked)" err &&
 	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
-- 
1.7.11.7

[PATCH 10/15] t7500: use test_config to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t7500-commit.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 1c908f4..436b7b6 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -36,8 +36,7 @@ test_expect_success 'nonexistent template file should return error' '
 '
 
 test_expect_success 'nonexistent template file in config should return error' '
-	git config commit.template "$PWD"/notexist &&
-	test_when_finished "git config --unset commit.template" &&
+	test_config commit.template "$PWD"/notexist &&
 	(
 		GIT_EDITOR="echo hello >\"\$1\"" &&
 		export GIT_EDITOR &&
@@ -93,14 +92,13 @@ test_expect_success '-t option should be short for --template' '
 
 test_expect_success 'config-specified template should commit' '
 	echo "new template" > "$TEMPLATE" &&
-	git config commit.template "$TEMPLATE" &&
+	test_config commit.template "$TEMPLATE" &&
 	echo "more content" >> foo &&
 	git add foo &&
 	(
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
 		git commit
 	) &&
-	git config --unset commit.template &&
 	commit_msg_is "new templatecommit message"
 '
 
-- 
1.7.11.7

[PATCH 11/15] t7502: use test_config to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t7502-commit.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index f9b44b7..619d438 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -171,10 +171,9 @@ test_expect_success 'verbose' '
 
 test_expect_success 'verbose respects diff config' '
 
-	git config color.diff always &&
+	test_config color.diff always &&
 	git status -v >actual &&
-	grep "\[1mdiff --git" actual &&
-	git config --unset color.diff
+	grep "\[1mdiff --git" actual
 '
 
 mesg_with_comment_and_newlines='
@@ -534,8 +533,7 @@ use_template="-t template"
 try_commit_status_combo
 
 test_expect_success 'commit --status with custom comment character' '
-	test_when_finished "git config --unset core.commentchar" &&
-	git config core.commentchar ";" &&
+	test_config core.commentchar ";" &&
 	try_commit --status &&
 	test_i18ngrep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
-- 
1.7.11.7

[PATCH 12/15] t7508: use test_config to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t7508-status.sh | 46 ++++++++++++++++------------------------------
 1 file changed, 16 insertions(+), 30 deletions(-)

diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index aecb4d1..e2ffdac 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -131,8 +131,7 @@ cat >expect <<\EOF
 EOF
 
 test_expect_success 'status (advice.statusHints false)' '
-	test_when_finished "git config --unset advice.statusHints" &&
-	git config advice.statusHints false &&
+	test_config advice.statusHints false &&
 	git status >output &&
 	test_i18ncmp expect output
 
@@ -332,8 +331,7 @@ test_expect_success 'status -uno' '
 '
 
 test_expect_success 'status (status.showUntrackedFiles no)' '
-	git config status.showuntrackedfiles no
-	test_when_finished "git config --unset status.showuntrackedfiles" &&
+	test_config status.showuntrackedfiles no &&
 	git status >output &&
 	test_i18ncmp expect output
 '
@@ -348,12 +346,11 @@ cat >expect <<EOF
 #
 # Untracked files not listed
 EOF
-git config advice.statusHints false
 test_expect_success 'status -uno (advice.statusHints false)' '
+	test_config advice.statusHints false &&
 	git status -uno >output &&
 	test_i18ncmp expect output
 '
-git config --unset advice.statusHints
 
 cat >expect << EOF
  M dir1/modified
@@ -400,8 +397,7 @@ test_expect_success 'status -unormal' '
 '
 
 test_expect_success 'status (status.showUntrackedFiles normal)' '
-	git config status.showuntrackedfiles normal
-	test_when_finished "git config --unset status.showuntrackedfiles" &&
+	test_config status.showuntrackedfiles normal
 	git status >output &&
 	test_i18ncmp expect output
 '
@@ -459,8 +455,7 @@ test_expect_success 'status -uall' '
 '
 
 test_expect_success 'status (status.showUntrackedFiles all)' '
-	git config status.showuntrackedfiles all
-	test_when_finished "git config --unset status.showuntrackedfiles" &&
+	test_config status.showuntrackedfiles all
 	git status >output &&
 	test_i18ncmp expect output
 '
@@ -485,10 +480,9 @@ test_expect_success 'status -s -uall' '
 	test_cmp expect output
 '
 test_expect_success 'status -s (status.showUntrackedFiles all)' '
-	git config status.showuntrackedfiles all
+	test_config status.showuntrackedfiles all &&
 	git status -s >output &&
 	rm -rf dir3 &&
-	git config --unset status.showuntrackedfiles &&
 	test_cmp expect output
 '
 
@@ -588,15 +582,13 @@ cat >expect <<\EOF
 EOF
 
 test_expect_success 'status with color.ui' '
-	git config color.ui always &&
-	test_when_finished "git config --unset color.ui" &&
+	test_config color.ui always &&
 	git status | test_decode_color >output &&
 	test_i18ncmp expect output
 '
 
 test_expect_success 'status with color.status' '
-	git config color.status always &&
-	test_when_finished "git config --unset color.status" &&
+	test_config color.status always &&
 	git status | test_decode_color >output &&
 	test_i18ncmp expect output
 '
@@ -720,8 +712,7 @@ EOF
 
 test_expect_success 'status without relative paths' '
 
-	git config status.relativePaths false &&
-	test_when_finished "git config --unset status.relativePaths" &&
+	test_config status.relativePaths false &&
 	(cd dir1 && git status) >output &&
 	test_i18ncmp expect output
 
@@ -740,8 +731,7 @@ EOF
 
 test_expect_success 'status -s without relative paths' '
 
-	git config status.relativePaths false &&
-	test_when_finished "git config --unset status.relativePaths" &&
+	test_config status.relativePaths false &&
 	(cd dir1 && git status -s) >output &&
 	test_cmp expect output
 
@@ -1038,15 +1028,14 @@ test_expect_success '--ignore-submodules=untracked suppresses submodules with un
 '
 
 test_expect_success '.gitmodules ignore=untracked suppresses submodules with untracked content' '
-	git config diff.ignoreSubmodules dirty &&
+	test_config diff.ignoreSubmodules dirty &&
 	git status >output &&
 	test_i18ncmp expect output &&
 	git config --add -f .gitmodules submodule.subname.ignore untracked &&
 	git config --add -f .gitmodules submodule.subname.path sm &&
 	git status >output &&
 	test_i18ncmp expect output &&
-	git config -f .gitmodules  --remove-section submodule.subname &&
-	git config --unset diff.ignoreSubmodules
+	git config -f .gitmodules  --remove-section submodule.subname
 '
 
 test_expect_success '.git/config ignore=untracked suppresses submodules with untracked content' '
@@ -1066,15 +1055,14 @@ test_expect_success '--ignore-submodules=dirty suppresses submodules with untrac
 '
 
 test_expect_success '.gitmodules ignore=dirty suppresses submodules with untracked content' '
-	git config diff.ignoreSubmodules dirty &&
+	test_config diff.ignoreSubmodules dirty &&
 	git status >output &&
 	! test -s actual &&
 	git config --add -f .gitmodules submodule.subname.ignore dirty &&
 	git config --add -f .gitmodules submodule.subname.path sm &&
 	git status >output &&
 	test_i18ncmp expect output &&
-	git config -f .gitmodules  --remove-section submodule.subname &&
-	git config --unset diff.ignoreSubmodules
+	git config -f .gitmodules  --remove-section submodule.subname
 '
 
 test_expect_success '.git/config ignore=dirty suppresses submodules with untracked content' '
@@ -1291,15 +1279,13 @@ cat > expect << EOF
 EOF
 
 test_expect_success "status (core.commentchar with submodule summary)" '
-	test_when_finished "git config --unset core.commentchar" &&
-	git config core.commentchar ";" &&
+	test_config core.commentchar ";" &&
 	git status >output &&
 	test_i18ncmp expect output
 '
 
 test_expect_success "status (core.commentchar with two chars with submodule summary)" '
-	test_when_finished "git config --unset core.commentchar" &&
-	git config core.commentchar ";;" &&
+	test_config core.commentchar ";;" &&
 	git status >output &&
 	test_i18ncmp expect output
 '
-- 
1.7.11.7

[PATCH 13/15] t7600: use test_config to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Tests are modified to assume default configuration at entry,
and to reset the modified configuration variables at the end.

Test 'merge log message' was relying on the presence of option `--no-ff`
in the configuration. With the option, git show -s --pretty=format:%b HEAD
produces an empty line and without the option, it produces an empty file.
The test is modified to check with and without `--no-ff` option.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t7600-merge.sh | 60 ++++++++++++++++++++++++--------------------------------
 1 file changed, 26 insertions(+), 34 deletions(-)

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 5e19598..2f70433 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -56,7 +56,8 @@ create_merge_msgs () {
 		echo &&
 		git log --no-merges ^HEAD c2 c3
 	} >squash.1-5-9 &&
-	echo >msg.nolog &&
+	: >msg.nologff &&
+	echo >msg.nolognoff &&
 	{
 		echo "* tag 'c3':" &&
 		echo "  commit 3" &&
@@ -244,8 +245,7 @@ test_expect_success 'merges with --ff-only' '
 test_expect_success 'merges with merge.ff=only' '
 	git reset --hard c1 &&
 	test_tick &&
-	test_when_finished "git config --unset merge.ff" &&
-	git config merge.ff only &&
+	test_config merge.ff "only" &&
 	test_must_fail git merge c2 &&
 	test_must_fail git merge c3 &&
 	test_must_fail git merge c2 c3 &&
@@ -336,7 +336,7 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c1 with c2 (no-commit in config)' '
 	git reset --hard c1 &&
-	git config branch.master.mergeoptions "--no-commit" &&
+	test_config branch.master.mergeoptions "--no-commit" &&
 	git merge c2 &&
 	verify_merge file result.1-5 &&
 	verify_head $c1 &&
@@ -346,12 +346,11 @@ test_expect_success 'merge c1 with c2 (no-commit in config)' '
 test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c1 with c2 (log in config)' '
-	git config branch.master.mergeoptions "" &&
 	git reset --hard c1 &&
 	git merge --log c2 &&
 	git show -s --pretty=tformat:%s%n%b >expect &&
 
-	git config branch.master.mergeoptions --log &&
+	test_config branch.master.mergeoptions "--log" &&
 	git reset --hard c1 &&
 	git merge c2 &&
 	git show -s --pretty=tformat:%s%n%b >actual &&
@@ -360,17 +359,12 @@ test_expect_success 'merge c1 with c2 (log in config)' '
 '
 
 test_expect_success 'merge c1 with c2 (log in config gets overridden)' '
-	test_when_finished "git config --remove-section branch.master" &&
-	test_when_finished "git config --remove-section merge" &&
-	test_might_fail git config --remove-section branch.master &&
-	test_might_fail git config --remove-section merge &&
-
 	git reset --hard c1 &&
 	git merge c2 &&
 	git show -s --pretty=tformat:%s%n%b >expect &&
 
-	git config branch.master.mergeoptions "--no-log" &&
-	git config merge.log true &&
+	test_config branch.master.mergeoptions "--no-log" &&
+	test_config merge.log "true" &&
 	git reset --hard c1 &&
 	git merge c2 &&
 	git show -s --pretty=tformat:%s%n%b >actual &&
@@ -380,7 +374,7 @@ test_expect_success 'merge c1 with c2 (log in config gets overridden)' '
 
 test_expect_success 'merge c1 with c2 (squash in config)' '
 	git reset --hard c1 &&
-	git config branch.master.mergeoptions "--squash" &&
+	test_config branch.master.mergeoptions "--squash" &&
 	git merge c2 &&
 	verify_merge file result.1-5 &&
 	verify_head $c1 &&
@@ -392,7 +386,7 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'override config option -n with --summary' '
 	git reset --hard c1 &&
-	git config branch.master.mergeoptions "-n" &&
+	test_config branch.master.mergeoptions "-n" &&
 	test_tick &&
 	git merge --summary c2 >diffstat.txt &&
 	verify_merge file result.1-5 msg.1-5 &&
@@ -406,7 +400,7 @@ test_expect_success 'override config option -n with --summary' '
 
 test_expect_success 'override config option -n with --stat' '
 	git reset --hard c1 &&
-	git config branch.master.mergeoptions "-n" &&
+	test_config branch.master.mergeoptions "-n" &&
 	test_tick &&
 	git merge --stat c2 >diffstat.txt &&
 	verify_merge file result.1-5 msg.1-5 &&
@@ -422,7 +416,7 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'override config option --stat' '
 	git reset --hard c1 &&
-	git config branch.master.mergeoptions "--stat" &&
+	test_config branch.master.mergeoptions "--stat" &&
 	test_tick &&
 	git merge -n c2 >diffstat.txt &&
 	verify_merge file result.1-5 msg.1-5 &&
@@ -438,7 +432,7 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c1 with c2 (override --no-commit)' '
 	git reset --hard c1 &&
-	git config branch.master.mergeoptions "--no-commit" &&
+	test_config branch.master.mergeoptions "--no-commit" &&
 	test_tick &&
 	git merge --commit c2 &&
 	verify_merge file result.1-5 msg.1-5 &&
@@ -449,7 +443,7 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c1 with c2 (override --squash)' '
 	git reset --hard c1 &&
-	git config branch.master.mergeoptions "--squash" &&
+	test_config branch.master.mergeoptions "--squash" &&
 	test_tick &&
 	git merge --no-squash c2 &&
 	verify_merge file result.1-5 msg.1-5 &&
@@ -460,7 +454,6 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c0 with c1 (no-ff)' '
 	git reset --hard c0 &&
-	git config branch.master.mergeoptions "" &&
 	test_tick &&
 	git merge --no-ff c1 &&
 	verify_merge file result.1 &&
@@ -471,10 +464,9 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c0 with c1 (merge.ff=false)' '
 	git reset --hard c0 &&
-	git config merge.ff false &&
+	test_config merge.ff "false" &&
 	test_tick &&
 	git merge c1 &&
-	git config --remove-section merge &&
 	verify_merge file result.1 &&
 	verify_parents $c0 $c1
 '
@@ -482,22 +474,19 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'combine branch.master.mergeoptions with merge.ff' '
 	git reset --hard c0 &&
-	git config branch.master.mergeoptions --ff &&
-	git config merge.ff false &&
+	test_config branch.master.mergeoptions "--ff" &&
+	test_config merge.ff "false" &&
 	test_tick &&
 	git merge c1 &&
-	git config --remove-section "branch.master" &&
-	git config --remove-section "merge" &&
 	verify_merge file result.1 &&
 	verify_parents "$c0"
 '
 
 test_expect_success 'tolerate unknown values for merge.ff' '
 	git reset --hard c0 &&
-	git config merge.ff something-new &&
+	test_config merge.ff "something-new" &&
 	test_tick &&
 	git merge c1 2>message &&
-	git config --remove-section "merge" &&
 	verify_head "$c1" &&
 	test_cmp empty message
 '
@@ -515,7 +504,7 @@ test_expect_success 'combining --ff-only and --no-ff is refused' '
 
 test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '
 	git reset --hard c0 &&
-	git config branch.master.mergeoptions "--no-ff" &&
+	test_config branch.master.mergeoptions "--no-ff" &&
 	git merge --ff c1 &&
 	verify_merge file result.1 &&
 	verify_head $c1
@@ -525,14 +514,20 @@ test_expect_success 'merge log message' '
 	git reset --hard c0 &&
 	git merge --no-log c2 &&
 	git show -s --pretty=format:%b HEAD >msg.act &&
-	test_cmp msg.nolog msg.act &&
+	test_cmp msg.nologff msg.act &&
+
+	git reset --hard c0 &&
+	test_config branch.master.mergeoptions "--no-ff" &&
+	git merge --no-log c2 &&
+	git show -s --pretty=format:%b HEAD >msg.act &&
+	test_cmp msg.nolognoff msg.act &&
 
 	git merge --log c3 &&
 	git show -s --pretty=format:%b HEAD >msg.act &&
 	test_cmp msg.log msg.act &&
 
 	git reset --hard HEAD^ &&
-	git config merge.log yes &&
+	test_config merge.log "yes" &&
 	git merge c3 &&
 	git show -s --pretty=format:%b HEAD >msg.act &&
 	test_cmp msg.log msg.act
@@ -542,7 +537,6 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c1 with c0, c2, c0, and c1' '
        git reset --hard c1 &&
-       git config branch.master.mergeoptions "" &&
        test_tick &&
        git merge c0 c2 c0 c1 &&
        verify_merge file result.1-5 &&
@@ -553,7 +547,6 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c1 with c0, c2, c0, and c1' '
        git reset --hard c1 &&
-       git config branch.master.mergeoptions "" &&
        test_tick &&
        git merge c0 c2 c0 c1 &&
        verify_merge file result.1-5 &&
@@ -564,7 +557,6 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c1 with c1 and c2' '
        git reset --hard c1 &&
-       git config branch.master.mergeoptions "" &&
        test_tick &&
        git merge c1 c2 &&
        verify_merge file result.1-5 &&
-- 
1.7.11.7

[PATCH 14/15] t9500: use test_config to set/unset git config variables

[Yann Droneaud]

Instead of using construct such as:
    test_when_finished "git config --unset <key>"
    git config <key> <value>
uses
    test_config <key> <value>
The latter takes care of removing <key> at the end of the test.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t9500-gitweb-standalone-no-errors.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 90bb605..6783c14 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -539,8 +539,7 @@ test_expect_success \
 	 test_when_finished "GIT_COMMITTER_NAME=\"C O Mitter\"" &&
 	 echo "ISO-8859-1" >> file &&
 	 git add file &&
-	 git config i18n.commitencoding ISO-8859-1 &&
-	 test_when_finished "git config --unset i18n.commitencoding" &&
+	 test_config i18n.commitencoding ISO-8859-1 &&
 	 git commit -F "$TEST_DIRECTORY"/t3900/ISO8859-1.txt &&
 	 gitweb_run "p=.git;a=commit"'
 
-- 
1.7.11.7

[PATCH 15/15] t7502: remove clear_config

[Yann Droneaud]

Using test_config ensure the configuration variable are removed
at the end of the test, there's no need to remove variable
at the beginning of the test.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 t/t7502-commit.sh | 32 ++++++--------------------------
 1 file changed, 6 insertions(+), 26 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 619d438..256137f 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -434,16 +434,6 @@ EOF
 
 echo '## Custom template' >template
 
-clear_config () {
-	(
-		git config --unset-all "$1"
-		case $? in
-		0|5)	exit 0 ;;
-		*)	exit 1 ;;
-		esac
-	)
-}
-
 try_commit () {
 	git reset --hard &&
 	echo >>negative &&
@@ -459,67 +449,57 @@ try_commit () {
 try_commit_status_combo () {
 
 	test_expect_success 'commit' '
-		clear_config commit.status &&
 		try_commit "" &&
 		test_i18ngrep "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
 
 	test_expect_success 'commit' '
-		clear_config commit.status &&
 		try_commit "" &&
 		test_i18ngrep "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
 
 	test_expect_success 'commit --status' '
-		clear_config commit.status &&
 		try_commit --status &&
 		test_i18ngrep "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
 
 	test_expect_success 'commit --no-status' '
-		clear_config commit.status &&
 		try_commit --no-status &&
 		test_i18ngrep ! "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
 
 	test_expect_success 'commit with commit.status = yes' '
-		clear_config commit.status &&
-		git config commit.status yes &&
+		test_config commit.status yes &&
 		try_commit "" &&
 		test_i18ngrep "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
 
 	test_expect_success 'commit with commit.status = no' '
-		clear_config commit.status &&
-		git config commit.status no &&
+		test_config commit.status no &&
 		try_commit "" &&
 		test_i18ngrep ! "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
 
 	test_expect_success 'commit --status with commit.status = yes' '
-		clear_config commit.status &&
-		git config commit.status yes &&
+		test_config commit.status yes &&
 		try_commit --status &&
 		test_i18ngrep "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
 
 	test_expect_success 'commit --no-status with commit.status = yes' '
-		clear_config commit.status &&
-		git config commit.status yes &&
+		test_config commit.status yes &&
 		try_commit --no-status &&
 		test_i18ngrep ! "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
 
 	test_expect_success 'commit --status with commit.status = no' '
-		clear_config commit.status &&
-		git config commit.status no &&
+		test_config commit.status no &&
 		try_commit --status &&
 		test_i18ngrep "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
 
 	test_expect_success 'commit --no-status with commit.status = no' '
-		clear_config commit.status &&
-		git config commit.status no &&
+		test_config commit.status no &&
 		try_commit --no-status &&
 		test_i18ngrep ! "^# Changes to be committed:" .git/COMMIT_EDITMSG
 	'
-- 
1.7.11.7

Re: [PATCH 15/15] t7502: remove clear_config

[Junio C Hamano]

Yann Droneaud <ydroneaud@opteya.com> writes:

> Using test_config ensure the configuration variable are removed
> at the end of the test, there's no need to remove variable
> at the beginning of the test.
>
> Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>

This is a good change in the longer term, but there must not be any
other topic in-flight that adds new tests that modify configuration
in a persistent way, which your previous patch based on 'master'
wouldn't have addressed, for this to be a safe change (I assume that
you already have checked that).

Otherwise these apparently redundant unsets need to be left as "belt
and suspenders" safety.  The same for the change to 4202 in [PATCH
07/15].

Thanks.

Re: [PATCH 00/15] Use test_config

[Junio C Hamano]

Yann Droneaud <ydroneaud@opteya.com> writes:

> Tested against master, 7b592fadf1e23b10b913e0771b9f711770597266

Is this because I suggested you to clean things up while you were
touching in a vicinity of something that could use this clean-up?

If so, please first clean _that_ script in a patch, and then add the
change you wanted to do in another patch, as a single two-patch
series, without touching anything else that is not related to that
change.  The patch to t7600 is the one that needs to become two
patches, one to clean up and the other to add tests for --no-ff.

The rest, as a separate "only cleaning up, doing nothing else"
series, are fine as a follow-up, but please make sure that they do
not touch anything in-flight (one easy way to check is to see "git
diff --name-only maint pu -- t/").  I would prefer to see "clean-up
only" changes that introduce unnecessary conflicts with other real
features and fixes held off until the dust settles.

> Yann Droneaud (15):
>   t4018: remove test_config implementation
>   t7810: remove test_config implementation
>   t7811: remove test_config implementation

We already have equivalents of these in-flight.

>   t7600: use test_config to set/unset git config variables

Needs to be split into two.

Re: [PATCH 00/15] Use test_config

[Yann Droneaud]

Hi,

Le 27.03.2013 16:05, Junio C Hamano a écrit :
> Yann Droneaud <ydroneaud@opteya.com> writes:
>
>> Tested against master, 7b592fadf1e23b10b913e0771b9f711770597266
>
> Is this because I suggested you to clean things up while you were
> touching in a vicinity of something that could use this clean-up?
>

Yes, grep'ing shows others usage of the test_config pattern. I patched 
them all.

> If so, please first clean _that_ script in a patch, and then add the
> change you wanted to do in another patch, as a single two-patch
> series, without touching anything else that is not related to that
> change.  The patch to t7600 is the one that needs to become two
> patches, one to clean up and the other to add tests for --no-ff.
>

Actually the initial patch adding test for --no-ff-only is not part of 
this series.


Patch against t7600 has a special note about a strange behavor found 
while testing
test_config "anyware", that's why there's somes line added to the test 
and a note
in the commit message.

I was waiting for your opinion on this change in the test, but more, on 
the difference
of behavior exhibited in the patched test "merge log message":

   git merge --no-log
   git show -s --pretty=format:%b HEAD

vs

   git merge --no-ff --no-log
   git show -s --pretty=format:%b HEAD


First produce an empty file, while the second produce an empty line.

This was revealed by changing test "merge c0 with c1 (ff overrides 
no-ff)
-    git config branch.master.mergeoptions "--no-ff" &&
-    test_config branch.master.mergeoptions "--no-ff" &&


I could split this patch in a first patch that add the behavor test to 
"merge log message" test,
than I could rebase the patch series against.
And later, submit my proposition for new tests in t7600 regarding 
--no-ff-only and tags.

> The rest, as a separate "only cleaning up, doing nothing else"
> series, are fine as a follow-up, but please make sure that they do
> not touch anything in-flight (one easy way to check is to see "git
> diff --name-only maint pu -- t/").  I would prefer to see "clean-up
> only" changes that introduce unnecessary conflicts with other real
> features and fixes held off until the dust settles.
>

It's a good advice that fit perfectly in 
Documentation/SubmittingPatches.

Regards.

-- 
Yann Droneaud
OPTEYA

Re: git merge <tag> behavior

[Junio C Hamano]

Yann Droneaud <ydroneaud@opteya.com> writes:

> Thanks. I wasn't aware of the --no-ff-only option and
> thought --no-ff would be the opposite of --ff-only,
> or at least disable it given the order of the options.
>
> Please find a patch to document option --no-ff-only
>
>   Documentation/merge-options.txt | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/merge-options.txt
> b/Documentation/merge-options.txt
> index 0bcbe0a..20a31cf 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -37,6 +37,10 @@ set to `no` at the beginning of them.
>  	current `HEAD` is already up-to-date or the merge can be
>  	resolved as a fast-forward.
>
> +--no-ff-only::
> +	Disable `--ff-only` behavior, eg. allows creation of merge commit.
> +	This is the default behavior.
> +

We should follow the usual

	--option::
        --no-option::
        	description for both

convention for this one, before or after fixing the existing --ff/--no-ff
description.

>   --log[=<n>]::
>   --no-log::
>  	In addition to branch names, populate the log message with

[PATCH] t7600: merge tag shoud create a merge commit

[y]

From: Yann Droneaud <ydroneaud@opteya.com>

This test ensures a merge commit is always created
when merging an annotated (signed) tag without --ff-only option.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---

Here's a proposition for a test tath check the creation of a merge commit
when merging a tag.

It's not in final shape: the line 

    EDITOR=false test_must_fail git merge signed

should failed, but doesn't: the commit merge is created with
the default message, just like --no-edit was given.

I'm making a mistake somewhere, since the EDITOR=false trick
works, it's even used in the next test, for --no-edit testing.

Regards.

 t/t7600-merge.sh | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index b524bdb..486f1bf 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -697,10 +697,30 @@ test_expect_success 'merge --no-ff --edit' '
 	test_cmp actual expected
 '
 
+test_expect_failure GPG 'merge tag should create a merge commit' '
+	git reset --hard c0 &&
+	git commit --allow-empty -m "A newer commit" &&
+	git tag -f -s -m "A newer commit" signed &&
+	git reset --hard c0 &&
+
+	git merge signed &&
+	git rev-parse signed^0 >expect &&
+	git rev-parse HEAD^0 >actual &&
+	! test_cmp actual expect &&
+
+	git reset --hard c0 &&
+	git commit --allow-empty -m "An other newer commit" &&
+	git tag -f -s -m "An other newer commit" signed &&
+	git reset --hard c0 &&
+
+	EDITOR=false test_must_fail git merge signed &&
+	verify_head "$c0"
+'
+
 test_expect_success GPG 'merge --ff-only tag' '
 	git reset --hard c0 &&
 	git commit --allow-empty -m "A newer commit" &&
-	git tag -s -m "A newer commit" signed &&
+	git tag -f -s -m "A newer commit" signed &&
 	git reset --hard c0 &&
 
 	git merge --ff-only signed &&
-- 
1.7.11.7

Re: [PATCH] t7600: merge tag shoud create a merge commit

[Junio C Hamano]

y@quest-ce.net writes:

> From: Yann Droneaud <ydroneaud@opteya.com>
>
> This test ensures a merge commit is always created
> when merging an annotated (signed) tag without --ff-only option.
>
> Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
> ---
>
> Here's a proposition for a test tath check the creation of a merge commit
> when merging a tag.
>
> It's not in final shape: the line 
>
>     EDITOR=false test_must_fail git merge signed

Because test_must_fail is a shell function, single-shot environment
assignment like this should not be used.

Re: [PATCH] t7600: merge tag shoud create a merge commit

[Yann Droneaud]

Le 22.03.2013 15:48, Junio C Hamano a écrit :
>>
>> It's not in final shape: the line
>>
>>     EDITOR=false test_must_fail git merge signed
>
> Because test_must_fail is a shell function, single-shot environment
> assignment like this should not be used.

It's used throughout the test. The test 'merge --no-edit tag should 
skip editor' is using it.

Before posting my half useful test, I used "EDITOR=false test_must_fail 
set" in --verbose mode to find if EDITOR was correctly defined passed 
test_must_fail, and it was.

So it's still not clear why it's failing at failing. And it's making me 
angry.

Regards.

-- 
Yann Droneaud
OPTEYA

Re: [PATCH] t7600: merge tag shoud create a merge commit

[Jeff King]

On Fri, Mar 22, 2013 at 03:56:15PM +0100, Yann Droneaud wrote:

> Le 22.03.2013 15:48, Junio C Hamano a écrit :
> >>
> >>It's not in final shape: the line
> >>
> >>    EDITOR=false test_must_fail git merge signed
> >
> >Because test_must_fail is a shell function, single-shot environment
> >assignment like this should not be used.
> 
> It's used throughout the test. The test 'merge --no-edit tag should
> skip editor' is using it.

It's OK to do:

  SINGLE_SHOT=foo some_real_command

and it's OK to do:

  some_fun args

but it's not OK to do:

  SINGLE_SHOT=foo some_function args

Because some POSIX shells do not create a new environment for the
function (and SINGLE_SHOT will persist after the call, polluting the
environment).

> Before posting my half useful test, I used "EDITOR=false
> test_must_fail set" in --verbose mode to find if EDITOR was correctly
> defined passed test_must_fail, and it was.

I do not think there is a shell that does not set it; it is only that
some shells do not _unset_ it.

-Peff

[PATCH] t7600: merge tag shoud create a merge commit

[y]

From: Yann Droneaud <ydroneaud@opteya.com>

This test ensures a merge commit is always created
when merging an annotated (signed) tag without --ff-only option.

Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---

Here's a proposition for a test tath check the creation of a merge commit
when merging a tag.

It's not in final shape: the line 

    EDITOR=false test_must_fail git merge signed

should failed, but doesn't: the commit merge is created with
the default message, just like --no-edit was given.

I'm making a mistake somewhere, since the EDITOR=false trick
works, it's even used in the next test, for --no-edit testing.

Regards.

 t/t7600-merge.sh | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index b524bdb..486f1bf 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -697,10 +697,30 @@ test_expect_success 'merge --no-ff --edit' '
 	test_cmp actual expected
 '
 
+test_expect_failure GPG 'merge tag should create a merge commit' '
+	git reset --hard c0 &&
+	git commit --allow-empty -m "A newer commit" &&
+	git tag -f -s -m "A newer commit" signed &&
+	git reset --hard c0 &&
+
+	git merge signed &&
+	git rev-parse signed^0 >expect &&
+	git rev-parse HEAD^0 >actual &&
+	! test_cmp actual expect &&
+
+	git reset --hard c0 &&
+	git commit --allow-empty -m "An other newer commit" &&
+	git tag -f -s -m "An other newer commit" signed &&
+	git reset --hard c0 &&
+
+	EDITOR=false test_must_fail git merge signed &&
+	verify_head "$c0"
+'
+
 test_expect_success GPG 'merge --ff-only tag' '
 	git reset --hard c0 &&
 	git commit --allow-empty -m "A newer commit" &&
-	git tag -s -m "A newer commit" signed &&
+	git tag -f -s -m "A newer commit" signed &&
 	git reset --hard c0 &&
 
 	git merge --ff-only signed &&
-- 
1.7.11.7