Re: [PATCH v3 2/8] test-ref-store: parse symbolic flag constants

[Jeff King <peff@peff.net>]

On Thu, Dec 02, 2021 at 06:39:56PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> This lets tests use REF_XXXX constants instead of hardcoded integers. The flag
> names should be separated by a ','.

So much nicer. Thank you for cleaning this up.

One small bug:

> +static unsigned int parse_flags(const char *str, struct flag_definition *defs)
> +{
> +	struct string_list masks = STRING_LIST_INIT_DUP;
> +	int i = 0;
> +	unsigned int result = 0;
> +
> +	if (!strcmp(str, "0"))
> +		return 0;
> +
> +	string_list_split(&masks, str, ',', 64);
> +	for (; i < masks.nr; i++) {
> +		const char *name = masks.items[i].string;
> +		struct flag_definition *def = defs;
> +		int found = 0;
> +		while (def->name) {
> +			if (!strcmp(def->name, name)) {
> +				result |= def->mask;
> +				found = 1;
> +				break;
> +			}
> +			def++;
> +		}

We assume defs ends with a NULL entry here...

> +static struct flag_definition empty_flags[] = {
> +	{ NULL, 0 },
> +};

...and this one does so...

> +static struct flag_definition pack_flags[] = {
> +	FLAG_DEF(PACK_REFS_PRUNE),
> +	FLAG_DEF(PACK_REFS_ALL),
> +};

...but this one does not. So passing an unknown flag will result in us
walking off the end of the list.

> +static struct flag_definition transaction_flags[] = {
> +	FLAG_DEF(REF_NO_DEREF),
> +	FLAG_DEF(REF_FORCE_CREATE_REFLOG),
> +	{ NULL, 0 },
> +};

This one is good. We might want to drop the trailing comma on the NULL
entries, to make it more clear that they have to come last.

The other option would be using ARRAY_SIZE() instead of a NULL
terminator, but of course that has to happen in the caller. Which means
either extra work there, or yet another macro. :)

-Peff