Re: [PATCH v3 4/4] transport.c: introduce core.alternateRefsPrefixes

[Jeff King <peff@peff.net>]

On Thu, Sep 27, 2018 at 09:25:45PM -0700, Taylor Blau wrote:

> The recently-introduced "core.alternateRefsCommand" allows callers to
> specify with high flexibility the tips that they wish to advertise from
> alternates. This flexibility comes at the cost of some inconvenience
> when the caller only wishes to limit the advertisement to one or more
> prefixes.
> 
> For example, to advertise only tags, a caller using
> 'core.alternateRefsCommand' would have to do:
> 
>   $ git config core.alternateRefsCommand ' \
>       git -C "$1" for-each-ref refs/tags --format="%(objectname)"'

This has the same "$@" issue as the previous one, I think (which only
makes your point about it being cumbersome more true!).

> In the case that the caller wishes to specify multiple prefixes, they
> may separate them by whitespace. If "core.alternateRefsCommand" is set,
> it will take precedence over "core.alternateRefsPrefixes".

Just a meta-comment: I don't particularly mind this discussion in the
commit message, but since these points ought to be in the documentation
anyway, it may make sense to omit them here in the name of brevity.

> +core.alternateRefsPrefixes::
> +	When listing references from an alternate, list only references that begin
> +	with the given prefix. Prefixes match as if they were given as arguments to
> +	linkgit:git-for-each-ref[1]. To list multiple prefixes, separate them with
> +	whitespace. If `core.alternateRefsCommand` is set, setting
> +	`core.alternateRefsPrefixes` has no effect.

Looks good.

> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> index 503dde35a4..3449967cc7 100755
> --- a/t/t5410-receive-pack.sh
> +++ b/t/t5410-receive-pack.sh
> @@ -46,4 +46,12 @@ test_expect_success 'with core.alternateRefsCommand' '
>  	test_cmp expect actual.haves
>  '
>  
> +test_expect_success 'with core.alternateRefsPrefixes' '
> +	test_config -C fork core.alternateRefsPrefixes "refs/tags" &&
> +	git rev-parse one three two >expect &&
> +	printf "0000" | git receive-pack fork >actual &&
> +	extract_haves <actual >actual.haves &&
> +	test_cmp expect actual.haves
> +'

If you follow my suggestion on the test setup from the last patch, it
would make sense to just put "refs/heads/public/" here. Although neither
that nor what you have here tests the whitespace separation. Possibly
there should be a third hierarchy.

> diff --git a/transport.c b/transport.c
> index e271b66603..83474add28 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1341,6 +1341,11 @@ static void fill_alternate_refs_command(struct child_process *cmd,
>  		argv_array_pushf(&cmd->args, "--git-dir=%s", repo_path);
>  		argv_array_push(&cmd->args, "for-each-ref");
>  		argv_array_push(&cmd->args, "--format=%(objectname)");
> +
> +		if (!git_config_get_value("core.alternateRefsPrefixes", &value)) {
> +			argv_array_push(&cmd->args, "--");
> +			argv_array_split(&cmd->args, value);
> +		}

And this part looks good.

-Peff