Should sendemail.confirm default to always?

Should sendemail.confirm default to always?

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]

I was recently having a conversation with some people used to the
GitHub-style Pull Request workflow, who told me that they feel scared of
using git send-email in case they make a mistake and e.g. get the
recipients wrong, since they can't correct it after sending.  They can
resend, but if they do that they'll feel like they're bothering some
very busy people by having sent them the same message twice (regardless
of whether those people are actually bothered by it, it's scary).

This made me remember feeling that same sense of fear when I used
git send-email the first few times.  At some point I discovered that I
could set sendemail.confirm to always, and then git would always prompt
me before sending a message, and I could review the recipients list,
edit the message if I wanted to, etc.  After that, I was happy, and
completely forgot that that wasn't the default behaviour until having
this conversation.

So I thought it was worth asking, might it be a good idea to change the
default, and have git always prompt before sending mail unless it's told
otherwise?  Expert users will be able to figure out how to change this
default if they don't like it, but novice users won't have bad first
experiences where they accidentally send out an email before they were
ready any more.

I don't think this change would cause too much hassle for people
expecting the current default, because the current default is for git to
prompt *sometimes*.  So anybody who doesn't like being prompted is
likely to have already disabled it.

These git users (who are probably in the majority!) are used to having
edit and delete buttons, so for them the idea of having to get things
right the first time is scary enough with a preview, let alone without
one.  I hope you can empathize.

Thanks!

Alyssa Ross

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Should sendemail.confirm default to always?

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 21 2022, Alyssa Ross wrote:

> [[PGP Signed Part:Undecided]]
> I was recently having a conversation with some people used to the
> GitHub-style Pull Request workflow, who told me that they feel scared of
> using git send-email in case they make a mistake and e.g. get the
> recipients wrong, since they can't correct it after sending.  They can
> resend, but if they do that they'll feel like they're bothering some
> very busy people by having sent them the same message twice (regardless
> of whether those people are actually bothered by it, it's scary).
>
> This made me remember feeling that same sense of fear when I used
> git send-email the first few times.  At some point I discovered that I
> could set sendemail.confirm to always, and then git would always prompt
> me before sending a message, and I could review the recipients list,
> edit the message if I wanted to, etc.  After that, I was happy, and
> completely forgot that that wasn't the default behaviour until having
> this conversation.
>
> So I thought it was worth asking, might it be a good idea to change the
> default, and have git always prompt before sending mail unless it's told
> otherwise?  Expert users will be able to figure out how to change this
> default if they don't like it, but novice users won't have bad first
> experiences where they accidentally send out an email before they were
> ready any more.
>
> I don't think this change would cause too much hassle for people
> expecting the current default, because the current default is for git to
> prompt *sometimes*.  So anybody who doesn't like being prompted is
> likely to have already disabled it.
>
> These git users (who are probably in the majority!) are used to having
> edit and delete buttons, so for them the idea of having to get things
> right the first time is scary enough with a preview, let alone without
> one.  I hope you can empathize.

As a regular user of it, I have it set to sendemail.confirm=always. I
can see that (goes and looks) a bit over 10% of it git blame's to me,
and I'd find it too scary to operate without that mode :)

Now, I'd honestly forgotten that wasn't the default mode (I set it many
years ago), but I for one think that setting makes sense.

I think a patch to do that would be worth having/reviewing (hint
hint!). As always the real work is updating docs, running the tests
etc., digging up ML discussions to see if there's been any past
arguments for/against it etc.

In this case the code change itself should be trivial (I honestly didn't
look this time, but really, it's a config default...). So this seems
like a perfect task for someone not too familiar with Git's codebase,
but still interested in contributing :)

Re: Should sendemail.confirm default to always?

[Junio C Hamano]

Alyssa Ross <hi@alyssa.is> writes:

> I was recently having a conversation with some people used to the
> GitHub-style Pull Request workflow, who told me that they feel scared of
> using git send-email in case they make a mistake and e.g. get the
> recipients wrong, since they can't correct it after sending.  They can
> resend, but if they do that they'll feel like they're bothering some
> very busy people by having sent them the same message twice (regardless
> of whether those people are actually bothered by it, it's scary).

If it truly makes sense to give a roadblock before sending to
prevent mistakes, I wonder if making "--dry-run" the default is even
a better idea.  Getting "are you sure [y/n]?" and saying "yes" out
of inertia is much more likely to happen than typing Ctrl-P on the
command line to take the previous command (which did a dry-run by
default) back on the command line and then adding "--no-dry-run" on
the command line to allow the command to actually send out the
files.

Another idea is to forbid the form of "git send-email" invocation
that internally runs format-patch by default and force users to
prepare format-patch into files beforehand.  Doing the format-patch
as a completely separate step means that the user has a final chance
to proofread the log messages (and correct them as needed) while
adding and verifying CC's, and removes the pressure of "pressing
this button is a point of no return; did I catch all the
embarrassing mistakes?" at the last second.

Re: Should sendemail.confirm default to always?

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 3132 bytes --]

On Thu, Apr 21, 2022 at 01:04:29PM -0700, Junio C Hamano wrote:
> Alyssa Ross <hi@alyssa.is> writes:
>
> > I was recently having a conversation with some people used to the
> > GitHub-style Pull Request workflow, who told me that they feel scared of
> > using git send-email in case they make a mistake and e.g. get the
> > recipients wrong, since they can't correct it after sending.  They can
> > resend, but if they do that they'll feel like they're bothering some
> > very busy people by having sent them the same message twice (regardless
> > of whether those people are actually bothered by it, it's scary).
>
> If it truly makes sense to give a roadblock before sending to
> prevent mistakes, I wonder if making "--dry-run" the default is even
> a better idea.  Getting "are you sure [y/n]?" and saying "yes" out
> of inertia is much more likely to happen than typing Ctrl-P on the
> command line to take the previous command (which did a dry-run by
> default) back on the command line and then adding "--no-dry-run" on
> the command line to allow the command to actually send out the
> files.

In all the time I've been using sendemail.confirm = always, I've never
accidentally said "yes".  I'd be curious whether Ævar has, since they
also said they've used it for some time.

I think always confirming is better than --dry-run because it allows
making a decision for each message in a series, and because it makes it
easy to insert commentary.  (If you end up sending the first few
messages in a series, then deciding not to send the rest before fixing
something, the man page is good enough that it's possible to figure out
how to resume sending, in my experience.)

If you had to pass --no-dry-run every time, I'd worry about that being
replayed unthinkingly from shell history, defeating the purpose.  By
contrast, interactive per-message confirmation won't end up with the
bypass being automatically saved in shell history.

> Another idea is to forbid the form of "git send-email" invocation
> that internally runs format-patch by default and force users to
> prepare format-patch into files beforehand.  Doing the format-patch
> as a completely separate step means that the user has a final chance
> to proofread the log messages (and correct them as needed) while
> adding and verifying CC's, and removes the pressure of "pressing
> this button is a point of no return; did I catch all the
> embarrassing mistakes?" at the last second.

I thought about that, but I don't think it would be as good either,
because it's difficult to predict what the final set of recipients will
be just from looking at the patch files — if I recall correctly,
automatic CCs (from trailers, cccmd, etc.) aren't added until send-email
time.  So even though I often run format-patch beforehand, I'm still
grateful to be prompted before each message.

I also think that encouraging novices to format-patch first would make
the workflow seem even more arcane and confusing.  I definitely found it
very strange the first time I encountered it (more so than all-in-one
send-email).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Should sendemail.confirm default to always?

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> .... As always the real work is updating docs, running the tests
> etc., digging up ML discussions to see if there's been any past
> arguments for/against it etc.

I agree with this part.  The code change itself may be easy, but it
takes a lot of work and moderate amount of help from a long timer or
two.

Failures in t9001-send-email

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1306 bytes --]

> I think a patch to do that would be worth having/reviewing (hint
> hint!). As always the real work is updating docs, running the tests
> etc., digging up ML discussions to see if there's been any past
> arguments for/against it etc.
>
> In this case the code change itself should be trivial (I honestly didn't
> look this time, but really, it's a config default...). So this seems
> like a perfect task for someone not too familiar with Git's codebase,
> but still interested in contributing :)

I thought I'd have a go at a patch, but I wasn't able to get the tests
running:

    prove ./t9001-send-email.sh
    ./t9001-send-email.sh .. Dubious, test returned 1 (wstat 256, 0x100)
    Failed 15/188 subtests
      (less 1 skipped subtest: 172 okay)

    Test Summary Report
    -------------------
    ./t9001-send-email.sh (Wstat: 256 Tests: 188 Failed: 15)
      Failed tests:  27, 51, 78-84, 123, 147-150, 185
      Non-zero exit status: 1
    Files=1, Tests=188, 17 wallclock secs ( 0.06 usr  0.01 sys + 11.66 cusr  5.37 csys = 17.10 CPU)
    Result: FAIL

I had a look at the tests to try to guess what was wrong, but I didn't
come up with anything.  Any ideas?  When I run make test, every other
test is fine, it's just the send-email tests that are problematic.

(My git source tree is v2.36.0.)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: Failures in t9001-send-email

[rsbecker]

On April 21, 2022 6:39 PM, Alyssa Ross wrote:
>> I think a patch to do that would be worth having/reviewing (hint
>> hint!). As always the real work is updating docs, running the tests
>> etc., digging up ML discussions to see if there's been any past
>> arguments for/against it etc.
>>
>> In this case the code change itself should be trivial (I honestly
>> didn't look this time, but really, it's a config default...). So this
>> seems like a perfect task for someone not too familiar with Git's
>> codebase, but still interested in contributing :)
>
>I thought I'd have a go at a patch, but I wasn't able to get the tests
>running:
>
>    prove ./t9001-send-email.sh
>    ./t9001-send-email.sh .. Dubious, test returned 1 (wstat 256, 0x100)
>    Failed 15/188 subtests
>      (less 1 skipped subtest: 172 okay)
>
>    Test Summary Report
>    -------------------
>    ./t9001-send-email.sh (Wstat: 256 Tests: 188 Failed: 15)
>      Failed tests:  27, 51, 78-84, 123, 147-150, 185
>      Non-zero exit status: 1
>    Files=1, Tests=188, 17 wallclock secs ( 0.06 usr  0.01 sys + 11.66 cusr
5.37 csys =
>17.10 CPU)
>    Result: FAIL
>
>I had a look at the tests to try to guess what was wrong, but I didn't come
up with
>anything.  Any ideas?  When I run make test, every other test is fine, it's
just the
>send-email tests that are problematic.

I have the same issue on NonStop. Not sure why, other than our sendmail is
massively broken.
--Randall

Re: Failures in t9001-send-email

[Johannes Sixt]

Am 22.04.22 um 00:38 schrieb Alyssa Ross:
> I thought I'd have a go at a patch, but I wasn't able to get the tests
> running:
> 
>     prove ./t9001-send-email.sh
>     ./t9001-send-email.sh .. Dubious, test returned 1 (wstat 256, 0x100)
>     Failed 15/188 subtests
> [...]
> 
> I had a look at the tests to try to guess what was wrong, but I didn't
> come up with anything.  Any ideas?

You run ./t9001-send-email.sh (without `prove`). Add -v to see some
verbiage from the tests, throw in -i to have it stop at the first
failure (so you don't have to scroll back to find it), and add -x for
additional traces of commands that the shell executes (to see the exact
command that failed).

IFAIC, I always go all in, i.e., either all or none of -v -i -x.

-- Hannes

Re: Failures in t9001-send-email

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> I had a look at the tests to try to guess what was wrong, but I didn't
>> come up with anything.  Any ideas?
>
> You run ./t9001-send-email.sh (without `prove`). Add -v to see some
> verbiage from the tests, throw in -i to have it stop at the first
> failure (so you don't have to scroll back to find it), and add -x for
> additional traces of commands that the shell executes (to see the exact
> command that failed).
>
> IFAIC, I always go all in, i.e., either all or none of -v -i -x.

Good suggestion.

I rarely (if ever) use "-x" myself but another useful thing to know,
while learning how the existing test works (i.e. studying a working
test, not debugging a broken test) is to use "-d", possibly together
with "-v", which refrains from removing the trash directory.  Then
you can go in and examine the state of the test repository the tests
left.

Thanks.

Re: Failures in t9001-send-email

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

On Thu, Apr 21, 2022 at 07:19:04PM -0400, rsbecker@nexbridge.com wrote:
> On April 21, 2022 6:39 PM, Alyssa Ross wrote:
> >I thought I'd have a go at a patch, but I wasn't able to get the tests
> >running:
> >
> >    prove ./t9001-send-email.sh
> >    ./t9001-send-email.sh .. Dubious, test returned 1 (wstat 256, 0x100)
> >    Failed 15/188 subtests
> >      (less 1 skipped subtest: 172 okay)
> >
> >    Test Summary Report
> >    -------------------
> >    ./t9001-send-email.sh (Wstat: 256 Tests: 188 Failed: 15)
> >      Failed tests:  27, 51, 78-84, 123, 147-150, 185
> >      Non-zero exit status: 1
> >    Files=1, Tests=188, 17 wallclock secs ( 0.06 usr  0.01 sys + 11.66 cusr
> 5.37 csys =
> >17.10 CPU)
> >    Result: FAIL
> >
> >I had a look at the tests to try to guess what was wrong, but I didn't come
> up with
> >anything.  Any ideas?  When I run make test, every other test is fine, it's
> just the
> >send-email tests that are problematic.
>
> I have the same issue on NonStop. Not sure why, other than our sendmail is
> massively broken.
> --Randall

For me the solution was to make Term::ReadLine available in my
development environment.  I hope that helps you too!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Failures in t9001-send-email

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1660 bytes --]

On Thu, Apr 21, 2022 at 11:51:07PM -0700, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> >> I had a look at the tests to try to guess what was wrong, but I didn't
> >> come up with anything.  Any ideas?
> >
> > You run ./t9001-send-email.sh (without `prove`). Add -v to see some
> > verbiage from the tests, throw in -i to have it stop at the first
> > failure (so you don't have to scroll back to find it), and add -x for
> > additional traces of commands that the shell executes (to see the exact
> > command that failed).
> >
> > IFAIC, I always go all in, i.e., either all or none of -v -i -x.
>
> Good suggestion.
>
> I rarely (if ever) use "-x" myself but another useful thing to know,
> while learning how the existing test works (i.e. studying a working
> test, not debugging a broken test) is to use "-d", possibly together
> with "-v", which refrains from removing the trash directory.  Then
> you can go in and examine the state of the test repository the tests
> left.

Thank you both for these tips.

I was able to determine the issue by running the test with -vixd,
noticing which command failed, and then trying to set up my shell
environment manually to match the test's (PATH, PERLLIB,
committer/author info, etc.), and ran the same command with the leftover
trash directory.  This produced error output that didn't show up when I
was running the test (not sure why) that gave me a line number in
git-send-email.perl.  I noticed that it was trying to load
Term::ReadLine near that line, and I didn't have that installed in my
development environment.

Now that I have that installed, all send-email tests pass.
Thanks again!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] send-email: always confirm by default

[Alyssa Ross]

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.

I think this is a better idea than defaulting to --dry-run, because
having to specify on the command line that you _really_ want to send the
patches now would mean the --no-dry-run version would quickly end up
being replayed unthinkingly from shell history, and because always
confirming has the nice side effect of making it easy to add patch
commentary.

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.  Additionally, teaching git send-email is hard enough
without having to also teach format-patch, and to make sure to clean up
afterwards, etc!

Signed-off-by: Alyssa Ross <hi@alyssa.is>
---

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

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

 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';
 };
 # Please update $__git_send_email_confirm_options in
 # git-completion.bash when you add new options.
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
 '
 
 test_expect_success $PREREQ 'confirm detects EOF (inform assumes y)' '

