[PATCH v2] send-email: prompt-dependent exit codes

[PATCH v2] send-email: prompt-dependent exit codes

[Oswald Buddenhagen]

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

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();
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v2] send-email: prompt-dependent exit codes

[Junio C Hamano]

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.

Re: [PATCH v2] send-email: prompt-dependent exit codes

[Felipe Contreras]

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

-- 
Felipe Contreras

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

[Oswald Buddenhagen]

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

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();
-- 
2.40.0.152.g15d061e6df

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

[Junio C Hamano]

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.

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

[Oswald Buddenhagen]

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

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

[Junio C Hamano]

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.

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

[Oswald Buddenhagen]

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

[PATCH v4] send-email: prompt-dependent exit codes

[Oswald Buddenhagen]

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 a non-zero exit code.

This is technically a backwards-incompatible behavior change, and
therefore would be safer to make opt-in. However, the damage resulting
from reporting the situation too eagerly is expected to be trivial,
while the damage resulting from callers failing to opt-in (which is
obviously the status quo, and is likely to persist in most cases) is
potentially at least somewhat serious.

For interactive calls from the command line, interactive cancellation is
arguably not really an error, but a human user will be able to interpret
the somewhat unusual exit code in light of their immediately preceding
interactions just fine.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---
v4:
- add tests (which also cover the partial confirmation behavior in the
  first place)
- add docu
- rework commit message again
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>
---
 Documentation/git-send-email.txt |  9 ++++++++
 git-send-email.perl              | 32 ++++++++++++++++++++++++----
 t/t9001-send-email.sh            | 36 ++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 492a82323d..d6ce3a0bd9 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -480,6 +480,15 @@ include::includes/cmd-config-section-all.txt[]
 
 include::config/sendemail.txt[]
 
+
+EXIT STATUS
+-----------
+
+Zero is returned when all specified patches were sent, while one is returned
+when an error occurs. 10 is returned if the user interactively skips sending
+some patches, and 11 if they skip all patches.
+
+
 EXAMPLES
 --------
 Use gmail as the smtp server
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();
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 48bf0af2ee..dce0b1c498 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1187,6 +1187,42 @@ test_expect_success $PREREQ 'confirm does not loop forever' '
 			$patches
 '
 
+test_confirm_many () {
+	clean_fake_sendmail
+	GIT_SEND_EMAIL_NOTTY=1 \
+	git send-email \
+		--confirm=auto \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		-2 <replies
+	echo $? >exit_sts
+	test_cmp expected_sts exit_sts || return 1
+	ls commandline* 2>/dev/null | wc -l > num_mails
+	test_cmp expected_num num_mails || return 1
+}
+
+test_expect_success $PREREQ 'interactively skip none' '
+	(echo y && echo y) > replies &&
+	echo 0 > expected_sts &&
+	echo 2 > expected_num &&
+	test_confirm_many
+'
+
+test_expect_success $PREREQ 'interactively skip some' '
+	(echo n && echo y) > replies &&
+	echo 10 > expected_sts &&
+	echo 1 > expected_num &&
+	test_confirm_many
+'
+
+test_expect_success $PREREQ 'interactively skip all' '
+	(echo n && echo n) > replies &&
+	echo 11 > expected_sts &&
+	echo 0 > expected_num &&
+	test_confirm_many
+'
+
 test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' '
 	clean_fake_sendmail &&
 	rm -fr outdir &&
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v4] send-email: prompt-dependent exit codes

[Junio C Hamano]

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 a non-zero exit code.

I would agree that there should be a way to ask the command to
indicate if some messages were not sent due to end-user request, but
I have to say "From the perspective of a scripted caller" is a gross
over generalization that I would not want to see in a commit log
message of the project I run.  Not all our scripters do not write
the scripts the way you do.

It should not be hard to make this opt-in, and I still think it
should be opt-in.

Thanks.

Re: [PATCH v4] send-email: prompt-dependent exit codes

[Oswald Buddenhagen]

On Wed, Aug 09, 2023 at 12:15:42PM -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 a non-zero exit code.
>
>I would agree that there should be a way to ask the command to
>indicate if some messages were not sent due to end-user request, but
>I have to say "From the perspective of a scripted caller" is a gross
>over generalization that I would not want to see in a commit log
>message of the project I run.
>
this gross error can be fixed by adding "likely" to the sentence.

>It should not be hard to make this opt-in,
>
that doesn't matter.

>and I still think it should be opt-in.
>
and i still think this would significantly reduce the value of the 
change.

the very idea of having to explicitly request that the obviously right 
thing is done is intuitively silly, and shouldn't be seriously 
entertained unless changing existing behavior can plausibly lead to 
serious adverse consequences. the minor nuisance of having to adjust 
wrappers to explicitly accept the most likely unexpected case does not 
qualify as such.

fwiw, other users who noticed this problem most probably preempted it by 
adding appropriate --confirm=* and --suppress-* options. but this 
doesn't fit my use case of a "light" wrapper.

regards

