git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Taylor Blau" <me@ttaylorr.com>
Subject: Re: [PATCH v2 2/3] revision: add new parameter to specify all visible refs
Date: Sat, 5 Nov 2022 08:46:05 -0400	[thread overview]
Message-ID: <Y2ZbDXYb1jGqSfTj@coredump.intra.peff.net> (raw)
In-Reply-To: <3ccd8fc0e35407e5c9ff896165f122b10598e0e2.1667485737.git.ps@pks.im>

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

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

So looking at the patch itself, if you wanted to take my suggestions:

> +--visible-refs=[transfer|receive|uploadpack]::
> +	Pretend as if all the refs that have not been hidden via either one of
> +	`transfer.hideRefs`, `receive.hideRefs` or `uploadpack.hideRefs` are
> +	listed on the command line.

This would drop "transfer" as a mode, and explain that the argument is
"hide the refs that receive-pack would use", etc.

Likewise, the name would switch and pick up explanation similar to
--exclude below about how it affects the next --all, etc.

> @@ -1542,11 +1545,13 @@ static int handle_one_ref(const char *path, const struct object_id *oid,
>  			  int flag UNUSED,
>  			  void *cb_data)
>  {
> +	const char *stripped_path = strip_namespace(path);
>  	struct all_refs_cb *cb = cb_data;
>  	struct object *object;
>  
> -	if (ref_excluded(cb->all_revs->ref_excludes, path))
> -	    return 0;
> +	if (ref_excluded(cb->all_revs->ref_excludes, path) ||
> +	    ref_is_hidden(stripped_path, path, &cb->hidden_refs))
> +		return 0;

This would stay the same. We'd still exclude hidden refs during the
iteration.

> @@ -2759,6 +2772,21 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
>  		parse_list_objects_filter(&revs->filter, arg);
>  	} else if (!strcmp(arg, ("--no-filter"))) {
>  		list_objects_filter_set_no_filter(&revs->filter);
> +	} else if (skip_prefix(arg, "--visible-refs=", &arg)) {
> +		struct all_refs_cb cb;
> +
> +		if (strcmp(arg, "transfer") && strcmp(arg, "receive") &&
> +		    strcmp(arg, "uploadpack"))
> +			die(_("unsupported section for --visible-refs: %s"), arg);
> +
> +		init_all_refs_cb(&cb, revs, *flags);
> +		cb.hidden_refs_section = arg;
> +		git_config(hide_refs_config, &cb);
> +
> +		refs_for_each_ref(refs, handle_one_ref, &cb);
> +
> +		string_list_clear(&cb.hidden_refs, 1);
> +		clear_ref_exclusion(&revs->ref_excludes);

And here we'd do the same git_config() call, but drop the
refs_for_each_ref() call. We'd clear the hidden_refs field in all the
places that call clear_ref_exclusion() now.

In fact, you could argue that all of this should just be folded into
clear_ref_exclusion() and ref_excluded(), since from the perspective of
the iterating code, they are all part of the same feature. I don't mind
leaving it separate from the perspective of rev-list, though I think
if you did so, it would all Just Work for "rev-parse", too (I doubt
anybody cares in practice, but it's probably better to keep it
consistent).

-Peff

  reply	other threads:[~2022-11-05 12:46 UTC|newest]

Thread overview: 88+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-28 14:42 [PATCH 0/2] receive-pack: use advertised reference tips to inform connectivity check Patrick Steinhardt
2022-10-28 14:42 ` [PATCH 1/2] connected: allow supplying different view of reachable objects Patrick Steinhardt
2022-10-28 14:54   ` Ævar Arnfjörð Bjarmason
2022-10-28 18:12   ` Junio C Hamano
2022-10-30 18:49     ` Taylor Blau
2022-10-31 13:10     ` Patrick Steinhardt
2022-11-01  1:16       ` Taylor Blau
2022-10-28 14:42 ` [PATCH 2/2] receive-pack: use advertised reference tips to inform connectivity check Patrick Steinhardt
2022-10-28 15:01   ` Ævar Arnfjörð Bjarmason
2022-10-31 14:21     ` Patrick Steinhardt
2022-10-31 15:36       ` Ævar Arnfjörð Bjarmason
2022-10-30 19:09   ` Taylor Blau
2022-10-31 14:45     ` Patrick Steinhardt
2022-11-01  1:28       ` Taylor Blau
2022-11-01  7:20         ` Patrick Steinhardt
2022-11-01 11:53           ` Patrick Steinhardt
2022-11-02  1:05             ` Taylor Blau
2022-11-01  8:28       ` Jeff King
2022-10-28 16:40 ` [PATCH 0/2] " Junio C Hamano
2022-11-01  1:30 ` Taylor Blau
2022-11-01  9:00 ` Jeff King
2022-11-01 11:49   ` Patrick Steinhardt
2022-11-03 14:37 ` [PATCH v2 0/3] receive-pack: only use visible refs for " Patrick Steinhardt
2022-11-03 14:37   ` [PATCH v2 1/3] refs: get rid of global list of hidden refs Patrick Steinhardt
2022-11-03 14:37   ` [PATCH v2 2/3] revision: add new parameter to specify all visible refs Patrick Steinhardt
2022-11-05 12:46     ` Jeff King [this message]
2022-11-07  8:20       ` Patrick Steinhardt
2022-11-08 14:32         ` Jeff King
2022-11-05 12:55     ` Jeff King
2022-11-03 14:37   ` [PATCH v2 3/3] receive-pack: only use visible refs for connectivity check Patrick Steinhardt
2022-11-05  0:40   ` [PATCH v2 0/3] " Taylor Blau
2022-11-05 12:55     ` Jeff King
2022-11-05 12:52   ` Jeff King
2022-11-07 12:16 ` [PATCH v3 0/6] " Patrick Steinhardt
2022-11-07 12:16   ` [PATCH v3 1/6] refs: get rid of global list of hidden refs Patrick Steinhardt
2022-11-07 12:16   ` [PATCH v3 2/6] revision: move together exclusion-related functions Patrick Steinhardt
2022-11-07 12:16   ` [PATCH v3 3/6] revision: introduce struct to handle exclusions Patrick Steinhardt
2022-11-07 12:51     ` Ævar Arnfjörð Bjarmason
2022-11-08  9:11       ` Patrick Steinhardt
2022-11-07 12:16   ` [PATCH v3 4/6] revision: add new parameter to exclude hidden refs Patrick Steinhardt
2022-11-07 13:34     ` Ævar Arnfjörð Bjarmason
2022-11-07 17:07       ` Ævar Arnfjörð Bjarmason
2022-11-08  9:48         ` Patrick Steinhardt
2022-11-08  9:22       ` Patrick Steinhardt
2022-11-08  0:57     ` Taylor Blau
2022-11-08  8:16       ` Patrick Steinhardt
2022-11-08 14:42         ` Jeff King
2022-11-07 12:16   ` [PATCH v3 5/6] revparse: add `--exclude-hidden=` option Patrick Steinhardt
2022-11-08 14:44     ` Jeff King
2022-11-07 12:16   ` [PATCH v3 6/6] receive-pack: only use visible refs for connectivity check Patrick Steinhardt
2022-11-08  0:59   ` [PATCH v3 0/6] " Taylor Blau
2022-11-08 10:03 ` [PATCH v4 " Patrick Steinhardt
2022-11-08 10:03   ` [PATCH v4 1/6] refs: get rid of global list of hidden refs Patrick Steinhardt
2022-11-08 13:36     ` Ævar Arnfjörð Bjarmason
2022-11-08 14:49       ` Patrick Steinhardt
2022-11-08 14:51     ` Jeff King
2022-11-08 10:03   ` [PATCH v4 2/6] revision: move together exclusion-related functions Patrick Steinhardt
2022-11-08 10:03   ` [PATCH v4 3/6] revision: introduce struct to handle exclusions Patrick Steinhardt
2022-11-08 10:03   ` [PATCH v4 4/6] revision: add new parameter to exclude hidden refs Patrick Steinhardt
2022-11-08 15:07     ` Jeff King
2022-11-08 21:13       ` Taylor Blau
2022-11-11  5:48       ` Patrick Steinhardt
2022-11-08 10:03   ` [PATCH v4 5/6] rev-parse: add `--exclude-hidden=` option Patrick Steinhardt
2022-11-08 10:04   ` [PATCH v4 6/6] receive-pack: only use visible refs for connectivity check Patrick Steinhardt
2022-11-11  6:49 ` [PATCH v5 0/7] " Patrick Steinhardt
2022-11-11  6:49   ` [PATCH v5 1/7] refs: fix memory leak when parsing hideRefs config Patrick Steinhardt
2022-11-11  6:49   ` [PATCH v5 2/7] refs: get rid of global list of hidden refs Patrick Steinhardt
2022-11-11  6:50   ` [PATCH v5 3/7] revision: move together exclusion-related functions Patrick Steinhardt
2022-11-11  6:50   ` [PATCH v5 4/7] revision: introduce struct to handle exclusions Patrick Steinhardt
2022-11-11  6:50   ` [PATCH v5 5/7] revision: add new parameter to exclude hidden refs Patrick Steinhardt
2022-11-11  6:50   ` [PATCH v5 6/7] rev-parse: add `--exclude-hidden=` option Patrick Steinhardt
2022-11-11  6:50   ` [PATCH v5 7/7] receive-pack: only use visible refs for connectivity check Patrick Steinhardt
2022-11-11 22:18   ` [PATCH v5 0/7] " Taylor Blau
2022-11-15 17:26     ` Jeff King
2022-11-16 21:22       ` Taylor Blau
2022-11-16 22:04         ` Jeff King
2022-11-16 22:33           ` Taylor Blau
2022-11-17  5:45             ` Patrick Steinhardt
2022-11-17  5:46 ` [PATCH v6 " Patrick Steinhardt
2022-11-17  5:46   ` [PATCH v6 1/7] refs: fix memory leak when parsing hideRefs config Patrick Steinhardt
2022-11-17  5:46   ` [PATCH v6 2/7] refs: get rid of global list of hidden refs Patrick Steinhardt
2022-11-17  5:46   ` [PATCH v6 3/7] revision: move together exclusion-related functions Patrick Steinhardt
2022-11-17  5:46   ` [PATCH v6 4/7] revision: introduce struct to handle exclusions Patrick Steinhardt
2022-11-17  5:46   ` [PATCH v6 5/7] revision: add new parameter to exclude hidden refs Patrick Steinhardt
2022-11-17  5:47   ` [PATCH v6 6/7] rev-parse: add `--exclude-hidden=` option Patrick Steinhardt
2022-11-17  5:47   ` [PATCH v6 7/7] receive-pack: only use visible refs for connectivity check Patrick Steinhardt
2022-11-17 15:03   ` [PATCH v6 0/7] " Jeff King
2022-11-17 21:24     ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y2ZbDXYb1jGqSfTj@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --cc=ps@pks.im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).