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

From: Jeff King <peff@peff.net>
Date: Nov 8, 2022, 14:32 UTC
Message-ID: <Y2pomzGndfhggXWz@coredump.intra.peff.net>
In-Reply-To: <Y2i/uJPm5KxqAdkE@ncase>

On Mon, Nov 07, 2022 at 09:20:08AM +0100, Patrick Steinhardt wrote:

Show 11 quoted lines

> >   1. The mutual-exclusion selection between "transfer", "uploadpack",
> >      and "receive" is not how those options work in their respective
> > [...]
> 
> 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.

OK. I don't have a real problem with having that mode, as long as it is documented accurately. I do think, though, that it's not that likely to be useful on its own (even less so than the hypotheticals I gave! ;) ), and it would be easy to add later in a backwards-compatible way. So my instinct would be to leave it out for now, but I don't think it's hurting too much as-is (again, with a correct explanation in the documentation).

Show 6 quoted lines

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

Yeah, I agree the implementation works OK here. It does a duplicate string-comparison for the section, but the important thing is that it doesn't add each entry to the list twice.

Show 13 quoted lines

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

Thanks. I agree these aren't incredibly likely cases to come up in practice. But unlike the "transfer" thing, it would be very hard to switch techniques later without adding an awkward almost-the-same option.

-Peff