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

> On Thu, Nov 03, 2022 at 03:37:32PM +0100, Patrick Steinhardt wrote:
> 
> > 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".