From: Jay Soffian Date: Sun, 01 Mar 2009 14:05:12 GMT Subject: Re: [PATCH] send-email: add --confirm option 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 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. >> @@ -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(); >> @@ -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 " \ >> -             --to=nobody@example.com \ >> -             --smtp-server="$(pwd)/fake.sendmail" \ >> -             $patches \ >> -             2>errors >> +     git send-email \ >> +     --compose --subject foo \ >> +     --from="Example " \ >> +     --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. >>  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 " \ > > 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.