Re: [PATCH] send-email: always confirm by default

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 22, 2022, 11:16 UTC
Message-ID: <220422.86zgkd4b1l.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220422083629.1404989-1-hi@alyssa.is>

On Fri, Apr 22 2022, Alyssa Ross wrote:

> [[ Whoops, sent to the list this time! ]]

It is somewhat ironic to have an accidental E-Mail send given the patch subject :)

> Ævar, thanks for encouraging me to send a patch.  At your suggestion,
> I've trawled through the list archives looking for any previous
> discussion of this default, but I didn't find anything.

Glad to see a follow-up to this. Thanks.

Show 30 quoted lines

>  Documentation/git-send-email.txt |  3 +--
>  git-send-email.perl              |  2 +-
>  t/t9001-send-email.sh            | 14 ++++----------
>  3 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 41cd8cb424..b791d83bb7 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -407,8 +407,7 @@ Administering
>  --
>  +
>  Default is the value of `sendemail.confirm` configuration value; if that
> -is unspecified, default to 'auto' unless any of the suppress options
> -have been specified, in which case default to 'compose'.
> +is unspecified, default to 'always'.
>  
>  --dry-run::
>  	Do everything except actually send the emails.
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 5861e99a6e..4aa7d83cdc 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -606,7 +606,7 @@ sub config_regexp {
>  # Set confirm's default value
>  my $confirm_unconfigured = !defined $confirm;
>  if ($confirm_unconfigured) {
> -	$confirm = scalar %suppress_cc ? 'compose' : 'auto';
> +	$confirm = 'always';
>  };

I squinted a bit at this and wonder now that we don't have that ternary whether we can simplify this a bit.

Isn't this the same now as:

    my $confirm_unconfigured = !defined $confirm;
    $confirm ||= 'always'; # well, defined-or, but I don't think we care here.

But probably not worth obsessing over it... :) I.e. it seems fine as-is.

Show 24 quoted lines

> -test_expect_success $PREREQ 'No confirm with --suppress-cc' '
> -	test_no_confirm --suppress-cc=sob &&
> +test_expect_success $PREREQ 'No confirm with --confirm=compose --suppress-cc' '
> +	test_no_confirm --confirm=compose --suppress-cc=sob &&
>  	check_no_confirm
>  '
>  
> @@ -1032,16 +1032,10 @@ test_expect_success $PREREQ '--confirm=compose' '
>  	test_confirm --confirm=compose --compose
>  '
>  
> -test_expect_success $PREREQ 'confirm by default (due to cc)' '
> +test_expect_success $PREREQ 'confirm by default' '
>  	test_when_finished git config sendemail.confirm never &&
>  	git config --unset sendemail.confirm &&
> -	test_confirm
> -'
> -
> -test_expect_success $PREREQ 'confirm by default (due to --compose)' '
> -	test_when_finished git config sendemail.confirm never &&
> -	git config --unset sendemail.confirm &&
> -	test_confirm --suppress-cc=all --compose
> +	test_confirm --suppress-cc=all
>  '

It looks like we're losing some coverage here (I may be wrong). I.e. yes we should update tests and descriptions, but for lines that test what we did with a given command-line before let's turn those into tests that positively assert what we do now.

I.e. we lost the "--suppress-cc=sob" test (with no other option), and also "--suppress-cc=all --compose"?