diff mbox series

[v3] send-email: prompt-dependent exit codes

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

Commit Message

Oswald Buddenhagen Aug. 7, 2023, 4:58 p.m. UTC
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(-)

Comments

Junio C Hamano Aug. 7, 2023, 6:55 p.m. UTC | #1
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.
Oswald Buddenhagen Aug. 8, 2023, 10:55 a.m. UTC | #2
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
Junio C Hamano Aug. 8, 2023, 4:08 p.m. UTC | #3
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.
Oswald Buddenhagen Aug. 8, 2023, 7:11 p.m. UTC | #4
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 mbox series

Patch

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();