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
