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

[Junio C Hamano <gitster@pobox.com>]

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.