Re: [PATCH v4 4/6] revision: add new parameter to exclude hidden refs

[Patrick Steinhardt <ps@pks.im>]

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

On Tue, Nov 08, 2022 at 10:07:35AM -0500, Jeff King wrote:
> On Tue, Nov 08, 2022 at 11:03:51AM +0100, Patrick Steinhardt wrote:
> 
> > --- a/Documentation/rev-list-options.txt
> > +++ b/Documentation/rev-list-options.txt
> > @@ -195,6 +195,13 @@ respectively, and they must begin with `refs/` when applied to `--glob`
> >  or `--all`. If a trailing '/{asterisk}' is intended, it must be given
> >  explicitly.
> >  
> > +--exclude-hidden=[receive|uploadpack]::
> > +	Do not include refs that have been hidden via either one of
> > +	`receive.hideRefs` or `uploadpack.hideRefs` (see linkgit:git-config[1])
> > +	that the next `--all`, `--branches`, `--tags`, `--remotes` or `--glob`
> > +	would otherwise consider. This option is cleared when seeing one of
> > +	these pseudo-refs.
> 
> OK, so this one drops "transfer", which I think is good. But again, I
> think the explanation here is subtly misleading, because it's not just
> "hidden via those sections", but "hidden as receive-pack or upload-pack
> would, respecting both those sections _and_ transfer".
> 
> I also wondered if we could simplify the explanation a bit by tying it
> into the existing --exclude, and then we don't need that mouthful of
> pseudo-ref options. So together something like:
> 
>   Do not include refs that would be hidden by `receive-pack` or
>   `upload-pack`, by consulting the appropriate `receive.hideRefs` or
>   `uploadpack.hideRefs`, along with `transfer.hideRefs`. Like
>   `--exclude`, this option affects the next pseudo-ref option (`--all`,
>   `--glob`, etc), and is cleared after processing them.

I'll try to come up with something, thanks.

> But then I read the next section in the --exclude docs, which says:
> 
>   The patterns given should not begin with refs/heads, refs/tags, or
>   refs/remotes when applied to --branches, --tags, or --remotes,
>   respectively, and they must begin with refs/ when applied to --glob or
>   --all. If a trailing /* is intended, it must be given explicitly.
> 
> Yikes. So --all is going to process "refs/heads/foo", but --branches
> will see just "foo". And that means that:
> 
>   git rev-list --exclude-hidden=receive --branches
> 
> will not work! Because receive.hideRefs is naming fully qualified refs,
> but we'll pass unqualified ones to ref_is_hidden(). Even though that
> function takes a "full" refname argument, I think that has to do with
> namespaces.

Oh, good catch. I noticed that paragraph before, but somehow it didn't
click.

> I'm sure this _could_ be made to work, but I wonder if it is worth the
> trouble. If it's not going to work, though, I think we'd want to detect
> the situation and complain, at least for now. And likewise the
> documentation needs to make clear it only works with --all and --glob.
> 
> Sorry to have misled in my initial suggestion to turn --visible-refs
> into --exclude-hidden. However, I do still stand by that suggestion.
> Even if we don't make it work with "--branches" now, the user-visible
> framework is still there, so it becomes a matter of extending the
> implementation later, rather than re-designing the options.

No worries, and I agree that it's ultimately still the better route to
use `--exclude-hidden=`. The things that don't work right now just go to
show that there is still some potential here and that it's overall the
more flexible design. And as you say, this is something we can build on
at a later point if any such usecases come up.

Patrick

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