On Mon, Nov 07, 2022 at 09:20:08AM +0100, Patrick Steinhardt wrote: > > 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). > 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. > > 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
