Re: [PATCH v2 2/3] revision: add new parameter to specify all visible refs

From: Jeff King <peff@peff.net>
Date: Nov 5, 2022, 12:46 UTC
Message-ID: <Y2ZbDXYb1jGqSfTj@coredump.intra.peff.net>
In-Reply-To: <3ccd8fc0e35407e5c9ff896165f122b10598e0e2.1667485737.git.ps@pks.im>

On Thu, Nov 03, 2022 at 03:37:32PM +0100, Patrick Steinhardt wrote:

Show 11 quoted lines

> Users can optionally hide refs from remote users in git-upload-pack(1),
> git-receive-pack(1) and others via the `transfer.hideRefs`, but there is
> not an easy way to obtain the list of all visible or hidden refs right
> now. We'll require just that though for a performance improvement in our
> connectivity check.
> 
> Add a new pseudo-ref `--visible-refs=` that pretends as if all refs have
> been added to the command line that are not hidden. The pseudo-ref
> requiers either one of "transfer", "uploadpack" or "receive" as argument
> to pay attention to `transfer.hideRefs`, `uploadpack.hideRefs` or
> `receive.hideRefs`, respectively.

Thanks for re-working this. I think it's a sensible path forward for the problem you're facing.

There were two parts of the implementation that surprised me a bit. These might just be nits, but because this is a new user-facing plumbing option that will be hard to change later, we should make sure it fits in with the adjacent features.

The two things I saw were:

  1. The mutual-exclusion selection between "transfer", "uploadpack",
     and "receive" is not how those options work in their respective
     programs. The "transfer.hideRefs" variable is respected for either
     program. So whichever program you are running, it will always look
     at both "transfer" and itself ("uploadpack" or "receive"). Another
     way to think of it is that the "section" argument to
     parse_hide_refs_config() is not a config section so much as a
     profile. And the profiles are:

       - uploadpack: respect transfer.hideRefs and uploadpack.hideRefs
       - receive: respect transfer.hideRefs and receive.hideRefs

     So it does not make sense to ask for "transfer" as a section; each
     of the modes is already looking at transfer.hideRefs.

     In theory if this option was "look just at $section.hideRefs", it
     could be more flexible to separate out the two. But that makes it
     more of a pain to use (for normal use, you have to specify both
     "transfer" and "receive"). And that is not what your patch
     implements anyway; because it relies on parse_hide_refs_config(),
     it is always adding in "transfer" under the hood (which is why your
     final patch is correct to just say "--visible-refs=receive" without
     specifying "transfer" as well).

  2. Your "--visible-refs" implies "--all", because it's really "all
     refs minus the hidden ones". That's convenient for the intended
     caller, but not as flexible as it could be. If it were instead
     treated the way "--exclude" is, as a modifier for the next
     iteration option, then you do a few extra things:

       a. Combine multiple exclusions in a single iteration:

            git rev-list --exclude-hidden=receive \
	                 --exclude-hidden=upload \
			 --all

          That excludes both types in a single iteration. Whereas if you
	  did:

	    git rev-list --visible-refs=receive \
	                 --visible-refs=upload

	  that will do _two_ iterations, and end up with the union of
	  the sets. Equivalent to:

	    git rev-list --exclude-hidden=receive --all \
	                 --exclude-hidden=upload  --all

       b. Do exclusions on smaller sets than --all:

            git rev-list --exclude-hidden=receive \
	                 --branches

	  which would show just the branches that we'd advertise.

     Now I don't have a particular use case for either of those things.
     But they're plausible things to want in the long run, and they fit
     in nicely with the existing ref-selection scheme of rev-list. They
     do make your call from check_connected() slightly longer, but it is
     pretty negligible. It's "--exclude-hidden=receive --all" instead of
     "--visible-refs=hidden".

So looking at the patch itself, if you wanted to take my suggestions:

> +--visible-refs=[transfer|receive|uploadpack]::
> +	Pretend as if all the refs that have not been hidden via either one of
> +	`transfer.hideRefs`, `receive.hideRefs` or `uploadpack.hideRefs` are
> +	listed on the command line.

This would drop "transfer" as a mode, and explain that the argument is "hide the refs that receive-pack would use", etc.

Likewise, the name would switch and pick up explanation similar to --exclude below about how it affects the next --all, etc.

Show 13 quoted lines

> @@ -1542,11 +1545,13 @@ static int handle_one_ref(const char *path, const struct object_id *oid,
>  			  int flag UNUSED,
>  			  void *cb_data)
>  {
> +	const char *stripped_path = strip_namespace(path);
>  	struct all_refs_cb *cb = cb_data;
>  	struct object *object;
>  
> -	if (ref_excluded(cb->all_revs->ref_excludes, path))
> -	    return 0;
> +	if (ref_excluded(cb->all_revs->ref_excludes, path) ||
> +	    ref_is_hidden(stripped_path, path, &cb->hidden_refs))
> +		return 0;

This would stay the same. We'd still exclude hidden refs during the iteration.

Show 19 quoted lines

> @@ -2759,6 +2772,21 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
>  		parse_list_objects_filter(&revs->filter, arg);
>  	} else if (!strcmp(arg, ("--no-filter"))) {
>  		list_objects_filter_set_no_filter(&revs->filter);
> +	} else if (skip_prefix(arg, "--visible-refs=", &arg)) {
> +		struct all_refs_cb cb;
> +
> +		if (strcmp(arg, "transfer") && strcmp(arg, "receive") &&
> +		    strcmp(arg, "uploadpack"))
> +			die(_("unsupported section for --visible-refs: %s"), arg);
> +
> +		init_all_refs_cb(&cb, revs, *flags);
> +		cb.hidden_refs_section = arg;
> +		git_config(hide_refs_config, &cb);
> +
> +		refs_for_each_ref(refs, handle_one_ref, &cb);
> +
> +		string_list_clear(&cb.hidden_refs, 1);
> +		clear_ref_exclusion(&revs->ref_excludes);

And here we'd do the same git_config() call, but drop the refs_for_each_ref() call. We'd clear the hidden_refs field in all the places that call clear_ref_exclusion() now.

In fact, you could argue that all of this should just be folded into clear_ref_exclusion() and ref_excluded(), since from the perspective of the iterating code, they are all part of the same feature. I don't mind leaving it separate from the perspective of rev-list, though I think if you did so, it would all Just Work for "rev-parse", too (I doubt anybody cares in practice, but it's probably better to keep it consistent).

-Peff