Re: [PATCH] send-email: add --confirm option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 1, 2009, 09:03 UTC
- Message-ID
- <7vhc2d8vjk.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1235895801-80414-1-git-send-email-jaysoffian@gmail.com>
Jay Soffian <jaysoffian@gmail.com> writes:
Show 18 quoted lines
> 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.
Show 22 quoted lines
> @@ -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.
Show 14 quoted lines
> @@ -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?
Show 21 quoted lines
> @@ -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.
Show 27 quoted lines
> @@ -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?
Show 12 quoted lines
> 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?