[v3,4/4] transport.c: introduce core.alternateRefsPrefixes
diff mbox series

Message ID 48eb774c9e36f468549a278fd8cf703d8a34af28.1538108385.git.me@ttaylorr.com
State New
Headers show
Series
  • Filter alternate references
Related show

Commit Message

Taylor Blau Sept. 28, 2018, 4:25 a.m. UTC
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)"'

The above is cumbersome to write, so let's introduce a
"core.alternateRefsPrefixes" to address this common case. Instead, the
caller can run:

  $ git config core.alternateRefsPrefixes 'refs/tags'

Which will behave identically to the longer example using
"core.alternateRefsCommand".

Since the value of "core.alternateRefsPrefixes" is appended to 'git
for-each-ref' and then executed, include a "--" before taking the
configured value to avoid misinterpreting arguments as flags to 'git
for-each-ref'.

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

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config.txt | 7 +++++++
 t/t5410-receive-pack.sh  | 8 ++++++++
 transport.c              | 5 +++++
 3 files changed, 20 insertions(+)

Comments

Jeff King Sept. 28, 2018, 5:30 a.m. UTC | #1
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
Taylor Blau Sept. 28, 2018, 10:05 p.m. UTC | #2
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
Jeff King Sept. 29, 2018, 7:34 a.m. UTC | #3
On Fri, Sep 28, 2018 at 03:05:57PM -0700, Taylor Blau wrote:

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

I think they're separate issues, right? I was just confused on the
earlier patch, but the "git config" command you show above is the actual
broken case isn't it?

I'm not overly concerned since this isn't recommending the technique to
end users (and in fact the whole point is to give an alternative), but
it may be worth showing a working command in case anybody runs across
it.

-Peff
Taylor Blau Oct. 2, 2018, 1:57 a.m. UTC | #4
On Sat, Sep 29, 2018 at 03:34:26AM -0400, Jeff King wrote:
> On Fri, Sep 28, 2018 at 03:05:57PM -0700, Taylor Blau wrote:
>
> > > > 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.
>
> I think they're separate issues, right? I was just confused on the
> earlier patch, but the "git config" command you show above is the actual
> broken case isn't it?

Ah, I certainly had these mixed up on Saturday when I wrote what is
quoted here. As I understand it now, you were talking about the
difference between $@ and "$@", which I did fix (by rewriting the former
to the later).

> I'm not overly concerned since this isn't recommending the technique to
> end users (and in fact the whole point is to give an alternative), but
> it may be worth showing a working command in case anybody runs across
> it.

Completely agree, and thanks for your review.

Thanks,
Taylor
Taylor Blau Oct. 2, 2018, 2 a.m. UTC | #5
On Mon, Oct 01, 2018 at 06:57:37PM -0700, Taylor Blau wrote:
> On Sat, Sep 29, 2018 at 03:34:26AM -0400, Jeff King wrote:
> > On Fri, Sep 28, 2018 at 03:05:57PM -0700, Taylor Blau wrote:
> >
> > > > > 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.
> >
> > I think they're separate issues, right? I was just confused on the
> > earlier patch, but the "git config" command you show above is the actual
> > broken case isn't it?
>
> Ah, I certainly had these mixed up on Saturday when I wrote what is
> quoted here. As I understand it now, you were talking about the
> difference between $@ and "$@", which I did fix (by rewriting the former
> to the later).

Double "ah!". You were talking about getting the path to the repository
stuck on the end, which _is_ a problem here. I'll fix that.

> > I'm not overly concerned since this isn't recommending the technique to
> > end users (and in fact the whole point is to give an alternative), but
> > it may be worth showing a working command in case anybody runs across
> > it.
>
> Completely agree, and thanks for your review.

Thanks,
Taylor

Patch
diff mbox series

diff --git a/Documentation/config.txt b/Documentation/config.txt
index afcb18331a..9ef792ef0d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -627,6 +627,13 @@  alternate's references as ".have"'s. For example, to only advertise branch
 heads, configure `core.alternateRefsCommand` to the path of a script which runs
 `git --git-dir="$1" for-each-ref --format='%(objectname)' refs/heads`.
 
+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.
+
 core.bare::
 	If true this repository is assumed to be 'bare' and has no
 	working directory associated with it.  If this is the case a
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
+'
+
 test_done
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);
+		}
 	}
 
 	cmd->env = local_repo_env;