| Message ID | 20230426061606.1495646-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] send-email: prompt-dependent exit codes | expand |
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > From the perspective of the 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. > > Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> > > --- > v2: > - fix do_quit() not resetting $sent_all > --- > git-send-email.perl | 27 +++++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) Administrivia: * Marking the patch as "v2" is very good to signal that this is an improved version of a previous effort. It also is very good that the difference from "v1" is summarized below the three-dash line. * When sending such a new iteration, it helps readers and reviewers to make it a reply to the previous round. You seem to be using "git send-email" and giving the option --in-reply-to=20230323162234.995435-1-oswald.buddenhagen@gmx.de would have made this message a reply to the previous one. Not everybody will remember your previous attempt. * It gives the new iteration a better chance to be reviewed if you CC'ed the right people, including the ones who gave reviews on the previous round. As to the motivation described in the proposed log message, I am not convinced. The end-user knows the best, they told the program to stop, and the program stopped as it was told. I tend to think that it should not be treated as an error. But if somebody is driving "git send-email" from a wrapper that wants to know what happened, I can understand that "did we send everything we were told to send" is *one* of the things such a wrapper may want to know. And my earlier "I am not convinced" is definitely not "I am convinced that what this patch wants to do is wrong". > diff --git a/git-send-email.perl b/git-send-email.perl > index 07f2a0cbea..7587cd2d20 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -254,6 +254,19 @@ sub system_or_die { > die $msg if $msg; > } > > +my $sent_any = 0; > +my $sent_all = 1; > + > +sub do_exit { > + exit($sent_any ? $sent_all ? 0 : 10 : 11); > +} Without knowing where "v1" was (see Administrivia above) and seeing the use of above two variables, I found them a bit unnatural and was going to suggest to have the total number of messages and the number of messages that were actually sent, but it turns out that it was the exact same reaction as Phillip had for "v1" iteration. It would work either way until we start caring how many we actually have sent, but keeping tallies may be more future-proof. In any case, before sending anything, we initialize ourselves to the "we haven't sent anything" and "we have sent everything we have been asked to send so far" state. The idea to flip the former flag to true once we sent a single message while flipping the latter flag to false once we failed to send a single message to maintain these two flags is simple and effective. > +sub do_quit { > + cleanup_compose_files(); > + $sent_all = 0; > + do_exit(); > +} > + > sub do_edit { > if (!defined($editor)) { > $editor = Git::command_oneline('var', 'GIT_EDITOR'); > @@ -1172,8 +1185,7 @@ sub validate_address { > if (/^d/i) { > return undef; > } elsif (/^q/i) { > - cleanup_compose_files(); > - exit(0); > + do_quit(); > } > $address = ask("$to_whom ", > default => "", > @@ -1593,8 +1605,7 @@ sub send_message { > } elsif (/^e/i) { > return -1; > } elsif (/^q/i) { > - cleanup_compose_files(); > - exit(0); > + do_quit(); > } elsif (/^a/i) { > $confirm = 'never'; > } OK, the above two covers the case where the end-user interactively tells the command to stop before sending the message in question, and by definition in such a case, at least one message (i.e. the one that triggered the interactive session) was not sent, so dropping the "all" flag makes sense. > @@ -1968,6 +1979,12 @@ sub process_file { > return 0; > } > > + if ($message_was_sent) { > + $sent_any = 1; > + } else { > + $sent_all = 0; > + } And after processing a single file, we adjust the "any" and "all" counter. The only semi-tricky part of the whole set-up, I presume, is that the "I've sent everything I was asked to send so far" flag needs to be initialized to true, and the "I have sent anything yet" flag to false. When the number of messages we were given is 0, after iterating over the loop 0 times and never calling process_file, do_exit will see that we haven't sent any and we have sent all that were asked to be sent. Is that an error? According to the hard-to-read nested ternary in do_exit, it is: exit($sent_any ? $sent_all ? 0 : 10 : 11); because $sent_any would be 0. I wonder if it can be remedied by a simple reordering to check $sent_all first? I.e. something like (as I do not think anybody can read nested ternary and keep their sanity) this? if ($sent_all) { exit(0); } elsif ($sent_any) { exit(11); } else { exit(10); } > # set up for the next message > if ($thread) { > if ($message_was_sent && > @@ -2187,3 +2204,5 @@ sub body_or_subject_has_nonascii { > } > return 0; > } > + > +do_exit(); Thanks.
Junio C Hamano wrote: > Administrivia: > > * Marking the patch as "v2" is very good to signal that this is an > improved version of a previous effort. It also is very good that > the difference from "v1" is summarized below the three-dash line. > > * When sending such a new iteration, it helps readers and reviewers > to make it a reply to the previous round. You seem to be using > "git send-email" and giving the option > > --in-reply-to=20230323162234.995435-1-oswald.buddenhagen@gmx.de > > would have made this message a reply to the previous one. Not > everybody will remember your previous attempt. You can also use `git send-series` which does that for you, and also sends the `git range-diff` automatically. https://github.com/felipec/git-send-series
diff --git a/git-send-email.perl b/git-send-email.perl index 07f2a0cbea..7587cd2d20 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -254,6 +254,19 @@ sub system_or_die { die $msg if $msg; } +my $sent_any = 0; +my $sent_all = 1; + +sub do_exit { + exit($sent_any ? $sent_all ? 0 : 10 : 11); +} + +sub do_quit { + cleanup_compose_files(); + $sent_all = 0; + do_exit(); +} + sub do_edit { if (!defined($editor)) { $editor = Git::command_oneline('var', 'GIT_EDITOR'); @@ -1172,8 +1185,7 @@ sub validate_address { if (/^d/i) { return undef; } elsif (/^q/i) { - cleanup_compose_files(); - exit(0); + do_quit(); } $address = ask("$to_whom ", default => "", @@ -1593,8 +1605,7 @@ sub send_message { } elsif (/^e/i) { return -1; } elsif (/^q/i) { - cleanup_compose_files(); - exit(0); + do_quit(); } elsif (/^a/i) { $confirm = 'never'; } @@ -1968,6 +1979,12 @@ sub process_file { return 0; } + if ($message_was_sent) { + $sent_any = 1; + } else { + $sent_all = 0; + } + # set up for the next message if ($thread) { if ($message_was_sent && @@ -2187,3 +2204,5 @@ sub body_or_subject_has_nonascii { } return 0; } + +do_exit();
From the perspective of the 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. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- v2: - fix do_quit() not resetting $sent_all --- git-send-email.perl | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)