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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Mon, 07 Nov 2022 14:34:45 +0100

On Mon, Nov 07 2022, Patrick Steinhardt wrote:

> +--exclude-hidden=[transfer|receive|uploadpack]::
> +	Do not include refs that have been hidden via either one of
> +	`transfer.hideRefs`, `receive.hideRefs` or `uploadpack.hideRefs` that

Maybe worth adding "(see linkgit:git-config[1]) after listing the config
variables.

>  int ref_excluded(const struct ref_exclusions *exclusions, const char *path)
>  {
> +	const char *stripped_path = strip_namespace(path);
>  	struct string_list_item *item;
> +

nit: stray whitespace in otherwise "clean" commit, but the post-image looks nicer, so...

>  void init_ref_exclusions(struct ref_exclusions *exclusions)
>  {
>  	string_list_init_dup(&exclusions->excluded_refs);
> +	string_list_init_dup(&exclusions->hidden_refs);
>  }

Per my comment on 3/6 we wouldn't need this when using the macro as a
source of truth.

>  void clear_ref_exclusions(struct ref_exclusions *exclusions)
>  {
>  	string_list_clear(&exclusions->excluded_refs, 0);
> +	string_list_clear(&exclusions->hidden_refs, 1);
>  }

Hrm, I'l read on, but I don't see any use of "util" here at a glance,
should the "1" here be "0", or maybe I've just missed how it's used...

> +	if (strcmp(section, "transfer") && strcmp(section, "receive") &&
> +	    strcmp(section, "uploadpack"))
> +		die(_("unsupported section for hidden refs: %s"), section);
> +
> +	if (exclusions->hidden_refs.nr)
> +		die(_("--exclude-hidden= passed more than once"));

We usually just ignore the first of --foo=bar --foo=baz and take "baz"
in our CLI use. Is it better to die here than just clear the previous
one & continue?

> -#define REF_EXCLUSIONS_INIT { .excluded_refs = STRING_LIST_INIT_DUP }
> +#define REF_EXCLUSIONS_INIT { .excluded_refs = STRING_LIST_INIT_DUP, .hidden_refs = STRING_LIST_INIT_DUP }

...the getting overly long line I mentioned in 3/6...

> +TEST_PASSES_SANITIZE_LEAK=true

Thanks for adding this! :)