Re: [PATCH v4] send-email: prompt-dependent exit codes

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> fwiw, other users who noticed this problem most probably preempted it
> by adding appropriate --confirm=* and --suppress-* options. but this
> doesn't fit my use case of a "light" wrapper.

I have no problem with giving the command an option to allow it to
report if one or more messages were held back via its exit status.

Others may even want to know which ones among many messages weren't
sent, not just "there were some that weren't sent", to make it
easier to identify which ones to be resent after touching them up.

In any case, I consider such changes (including the "were there any
that weren't sent" bit) optional new features.

Thanks.

Re: [PATCH v4] send-email: prompt-dependent exit codes

[Oswald Buddenhagen]

On Thu, Aug 10, 2023 at 12:56:31PM -0700, Junio C Hamano wrote:
>I have no problem with giving the command an option to allow it to
>report if one or more messages were held back via its exit status.
>
that doesn't give credit to the observation that there is an opportunity 
cost to not doing it automatically. it is much easier to imagine that a 
scripting user simply didn't consider the possibility of an interactive 
cancellation than that they really meant to ignore it. therefore, the 
status quo is very likely more harmful than the fallout of starting to 
more aggressively report the situation.

>Others may even want to know which ones among many messages weren't
>sent, not just "there were some that weren't sent", to make it
>easier to identify which ones to be resent after touching them up.
>
yeah, i've considered that as well.
i think that on the exit status side my solution is adequate - it 
reports in a sufficiently distinctive way that something didn't go quite 
as probably expected.

going further, i'm considering producing a mapping from sha1s to 
message-ids (which i'd persist for automatic chaining of re-rolls). this 
would provide the fine-grained reporting as a side effect.

regards

[PATCH v5] send-email: prompt-dependent exit codes

[Oswald Buddenhagen]

It seems very likely that most scripted callers would want to know when
(some) mails were interactively requested to be not sent, so indicate
this situation with a non-zero exit code. We use 10/11, because these
seem sufficiently distinct from "regular" error codes one would expect.

This is technically a backwards-incompatible behavior change, and
therefore would be safer to make opt-in. However, it is much easier to
imagine that a scripting user simply didn't consider the possibility of
an interactive cancellation, than that they really meant to ignore it.
Also, the damage resulting from reporting the situation too eagerly is
expected to be trivial, while the damage resulting from callers failing
to opt-in (which is obviously the status quo, and is likely to persist
in most cases) is potentially at least somewhat serious. This means that
making it opt-in has an opportunity cost, and I think the trade-off
favors making the new behavior unconditional.

For interactive calls from the command line, interactive cancellation is
arguably not unexpected, but a human user will be able to interpret the
somewhat unusual exit code in light of their immediately preceding
interactions just fine.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---
Proposed content for RelNotes:

 * "git send-email" now reports interactive cancellation via a distinct
   non-zero exit status. Callers which do not consider this situation an
   error need to be adjusted.

---
v5:
- fix whitespace in tests' redirections
- tweak commit message some more
- tweak manual (notably, it now says "1" instead of "one", which is
  linguistically incorrect, but imo less confusing)
- fix inaccuracy in a comment in do_exit()
v4:
- add tests (which also cover the partial confirmation behavior in the
  first place)
- add docu
- rework commit message again
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>
---
 Documentation/git-send-email.txt |  9 ++++++++
 git-send-email.perl              | 32 ++++++++++++++++++++++++----
 t/t9001-send-email.sh            | 36 ++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 492a82323d..766c190ef0 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -480,6 +480,15 @@ include::includes/cmd-config-section-all.txt[]
 
 include::config/sendemail.txt[]
 
+
+EXIT STATUS
+-----------
+
+Zero is returned when all specified patches were sent, while 1 is returned
+when an error occurs. 10 is returned if the user interactively skips sending
+only some patches, and 11 if they skip all patches.
+
+
 EXAMPLES
 --------
 Use gmail as the smtp server
diff --git a/git-send-email.perl b/git-send-email.perl
index affbb88509..2c62de07bc 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 skipped all messages or quit before sending the first one
+		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();
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 48bf0af2ee..64f9c7c154 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1187,6 +1187,42 @@ test_expect_success $PREREQ 'confirm does not loop forever' '
 			$patches
 '
 
+test_confirm_many () {
+	clean_fake_sendmail
+	GIT_SEND_EMAIL_NOTTY=1 \
+	git send-email \
+		--confirm=auto \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		-2 <replies
+	echo $? >exit_sts
+	test_cmp expected_sts exit_sts || return 1
+	ls commandline* 2>/dev/null | wc -l >num_mails
+	test_cmp expected_num num_mails || return 1
+}
+
+test_expect_success $PREREQ 'interactively skip none' '
+	(echo y && echo y) >replies &&
+	echo 0 >expected_sts &&
+	echo 2 >expected_num &&
+	test_confirm_many
+'
+
+test_expect_success $PREREQ 'interactively skip some' '
+	(echo n && echo y) >replies &&
+	echo 10 >expected_sts &&
+	echo 1 >expected_num &&
+	test_confirm_many
+'
+
+test_expect_success $PREREQ 'interactively skip all' '
+	(echo n && echo n) >replies &&
+	echo 11 >expected_sts &&
+	echo 0 >expected_num &&
+	test_confirm_many
+'
+
 test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' '
 	clean_fake_sendmail &&
 	rm -fr outdir &&
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v5] send-email: prompt-dependent exit codes

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> to opt-in (which is obviously the status quo, and is likely to persist
> in most cases) is potentially at least somewhat serious.

What's the weaseling "potentially at least somewhat" phrase about?

Quite honestly, it makes it apparent that the patch is trying to
make a change that cannot be justified and makes it lose all the
remaining credibility.

The users have lived with the current behaviour practically forever
in Git's timescale and not changing the default and letting them
"live with" the status quo cannot be any _serious_ at all.

Will replace what I had kept but out of 'seen', as the changed parts
of the patch are definite improvements, but I do not think I want to
merge it without starting this as an optional feature.

Thanks.

Re: [PATCH v5] send-email: prompt-dependent exit codes

[Oswald Buddenhagen]

On Mon, Aug 21, 2023 at 10:57:05AM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> to opt-in (which is obviously the status quo, and is likely to persist
>> in most cases) is potentially at least somewhat serious.
>
>What's the weaseling "potentially at least somewhat" phrase about?
>
i think that mails not being sent (timely, until somebody notices the 
mistake) is "at least somewhat serious". at least serious enough to 
justify a few people having to make a minor ajustment to their scripts 
to confirm that they really want the old behavior.

regards

Re: [PATCH v5] send-email: prompt-dependent exit codes

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> Proposed content for RelNotes:
>
>  * "git send-email" now reports interactive cancellation via a distinct
>    non-zero exit status. Callers which do not consider this situation an
>    error need to be adjusted.

If we still want to deliberately break folks with this change, we
should not blame users for becoming accustomed to the traditional
behaviour and pretend as if burdening them to change their scripts
is within our rights.  We should be a lot more apologetic in the
backward compatibility notes than what you wrote in the above.

Having said that, quite honestly, I do not think this new behaviour
deserves to be a new default from the day one, with need for an
apology to existing users by the project.  Given that the users have
lived with the current behaviour practically forever in Git's
timescale and that not changing the default and letting them "live
with" the status quo cannot cause any serious problem, I cannot
stand behind such a default change myself.  It should be a new
feature that is opt-in, just like any other new and useful feature..

By the way, I just noticed that the test will not pass on BSD
derived systems.  Something like this need to be squashed in if we
want to proceed further.

--- >8 --- squash --- >8 ---
Subject: [PATCH] SQUASH???

Sending "wc -l" output to a file and expecting that it has a run of
digits with terminating newline and nothing else is prone to break
on BSD derived systems, including macOS.
---
 t/t9001-send-email.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 64f9c7c154..8323783963 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1198,7 +1198,7 @@ test_confirm_many () {
 		-2 <replies
 	echo $? >exit_sts
 	test_cmp expected_sts exit_sts || return 1
-	ls commandline* 2>/dev/null | wc -l >num_mails
+	echo $(ls commandline* 2>/dev/null | wc -l) >num_mails
 	test_cmp expected_num num_mails || return 1
 }
 
-- 
2.42.0-81-g1a190bc14a

Re: [PATCH v5] send-email: prompt-dependent exit codes

[Oswald Buddenhagen]

On Tue, Aug 29, 2023 at 05:46:32PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> Proposed content for RelNotes:
>>
>>  * "git send-email" now reports interactive cancellation via a distinct
>>    non-zero exit status. Callers which do not consider this situation an
>>    error need to be adjusted.
>
>If we still want to deliberately break folks with this change, we
>should not blame users for becoming accustomed to the traditional
>behaviour and pretend as if burdening them to change their scripts
>is within our rights.  We should be a lot more apologetic in the
>backward compatibility notes than what you wrote in the above.
>
i find this deference mind-boggling.

i'm sure you're somewhat cautious because you're on the frontline when 
people complain, and *someone* will _always_ complain when things 
change.  however, it's completely unreasonable to let this dictate one's 
possibilities. there is a trade-off between the cost of change and the 
cost of non-change.

>Given that the users have
>lived with the current behaviour practically forever in Git's
>timescale and that not changing the default and letting them "live
>with" the status quo cannot cause any serious problem, I cannot
>stand behind such a default change myself.
>
the exit code really only matters when there is a wrapper which decides 
significant follow-up actions based on it (that is, doesn't exit right 
afterwards anyway). i don't think many such scripts exist for 
git-send-email. those which do will most likely suppress interaction.
which is why nobody else complained yet. and also why likely nobody will 
complain if we change it. the few people that will notice at all will 
most likely welcome the change, as i would.

>It should be a new feature that is opt-in,
>
do as you like, but i won't spend the time on roughly doubling the size 
of the patch, nor on dealing with this being an opt-in (i don't want a 
hard dep on a new git version in the near term), and i won't contribute 
to sabotaging the discovery of hidden bugs.

>just like any other new and useful feature..
>
not reporting a significant event is a bug.

>Subject: [PATCH] SQUASH???
>
yes, please.

regards