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

[Junio C Hamano <gitster@pobox.com>]

Alyssa Ross <hi@alyssa.is> writes:

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

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