Re: [PATCH v2 2/4] revision.h: unify "disable_stdin" and "read_from_stdin"

[Emily Shaffer <emilyshaffer@google.com>]

On Thu, Jun 17, 2021 at 12:57:35PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> In 8b3dce56508 (Teach --stdin option to "log" family, 2009-11-03) we
> added the "disable_stdin" flag, and then much later in
> a12cbe23ef7 (rev-list: make empty --stdin not an error, 2018-08-22) we
> gained a "read_from_stdin" flag.
> 
> The interaction between these is more subtle than they might appear at
> first sight, as noted in a12cbe23ef7. "read_stdin" is not the inverse
> of "disable_stdin", rather we read stdin if we see the "--stdin"
> option.
> 
> The "read" is intended to understood as "I read it", not "you should
> read it". Let's avoid this confusion by using "consume" and "consumed"
> instead, i.e. a word whose present and past tense isn't the same.

Unfortunately, I still find your disambiguation text to be ambiguous...

> diff --git a/revision.c b/revision.c
> index 8140561b6c..69b3812093 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2741,11 +2741,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  			}
>  
>  			if (!strcmp(arg, "--stdin")) {
> -				if (revs->disable_stdin) {
> +				if (revs->stdin_handling == REV_INFO_STDIN_IGNORE) {
>  					argv[left++] = arg;
>  					continue;
>  				}
> -				if (revs->read_from_stdin++)
> +				if (revs->consumed_stdin_per_option++)
>  					die("--stdin given twice?");
>  				read_revisions_from_stdin(revs, &prune_data);
>  				continue;

Eeh. I know this is not logic introduced by your patch, BUT your patch
does change the semantics of the replacement to ->disable_stdin from a
bool to an enum. There is an implicit assumption here - "if
stdin_handling isn't REV_INFO_STDIN_IGNORE, then it must be
REV_INFO_CONSUME_ON_OPTION." That's true during this patch, but becomes
false in the next patch in this series, and it didn't look to me like
this section of code was changed accordingly.
[snip]
> +	/*
> +	 * Did we read from stdin due to stdin_handling ==
> +	 * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin
> +	 * option?
>  	 */
> -	int read_from_stdin;
> +	int consumed_stdin_per_option;

And indeed, the comment here confirms the implicit assumption made in
the code. Based on this comment, I'd expect the implementation to
explicitly check that stdin_handling == CONSUME_ON_OPTION.

 - Emily