Re: [PATCH v2 2/4] revision.h: unify "disable_stdin" and "read_from_stdin"
From: Emily Shaffer <hidden>
Date: 2021-06-17 23:44:47
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...
quoted hunk ↗ jump to hunk
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