| Message ID | 48eb774c9e36f468549a278fd8cf703d8a34af28.1538108385.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Filter alternate references | expand |
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
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
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
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
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
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;
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(+)