threads / patch / 47415

patch, 3 partst: add tests for pull --verify-signatures

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

## tl;dr

15 messages between Dec 9, 2017 and Dec 19, 2017. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Hans Jerry Illikainen· Dec 9, 2017, 09:05 UTC · lore

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

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(-)
Show changes to 3 files +50 −2

Documentation/merge-config.txt, builtin/merge.c, t/t7612-merge-verify-signatures.sh

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
Hans Jerry Illikainen· Dec 9, 2017, 09:05 UTC · re: Hans Jerry Illikainen · lore

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
Show changes to t/t5573-pull-verify-signatures.sh +78 −0
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
Kevin Daudt· Dec 9, 2017, 12:06 UTC · re: Hans Jerry Illikainen · lore

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

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.

Show 94 quoted lines
> 
> 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
>
Hans Jerry Illikainen· Dec 9, 2017, 09:05 UTC · re: Hans Jerry Illikainen · lore

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

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(+)
Show changes to 4 files +83 −0

Documentation/config.txt, builtin/pull.c, t/t5520-pull.sh, t/t5573-pull-verify-signatures.sh

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
Kevin Daudt· Dec 9, 2017, 12:06 UTC · re: Hans Jerry Illikainen · lore

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

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

Show 169 quoted lines
> 
> 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
>
Hans Jerry Illikainen· Dec 10, 2017, 06:48 UTC · re: Kevin Daudt · lore

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

On Sat, Dec 09, 2017 at 01:06:23PM +0100, Kevin Daudt wrote:
Show 14 quoted lines
> 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
Kevin Daudt· Dec 9, 2017, 12:05 UTC · re: Hans Jerry Illikainen · lore

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

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.
Show 21 quoted lines
> ---
>  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?

Show 57 quoted lines
>  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.

Show 28 quoted lines
> +'
> +
> +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.
Junio C Hamano· Dec 12, 2017, 18:56 UTC · re: Kevin Daudt · lore

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

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.
Hans Jerry Illikainen· Dec 10, 2017, 06:53 UTC · re: Hans Jerry Illikainen · lore

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

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(+)
Show changes to 3 files +45 −0

Documentation/merge-config.txt, builtin/merge.c, t/t7612-merge-verify-signatures.sh

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
Hans Jerry Illikainen· Dec 10, 2017, 06:53 UTC · re: Hans Jerry Illikainen · lore

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

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
Show changes to t/t5573-pull-verify-signatures.sh +78 −0
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
Junio C Hamano· Dec 12, 2017, 19:03 UTC · re: Hans Jerry Illikainen · lore

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

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:

Show 6 quoted lines
> +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.
Junio C Hamano· Dec 15, 2017, 19:48 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> 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(-)
Show changes to t/t5573-pull-verify-signatures.sh +6 −3
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
Hans Jerry Illikainen· Dec 16, 2017, 09:34 UTC · re: Junio C Hamano · lore

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

On Fri, Dec 15, 2017 at 11:48:29AM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> 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
Junio C Hamano· Dec 17, 2017, 06:18 UTC · re: Hans Jerry Illikainen · lore

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

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.
Junio C Hamano· Dec 19, 2017, 21:01 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +17 −8

t/t5573-pull-verify-signatures.sh, t/t7612-merge-verify-signatures.sh

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

← back to recent threads