Re: [PATCH 5/9] refspec_ref_prefixes(): clean up refspec_item logic

[Taylor Blau <me@xxxxxxxxxxxx>]

On Thu, Mar 13, 2025 at 01:41:07AM -0400, Jeff King wrote:
> > 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