Re: [PATCH 5/9] refspec_ref_prefixes(): clean up refspec_item logic
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Mar 12, 2025, 21:38 UTC
- Message-ID
- <Z9H+vWHFkATWNLxt@nand.local>
- In-Reply-To
- <20250309030706.GE2334191@coredump.intra.peff.net>
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.
Show 7 quoted lines
> 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.
Show 35 quoted lines
> 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 <peff@peff.net>
> ---
> 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 :-).
Show 14 quoted lines
> + 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