Re: [PATCH] remote: handle negative refspecs in git remote show

From: Jacob Keller <jacob.keller@gmail.com>
Date: Jun 16, 2022, 22:33 UTC
Message-ID: <CA+P7+xrxskcnoARjc5fL6zKys312B8XdQezrT7wHuN3wPaP-sQ@mail.gmail.com>
In-Reply-To: <CA+P7+xrx-Cec0mUJvDtWn0Fxu5vLdVkTbteWKiZyouH=64pWvA@mail.gmail.com>

On Thu, Jun 16, 2022 at 3:09 PM Jacob Keller <jacob.keller@gmail.com> wrote:

Show 45 quoted lines

>
> On Thu, Jun 16, 2022 at 2:52 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Jacob Keller <jacob.keller@gmail.com> writes:
> >
> > >> This is somewhat curious.  Do we really need to destroy the
> > >> fetch_map like the above?  I know by removing skipped items from the
> > >> list, the existing loop (below) can stop having to worry about them,
> > >> but the caller of get_ref_states() may later want to iterate over
> > >> the full fetch_map for other reasons (even if the current one does
> > >> not, a future version of the caller may have a reason to do so that
> > >> we do not know right now yet).
> > >>
> > >
> > > Good point. I'll fix this. I think we can just move the
> > > omit_name_by_refspec into the other loop.
> > >
> > >> > +
> > >> >       for (ref = fetch_map; ref; ref = ref->next) {
> > >> >               if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))
> > >> >                       string_list_append(&states->new_refs, abbrev_branch(ref->name));
> > >>
> > >> IOW, is adding a new condition to this existing loop insufficient?
> > >>
> > >
> > > The tricky part here is that we don't have a simple check, and we're
> > > currently iterating over all of the refspecs each time. But we have to
> > > do that regardless so I think this makes sense. Will fix.
> >
> > Another thing that worries me is that get_stale_heads() will not see
> > the filtered refs with your original implementation, because you cull
> > them from the fetch_map in the extra loop upfront.
> >
>
> I think the new implementation fixed this, but I'll see about adding a test!
>
> > I do not know offhand what its effect would be, but it probably is
> > worth testing.  In your original scenario, if we locally have
> > refs/remotes/jdk19/old and refs/remotes/jdk19/pr/1 (perhaps obtained
> > before we configured ^refs/pr/* negative refspec), we'd want to see
> > that pr/1 exists here but will not be updated.
> >
>
> Yea, I will see if I can check that.
>

Ok so this raises another issue: get_stale_heads currently never treats a refname as stale if it matches a negative refspec. The function first queries the name against all refspecs via query_refspecs_mutiple. It only considers a ref stale if it matches at least one refspec but no longer exists in the remote.

The problem is that query_refspecs_multple returns no matches if the refname matches any negative refspec. Thus, all refs matching a negative refspec will be ignored by get_stale_heads.

The query_refspecs_multiple is only used by get_stale_heads_cb, so we could change its implementation possibly, but that would make it not quite match the name. I'll try to think of a better solution.

Thanks, Jake

Show 12 quoted lines

> >   * remote jdk19
> >     Fetch URL: git@github.com:openjdk/jdk19.git
> >     Push  URL: git@github.com:openjdk/jdk19.git
> >     HEAD branch: master
> >     Remote branches:
> >       master tracked
> >       old    stale
> >       pr/1   stale
> >       pr/2   skipped
> >       pr/3   skipped
> >     Local ref configured for 'git push':
> >       master pushes to master (fast-forwardable)