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

[Taylor Blau <me@ttaylorr.com>]

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.

> 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 :-).

> +			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