[PATCH 1/3] merge: add config option for verifySignatures

[PATCH 1/3] merge: add config option for verifySignatures

[Hans Jerry Illikainen]

Verify the signature of the tip commit when `merge.verifySignatures` is
true.  This can be overridden with `--no-verify-signatures`.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 Documentation/merge-config.txt     |  7 +++++++
 builtin/merge.c                    |  2 ++
 t/t7612-merge-verify-signatures.sh | 43 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index df3ea3779..76571cd3b 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -26,6 +26,13 @@ merge.ff::
 	allowed (equivalent to giving the `--ff-only` option from the
 	command line).
 
+merge.verifySignatures::
+	Verify that the tip commit of the side branch being merged is
+	signed with a valid key, i.e. a key that has a valid uid: in the
+	default trust model, this means the signing key has been signed
+	by a trusted key. If the tip commit of the side branch is not
+	signed with a valid key, the merge is aborted.
+
 include::fmt-merge-msg-config.txt[]
 
 merge.renameLimit::
diff --git a/builtin/merge.c b/builtin/merge.c
index 612dd7bfb..30264cfd7 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -567,6 +567,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
 		show_diffstat = git_config_bool(k, v);
+	else if (!strcmp(k, "merge.verifysignatures"))
+		verify_signatures = git_config_bool(k, v);
 	else if (!strcmp(k, "pull.twohead"))
 		return git_config_string(&pull_twohead, k, v);
 	else if (!strcmp(k, "pull.octopus"))
diff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh
index 8ae69a61c..f1a74a683 100755
--- a/t/t7612-merge-verify-signatures.sh
+++ b/t/t7612-merge-verify-signatures.sh
@@ -39,23 +39,62 @@ test_expect_success GPG 'merge unsigned commit with verification' '
 	test_i18ngrep "does not have a GPG signature" mergeerror
 '
 
+test_expect_success GPG 'merge unsigned commit with merge.verifySignatures=true' '
+	test_config merge.verifySignatures true &&
+	test_must_fail git merge --ff-only side-unsigned 2>mergeerror &&
+	test_i18ngrep "does not have a GPG signature" mergeerror
+'
+
 test_expect_success GPG 'merge commit with bad signature with verification' '
 	test_must_fail git merge --ff-only --verify-signatures $(cat forged.commit) 2>mergeerror &&
 	test_i18ngrep "has a bad GPG signature" mergeerror
 '
 
+test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=true' '
+	test_config merge.verifySignatures true &&
+	test_must_fail git merge --ff-only $(cat forged.commit) 2>mergeerror &&
+	test_i18ngrep "has a bad GPG signature" mergeerror
+'
+
 test_expect_success GPG 'merge commit with untrusted signature with verification' '
 	test_must_fail git merge --ff-only --verify-signatures side-untrusted 2>mergeerror &&
 	test_i18ngrep "has an untrusted GPG signature" mergeerror
 '
 
+test_expect_success GPG 'merge commit with untrusted signature with merge.verifySignatures=true' '
+	test_config merge.verifySignatures true &&
+	test_must_fail git merge --ff-only side-untrusted 2>mergeerror &&
+	test_i18ngrep "has an untrusted GPG signature" mergeerror
+'
+
 test_expect_success GPG 'merge signed commit with verification' '
 	git merge --verbose --ff-only --verify-signatures side-signed >mergeoutput &&
-	test_i18ngrep "has a good GPG signature" mergeoutput
+	test_i18ngrep "has a good GPG signature" mergeoutput &&
+	git checkout initial
+'
+
+test_expect_success GPG 'merge signed commit with merge.verifySignatures=true' '
+	test_config merge.verifySignatures true &&
+	git merge --verbose --ff-only side-signed >mergeoutput &&
+	test_i18ngrep "has a good GPG signature" mergeoutput &&
+	git checkout initial
 '
 
 test_expect_success GPG 'merge commit with bad signature without verification' '
-	git merge $(cat forged.commit)
+	git merge $(cat forged.commit) &&
+	git checkout initial
+'
+
+test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=false' '
+	test_config merge.verifySignatures false &&
+	git merge $(cat forged.commit) &&
+	git checkout initial
+'
+
+test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=true and --no-verify-signatures' '
+	test_config merge.verifySignatures true &&
+	git merge --no-verify-signatures $(cat forged.commit) &&
+	git checkout initial
 '
 
 test_done
-- 
2.11.0

[PATCH 2/3] t: add tests for pull --verify-signatures

[Hans Jerry Illikainen]

Add tests for `pull --verify-signatures` with untrusted, bad and no
signatures.  Previously the only test for `--verify-signatures` was to
make sure that `pull --rebase --verify-signatures` result in a warning
(t5520-pull.sh).

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 t/t5573-pull-verify-signatures.sh | 78 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100755 t/t5573-pull-verify-signatures.sh

diff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh
new file mode 100755
index 000000000..700247910
--- /dev/null
+++ b/t/t5573-pull-verify-signatures.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='pull signature verification tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
+
+test_expect_success GPG 'create repositories with signed commits' '
+	echo 1 >a && git add a &&
+	test_tick && git commit -m initial &&
+	git tag initial &&
+
+	git clone . signed &&
+	(
+		cd signed &&
+		echo 2 >b && git add b &&
+		test_tick && git commit -S -m "signed"
+	) &&
+
+	git clone . unsigned &&
+	(
+		cd unsigned &&
+		echo 3 >c && git add c &&
+		test_tick && git commit -m "unsigned"
+	) &&
+
+	git clone . bad &&
+	(
+		cd bad &&
+		echo 4 >d && git add d &&
+		test_tick && git commit -S -m "bad" &&
+		git cat-file commit HEAD >raw &&
+		sed -e "s/bad/forged bad/" raw >forged &&
+		git hash-object -w -t commit forged >forged.commit &&
+		git checkout $(cat forged.commit)
+	) &&
+
+	git clone . untrusted &&
+	(
+		cd untrusted &&
+		echo 5 >e && git add e &&
+		test_tick && git commit -SB7227189 -m "untrusted"
+	)
+'
+
+test_expect_success GPG 'pull unsigned commit with --verify-signatures' '
+	test_must_fail git pull --ff-only --verify-signatures unsigned 2>pullerror &&
+	test_i18ngrep "does not have a GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit with bad signature with --verify-signatures' '
+	test_must_fail git pull --ff-only --verify-signatures bad 2>pullerror &&
+	test_i18ngrep "has a bad GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit with untrusted signature with --verify-signatures' '
+	test_must_fail git pull --ff-only --verify-signatures untrusted 2>pullerror &&
+	test_i18ngrep "has an untrusted GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull signed commit with --verify-signatures' '
+	git pull --verify-signatures signed >pulloutput &&
+	test_i18ngrep "has a good GPG signature" pulloutput &&
+	git checkout initial
+'
+
+test_expect_success GPG 'pull commit with bad signature without verification' '
+	git pull --ff-only bad 2>pullerror &&
+	git checkout initial
+'
+
+test_expect_success GPG 'pull commit with bad signature with --no-verify-signatures' '
+	test_config merge.verifySignatures true &&
+	test_config pull.verifySignatures true &&
+	git pull --ff-only --no-verify-signatures bad 2>pullerror &&
+	git checkout initial
+'
+
+test_done
-- 
2.11.0

