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

[Junio C Hamano <gitster@pobox.com>]

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.