Re: [PATCH v2 2/3] ref-filter: add new "describe" atom

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 15, 2023, 18:56 UTC
Message-ID: <xmqq351pm2ai.fsf@gitster.g>
In-Reply-To: <ZLLkZ4Vx2quwWwRz@five231003>

Kousik Sanagavarapu <five231003@gmail.com> writes:

Show 9 quoted lines

> conveys it better or is it too much unnecessary stuff to and should we
> just do
>
> 	struct {
> 		const char **describe_args;
> 	} describe;
>
> leaving the describe_opts array as is and changing the how the switch is
> written.

I think this struct can be replaced with a single

	const char **describe_args;

and then

Show 12 quoted lines

>> > +static int describe_atom_parser(struct ref_format *format UNUSED,
>> > +				struct used_atom *atom,
>> > +				const char *arg, struct strbuf *err)
>> > +{
>> > +	const char *describe_opts[] = {
>> > +		"",
>> > +		"tags",
>> > +		"abbrev",
>> > +		"match",
>> > +		"exclude",
>> > +		NULL
>> > +	};

this array can simply go away.  Then you can

Show 12 quoted lines

>> > +	struct strvec args = STRVEC_INIT;
>> > +	for (;;) {
>> > +		int found = 0;
>> > +		const char *argval;
>> > +		size_t arglen = 0;
>> > +		int optval = 0;
>> > +		int opt;
>> > +
>> > +		if (!arg)
>> > +			break;
>> > +
>> > +		for (opt = D_BARE; !found && describe_opts[opt]; opt++) {

rewrite this "for" loop plus the "switch" inside to an if/else if/else cascade:

		if (match_atom_bool_arg(arg, "tags", &arg, &optval)) {
			... do "tags" thing ...
		} else if (match_atom_arg_value(arg, "abbrev", ...)) {
			... do "abbrev" thing ...
		} else if ...

That way, you do not need any enum anywhere and there is no reason to have desribe_opts[] array, either.