Re: [PATCH 5/9] refspec_ref_prefixes(): clean up refspec_item logic
From: Taylor Blau <hidden>
Date: 2025-03-17 22:00:39
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Thu, Mar 13, 2025 at 01:41:07AM -0400, Jeff King wrote:
quoted
Do you think it'd be worth handling rs->fetch in a switch/case block? At least that would allow us to catch unknown values more easily, though it seems unlikely we'd ever add any :-)....this whole thing is badly named. It is called "fetch", but the only two values are true/false. But for some reason we named them REFSPEC_FETCH and REFSPEC_PUSH. Surely it should be "type" or "operation" or something if we were going to use an enum and switch? I tried to limit the extent of my changes on opinionated matters like this. I almost dropped the patch entirely, but I did enough head-scratching to find that latent bug that I didn't want to lose it. If you want to fix the name and other clarity issues on top, I don't mind, though. ;)
OK... I agree that these are at least named confusingly ;-). We could do something like:
--- 8< ---
diff --git a/refspec.c b/refspec.c
index c6ad515f04..07d401bc71 100644
--- a/refspec.c
+++ b/refspec.c@@ -181,14 +181,14 @@ void refspec_item_clear(struct refspec_item *item) void refspec_init(struct refspec *rs, int fetch) { memset(rs, 0, sizeof(*rs)); - rs->fetch = fetch; + rs->type = fetch ? REFSPEC_FETCH : REFSPEC_PUSH; } void refspec_append(struct refspec *rs, const char *refspec) { struct refspec_item item; - refspec_item_init_or_die(&item, refspec, rs->fetch); + refspec_item_init_or_die(&item, refspec, rs->type); ALLOC_GROW(rs->items, rs->nr + 1, rs->alloc); rs->items[rs->nr] = item;
@@ -227,7 +227,7 @@ void refspec_clear(struct refspec *rs) rs->alloc = 0; rs->nr = 0; - rs->fetch = 0; + rs->type = 0; } int valid_fetch_refspec(const char *fetch_refspec_str)
@@ -249,11 +249,13 @@ void refspec_ref_prefixes(const struct refspec *rs, if (item->negative) continue; - if (rs->fetch == REFSPEC_FETCH) { + switch (rs->type) { + case REFSPEC_FETCH: if (item->exact_sha1) continue; prefix = item->src; - } else { + break; + case REFSPEC_PUSH: /* * Pushes can have an explicit destination like * "foo:bar", or can implicitly use the src for both
@@ -263,6 +265,9 @@ void refspec_ref_prefixes(const struct refspec *rs, prefix = item->dst; else if (item->src && !item->exact_sha1) prefix = item->src; + break; + default: + BUG("unexpected refspec type %d", rs->type); } if (!prefix)
diff --git a/refspec.h b/refspec.h
index 382ba2d5c1..a20cf883e4 100644
--- a/refspec.h
+++ b/refspec.h@@ -32,8 +32,8 @@ struct refspec_item { struct string_list; -#define REFSPEC_INIT_PUSH { .fetch = REFSPEC_PUSH } -#define REFSPEC_INIT_FETCH { .fetch = REFSPEC_FETCH } +#define REFSPEC_INIT_PUSH { .type = REFSPEC_PUSH } +#define REFSPEC_INIT_FETCH { .type = REFSPEC_FETCH } /** * An array of strings can be parsed into a struct refspec using
@@ -45,9 +45,9 @@ struct refspec { int nr; enum { - REFSPEC_PUSH + REFSPEC_PUSH, REFSPEC_FETCH, - } fetch; + } type; }; int refspec_item_init(struct refspec_item *item, const char *refspec, --- >8 ---
, which gives us the "default" case in the switch statement. But this
really is a boolean. I wonder if we should just use 0/1 constants and
leave the field name alone. That would turn something like:
if (rs->fetch == REFSPEC_FETCH) { ... }
into:
if (rs->fetch) { ... }
, which I think is cleaner. There's no reason to rename true/false to
FETCH and PUSH if the field name itself is already 'fetch'.
Thanks,
Taylor