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@xxxxxxxx>
> ---
> 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
