Re: [PATCH 2/4] missing: support rejecting --missing=print

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 18, 2024, 21:47 UTC
Message-ID: <xmqqfrvipcm9.fsf@gitster.g>
In-Reply-To: <20240418184043.2900955-3-christian.couder@gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 27 quoted lines

> `git pack-objects` supports the `--missing=<missing-action>` option in
> the same way as `git rev-list` except when '<missing-action>' is
> "print", which `git pack-objects` doesn't support.
>
> As we want to refactor `git pack-objects` to use the same code from
> "missing.{c,h}" as `git rev-list` for the `--missing=...` feature, let's
> make it possible for that code to reject `--missing=print`.
>
> `git pack-objects` will then use that code in a following commit.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/rev-list.c | 2 +-
>  missing.c          | 4 ++--
>  missing.h          | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index f71cc5ebe1..a712a6fd62 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -539,7 +539,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  			int res;
>  			if (revs.exclude_promisor_objects)
>  				die(_("options '%s' and '%s' cannot be used together"), "--exclude-promisor-objects", "--missing");
> -			res = parse_missing_action_value(arg);
> +			res = parse_missing_action_value(arg, 1);

Hmph, this smells like a horribly unscalable design, as we make the vocabulary of missing-action richer, you'd end up piling on "this choice is allowed in this call" parameters, wouldn't you? The first person who adds such an ad-hoc parameter would say "hey, what's just one extra parameter print_ok between friends", but the next person would say the same thing for their new choice and adds frotz_ok, and we'd be in chaos.

Rather, shouldn't the _caller_ decide if the parsed value is something it does not like and barf?

Alternatively, add a _single_ "reject" bitset and do something like

	int parse_missing_action_value(const char *value, unsigned reject)
	{
		...
		if (!(reject & (1<<MA_ERROR) && !strcmp(value, "error")))
			return MA_ERROR;
		if (!(reject & (1<<MA_PRINT) && !strcmp(value, "print")))
			return MA_PRINT;
		...

which would scale better (but still my preference is to have the caller deal with only the values it recognises---do not make the caller say "if (res >= 0 && res != MA_PRINT)" as that will not scale when new choices that are accepted elsewhere are added.