[PATCH 3/3] pull: add config option for verifySignatures

[Hans Jerry Illikainen]

Verify the signature of the tip commit when `pull.verifySignatures` is
true.  This option overrides `merge.verifySignatures` on pull, and can
be disabled with the option `--no-verify-signatures`.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 Documentation/config.txt          |  8 ++++++++
 builtin/pull.c                    | 25 +++++++++++++++++++++++++
 t/t5520-pull.sh                   | 18 ++++++++++++++++++
 t/t5573-pull-verify-signatures.sh | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c1598ee70..0cd2bc597 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2596,6 +2596,14 @@ pull.ff::
 	allowed (equivalent to giving the `--ff-only` option from the
 	command line). This setting overrides `merge.ff` when pulling.
 
+pull.verifySignatures::
+	Verify that the tip commit of the side branch being merged is
+	signed with a valid key, i.e. a key that has a valid uid: in the
+	default trust model, this means the signing key has been signed
+	by a trusted key. If the tip commit of the side branch is not
+	signed with a valid key, the merge is aborted. This setting
+	overrides `merge.verifySignatures` when pulling.
+
 pull.rebase::
 	When true, rebase branches on top of the fetched branch, instead
 	of merging the default branch from the default remote when "git
diff --git a/builtin/pull.c b/builtin/pull.c
index 166b777ed..791365915 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -300,6 +300,28 @@ static const char *config_get_ff(void)
 }
 
 /**
+ * If pull.verifySignatures is unset, returns NULL. If pull.verifySignatures is
+ * "true", returns "--verify-signatures". If pull.verifySignatures is "false",
+ * returns "--no-verify-signatures". Otherwise, die with an error.
+ */
+static const char *config_get_verify_signatures(void)
+{
+	const char *value;
+
+	if (git_config_get_value("pull.verifysignatures", &value))
+		return NULL;
+
+	switch (git_parse_maybe_bool(value)) {
+	case 0:
+		return "--no-verify-signatures";
+	case 1:
+		return "--verify-signatures";
+	default:
+		die(_("Invalid value for pull.verifysignatures: %s"), value);
+	}
+}
+
+/**
  * Returns the default configured value for --rebase. It first looks for the
  * value of "branch.$curr_branch.rebase", where $curr_branch is the current
  * branch, and if HEAD is detached or the configuration key does not exist,
@@ -849,6 +871,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (!opt_ff)
 		opt_ff = xstrdup_or_null(config_get_ff());
 
+	if (!opt_verify_signatures)
+		opt_verify_signatures = xstrdup_or_null(config_get_verify_signatures());
+
 	if (opt_rebase < 0)
 		opt_rebase = config_get_rebase();
 
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 59c4b778d..cdf1fd213 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -416,6 +416,15 @@ test_expect_success "pull --rebase warns on --verify-signatures" '
 	test_i18ngrep "ignoring --verify-signatures for rebase" err
 '
 
+test_expect_success "pull --rebase warns on pull.verifySignatures=true" '
+	test_config pull.verifySignatures true &&
+	git reset --hard before-rebase &&
+	git pull --rebase . copy 2>err &&
+	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	test new = "$(git show HEAD:file2)" &&
+	test_i18ngrep "ignoring --verify-signatures for rebase" err
+'
+
 test_expect_success "pull --rebase does not warn on --no-verify-signatures" '
 	git reset --hard before-rebase &&
 	git pull --rebase --no-verify-signatures . copy 2>err &&
@@ -424,6 +433,15 @@ test_expect_success "pull --rebase does not warn on --no-verify-signatures" '
 	test_i18ngrep ! "verify-signatures" err
 '
 
+test_expect_success "pull --rebase does not warn on pull.verifySignatures=false" '
+	test_config pull.verifySignatures false &&
+	git reset --hard before-rebase &&
+	git pull --rebase . copy 2>err &&
+	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	test new = "$(git show HEAD:file2)" &&
+	test_i18ngrep ! "verify-signatures" err
+'
+
 # add a feature branch, keep-merge, that is merged into master, so the
 # test can try preserving the merge commit (or not) with various
 # --rebase flags/pull.rebase settings.
diff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh
index 700247910..d1e8263d9 100755
--- a/t/t5573-pull-verify-signatures.sh
+++ b/t/t5573-pull-verify-signatures.sh
@@ -47,22 +47,54 @@ test_expect_success GPG 'pull unsigned commit with --verify-signatures' '
 	test_i18ngrep "does not have a GPG signature" pullerror
 '
 
+test_expect_success GPG 'pull unsigned commit with pull.verifySignatures=true' '
+	test_config pull.verifySignatures true &&
+	test_must_fail git pull --ff-only unsigned 2>pullerror &&
+	test_i18ngrep "does not have a GPG signature" pullerror
+'
+
 test_expect_success GPG 'pull commit with bad signature with --verify-signatures' '
 	test_must_fail git pull --ff-only --verify-signatures bad 2>pullerror &&
 	test_i18ngrep "has a bad GPG signature" pullerror
 '
 
+test_expect_success GPG 'pull commit with bad signature with pull.verifySignatures=true' '
+	test_config pull.verifySignatures true &&
+	test_must_fail git pull --ff-only bad 2>pullerror &&
+	test_i18ngrep "has a bad GPG signature" pullerror
+'
+
 test_expect_success GPG 'pull commit with untrusted signature with --verify-signatures' '
 	test_must_fail git pull --ff-only --verify-signatures untrusted 2>pullerror &&
 	test_i18ngrep "has an untrusted GPG signature" pullerror
 '
 
+test_expect_success GPG 'pull commit with untrusted signature with pull.verifySignatures=true' '
+	test_config pull.verifySignatures true &&
+	test_must_fail git pull --ff-only untrusted 2>pullerror &&
+	test_i18ngrep "has an untrusted GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit with untrusted signature with pull.verifySignatures=true and merge.verifySignatures=false' '
+	test_config merge.verifySignatures false &&
+	test_config pull.verifySignatures true &&
+	test_must_fail git pull --ff-only untrusted 2>pullerror &&
+	test_i18ngrep "has an untrusted GPG signature" pullerror
+'
+
 test_expect_success GPG 'pull signed commit with --verify-signatures' '
 	git pull --verify-signatures signed >pulloutput &&
 	test_i18ngrep "has a good GPG signature" pulloutput &&
 	git checkout initial
 '
 
+test_expect_success GPG 'pull signed commit with pull.verifySignatures=true' '
+	test_config pull.verifySignatures true &&
+	git pull signed >pulloutput &&
+	test_i18ngrep "has a good GPG signature" pulloutput &&
+	git checkout initial
+'
+
 test_expect_success GPG 'pull commit with bad signature without verification' '
 	git pull --ff-only bad 2>pullerror &&
 	git checkout initial
-- 
2.11.0

Re: [PATCH 1/3] merge: add config option for verifySignatures

[Kevin Daudt]

Hello Hans Jerry,

Thank you for your contribution. I have soem remarks below.

On Sat, Dec 09, 2017 at 09:05:28AM +0000, Hans Jerry Illikainen wrote:
> Verify the signature of the tip commit when `merge.verifySignatures` is
> true.  This can be overridden with `--no-verify-signatures`.
> 
> Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>

I miss the motivation in the commit message. I imagine something like:

    git merge --verify-signatures can be used to verify that the tip
    commit of the branch being merged in is properly signed, but it's
    cumbersome to have to specify that every time.
    
    Add a configuration option that enables this behaviour by default,
    which can be overridden by --no-verify-signatures.


> ---
>  Documentation/merge-config.txt     |  7 +++++++
>  builtin/merge.c                    |  2 ++
>  t/t7612-merge-verify-signatures.sh | 43 ++++++++++++++++++++++++++++++++++++--
>  3 files changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
> index df3ea3779..76571cd3b 100644
> --- a/Documentation/merge-config.txt
> +++ b/Documentation/merge-config.txt
> @@ -26,6 +26,13 @@ merge.ff::
>  	allowed (equivalent to giving the `--ff-only` option from the
>  	command line).
>  
> +merge.verifySignatures::
> +	Verify that the tip commit of the side branch being merged is
> +	signed with a valid key, i.e. a key that has a valid uid: in the
> +	default trust model, this means the signing key has been signed
> +	by a trusted key. If the tip commit of the side branch is not
> +	signed with a valid key, the merge is aborted.
> +

This is a verbatim copy of the explenation of --verify-signatures. Would
it be an idea to refer to the explenation of merge --verify-signatures?

>  include::fmt-merge-msg-config.txt[]
>  
>  merge.renameLimit::
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 612dd7bfb..30264cfd7 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -567,6 +567,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  
>  	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
>  		show_diffstat = git_config_bool(k, v);
> +	else if (!strcmp(k, "merge.verifysignatures"))
> +		verify_signatures = git_config_bool(k, v);
>  	else if (!strcmp(k, "pull.twohead"))
>  		return git_config_string(&pull_twohead, k, v);
>  	else if (!strcmp(k, "pull.octopus"))
> diff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh
> index 8ae69a61c..f1a74a683 100755
> --- a/t/t7612-merge-verify-signatures.sh
> +++ b/t/t7612-merge-verify-signatures.sh
> @@ -39,23 +39,62 @@ test_expect_success GPG 'merge unsigned commit with verification' '
>  	test_i18ngrep "does not have a GPG signature" mergeerror
>  '
>  
> +test_expect_success GPG 'merge unsigned commit with merge.verifySignatures=true' '
> +	test_config merge.verifySignatures true &&
> +	test_must_fail git merge --ff-only side-unsigned 2>mergeerror &&
> +	test_i18ngrep "does not have a GPG signature" mergeerror
> +'
> +
>  test_expect_success GPG 'merge commit with bad signature with verification' '
>  	test_must_fail git merge --ff-only --verify-signatures $(cat forged.commit) 2>mergeerror &&
>  	test_i18ngrep "has a bad GPG signature" mergeerror
>  '
>  
> +test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=true' '
> +	test_config merge.verifySignatures true &&
> +	test_must_fail git merge --ff-only $(cat forged.commit) 2>mergeerror &&
> +	test_i18ngrep "has a bad GPG signature" mergeerror
> +'
> +
>  test_expect_success GPG 'merge commit with untrusted signature with verification' '
>  	test_must_fail git merge --ff-only --verify-signatures side-untrusted 2>mergeerror &&
>  	test_i18ngrep "has an untrusted GPG signature" mergeerror
>  '
>  
> +test_expect_success GPG 'merge commit with untrusted signature with merge.verifySignatures=true' '
> +	test_config merge.verifySignatures true &&
> +	test_must_fail git merge --ff-only side-untrusted 2>mergeerror &&
> +	test_i18ngrep "has an untrusted GPG signature" mergeerror
> +'
> +
>  test_expect_success GPG 'merge signed commit with verification' '
>  	git merge --verbose --ff-only --verify-signatures side-signed >mergeoutput &&
> -	test_i18ngrep "has a good GPG signature" mergeoutput
> +	test_i18ngrep "has a good GPG signature" mergeoutput &&
> +	git checkout initial

This looks like a clean up step. If so, it's better to add
`test_when_finished 'git checkout initial'` at the beginning to clearly
mark it as a clean up step and make sure it's run even when the test
fails. Same counts for the other occurances.

> +'
> +
> +test_expect_success GPG 'merge signed commit with merge.verifySignatures=true' '
> +	test_config merge.verifySignatures true &&
> +	git merge --verbose --ff-only side-signed >mergeoutput &&
> +	test_i18ngrep "has a good GPG signature" mergeoutput &&
> +	git checkout initial
>  '
>  
>  test_expect_success GPG 'merge commit with bad signature without verification' '
> -	git merge $(cat forged.commit)
> +	git merge $(cat forged.commit) &&
> +	git checkout initial
> +'
> +
> +test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=false' '
> +	test_config merge.verifySignatures false &&
> +	git merge $(cat forged.commit) &&
> +	git checkout initial
> +'
> +
> +test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=true and --no-verify-signatures' '
> +	test_config merge.verifySignatures true &&
> +	git merge --no-verify-signatures $(cat forged.commit) &&
> +	git checkout initial
>  '
>  
>  test_done

Kevin.

Re: [PATCH 2/3] t: add tests for pull --verify-signatures

[Kevin Daudt]

On Sat, Dec 09, 2017 at 09:05:29AM +0000, Hans Jerry Illikainen wrote:
> Add tests for `pull --verify-signatures` with untrusted, bad and no
> signatures.  Previously the only test for `--verify-signatures` was to
> make sure that `pull --rebase --verify-signatures` result in a warning
> (t5520-pull.sh).

Nice!

Same thing regarding the `git checkout initial` commands counts
here too.

> 
> Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
> ---
>  t/t5573-pull-verify-signatures.sh | 78 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100755 t/t5573-pull-verify-signatures.sh
> 
> diff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh
> new file mode 100755
> index 000000000..700247910
> --- /dev/null
> +++ b/t/t5573-pull-verify-signatures.sh
> @@ -0,0 +1,78 @@
> +#!/bin/sh
> +
> +test_description='pull signature verification tests'
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-gpg.sh"
> +
> +test_expect_success GPG 'create repositories with signed commits' '
> +	echo 1 >a && git add a &&
> +	test_tick && git commit -m initial &&
> +	git tag initial &&
> +
> +	git clone . signed &&
> +	(
> +		cd signed &&
> +		echo 2 >b && git add b &&
> +		test_tick && git commit -S -m "signed"
> +	) &&
> +
> +	git clone . unsigned &&
> +	(
> +		cd unsigned &&
> +		echo 3 >c && git add c &&
> +		test_tick && git commit -m "unsigned"
> +	) &&
> +
> +	git clone . bad &&
> +	(
> +		cd bad &&
> +		echo 4 >d && git add d &&
> +		test_tick && git commit -S -m "bad" &&
> +		git cat-file commit HEAD >raw &&
> +		sed -e "s/bad/forged bad/" raw >forged &&
> +		git hash-object -w -t commit forged >forged.commit &&
> +		git checkout $(cat forged.commit)
> +	) &&
> +
> +	git clone . untrusted &&
> +	(
> +		cd untrusted &&
> +		echo 5 >e && git add e &&
> +		test_tick && git commit -SB7227189 -m "untrusted"
> +	)
> +'
> +
> +test_expect_success GPG 'pull unsigned commit with --verify-signatures' '
> +	test_must_fail git pull --ff-only --verify-signatures unsigned 2>pullerror &&
> +	test_i18ngrep "does not have a GPG signature" pullerror
> +'
> +
> +test_expect_success GPG 'pull commit with bad signature with --verify-signatures' '
> +	test_must_fail git pull --ff-only --verify-signatures bad 2>pullerror &&
> +	test_i18ngrep "has a bad GPG signature" pullerror
> +'
> +
> +test_expect_success GPG 'pull commit with untrusted signature with --verify-signatures' '
> +	test_must_fail git pull --ff-only --verify-signatures untrusted 2>pullerror &&
> +	test_i18ngrep "has an untrusted GPG signature" pullerror
> +'
> +
> +test_expect_success GPG 'pull signed commit with --verify-signatures' '
> +	git pull --verify-signatures signed >pulloutput &&
> +	test_i18ngrep "has a good GPG signature" pulloutput &&
> +	git checkout initial
> +'
> +
> +test_expect_success GPG 'pull commit with bad signature without verification' '
> +	git pull --ff-only bad 2>pullerror &&
> +	git checkout initial
> +'
> +
> +test_expect_success GPG 'pull commit with bad signature with --no-verify-signatures' '
> +	test_config merge.verifySignatures true &&
> +	test_config pull.verifySignatures true &&
> +	git pull --ff-only --no-verify-signatures bad 2>pullerror &&
> +	git checkout initial
> +'
> +
> +test_done
> -- 
> 2.11.0
> 

Re: [PATCH 3/3] pull: add config option for verifySignatures

[Kevin Daudt]

On Sat, Dec 09, 2017 at 09:05:30AM +0000, Hans Jerry Illikainen wrote:
> Verify the signature of the tip commit when `pull.verifySignatures` is
> true.  This option overrides `merge.verifySignatures` on pull, and can
> be disabled with the option `--no-verify-signatures`.

Is there a reason why git pull would need a different behaviour from git
merge? Pull itself is just a convenience command for fetch +
merge/rebase.

One precedent for having a separate configuration option for pull
however is 'pull.ff', so there might be a usecase for it.

I guess your commit message could use a motivation on why you want to
set this differently from 'merge.verifySignature'.

> 
> Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
> ---
>  Documentation/config.txt          |  8 ++++++++
>  builtin/pull.c                    | 25 +++++++++++++++++++++++++
>  t/t5520-pull.sh                   | 18 ++++++++++++++++++
>  t/t5573-pull-verify-signatures.sh | 32 ++++++++++++++++++++++++++++++++
>  4 files changed, 83 insertions(+)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c1598ee70..0cd2bc597 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2596,6 +2596,14 @@ pull.ff::
>  	allowed (equivalent to giving the `--ff-only` option from the
>  	command line). This setting overrides `merge.ff` when pulling.
>  
> +pull.verifySignatures::
> +	Verify that the tip commit of the side branch being merged is
> +	signed with a valid key, i.e. a key that has a valid uid: in the
> +	default trust model, this means the signing key has been signed
> +	by a trusted key. If the tip commit of the side branch is not
> +	signed with a valid key, the merge is aborted. This setting
> +	overrides `merge.verifySignatures` when pulling.
> +
>  pull.rebase::
>  	When true, rebase branches on top of the fetched branch, instead
>  	of merging the default branch from the default remote when "git
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 166b777ed..791365915 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -300,6 +300,28 @@ static const char *config_get_ff(void)
>  }
>  
>  /**
> + * If pull.verifySignatures is unset, returns NULL. If pull.verifySignatures is
> + * "true", returns "--verify-signatures". If pull.verifySignatures is "false",
> + * returns "--no-verify-signatures". Otherwise, die with an error.
> + */
> +static const char *config_get_verify_signatures(void)
> +{
> +	const char *value;
> +
> +	if (git_config_get_value("pull.verifysignatures", &value))
> +		return NULL;
> +
> +	switch (git_parse_maybe_bool(value)) {
> +	case 0:
> +		return "--no-verify-signatures";
> +	case 1:
> +		return "--verify-signatures";
> +	default:
> +		die(_("Invalid value for pull.verifysignatures: %s"), value);
> +	}
> +}
> +
> +/**
>   * Returns the default configured value for --rebase. It first looks for the
>   * value of "branch.$curr_branch.rebase", where $curr_branch is the current
>   * branch, and if HEAD is detached or the configuration key does not exist,
> @@ -849,6 +871,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  	if (!opt_ff)
>  		opt_ff = xstrdup_or_null(config_get_ff());
>  
> +	if (!opt_verify_signatures)
> +		opt_verify_signatures = xstrdup_or_null(config_get_verify_signatures());
> +
>  	if (opt_rebase < 0)
>  		opt_rebase = config_get_rebase();
>  
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 59c4b778d..cdf1fd213 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -416,6 +416,15 @@ test_expect_success "pull --rebase warns on --verify-signatures" '
>  	test_i18ngrep "ignoring --verify-signatures for rebase" err
>  '
>  
> +test_expect_success "pull --rebase warns on pull.verifySignatures=true" '
> +	test_config pull.verifySignatures true &&
> +	git reset --hard before-rebase &&
> +	git pull --rebase . copy 2>err &&
> +	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
> +	test new = "$(git show HEAD:file2)" &&
> +	test_i18ngrep "ignoring --verify-signatures for rebase" err
> +'
> +
>  test_expect_success "pull --rebase does not warn on --no-verify-signatures" '
>  	git reset --hard before-rebase &&
>  	git pull --rebase --no-verify-signatures . copy 2>err &&
> @@ -424,6 +433,15 @@ test_expect_success "pull --rebase does not warn on --no-verify-signatures" '
>  	test_i18ngrep ! "verify-signatures" err
>  '
>  
> +test_expect_success "pull --rebase does not warn on pull.verifySignatures=false" '
> +	test_config pull.verifySignatures false &&
> +	git reset --hard before-rebase &&
> +	git pull --rebase . copy 2>err &&
> +	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
> +	test new = "$(git show HEAD:file2)" &&
> +	test_i18ngrep ! "verify-signatures" err
> +'
> +
>  # add a feature branch, keep-merge, that is merged into master, so the
>  # test can try preserving the merge commit (or not) with various
>  # --rebase flags/pull.rebase settings.
> diff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh
> index 700247910..d1e8263d9 100755
> --- a/t/t5573-pull-verify-signatures.sh
> +++ b/t/t5573-pull-verify-signatures.sh
> @@ -47,22 +47,54 @@ test_expect_success GPG 'pull unsigned commit with --verify-signatures' '
>  	test_i18ngrep "does not have a GPG signature" pullerror
>  '
>  
> +test_expect_success GPG 'pull unsigned commit with pull.verifySignatures=true' '
> +	test_config pull.verifySignatures true &&
> +	test_must_fail git pull --ff-only unsigned 2>pullerror &&
> +	test_i18ngrep "does not have a GPG signature" pullerror
> +'
> +
>  test_expect_success GPG 'pull commit with bad signature with --verify-signatures' '
>  	test_must_fail git pull --ff-only --verify-signatures bad 2>pullerror &&
>  	test_i18ngrep "has a bad GPG signature" pullerror
>  '
>  
> +test_expect_success GPG 'pull commit with bad signature with pull.verifySignatures=true' '
> +	test_config pull.verifySignatures true &&
> +	test_must_fail git pull --ff-only bad 2>pullerror &&
> +	test_i18ngrep "has a bad GPG signature" pullerror
> +'
> +
>  test_expect_success GPG 'pull commit with untrusted signature with --verify-signatures' '
>  	test_must_fail git pull --ff-only --verify-signatures untrusted 2>pullerror &&
>  	test_i18ngrep "has an untrusted GPG signature" pullerror
>  '
>  
> +test_expect_success GPG 'pull commit with untrusted signature with pull.verifySignatures=true' '
> +	test_config pull.verifySignatures true &&
> +	test_must_fail git pull --ff-only untrusted 2>pullerror &&
> +	test_i18ngrep "has an untrusted GPG signature" pullerror
> +'
> +
> +test_expect_success GPG 'pull commit with untrusted signature with pull.verifySignatures=true and merge.verifySignatures=false' '
> +	test_config merge.verifySignatures false &&
> +	test_config pull.verifySignatures true &&
> +	test_must_fail git pull --ff-only untrusted 2>pullerror &&
> +	test_i18ngrep "has an untrusted GPG signature" pullerror
> +'
> +
>  test_expect_success GPG 'pull signed commit with --verify-signatures' '
>  	git pull --verify-signatures signed >pulloutput &&
>  	test_i18ngrep "has a good GPG signature" pulloutput &&
>  	git checkout initial
>  '
>  
> +test_expect_success GPG 'pull signed commit with pull.verifySignatures=true' '
> +	test_config pull.verifySignatures true &&
> +	git pull signed >pulloutput &&
> +	test_i18ngrep "has a good GPG signature" pulloutput &&
> +	git checkout initial
> +'
> +
>  test_expect_success GPG 'pull commit with bad signature without verification' '
>  	git pull --ff-only bad 2>pullerror &&
>  	git checkout initial
> -- 
> 2.11.0
> 

Re: [PATCH 3/3] pull: add config option for verifySignatures

[Hans Jerry Illikainen]

On Sat, Dec 09, 2017 at 01:06:23PM +0100, Kevin Daudt wrote:
> On Sat, Dec 09, 2017 at 09:05:30AM +0000, Hans Jerry Illikainen wrote:
> > Verify the signature of the tip commit when `pull.verifySignatures` is
> > true.  This option overrides `merge.verifySignatures` on pull, and can
> > be disabled with the option `--no-verify-signatures`.
> 
> Is there a reason why git pull would need a different behaviour from git
> merge? Pull itself is just a convenience command for fetch +
> merge/rebase.
> 
> One precedent for having a separate configuration option for pull
> however is 'pull.ff', so there might be a usecase for it.
> 
> I guess your commit message could use a motivation on why you want to
> set this differently from 'merge.verifySignature'.

Thanks for the review!  I wasn't sure whether pull.verifySignatures made
sense -- I included it to be consistent with pull.ff/merge.ff, but it's
scrapped in v2.

-- 
hji

[PATCH v2 1/2] merge: add config option for verifySignatures

[Hans Jerry Illikainen]

git merge --verify-signatures can be used to verify that the tip commit
of the branch being merged in is properly signed, but it's cumbersome to
have to specify that every time.

Add a configuration option that enables this behaviour by default, which
can be overridden by --no-verify-signatures.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 Documentation/merge-config.txt     |  4 ++++
 builtin/merge.c                    |  2 ++
 t/t7612-merge-verify-signatures.sh | 39 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+)

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index df3ea3779..12b6bbf59 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -26,6 +26,10 @@ merge.ff::
 	allowed (equivalent to giving the `--ff-only` option from the
 	command line).
 
+merge.verifySignatures::
+	If true, this is equivalent to the --verify-signatures command
+	line option. See linkgit:git-merge[1] for details.
+
 include::fmt-merge-msg-config.txt[]
 
 merge.renameLimit::
diff --git a/builtin/merge.c b/builtin/merge.c
index 612dd7bfb..30264cfd7 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -567,6 +567,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
 		show_diffstat = git_config_bool(k, v);
+	else if (!strcmp(k, "merge.verifysignatures"))
+		verify_signatures = git_config_bool(k, v);
 	else if (!strcmp(k, "pull.twohead"))
 		return git_config_string(&pull_twohead, k, v);
 	else if (!strcmp(k, "pull.octopus"))
diff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh
index 8ae69a61c..2344995a1 100755
--- a/t/t7612-merge-verify-signatures.sh
+++ b/t/t7612-merge-verify-signatures.sh
@@ -39,23 +39,62 @@ test_expect_success GPG 'merge unsigned commit with verification' '
 	test_i18ngrep "does not have a GPG signature" mergeerror
 '
 
+test_expect_success GPG 'merge unsigned commit with merge.verifySignatures=true' '
+	test_config merge.verifySignatures true &&
+	test_must_fail git merge --ff-only side-unsigned 2>mergeerror &&
+	test_i18ngrep "does not have a GPG signature" mergeerror
+'
+
 test_expect_success GPG 'merge commit with bad signature with verification' '
 	test_must_fail git merge --ff-only --verify-signatures $(cat forged.commit) 2>mergeerror &&
 	test_i18ngrep "has a bad GPG signature" mergeerror
 '
 
+test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=true' '
+	test_config merge.verifySignatures true &&
+	test_must_fail git merge --ff-only $(cat forged.commit) 2>mergeerror &&
+	test_i18ngrep "has a bad GPG signature" mergeerror
+'
+
 test_expect_success GPG 'merge commit with untrusted signature with verification' '
 	test_must_fail git merge --ff-only --verify-signatures side-untrusted 2>mergeerror &&
 	test_i18ngrep "has an untrusted GPG signature" mergeerror
 '
 
+test_expect_success GPG 'merge commit with untrusted signature with merge.verifySignatures=true' '
+	test_config merge.verifySignatures true &&
+	test_must_fail git merge --ff-only side-untrusted 2>mergeerror &&
+	test_i18ngrep "has an untrusted GPG signature" mergeerror
+'
+
 test_expect_success GPG 'merge signed commit with verification' '
+	test_when_finished "git checkout initial" &&
 	git merge --verbose --ff-only --verify-signatures side-signed >mergeoutput &&
 	test_i18ngrep "has a good GPG signature" mergeoutput
 '
 
+test_expect_success GPG 'merge signed commit with merge.verifySignatures=true' '
+	test_when_finished "git checkout initial" &&
+	test_config merge.verifySignatures true &&
+	git merge --verbose --ff-only side-signed >mergeoutput &&
+	test_i18ngrep "has a good GPG signature" mergeoutput
+'
+
 test_expect_success GPG 'merge commit with bad signature without verification' '
+	test_when_finished "git checkout initial" &&
+	git merge $(cat forged.commit)
+'
+
+test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=false' '
+	test_when_finished "git checkout initial" &&
+	test_config merge.verifySignatures false &&
 	git merge $(cat forged.commit)
 '
 
+test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=true and --no-verify-signatures' '
+	test_when_finished "git checkout initial" &&
+	test_config merge.verifySignatures true &&
+	git merge --no-verify-signatures $(cat forged.commit)
+'
+
 test_done
-- 
2.15.1.356.g13e4cf275

[PATCH v2 2/2] t: add tests for pull --verify-signatures

[Hans Jerry Illikainen]

Add tests for pull --verify-signatures with untrusted, bad and no
signatures.  Previously the only test for --verify-signatures was to
make sure that pull --rebase --verify-signatures result in a warning
(t5520-pull.sh).

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 t/t5573-pull-verify-signatures.sh | 78 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100755 t/t5573-pull-verify-signatures.sh

diff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh
new file mode 100755
index 000000000..8ae331f40
--- /dev/null
+++ b/t/t5573-pull-verify-signatures.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='pull signature verification tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
+
+test_expect_success GPG 'create repositories with signed commits' '
+	echo 1 >a && git add a &&
+	test_tick && git commit -m initial &&
+	git tag initial &&
+
+	git clone . signed &&
+	(
+		cd signed &&
+		echo 2 >b && git add b &&
+		test_tick && git commit -S -m "signed"
+	) &&
+
+	git clone . unsigned &&
+	(
+		cd unsigned &&
+		echo 3 >c && git add c &&
+		test_tick && git commit -m "unsigned"
+	) &&
+
+	git clone . bad &&
+	(
+		cd bad &&
+		echo 4 >d && git add d &&
+		test_tick && git commit -S -m "bad" &&
+		git cat-file commit HEAD >raw &&
+		sed -e "s/bad/forged bad/" raw >forged &&
+		git hash-object -w -t commit forged >forged.commit &&
+		git checkout $(cat forged.commit)
+	) &&
+
+	git clone . untrusted &&
+	(
+		cd untrusted &&
+		echo 5 >e && git add e &&
+		test_tick && git commit -SB7227189 -m "untrusted"
+	)
+'
+
+test_expect_success GPG 'pull unsigned commit with --verify-signatures' '
+	test_must_fail git pull --ff-only --verify-signatures unsigned 2>pullerror &&
+	test_i18ngrep "does not have a GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit with bad signature with --verify-signatures' '
+	test_must_fail git pull --ff-only --verify-signatures bad 2>pullerror &&
+	test_i18ngrep "has a bad GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull commit with untrusted signature with --verify-signatures' '
+	test_must_fail git pull --ff-only --verify-signatures untrusted 2>pullerror &&
+	test_i18ngrep "has an untrusted GPG signature" pullerror
+'
+
+test_expect_success GPG 'pull signed commit with --verify-signatures' '
+	test_when_finished "git checkout initial" &&
+	git pull --verify-signatures signed >pulloutput &&
+	test_i18ngrep "has a good GPG signature" pulloutput
+'
+
+test_expect_success GPG 'pull commit with bad signature without verification' '
+	test_when_finished "git checkout initial" &&
+	git pull --ff-only bad 2>pullerror
+'
+
+test_expect_success GPG 'pull commit with bad signature with --no-verify-signatures' '
+	test_when_finished "git checkout initial" &&
+	test_config merge.verifySignatures true &&
+	test_config pull.verifySignatures true &&
+	git pull --ff-only --no-verify-signatures bad 2>pullerror
+'
+
+test_done
-- 
2.15.1.356.g13e4cf275

Re: [PATCH 1/3] merge: add config option for verifySignatures

[Junio C Hamano]

Kevin Daudt <me@ikke.info> writes:

> Hello Hans Jerry,
>
> Thank you for your contribution. I have soem remarks below.
> ...

Thanks for a detailed review.  I agree with all the things you
pointed out, and I see they are reflected in v2 of the series.

Very much appreciated; thanks, both.

Re: [PATCH v2 2/2] t: add tests for pull --verify-signatures

[Junio C Hamano]

Hans Jerry Illikainen <hji@dyntopia.com> writes:

> +test_expect_success GPG 'pull unsigned commit with --verify-signatures' '
> +	test_must_fail git pull --ff-only --verify-signatures unsigned 2>pullerror &&
> +	test_i18ngrep "does not have a GPG signature" pullerror
> +'

Note that this is without "when-finished"; if 'git pull' got broken
and does not fail as expected, the next test will start from a state
that it is not expecting.  Same for the ones that run 'git pull'
under test_must_fail.

Interestingly, the tests that do expect 'git pull' to succeed
protect themselves with "when-finished" mechanism correctly [*1*],
like so:

> +test_expect_success GPG 'pull signed commit with --verify-signatures' '
> +	test_when_finished "git checkout initial" &&
> +	git pull --verify-signatures signed >pulloutput &&
> +	test_i18ngrep "has a good GPG signature" pulloutput
> +'
> +

Other than that, looked nicely done.  Thanks.


[Footnote]

*1* I am guessing that the branches that are being pulled in tests
    are designed in such a way to never produce merge conflicts, and
    failures are possible only due to signature verification.  If
    that were not the case, "when-finished" would want to do a hard
    reset before checking out the initial to go back to a known
    state.

Re* [PATCH v2 2/2] t: add tests for pull --verify-signatures

[Junio C Hamano]

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

> Interestingly, the tests that do expect 'git pull' to succeed
> protect themselves with "when-finished" mechanism correctly [*1*],
> like so:
>
>> +test_expect_success GPG 'pull signed commit with --verify-signatures' '
>> +	test_when_finished "git checkout initial" &&
>> +	git pull --verify-signatures signed >pulloutput &&
>> +	test_i18ngrep "has a good GPG signature" pulloutput
>> +'
>> +
>
> Other than that, looked nicely done.  Thanks.

Here is what I would propose as a follow-up polishing.

-- >8 --
Subject: [PATCH 3/2] t5573: clean up after unexpected success of 'pull', too

The previous step added test_when_finished to tests that run 'git
pull' with expectation of success, so that the test after them can
start from a known state even when their 'git pull' invocation
unexpectedly fails.  However, tests that run 'git pull' expecting
it not to succeed forgot to protect later tests the same way---if
they unexpectedly succeed, the test after them would start from an
unexpected state.

Reset and checkout the initial commit after all these tests, whether
they expect their 'git pull' invocations to succeed or fail.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5573-pull-verify-signatures.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh
index 8ae331f40e..9594e891f4 100755
--- a/t/t5573-pull-verify-signatures.sh
+++ b/t/t5573-pull-verify-signatures.sh
@@ -43,33 +43,36 @@ test_expect_success GPG 'create repositories with signed commits' '
 '
 
 test_expect_success GPG 'pull unsigned commit with --verify-signatures' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_must_fail git pull --ff-only --verify-signatures unsigned 2>pullerror &&
 	test_i18ngrep "does not have a GPG signature" pullerror
 '
 
 test_expect_success GPG 'pull commit with bad signature with --verify-signatures' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_must_fail git pull --ff-only --verify-signatures bad 2>pullerror &&
 	test_i18ngrep "has a bad GPG signature" pullerror
 '
 
 test_expect_success GPG 'pull commit with untrusted signature with --verify-signatures' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_must_fail git pull --ff-only --verify-signatures untrusted 2>pullerror &&
 	test_i18ngrep "has an untrusted GPG signature" pullerror
 '
 
 test_expect_success GPG 'pull signed commit with --verify-signatures' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	git pull --verify-signatures signed >pulloutput &&
 	test_i18ngrep "has a good GPG signature" pulloutput
 '
 
 test_expect_success GPG 'pull commit with bad signature without verification' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	git pull --ff-only bad 2>pullerror
 '
 
 test_expect_success GPG 'pull commit with bad signature with --no-verify-signatures' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_config merge.verifySignatures true &&
 	test_config pull.verifySignatures true &&
 	git pull --ff-only --no-verify-signatures bad 2>pullerror
-- 
2.15.1-558-ged696bbdd8

Re: Re* [PATCH v2 2/2] t: add tests for pull --verify-signatures

[Hans Jerry Illikainen]

On Fri, Dec 15, 2017 at 11:48:29AM -0800, Junio C Hamano wrote:
> Here is what I would propose as a follow-up polishing.
> 
> -- >8 --
> Subject: [PATCH 3/2] t5573: clean up after unexpected success of 'pull', too
> 
> The previous step added test_when_finished to tests that run 'git
> pull' with expectation of success, so that the test after them can
> start from a known state even when their 'git pull' invocation
> unexpectedly fails.  However, tests that run 'git pull' expecting
> it not to succeed forgot to protect later tests the same way---if
> they unexpectedly succeed, the test after them would start from an
> unexpected state.
> 
> Reset and checkout the initial commit after all these tests, whether
> they expect their 'git pull' invocations to succeed or fail.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t5573-pull-verify-signatures.sh | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Thanks!  t7612-merge-verify-signatures.sh also lacks cleanup for
test_must_fail brokenness.  Would you prefer test_when_finished to be
included in the two patches as a v3?

-- 
hji

Re: Re* [PATCH v2 2/2] t: add tests for pull --verify-signatures

[Junio C Hamano]

Hans Jerry Illikainen <hji@dyntopia.com> writes:

> Thanks!  t7612-merge-verify-signatures.sh also lacks cleanup for
> test_must_fail brokenness.  Would you prefer test_when_finished to be
> included in the two patches as a v3?

No, I do not want a v3 as these are already in 'next'.  Just like my
follow-up clean-up you are responding to is [PATCH 3/2], if you want
further work on it, the preferred method from now on is to send in
incremental updates on top of what has already been accepted.


Thanks.

[PATCH v2 3/2] t5573, t7612: clean up after unexpected success of 'pull' and 'merge'

[Junio C Hamano]

The previous steps added test_when_finished to tests that run 'git
pull' or 'git merge' with expectation of success, so that the test
after them can start from a known state even when their 'git pull'
invocation unexpectedly fails.  However, tests that run 'git pull'
or 'git merge' expecting it not to succeed forgot to protect later
tests the same way---if they unexpectedly succeed, the test after
them would start from an unexpected state.

Reset and checkout the initial commit after all these tests, whether
they expect their invocations to succeed or fail.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Let's do this and move the whole thing forward.

 t/t5573-pull-verify-signatures.sh  |  9 ++++++---
 t/t7612-merge-verify-signatures.sh | 16 +++++++++++-----
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh
index 8ae331f40e..9594e891f4 100755
--- a/t/t5573-pull-verify-signatures.sh
+++ b/t/t5573-pull-verify-signatures.sh
@@ -43,33 +43,36 @@ test_expect_success GPG 'create repositories with signed commits' '
 '
 
 test_expect_success GPG 'pull unsigned commit with --verify-signatures' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_must_fail git pull --ff-only --verify-signatures unsigned 2>pullerror &&
 	test_i18ngrep "does not have a GPG signature" pullerror
 '
 
 test_expect_success GPG 'pull commit with bad signature with --verify-signatures' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_must_fail git pull --ff-only --verify-signatures bad 2>pullerror &&
 	test_i18ngrep "has a bad GPG signature" pullerror
 '
 
 test_expect_success GPG 'pull commit with untrusted signature with --verify-signatures' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_must_fail git pull --ff-only --verify-signatures untrusted 2>pullerror &&
 	test_i18ngrep "has an untrusted GPG signature" pullerror
 '
 
 test_expect_success GPG 'pull signed commit with --verify-signatures' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	git pull --verify-signatures signed >pulloutput &&
 	test_i18ngrep "has a good GPG signature" pulloutput
 '
 
 test_expect_success GPG 'pull commit with bad signature without verification' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	git pull --ff-only bad 2>pullerror
 '
 
 test_expect_success GPG 'pull commit with bad signature with --no-verify-signatures' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_config merge.verifySignatures true &&
 	test_config pull.verifySignatures true &&
 	git pull --ff-only --no-verify-signatures bad 2>pullerror
diff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh
index 2344995a11..e797c74112 100755
--- a/t/t7612-merge-verify-signatures.sh
+++ b/t/t7612-merge-verify-signatures.sh
@@ -35,64 +35,70 @@ test_expect_success GPG 'create signed commits' '
 '
 
 test_expect_success GPG 'merge unsigned commit with verification' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_must_fail git merge --ff-only --verify-signatures side-unsigned 2>mergeerror &&
 	test_i18ngrep "does not have a GPG signature" mergeerror
 '
 
 test_expect_success GPG 'merge unsigned commit with merge.verifySignatures=true' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_config merge.verifySignatures true &&
 	test_must_fail git merge --ff-only side-unsigned 2>mergeerror &&
 	test_i18ngrep "does not have a GPG signature" mergeerror
 '
 
 test_expect_success GPG 'merge commit with bad signature with verification' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_must_fail git merge --ff-only --verify-signatures $(cat forged.commit) 2>mergeerror &&
 	test_i18ngrep "has a bad GPG signature" mergeerror
 '
 
 test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=true' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_config merge.verifySignatures true &&
 	test_must_fail git merge --ff-only $(cat forged.commit) 2>mergeerror &&
 	test_i18ngrep "has a bad GPG signature" mergeerror
 '
 
 test_expect_success GPG 'merge commit with untrusted signature with verification' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_must_fail git merge --ff-only --verify-signatures side-untrusted 2>mergeerror &&
 	test_i18ngrep "has an untrusted GPG signature" mergeerror
 '
 
 test_expect_success GPG 'merge commit with untrusted signature with merge.verifySignatures=true' '
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_config merge.verifySignatures true &&
 	test_must_fail git merge --ff-only side-untrusted 2>mergeerror &&
 	test_i18ngrep "has an untrusted GPG signature" mergeerror
 '
 
 test_expect_success GPG 'merge signed commit with verification' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	git merge --verbose --ff-only --verify-signatures side-signed >mergeoutput &&
 	test_i18ngrep "has a good GPG signature" mergeoutput
 '
 
 test_expect_success GPG 'merge signed commit with merge.verifySignatures=true' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_config merge.verifySignatures true &&
 	git merge --verbose --ff-only side-signed >mergeoutput &&
 	test_i18ngrep "has a good GPG signature" mergeoutput
 '
 
 test_expect_success GPG 'merge commit with bad signature without verification' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	git merge $(cat forged.commit)
 '
 
 test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=false' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_config merge.verifySignatures false &&
 	git merge $(cat forged.commit)
 '
 
 test_expect_success GPG 'merge commit with bad signature with merge.verifySignatures=true and --no-verify-signatures' '
-	test_when_finished "git checkout initial" &&
+	test_when_finished "git reset --hard && git checkout initial" &&
 	test_config merge.verifySignatures true &&
 	git merge --no-verify-signatures $(cat forged.commit)
 '
-- 
2.15.1-574-g4ecdd25846