Re: [PATCH v2 2/4] revision.h: unify "disable_stdin" and "read_from_stdin"
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Jun 17, 2021, 23:44 UTC
- Message-ID
- <YMveZXZRM5vHYRx5@google.com>
- In-Reply-To
- <patch-2.4-d88b2c0410-20210617T105537Z-avarab@gmail.com>
On Thu, Jun 17, 2021 at 12:57:35PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 14 quoted lines
> > 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...
Show 18 quoted lines
> 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]
Show 7 quoted lines
> + /* > + * 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