On Sun, Mar 1, 2009 at 4:03 AM, Junio C Hamano <gitster@xxxxxxxxx> 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 <nobody@xxxxxxxxxxx>" \
>> - --to=nobody@xxxxxxxxxxx \
>> - --smtp-server="$(pwd)/fake.sendmail" \
>> - $patches \
>> - 2>errors
>> + git send-email \
>> + --compose --subject foo \
>> + --from="Example <nobody@xxxxxxxxxxx>" \
>> + --to=nobody@xxxxxxxxxxx \
>> + --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 <nobody@xxxxxxxxxxx>" \
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
