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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 5567 bytes --]

On Sat, Nov 05, 2022 at 08:46:05AM -0400, Jeff King wrote:
> 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).

Yup, I'm aware of this. And as you say, the current implementation
already handles this alright for both `receive` and `uploadpack` as we
rely on `parse_hide_refs_config()`, which knows to look at both
`transfer.hideRefs` and `$section.hideRefs`. But I don't see a reason
why we shouldn't allow users to ask "What is the set of hidden refs that
are shared by `uploadpack` and `receive`?", which is exactly what
`--visible-refs=transfer` does.

The implementation is not really explicit about this as we cheat a
little bit here by passing "transfer" as a section to the parsing
function. So what it does right now is to basically check for the same
section twice, once via the hard-coded "transfer.hideRefs" and once for
the "$section.hideRefs" with `section == "transfer"`. But I didn't see
much of a point in making this more explicit.

I might update the commit message and/or documentation though to point
this out.

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

Fair enough. I guess that the usecase where you e.g. only hide a subset
of branches via `hideRefs` is going to be rare, so in most cases you
don't gain much by modelling this so that you can `--exclude-hidden
--branches`. But as you rightfully point out, modelling it that way fits
neatly with the existing `--exclude` switch and is overall more
flexible. So there's not much of a reason to not do so.

I'll send a v3 and include your suggestion, thanks.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]