Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 20, 2023, 05:21 UTC
Message-ID: <xmqqcz0n40pu.fsf@gitster.g>
In-Reply-To: <xmqqjzuv5vvg.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 13 quoted lines

>> +static int match_atom_arg_value(const char *to_parse, const char *candidate,
>> +				const char **end, const char **valuestart,
>> +				size_t *valuelen)
>> +{
>> +	const char *atom;
>> +
>> +	if (!(skip_prefix(to_parse, candidate, &atom)))
>> +		return 0;
>> +	if (valuestart) {
>
> As far as I saw, no callers pass NULL to valuestart.  Getting rid of
> this if() statement and always entering its body would clarify what
> is going on, I think.

Specifically, ...

Show 21 quoted lines

>> +		if (*atom == '=') {
>> +			*valuestart = atom + 1;
>> +			*valuelen = strcspn(*valuestart, ",\0");
>> +			atom = *valuestart + *valuelen;
>> +		} else {
>> +			if (*atom != ',' && *atom != '\0')
>> +				return 0;
>> +			*valuestart = NULL;
>> +			*valuelen = 0;
>> +		}
>> +	}
>> +	if (*atom == ',') {
>> +		*end = atom + 1;
>> +		return 1;
>> +	}
>> +	if (*atom == '\0') {
>> +		*end = atom;
>> +		return 1;
>> +	}
>> +	return 0;
>> +}

... I think the body of the function would become easier to read if written like so:

	if (!skip_prefix(to_parse, candidate, &atom))
		return 0; /* definitely not "candidate" */

	if (*atom == '=') {
		/* we just saw "candidate=" */
		*valuestart = atom + 1;
                atom = strchrnul(*valuestart, ',');
		*valuelen = atom - *valuestart;
	} else if (*atom != ',' && *atom != '\0') {
        	 /* key begins with "candidate" but has more chars */
		return 0;
	} else {
        	/* just "candidate" without "=val" */
		*valuestart = NULL;
		*valuelen = 0;
	}

        /* atom points at either the ',' or NUL after this key[=val] */
	if (*atom == ',')
		atom++;
	else if (*atom)
		BUG("should not happen");

	*end = atom;
	return 1;

as it is clear that *valuestart, *valuelen, and *end are not touched when the function returns 0 and they are all filled when the function returns 1.

Also, avoid passing ",\0" to strcspn(); its effect is exactly the same as passing ",", and at that point you are better off using strchnul().

Thanks.