On Wed, Mar 12, 2025 at 05:38:05PM -0400, Taylor Blau wrote:
> 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.
I think a "struct refspec" is plural (despite the name). The singular is
"refspec_item".
And...
> > 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 :-).
...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. ;)
-Peff
