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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 7, 2022, 13:34 UTC
Message-ID: <221107.86pmdyx4me.gmgdl@evledraar.gmail.com>
In-Reply-To: <de7c1aa210c2df9bdbbb6c19f44f72c37f56c5da.1667823042.git.ps@pks.im>

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.

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

> +	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! :)