Re: [PATCH v3 2/6] cherry-pick/revert: honour --no-gpg-sign in all case

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Đoàn

On 03/04/2020 11:28, Đoàn Trần Công Danh wrote:
> {cherry-pick,revert} --edit hasn't honoured --no-gpg-sign yet.
> 
> Pass this option down to git-commit to honour it.

I did wonder if try_to_commit() needed any changes as we do not fork 
'git commit' unless the message is being edited but the tests seem to 
cover that case. It might be worth checking the code just to be sure if 
you haven't done so already.

Best Wishes

Phillip

> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>   Documentation/git-cherry-pick.txt |  5 +-
>   Documentation/git-revert.txt      |  5 +-
>   sequencer.c                       |  2 +
>   t/t3514-cherry-pick-revert-gpg.sh | 86 +++++++++++++++++++++++++++++++
>   4 files changed, 96 insertions(+), 2 deletions(-)
>   create mode 100755 t/t3514-cherry-pick-revert-gpg.sh
> 
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index 83ce51aedf..75feeef08a 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -109,9 +109,12 @@ effect to your index in a row.
>   
>   -S[<keyid>]::
>   --gpg-sign[=<keyid>]::
> +--no-gpg-sign::
>   	GPG-sign commits. The `keyid` argument is optional and
>   	defaults to the committer identity; if specified, it must be
> -	stuck to the option without a space.
> +	stuck to the option without a space. `--no-gpg-sign` is useful to
> +	countermand both `commit.gpgSign` configuration variable, and
> +	earlier `--gpg-sign`.
>   
>   --ff::
>   	If the current HEAD is the same as the parent of the
> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> index 9d22270757..044276e9da 100644
> --- a/Documentation/git-revert.txt
> +++ b/Documentation/git-revert.txt
> @@ -90,9 +90,12 @@ effect to your index in a row.
>   
>   -S[<keyid>]::
>   --gpg-sign[=<keyid>]::
> +--no-gpg-sign::
>   	GPG-sign commits. The `keyid` argument is optional and
>   	defaults to the committer identity; if specified, it must be
> -	stuck to the option without a space.
> +	stuck to the option without a space. `--no-gpg-sign` is useful to
> +	countermand both `commit.gpgSign` configuration variable, and
> +	earlier `--gpg-sign`.
>   
>   -s::
>   --signoff::
> diff --git a/sequencer.c b/sequencer.c
> index 6fd2674632..9969355de7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -946,6 +946,8 @@ static int run_git_commit(struct repository *r,
>   		argv_array_push(&cmd.args, "--amend");
>   	if (opts->gpg_sign)
>   		argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
> +	else
> +		argv_array_push(&cmd.args, "--no-gpg-sign");
>   	if (defmsg)
>   		argv_array_pushl(&cmd.args, "-F", defmsg, NULL);
>   	else if (!(flags & EDIT_MSG))
> diff --git a/t/t3514-cherry-pick-revert-gpg.sh b/t/t3514-cherry-pick-revert-gpg.sh
> new file mode 100755
> index 0000000000..5b2e250eaa
> --- /dev/null
> +++ b/t/t3514-cherry-pick-revert-gpg.sh
> @@ -0,0 +1,86 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2020 Doan Tran Cong Danh
> +#
> +
> +test_description='test {cherry-pick,revert} --[no-]gpg-sign'
> +
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-gpg.sh"
> +
> +if ! test_have_prereq GPG
> +then
> +	skip_all='skip all test {cherry-pick,revert} --[no-]gpg-sign, gpg not available'
> +	test_done
> +fi
> +
> +test_gpg_sign () {
> +	local must_fail= will=will fake_editor=
> +	if test "x$1" = "x!"
> +	then
> +		must_fail=test_must_fail
> +		will="won't"
> +		shift
> +	fi
> +	conf=$1
> +	cmd=$2
> +	cmit=$3
> +	shift 3
> +	test_expect_success "$cmd $* $cmit with commit.gpgsign=$conf $will sign commit" "
> +		git reset --hard tip &&
> +		git config commit.gpgsign $conf &&
> +		git $cmd $* $cmit &&
> +		git rev-list tip.. >rev-list &&
> +		$must_fail git verify-commit \$(cat rev-list)
> +	"
> +}
> +
> +test_expect_success 'setup' '
> +	test_commit one &&
> +	git switch -c side &&
> +	test_commit side1 &&
> +	test_commit side2 &&
> +	git switch - &&
> +	test_commit two &&
> +	test_commit three &&
> +	test_commit tip
> +'
> +
> +test_gpg_sign ! false cherry-pick   side
> +test_gpg_sign ! false cherry-pick ..side
> +test_gpg_sign   true  cherry-pick   side
> +test_gpg_sign   true  cherry-pick ..side
> +test_gpg_sign ! true  cherry-pick   side --no-gpg-sign
> +test_gpg_sign ! true  cherry-pick ..side --no-gpg-sign
> +test_gpg_sign ! true  cherry-pick   side --gpg-sign --no-gpg-sign
> +test_gpg_sign ! true  cherry-pick ..side --gpg-sign --no-gpg-sign
> +test_gpg_sign   false cherry-pick   side --no-gpg-sign --gpg-sign
> +test_gpg_sign   false cherry-pick ..side --no-gpg-sign --gpg-sign
> +test_gpg_sign   true  cherry-pick   side --edit
> +test_gpg_sign   true  cherry-pick ..side --edit
> +test_gpg_sign ! true  cherry-pick   side --edit --no-gpg-sign
> +test_gpg_sign ! true  cherry-pick ..side --edit --no-gpg-sign
> +test_gpg_sign ! true  cherry-pick   side --edit --gpg-sign --no-gpg-sign
> +test_gpg_sign ! true  cherry-pick ..side --edit --gpg-sign --no-gpg-sign
> +test_gpg_sign   false cherry-pick   side --edit --no-gpg-sign --gpg-sign
> +test_gpg_sign   false cherry-pick ..side --edit --no-gpg-sign --gpg-sign
> +
> +test_gpg_sign ! false revert HEAD  --edit
> +test_gpg_sign ! false revert two.. --edit
> +test_gpg_sign   true  revert HEAD  --edit
> +test_gpg_sign   true  revert two.. --edit
> +test_gpg_sign ! true  revert HEAD  --edit --no-gpg-sign
> +test_gpg_sign ! true  revert two.. --edit --no-gpg-sign
> +test_gpg_sign ! true  revert HEAD  --edit --gpg-sign --no-gpg-sign
> +test_gpg_sign ! true  revert two.. --edit --gpg-sign --no-gpg-sign
> +test_gpg_sign   false revert HEAD  --edit --no-gpg-sign --gpg-sign
> +test_gpg_sign   false revert two.. --edit --no-gpg-sign --gpg-sign
> +test_gpg_sign   true  revert HEAD  --no-edit
> +test_gpg_sign   true  revert two.. --no-edit
> +test_gpg_sign ! true  revert HEAD  --no-edit --no-gpg-sign
> +test_gpg_sign ! true  revert two.. --no-edit --no-gpg-sign
> +test_gpg_sign ! true  revert HEAD  --no-edit --gpg-sign --no-gpg-sign
> +test_gpg_sign ! true  revert two.. --no-edit --gpg-sign --no-gpg-sign
> +test_gpg_sign   false revert HEAD  --no-edit --no-gpg-sign --gpg-sign
> +
> +test_done
>