Re: [PATCH] send-email: add --confirm option
- From
Jay Soffian <jaysoffian@gmail.com>
- Date
- Mar 1, 2009, 14:05 UTC
- Message-ID
- <76718490903010605o2083ba24oda781fb7053c4321@mail.gmail.com>
- In-Reply-To
- <7vhc2d8vjk.fsf@gitster.siamese.dyndns.org>
On Sun, Mar 1, 2009 at 4:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
No, I won't. This is not my itch, I don't know who those users are, and I think I provided any such users several easy ways out:
1) They can type "a" after the first prompt. 2) They can git config --global sendemail.confirm never 3) They won't even notice if they're using any suppresscc option, such as yourself.
I know you're sensitive to existing users after getting burned by the dashless-git issue, but I think you're over applying that lesson in *this* case.
> Especially the changes (not additions) to existing tests worries me; each > change to the existing one is a demonstration of breaking existing users.
There were two classes of changes:
1) Many of the tests Cc'd w/o warning. Those now prompt. Hence the "git config sendemail.suppress never" at the top.
2) Since I added (1), the "echo y |" in the other tests was redundant. I believe had a no-confirm option been available in the past, that the test authors would not have used "echo y". And I thought it confusing to leave the "echo y|" as someone might ask "if it's not supposed to confirm, why is there an echo y here?"
> You cannot retrofit safety by disallowing things once you used to allow, > without upsetting existing users.
And you hamper your ability to improve the product by not allowing even minor backward incompatible changes.
But I anticipated this objection and I thought very much about how an existing user might hypothetically be upset, while accommodating the very real concern of a new user.
Aside, when I upgrade a product, I expect it to change in some ways. If I really don't want change, I don't upgrade.
On balance, I think this change will be silently appreciated by many more users than the few, if any, vocal objectors.
Show 17 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?The former. You'll notice there is a "quit" option in the prompt (in addition to yes/no/all). The quit option needs to call cleanup_compose_files(), so it now has two callers.
Instead of both callers having to do this:
if ($compose) {
cleanup_compose_files();
}I just moved the if compose test into the function so each caller can simply ask:
cleanup_compose_files();
Show 30 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.
The hunk is there because the "echo y" is redundant with confirm=never which is now set early.
> 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?
Yes, I couldn't think how to test that last night for some reason, but it is obvious to me now. I can send a followup.
Show 17 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?
Many of the tests (because they automatically cc) will hang waiting on input if git-send-email.perl is later broken in such a way as to start ignoring --compose.
I could not think of a good way to avoid this. We could possibly "echo y|" redundantly to every test, but even that does not guarantee that one of the other prompts cannot cause a hang. So this potential to hang, I think, already exists.
The best I could think of was something like (sleep 60 && kill $$) &; sleep_pid=$!; ... kill $sleep_pid; wrapping each test, or even the entire script. Suggestions here appreciated.
But, I think this can largely be addressed by testing that the change does *not* prompt when it shouldn't, and moving that test before any of the existing tests which could prompt.
j.