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

From: Jeff King <peff@peff.net>
Date: Nov 8, 2022, 15:07 UTC
Message-ID: <Y2pwt77e9KP5J3XD@coredump.intra.peff.net>
In-Reply-To: <c7fa6698dbe7b0eb5e2b9c4725e9bdc6cdb22983.1667901510.git.ps@pks.im>

On Tue, Nov 08, 2022 at 11:03:51AM +0100, Patrick Steinhardt wrote:

Show 12 quoted lines

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

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.

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.

-Peff