Re: [PATCH v3 2/8] test-ref-store: parse symbolic flag constants
- From
Jeff King <peff@peff.net>
- Date
- Dec 3, 2021, 06:22 UTC
- Message-ID
- <Yam3iHj1a48X+psK@coredump.intra.peff.net>
- In-Reply-To
- <3cdebd2dbcad2f6d428d88846569d6563249dad8.1638470403.git.gitgitgadget@gmail.com>
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:
Show 22 quoted lines
> +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.
Show 5 quoted lines
> +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