patchremote: handle negative refspecs in git remote show

> +	/* handle negative refspecs first */
> +	for (tail = &fetch_map; *tail; ) {
> +		ref = *tail;
> +
> +		if (omit_name_by_refspec(ref->name, &states->remote->fetch)) {
> +			string_list_append(&states->skipped, abbrev_branch(ref->name));
> +
> +			/* Matched a negative refspec, so remove this ref from
> +			 * consideration for being a new or tracked ref.
> +			 */
> +			*tail = ref->next;
> +			free(ref->peer_ref);
> +			free(ref);
> +		} else {
> +			tail = &ref->next;
> +		}
> +	}
> +

> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index fff14e13ed43..e19b8d666c73 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -302,6 +302,33 @@ test_expect_success 'show' '
>  	)
>  '
>
> +cat >test/expect <<EOF
> +* remote origin
> +  Fetch URL: $(pwd)/one
> +  Push  URL: $(pwd)/one
> +  HEAD branch: main
> +  Remote branches:
> +    main     skipped
> +    side     tracked
> +    upstream stale (use 'git remote prune' to remove)
> +  Local branches configured for 'git pull':
> +    ahead merges with remote main
> +    main  merges with remote main
> +  Local refs configured for 'git push':
> +    main pushes to main     (local out of date)
> +    main pushes to upstream (create)
> +EOF
> +
> +test_expect_success 'show with negative refspecs' '
> +	test_when_finished "git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main" &&
> +	(
> +		cd test &&
> +		git config --add remote.origin.fetch ^refs/heads/main &&

>
> On Mon, Jun 13, 2022 at 05:32:51PM -0700, Jacob Keller wrote:
> > Fix this by checking negative refspecs inside of get_ref_states. For
> > each ref which matches a negative refspec, copy it into a "skipped" list
> > and remove it from the fetch map. This allows us to show the following
> > output instead:
>
> Seems sensible.
>
> > +     /* handle negative refspecs first */
> > +     for (tail = &fetch_map; *tail; ) {
> > +             ref = *tail;
> > +
> > +             if (omit_name_by_refspec(ref->name, &states->remote->fetch)) {
> > +                     string_list_append(&states->skipped, abbrev_branch(ref->name));
> > +
> > +                     /* Matched a negative refspec, so remove this ref from
> > +                      * consideration for being a new or tracked ref.
> > +                      */
> > +                     *tail = ref->next;
> > +                     free(ref->peer_ref);
> > +                     free(ref);
> > +             } else {
> > +                     tail = &ref->next;
> > +             }
> > +     }
> > +
>
> Not being overly familiar with the "git remote show" code, this
> implementation looks very reasonable to me. If we see a negative
> refspec, we remove it from the fetch_map list and append it to the
> skipped list. Otherwise, we increment our pointer, and continue along
> until we reach the end of the list.
>
> > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> > index fff14e13ed43..e19b8d666c73 100755
> > --- a/t/t5505-remote.sh
> > +++ b/t/t5505-remote.sh
> > @@ -302,6 +302,33 @@ test_expect_success 'show' '
> >       )
> >  '
> >
> > +cat >test/expect <<EOF
> > +* remote origin
> > +  Fetch URL: $(pwd)/one
> > +  Push  URL: $(pwd)/one
> > +  HEAD branch: main
> > +  Remote branches:
> > +    main     skipped
> > +    side     tracked
> > +    upstream stale (use 'git remote prune' to remove)
> > +  Local branches configured for 'git pull':
> > +    ahead merges with remote main
> > +    main  merges with remote main
> > +  Local refs configured for 'git push':
> > +    main pushes to main     (local out of date)
> > +    main pushes to upstream (create)
> > +EOF
> > +
> > +test_expect_success 'show with negative refspecs' '
> > +     test_when_finished "git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main" &&
> > +     (
> > +             cd test &&
> > +             git config --add remote.origin.fetch ^refs/heads/main &&
>
> Doing "git config --unset" outside of the subshell could be avoided by
> ditching the subshell altogether, perhaps with something like:
>
>     test_config -C test remote.origin.fetch ^refs/heads/main &&

>     git -C test remote show origin >actual &&
>     test_cmp test/expect actual
>
> Thanks,
> Taylor

> > > +test_expect_success 'show with negative refspecs' '
> > > +     test_when_finished "git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main" &&
> > > +     (
> > > +             cd test &&
> > > +             git config --add remote.origin.fetch ^refs/heads/main &&
> >
> > Doing "git config --unset" outside of the subshell could be avoided by
> > ditching the subshell altogether, perhaps with something like:
> >
> >     test_config -C test remote.origin.fetch ^refs/heads/main &&
>
> We need "--add" semantics here which test_config doesn't seem to
> support at the moment.

>
> On Mon, Jun 13, 2022 at 06:56:55PM -0700, Jacob Keller wrote:
> > > > +test_expect_success 'show with negative refspecs' '
> > > > +     test_when_finished "git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main" &&
> > > > +     (
> > > > +             cd test &&
> > > > +             git config --add remote.origin.fetch ^refs/heads/main &&
> > >
> > > Doing "git config --unset" outside of the subshell could be avoided by
> > > ditching the subshell altogether, perhaps with something like:
> > >
> > >     test_config -C test remote.origin.fetch ^refs/heads/main &&
> >
> > We need "--add" semantics here which test_config doesn't seem to
> > support at the moment.
>
> Makes sense, thanks for explaining. This patch looks good to me.
>
> Thanks,
> Taylor

>
> On Mon, Jun 13, 2022 at 05:32:51PM -0700, Jacob Keller wrote:
> > Fix this by checking negative refspecs inside of get_ref_states. For
> > each ref which matches a negative refspec, copy it into a "skipped" list
> > and remove it from the fetch map. This allows us to show the following
> > output instead:
>
> Seems sensible.
>
> > +     /* handle negative refspecs first */
> > +     for (tail = &fetch_map; *tail; ) {
> > +             ref = *tail;
> > +
> > +             if (omit_name_by_refspec(ref->name, &states->remote->fetch)) {
> > +                     string_list_append(&states->skipped, abbrev_branch(ref->name));
> > +
> > +                     /* Matched a negative refspec, so remove this ref from
> > +                      * consideration for being a new or tracked ref.
> > +                      */
> > +                     *tail = ref->next;
> > +                     free(ref->peer_ref);
> > +                     free(ref);
> > +             } else {
> > +                     tail = &ref->next;
> > +             }
> > +     }
> > +
>
> Not being overly familiar with the "git remote show" code, this
> implementation looks very reasonable to me. If we see a negative
> refspec, we remove it from the fetch_map list and append it to the
> skipped list. Otherwise, we increment our pointer, and continue along
> until we reach the end of the list.
>

> > +test_expect_success 'show with negative refspecs' '
> > +     test_when_finished "git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main" &&
> > +     (
> > +             cd test &&
> > +             git config --add remote.origin.fetch ^refs/heads/main &&
>
> Doing "git config --unset" outside of the subshell could be avoided by
> ditching the subshell altogether, perhaps with something like:
>
>     test_config -C test remote.origin.fetch ^refs/heads/main &&
>     git -C test remote show origin >actual &&
>     test_cmp test/expect actual
>

> Fix this by checking negative refspecs inside of get_ref_states. For
> each ref which matches a negative refspec, copy it into a "skipped" list
> and remove it from the fetch map. This allows us to show the following
> output instead:
>
>   * 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
>       pr/1   skipped
>       pr/2   skipped
>       pr/3   skipped
>     Local ref configured for 'git push':
>       master pushes to master (fast-forwardable)
>
> By showing the refs as skipped, it helps clarify that these references
> won't actually be fetched. Alternatively, we could simply remove them
> entirely.

> @@ -367,6 +368,24 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
>  			die(_("Could not get fetch map for refspec %s"),
>  				states->remote->fetch.raw[i]);
>  
> +	/* handle negative refspecs first */
> +	for (tail = &fetch_map; *tail; ) {
> +		ref = *tail;
> +
> +		if (omit_name_by_refspec(ref->name, &states->remote->fetch)) {
> +			string_list_append(&states->skipped, abbrev_branch(ref->name));
> +
> +			/* Matched a negative refspec, so remove this ref from
> +			 * consideration for being a new or tracked ref.
> +			 */
> +			*tail = ref->next;
> +			free(ref->peer_ref);
> +			free(ref);
> +		} else {
> +			tail = &ref->next;
> +		}
> +	}

>
> Jacob Keller <jacob.keller@gmail.com> writes:
>
> > Fix this by checking negative refspecs inside of get_ref_states. For
> > each ref which matches a negative refspec, copy it into a "skipped" list
> > and remove it from the fetch map. This allows us to show the following
> > output instead:
> >
> >   * 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
> >       pr/1   skipped
> >       pr/2   skipped
> >       pr/3   skipped
> >     Local ref configured for 'git push':
> >       master pushes to master (fast-forwardable)
> >
> > By showing the refs as skipped, it helps clarify that these references
> > won't actually be fetched. Alternatively, we could simply remove them
> > entirely.
>
> Very sensible.
>
> > @@ -367,6 +368,24 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
> >                       die(_("Could not get fetch map for refspec %s"),
> >                               states->remote->fetch.raw[i]);
> >
> > +     /* handle negative refspecs first */
> > +     for (tail = &fetch_map; *tail; ) {
> > +             ref = *tail;
> > +
> > +             if (omit_name_by_refspec(ref->name, &states->remote->fetch)) {
> > +                     string_list_append(&states->skipped, abbrev_branch(ref->name));
> > +
> > +                     /* Matched a negative refspec, so remove this ref from
> > +                      * consideration for being a new or tracked ref.
> > +                      */
> > +                     *tail = ref->next;
> > +                     free(ref->peer_ref);
> > +                     free(ref);
> > +             } else {
> > +                     tail = &ref->next;
> > +             }
> > +     }
>
>
> 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).
>

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

>         for (ref = fetch_map; ref; ref = ref->next) {
> -               if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))
> +               if (omit_name_by_refspec(ref->name, &states->remote->fetch))
> +                       string_list_append(&states->skipped, abbrev_branch(ref->name));
> +               else if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))
>                         string_list_append(&states->new_refs, abbrev_branch(ref->name));
>                 else
>                         string_list_append(&states->tracked, abbrev_branch(ref->name));
>         }
>
>
> Thanks.

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

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

>   * 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)

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

> >   * 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)