Re: [PATCH] send-email: add --confirm option

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

Jay Soffian <jaysoffian@gmail.com> writes:

> send-email violates the principle of least surprise by automatically
> cc'ing additional recipients without confirming this with the user.
>
> This patch teaches send-email a --confirm option. It takes the
> following values:
>
>  --confirm=always   always confirm before sending
>  --confirm=never    never confirm before sending
>  --confirm=cc       confirm before sending when send-email has
>                     automatically added addresses from the patch to
>                     the Cc list
>  --confirm=compose  confirm before sending the first message when
>                     using --compose. (Needed to maintain backwards
>                     compatibility with existing behavior.)
>  --confirm=auto     'cc' + 'compose'
>
> The option defaults to 'compose' if any suppress Cc related options have
> been used, otherwise it defaults to 'auto'.

It is hard to judge if this is a good thing to do _at this point_.  Those
who complained may welcome it but that is hardly the point.  You need to
convince those who stayed silent (because they thought the default won't
change and were not paying attention) that it is a good change.

Especially the changes (not additions) to existing tests worries me; each
change to the existing one is a demonstration of breaking existing users.

You cannot retrofit safety by disallowing things once you used to allow,
without upsetting existing users.

> @@ -837,6 +834,25 @@ X-Mailer: git-send-email $gitversion
>  	unshift (@sendmail_parameters,
>  			'-f', $raw_from) if(defined $envelope_sender);
>  
> +	if ($needs_confirm && !$dry_run) {
> +		print "\n$header\n";
> +		while (1) {
> +			chomp ($_ = $term->readline(
> +				"Send this email? ([y]es|[n]o|[q]uit|[a]ll): "
> +			));
> +			last if /^(?:yes|y|no|n|quit|q|all|a)/i;
> +			print "\n";
> +		}
> +		if (/^n/i) {
> +			return;
> +		} elsif (/^q/i) {
> +			cleanup_compose_files();
> +			exit(0);
> +		} elsif (/^a/i) {
> +			$confirm = 'never';
> +		}
> +	}

I think "[a]ll" would make it a bit less objectionable to people who hate
unsolicited confirmation dialogs.  Nice touch.

> @@ -1094,13 +1119,10 @@ foreach my $t (@files) {
>  	$message_id = undef;
>  }
>  
> -if ($compose) {
> -	cleanup_compose_files();
> -}
> +cleanup_compose_files();
>  
>  sub cleanup_compose_files() {
> -	unlink($compose_filename, $compose_filename . ".final");
> -
> +	unlink($compose_filename, $compose_filename . ".final") if $compose;
>  }

Does this hunk have anything to do with this topic, or is it just a churn
that does not change any behaviour?

> @@ -175,15 +180,13 @@ test_set_editor "$(pwd)/fake-editor"
>  
>  test_expect_success '--compose works' '
>  	clean_fake_sendmail &&
> -	echo y | \
> -		GIT_SEND_EMAIL_NOTTY=1 \
> -		git send-email \
> -		--compose --subject foo \
> -		--from="Example <nobody@example.com>" \
> -		--to=nobody@example.com \
> -		--smtp-server="$(pwd)/fake.sendmail" \
> -		$patches \
> -		2>errors
> +	git send-email \
> +	--compose --subject foo \
> +	--from="Example <nobody@example.com>" \
> +	--to=nobody@example.com \
> +	--smtp-server="$(pwd)/fake.sendmail" \
> +	$patches \
> +	2>errors
>  '

How would test this break without this hunk?  "echo" dies of sigpipe, or
something?

I am not objecting to this particular change; just asking why this change
is here.  "It does not break, but the command shouldn't ask for
confirmation, and giving 'y' into it is unnecessary" is a perfectly
acceptable answer, but if that is the case you probably would want to
verify that the command indeed does go ahead without asking.

> @@ -375,15 +378,56 @@ test_expect_success '--suppress-cc=cc' '
>  	test_suppression cc
>  '
>  
> +test_confirm () {
> +	echo y | \
> +		GIT_SEND_EMAIL_NOTTY=1 \
> +		git send-email \
> +		--from="Example <nobody@example.com>" \
> +		--to=nobody@example.com \
> +		--smtp-server="$(pwd)/fake.sendmail" \
> +		$@ \
> +		$patches | grep "Send this email"
> +}
> +
> +test_expect_success '--confirm=always' '
> +	test_confirm --confirm=always --suppress-cc=all
> +'
> + ...
> +test_expect_success 'confirm by default (due to --compose)' '
> +	CONFIRM=$(git config --get sendemail.confirm) &&
> +	git config --unset sendemail.confirm &&
> +	test_confirm --suppress-cc=all --compose
> +	ret="$?"
> +	git config sendemail.confirm ${CONFIRM:-never}
> +	test $ret = "0"
> +'

These all test you would get a prompt when you as the author expect the
code to give one.  Do you also need tests that verify you do not ask
needless confirmation when the code shouldn't?

>  test_expect_success '--compose adds MIME for utf8 body' '
>  	clean_fake_sendmail &&
>  	(echo "#!$SHELL_PATH" &&
>  	 echo "echo utf8 body: àéìöú >>\"\$1\""
>  	) >fake-editor-utf8 &&
>  	chmod +x fake-editor-utf8 &&
> -	echo y | \
>  	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8\"" \
> -	  GIT_SEND_EMAIL_NOTTY=1 \
>  	  git send-email \
>  	  --compose --subject foo \
>  	  --from="Example <nobody@example.com>" \

If the change you made to git-send-email.perl is later broken and the
command (incorrectly) starts asking for confirmation with these command
line options, what does this test do?  Does it get stuck, forbidding the
test suite to be run unattended?