base-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e
-- 
2.35.1

RE: Failures in t9001-send-email

[rsbecker]

On April 22, 2022 3:57 AM, Alyssa Ross wrote:
>On Thu, Apr 21, 2022 at 07:19:04PM -0400, rsbecker@nexbridge.com wrote:
>> On April 21, 2022 6:39 PM, Alyssa Ross wrote:
>> >I thought I'd have a go at a patch, but I wasn't able to get the
>> >tests
>> >running:
>> >
>> >    prove ./t9001-send-email.sh
>> >    ./t9001-send-email.sh .. Dubious, test returned 1 (wstat 256, 0x100)
>> >    Failed 15/188 subtests
>> >      (less 1 skipped subtest: 172 okay)
>> >
>> >    Test Summary Report
>> >    -------------------
>> >    ./t9001-send-email.sh (Wstat: 256 Tests: 188 Failed: 15)
>> >      Failed tests:  27, 51, 78-84, 123, 147-150, 185
>> >      Non-zero exit status: 1
>> >    Files=1, Tests=188, 17 wallclock secs ( 0.06 usr  0.01 sys +
>> > 11.66 cusr
>> 5.37 csys =
>> >17.10 CPU)
>> >    Result: FAIL
>> >
>> >I had a look at the tests to try to guess what was wrong, but I
>> >didn't come
>> up with
>> >anything.  Any ideas?  When I run make test, every other test is
>> >fine, it's
>> just the
>> >send-email tests that are problematic.
>>
>> I have the same issue on NonStop. Not sure why, other than our
>> sendmail is massively broken.
>> --Randall
>
>For me the solution was to make Term::ReadLine available in my development
>environment.  I hope that helps you too!

Thanks. I will look into this.
--Randall

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

[Ævar Arnfjörð Bjarmason]

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.

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

> -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"?

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

[Junio C Hamano]

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.