Re: [PATCH v3 2/4] revision.h: refactor "disable_stdin" and "read_from_stdin"

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 8, 2021, 22:10 UTC
Message-ID: <xmqqwnq0a24b.fsf@gitster.g>
In-Reply-To: <patch-2.4-6f69644b808-20210621T150651Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 9 quoted lines

> Change the two "disable_stdin" and "read_from_stdin" flags to an enum,
> in preparation for a subsequent commit adding more flags.
>
> 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

"I've read it already" would have been easier to understand.

Show 12 quoted lines

>  			if (!strcmp(arg, "--stdin")) {
> -				if (revs->disable_stdin) {
> +				switch (revs->stdin_handling) {
> +				case REV_INFO_STDIN_IGNORE:
>  					argv[left++] = arg;
>  					continue;
> +				case REV_INFO_STDIN_CONSUME_ON_OPTION:
> +					if (revs->consumed_stdin++)
> +						die("--stdin given twice?");
> +					read_revisions_from_stdin(revs, &prune_data);
> +					continue;
>  				}

More importantly, consumed_stdin will start at zero and if it is found to be non-zero here, it is an immediate error, hence ...

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;

... it does not have to be a full integer. Just a single-bit would do.

But none of the above is grave enough to require an update.