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:
signature.asc
Description: PGP signature
