| Message ID | 20230807165850.2335067-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] send-email: prompt-dependent exit codes | expand |
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > From the perspective of a scripted caller, failure to send (some) mails > is an error even if it was interactively requested, so it should be > indicated by the exit code. I am not sure if unconditional change of exit code this late in the game. When was the interactive "no, do not send this one" feature introduced? The end-users (not necessarily all of them, of course) have been happily using the command and have appreciated not having to see non-zero exit code after skipping some messages. I wonder if this should be hidden behind an opt-in command line option and possibly a configuration variable that defaults to "no". > To make it somewhat specific, the exit code is 10 when only some mails > were skipped, and 11 if the user quit on the first prompt. If 10 and 11 were *not* taken out of thin air, but there is a precedent to use these two values in e-mail related programs, please share. It may give us a good justification. With or without other people's precedents, and with or without making it conditional, the new behaviour must be documented, if the command has already a documentation (and it seems that there exists the Documentation/git-send-email.txt file). It may be preferrable to protect the new feature with a test or two added to t9001 but it obviously depends on how hard you find testing interactive stuff is. > For interactive calls from the command line, interactive cancellation is > arguably not really an error, but there the exit code will be more or > less ignored anyway. Not necessarily. Some people prefer to see it and show it in their command line prompt. > diff --git a/git-send-email.perl b/git-send-email.perl > index affbb88509..cd4db84b7f 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -256,6 +256,26 @@ sub system_or_die { > die $msg if $msg; > } > > +my $sent_files = 0; > + > +sub do_exit { > + if ($sent_files == @files) { > + # All specified messages were sent > + exit(0); > + } elsif ($sent_files) { > + # At least some messages were sent > + exit(10); > + } else { > + # User cancelled on first message already > + exit(11); > + } > +} OK. As log as we won't start doing while (loop) { $file = shift @files; send $file; } this will keep working fine, and the logic is very clear to see. Thanks. Will queue but expect at least some documentation updates.
On Mon, Aug 07, 2023 at 11:55:29AM -0700, Junio C Hamano wrote: >Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > >> From the perspective of a scripted caller, failure to send (some) mails >> is an error even if it was interactively requested, so it should be >> indicated by the exit code. > >I am not sure if unconditional change of exit code this late in the >game. > When was the interactive "no, do not send this one" feature > introduced? > c1f2aa45b7 (add --confirm option and configuration setting, 2009-03-02), and extended by 5c80afed02 (ask what to do with an invalid email address, 2012-11-26), so basically it's been there forever. >I wonder if this should be hidden behind an opt-in command line option >and possibly a configuration variable that defaults to "no". > i wondered, too, but i think it isn't really worth it. interactive users won't really care (see below), and most scripted users presumably simply suppressed the condition by passing --confirm=never and/or appropriate --suppress* options. others didn't notice (possibly because of their config options), or ignored the problem, and therefore have a good chance of being broken. some were unhappy about it, but didn't bother reporting/patching it, which always constitutes the vast majority of affected users. this still leaves us with a hypothetical small set of wrapper scripts that _really_ want to remain ignorant of messages being skipped. i think it's acceptable to expect them to adjust to the (from their POV) false positives, as it should be mostly harmless, and have the effect of getting some scripts fixed. >> To make it somewhat specific, the exit code is 10 when only some >> mails >> were skipped, and 11 if the user quit on the first prompt. > >If 10 and 11 were *not* taken out of thin air, but there is a >precedent to use these two values in e-mail related programs, please >share. It may give us a good justification. > the numbers are indeed pulled out of thin air. the offset is chosen such that it's sufficiently far off normally expected exit codes. >With or without other people's precedents, and with or without >making it conditional, the new behaviour must be documented, if the >command has already a documentation (and it seems that there exists >the Documentation/git-send-email.txt file). > ack, will add an EXIT STATUS section. >It may be preferrable >to protect the new feature with a test or two added to t9001 but it >obviously depends on how hard you find testing interactive stuff is. > shouldn't be too hard; test_{,no_}confirm show how to do it. >> For interactive calls from the command line, interactive cancellation is >> arguably not really an error, but there the exit code will be more or >> less ignored anyway. > >Not necessarily. Some people prefer to see it and show it in their >command line prompt. > hence "mostly". but users generally know what they did just a few secs ago, so likely won't be bothered by the special exit code. >Thanks. Will queue but expect at least some documentation updates. > do you want a followup, or a v4 to replace the commit? regards
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > i think it's acceptable to expect them to adjust to the (from > their POV) false positives,... You are not in the position to unilaterally declare "it isn't really worth it" or "it's acceptable to expect", though. We know at least one person want the program to signal failure when interactively aborted. We know that all other current users did not complain that the program did not signal failure when they interactively aborted, they may be they did not care enough, or they may be they are happy. You do not know the population and neither do I, but that is not the point. This behaviour change is, even among Git developers, not a bugfix but is a new feature that not everybody would want to be subjected to. It always is the safest to make such a backward incompatible change a strictly opt-in feature. >>Thanks. Will queue but expect at least some documentation updates. >> > do you want a followup, or a v4 to replace the commit? Replace it. We do not want to do "oops that was lacking, here is an incremental update" for anything that is not in 'next'. Thanks.
On Tue, Aug 08, 2023 at 09:08:56AM -0700, Junio C Hamano wrote: >This behaviour change is, even among Git developers, not a bugfix but >is a new feature that not everybody would want to be subjected to. > that's debatable, depending on one's expectations. at the very least it's an enabler for bugfixes. >It always is the safest to make such a backward incompatible change a >strictly opt-in feature. > well, yes. the question is whether we expect non-trivial problems arising from imposing it on unsuspecting users. and given that there is an (opportunity) cost of having it opt-in only, there is a trade-off. my gut feeling is that having it always-on is probably better. regards
diff --git a/git-send-email.perl b/git-send-email.perl index affbb88509..cd4db84b7f 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -256,6 +256,26 @@ sub system_or_die { die $msg if $msg; } +my $sent_files = 0; + +sub do_exit { + if ($sent_files == @files) { + # All specified messages were sent + exit(0); + } elsif ($sent_files) { + # At least some messages were sent + exit(10); + } else { + # User cancelled on first message already + exit(11); + } +} + +sub do_quit { + cleanup_compose_files(); + do_exit(); +} + sub do_edit { if (!defined($editor)) { $editor = Git::command_oneline('var', 'GIT_EDITOR'); @@ -1195,8 +1215,7 @@ sub validate_address { if (/^d/i) { return undef; } elsif (/^q/i) { - cleanup_compose_files(); - exit(0); + do_quit(); } $address = ask("$to_whom ", default => "", @@ -1619,8 +1638,7 @@ sub send_message { } elsif (/^e/i) { return -1; } elsif (/^q/i) { - cleanup_compose_files(); - exit(0); + do_quit(); } elsif (/^a/i) { $confirm = 'never'; } @@ -2001,6 +2019,10 @@ sub process_file { return 0; } + if ($message_was_sent) { + $sent_files++; + } + # set up for the next message if ($thread) { if ($message_was_sent && @@ -2278,3 +2300,5 @@ sub body_or_subject_has_nonascii { } return 0; } + +do_exit();
From the perspective of a scripted caller, failure to send (some) mails is an error even if it was interactively requested, so it should be indicated by the exit code. To make it somewhat specific, the exit code is 10 when only some mails were skipped, and 11 if the user quit on the first prompt. For interactive calls from the command line, interactive cancellation is arguably not really an error, but there the exit code will be more or less ignored anyway. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- v3: - use a tally instead of flags after all, as my seemingly simple idea apparently requires lots of thinking to grasp fully - correct exit code when zero messages are to be sent. this cannot actually happen, as it induces an exit via usage() earlier on. - unfold nested ternary to save junio's sanity (who proved his point by unfolding it slightly incorrectly) - expand commit message v2: - fix do_quit() not resetting $sent_all Cc: Junio C Hamano <gitster@pobox.com> Cc: Phillip Wood <phillip.wood123@gmail.com> --- git-send-email.perl | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)