Re: [PATCH 5/9] refspec_ref_prefixes(): clean up refspec_item logic
From: Taylor Blau <hidden>
Date: 2025-03-12 21:38:07
On Sat, Mar 08, 2025 at 10:07:06PM -0500, Jeff King wrote:
The point of refspec_ref_prefixes() is to look over the set of refspecs and set up an appropriate list of "ref-prefix" strings to send to the server.
While we're cleaning things up, I wonder if it is worth (slightly)
renaming this function to something more descriptive, like:
refspecs_to_ref_prefixes()
, where we pluralize "refspec" and add "to" to make it clear that we're
converting from one to the other.
Of course this is all completely academic. We have still not implemented a v2 push protocol, so even though we do call this function for pushes, we'd never actually send these ref-prefix lines. However, given the effort I spent to figure out what was going on here, and the overlapping exact_sha1 checks, I'd like to rewrite this to preemptively fix the bug, and hopefully make it less confusing.
All makes sense.
quoted hunk ↗ jump to hunk
This splits the "if" at the top-level into fetch vs push, and then each handles exact_sha1 appropriately itself. The check for negative refspecs remains outside of either (there is no protocol support for them, so we never send them to the server, but rather use them only to reduce the advertisement we receive). The resulting behavior should be identical for fetches, but hopefully sets us up better for a potential future v2 push. Signed-off-by: Jeff King <redacted> --- This could be dropped without affecting the rest of the series if it's too churn-y. refspec.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)diff --git a/refspec.c b/refspec.c index 4cb80b5208..c6ad515f04 100644 --- a/refspec.c +++ b/refspec.c@@ -246,14 +246,24 @@ void refspec_ref_prefixes(const struct refspec *rs, const struct refspec_item *item = &rs->items[i]; const char *prefix = NULL; - if (item->exact_sha1 || item->negative) + if (item->negative) continue; - if (rs->fetch == REFSPEC_FETCH) - prefix = item->src; - else if (item->dst) - prefix = item->dst; - else if (item->src && !item->exact_sha1) + + if (rs->fetch == REFSPEC_FETCH) {
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 :-).
+ if (item->exact_sha1)
+ continue;
prefix = item->src;
+ } else {
+ /*
+ * Pushes can have an explicit destination like
+ * "foo:bar", or can implicitly use the src for both
+ * ("foo" is the same as "foo:foo").
+ */
+ if (item->dst)
+ prefix = item->dst;
+ else if (item->src && !item->exact_sha1)
+ prefix = item->src;
+ }All makes sense. Thanks, Taylor