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

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

[Jacob Keller]

By default, the git remote show command will query data from remotes to
show data about what might be done on a future git fetch. This process
currently does not handle negative refspecs. This can be confusing,
because the show command will list refs as if they would be fetched. For
example if the fetch refspec "^refs/heads/pr/*", it still displays the
following:

  * 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   new (next fetch will store in remotes/jdk19)
      pr/2   new (next fetch will store in remotes/jdk19)
      pr/3   new (next fetch will store in remotes/jdk19)
    Local ref configured for 'git push':
      master pushes to master (fast-forwardable)

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.

Add a new test case to cover this functionality.

Reported-by: Pavel Rappo <pavel.rappo@gmail.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 builtin/remote.c  | 28 ++++++++++++++++++++++++++--
 remote.c          |  2 +-
 remote.h          |  6 ++++++
 t/t5505-remote.sh | 27 +++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index d4b69fe77898..243e60e19bdb 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -344,12 +344,13 @@ static void read_branches(void)
 
 struct ref_states {
 	struct remote *remote;
-	struct string_list new_refs, stale, tracked, heads, push;
+	struct string_list new_refs, skipped, stale, tracked, heads, push;
 	int queried;
 };
 
 #define REF_STATES_INIT { \
 	.new_refs = STRING_LIST_INIT_DUP, \
+	.skipped = STRING_LIST_INIT_DUP, \
 	.stale = STRING_LIST_INIT_DUP, \
 	.tracked = STRING_LIST_INIT_DUP, \
 	.heads = STRING_LIST_INIT_DUP, \
@@ -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;
+		}
+	}
+
 	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));
@@ -383,6 +402,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
 	free_refs(fetch_map);
 
 	string_list_sort(&states->new_refs);
+	string_list_sort(&states->skipped);
 	string_list_sort(&states->tracked);
 	string_list_sort(&states->stale);
 
@@ -941,6 +961,7 @@ static void clear_push_info(void *util, const char *string)
 static void free_remote_ref_states(struct ref_states *states)
 {
 	string_list_clear(&states->new_refs, 0);
+	string_list_clear(&states->skipped, 0);
 	string_list_clear(&states->stale, 1);
 	string_list_clear(&states->tracked, 0);
 	string_list_clear(&states->heads, 0);
@@ -1033,7 +1054,9 @@ static int show_remote_info_item(struct string_list_item *item, void *cb_data)
 		if (string_list_has_string(&states->new_refs, name)) {
 			fmt = _(" new (next fetch will store in remotes/%s)");
 			arg = states->remote->name;
-		} else if (string_list_has_string(&states->tracked, name))
+		} else if (string_list_has_string(&states->skipped, name))
+			arg = _(" skipped");
+		else if (string_list_has_string(&states->tracked, name))
 			arg = _(" tracked");
 		else if (string_list_has_string(&states->stale, name))
 			arg = _(" stale (use 'git remote prune' to remove)");
@@ -1308,6 +1331,7 @@ static int show(int argc, const char **argv)
 		/* remote branch info */
 		info.width = 0;
 		for_each_string_list(&info.states.new_refs, add_remote_to_show_info, &info);
+		for_each_string_list(&info.states.skipped, add_remote_to_show_info, &info);
 		for_each_string_list(&info.states.tracked, add_remote_to_show_info, &info);
 		for_each_string_list(&info.states.stale, add_remote_to_show_info, &info);
 		if (info.list.nr)
diff --git a/remote.c b/remote.c
index 404e1e0a0ddb..7d68b5632bb5 100644
--- a/remote.c
+++ b/remote.c
@@ -804,7 +804,7 @@ static int refspec_match(const struct refspec_item *refspec,
 	return !strcmp(refspec->src, name);
 }
 
-static int omit_name_by_refspec(const char *name, struct refspec *rs)
+int omit_name_by_refspec(const char *name, struct refspec *rs)
 {
 	int i;
 
diff --git a/remote.h b/remote.h
index dd4402436f1f..448675e11259 100644
--- a/remote.h
+++ b/remote.h
@@ -247,6 +247,12 @@ int resolve_remote_symref(struct ref *ref, struct ref *list);
  */
 struct ref *ref_remove_duplicates(struct ref *ref_map);
 
+/*
+ * Check whether a name matches any negative refspec in rs. Returns 1 if the
+ * name matches at least one negative refspec, and 0 otherwise.
+ */
+int omit_name_by_refspec(const char *name, struct refspec *rs);
+
 /*
  * Remove all entries in the input list which match any negative refspec in
  * the refspec 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 &&
+		git remote show origin >output &&
+		test_cmp expect output
+	)
+'
+
 cat >test/expect <<EOF
 * remote origin
   Fetch URL: $(pwd)/one
-- 
2.36.1

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

[Taylor Blau]

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

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

[Jacob Keller]

On Mon, Jun 13, 2022 at 6:03 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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 &&

We need "--add" semantics here which test_config doesn't seem to
support at the moment.

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

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

[Taylor Blau]

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

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

[Jacob Keller]

On Mon, Jun 13, 2022 at 7:26 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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

I ended up digging into this, and have now additionally added some
patches to clean these tests up:

1) drop all the subshells in t5505 where possible
2) modify test_config and test_unconfig to handle options, including
--global, --add, and --fixed-value
3) make test_config use --fixed-value with test_unconfig so that only
the keys with the exact value are removed
4) use test_config more so that we don't have config values from
previous tests kept around

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

[Jacob Keller]

On Mon, Jun 13, 2022 at 6:03 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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.
>

The specific way the loop works is similar to other ref looping code
but it feels a little odd to me. Still, it seems to be the right
approach overall.

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

I still think that removing the subshell is a good idea here. I'll
investigate this.

I also wonder if it would be difficult to enable "--add" semantics for
test_config.

> Thanks,
> Taylor

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

[Junio C Hamano]

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.

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

[Jacob Keller]

On Wed, Jun 15, 2022 at 2:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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).
>

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.

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

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

[Junio C Hamano]

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)

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

[Jacob Keller]

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)

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

[Jacob Keller]

On Thu, Jun 16, 2022 at 3:09 PM Jacob Keller <jacob.keller@gmail.com> wrote:
>
> 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

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