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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 22, 2022, 17:09 UTC
Message-ID: <xmqq7d7havuy.fsf@gitster.g>
In-Reply-To: <20220422083629.1404989-1-hi@alyssa.is>

Alyssa Ross <hi@alyssa.is> writes:

Show 8 quoted lines

> For novice users, it can be very intimidating for git send-email to send
> off a lot of mail without prompting for confirmation.  These users are
> likely to not know it's even possible to configure git to behave
> differently.  So let's set a novice-friendly default — expert users who
> don't need to be prompted every time will be able to set
> sendemail.confirm to their preference, although from my small sample it
> sounds plenty of expert users already rely on sendemail.confirm =
> always.

One thing the current behaviour of "confirm" makes a bad end-user experience for recipients is that you can individually say "no, this message has wrong address on the CC line, do not send it" and the receivers end up seeing [1/4] [2/4] [4/4] and left wondering what happend to [3/4], until [3/4] alone is resent.

Iterating over messages and letting the user examine the headers for each of them is perfectly fine, but it would make it much nicer to recipients if the choices are collected first and then even a single NO stops the entire series from getting sent.

	[Side note] As with everything else in Git, which is a tool
	to help people interact with each other better, we give
	priority to avoid wasting time of the more populous side,
	and naturally, we should aim to make it less annoying to
	receivers even if it means that the sender needs to spend a
	bit more time to send the right things to the right people.

And making "confirm" the default before it is fixed may make it easier to annoy receivers.

> I also think this approach is better than forbidding the all-in-one
> format-patch + send-email, because it wouldn't give an accurate preview
> of recipients, because automatic CCs are added by send-email, not
> format-patch.

I do not think this part matches reality. format-patch alone does not manage CC or To at all. What the separation of send-email and format-patch does is to train people to actually proofread what they are going to send out. Being able to add Cc: and To: at the header part (instead of adding Cc: to trailers and have send-email to pick them up, which is error prone and forcing you to advocate this default change so that you can check at the last minute) of the generated patch text is icing on the cake ;-)

>  if ($confirm_unconfigured) {
> -	$confirm = scalar %suppress_cc ? 'compose' : 'auto';
> +	$confirm = 'always';

This got a lot simpler. I've always wondered why this "if suppress CC is there, then default to 'compose' and otherwise 'auto'" makes a sensible default.

Show 32 quoted lines

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 42694fe584..e11730f3dc 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -74,8 +74,8 @@ check_no_confirm () {
>  	return 0
>  }
>  
> -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 is curious that the change to the test this patch adds are only to adjust to the fallout the change of the default causes, and there is no new test to make sure that unconfigured sendemail.confirm triggers confirmation dialogue.

I still have reservations, as I would eventually have to sell those existing users that this usability regression [*1*] is worth having in the new release. It will need a large backward-compatibility note in the Release Notes, at least.

But assuming that you can help convince these few dozens of existing Git users out there that it is worth doing, the implementation seems correct to me.

Thanks.

[Footnote]

*1* The existing users will be surprised by sudden appearance of unexpected confirmation dialogue, which they may have never seen, and have to decide

 (1) keep saying "yes" to send out the current series, or
 (2) abort, read release notes and finally configure the
     sendemail.confirm away before resending

soon after updating their Git. They will be extremely annoyed if that happens when they were about to send out a large series.