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

From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, gitster@pobox.com, sunshine@sunshineco.com,
	sbeller@google.com
Subject: Re: [PATCH v3 4/4] transport.c: introduce core.alternateRefsPrefixes
Date: Fri, 28 Sep 2018 15:05:57 -0700	[thread overview]
Message-ID: <20180928220557.GB45367@syl> (raw)
In-Reply-To: <20180928053057.GD25850@sigill.intra.peff.net>

On Fri, Sep 28, 2018 at 01:30:57AM -0400, Jeff King wrote:
> 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!).

Hmm. I'll be curious to how you respond to my other message about the
same topic. I feel that whatever the outcome there is will affect both
locations in the same way.

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

Sure, that makes sense.

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

Sounds good; that's what I did.

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

Thanks for the review of this patch, too.

Thanks,
Taylor