| Message ID | 3639e9058859b326f64600fcd0b608171b56ce9f.1537466087.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Filter alternate references | expand |
On Thu, Sep 20, 2018 at 02:04:13PM -0400, 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. To be clear: this isn't something we plan to use at GitHub at all. It just seemed like a nice "in between" the current inflexible state and the "incredibly flexible but not trivial to use" command from patch 2. Note that unlike core.alternateRefsCommand, there are no security issues here with reading this from the alternate, although: - it's a little awkward to read the config from the alternate - since these are clearly related config, it probably makes sense for them to be consistent > 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) %(refname)" \ > ' I think it's more likely that advertising only heads would make sense. The pathological repos I see are usually a sane number of branches and then an absurd number of tags. Not that it's super important, but I wonder if we should give a motivating example like this in the documentation. In which case we'd probably want to give the most plausible one. > 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'. Good idea. > diff --git a/Documentation/config.txt b/Documentation/config.txt > index b908bc5825..d768c57310 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -622,6 +622,12 @@ core.alternateRefsCommand:: > linkgit:git-for-each-ref[1]. The first argument is the path of the alternate. > Output must be of the form: `%(objectname) SPC %(refname)`. > > +core.alternateRefsPrefixes:: > + When listing references from an alternate, list only references that begin > + with the given prefix. To list multiple prefixes, separate them with a > + whitespace character. If `core.alternateRefsCommand` is set, setting > + `core.alternateRefsPrefixes` has no effect. I can't remember all of the rules for how for-each-ref matches prefixes, but I remember that it's subtly different than git-branch (and that's why ref-filter.c has two matching modes). Do we need to spell out the rules here (or at least say "it matches like for-each-ref")? Also, a minor nit, but I think the argv_array_split() helper you're using soaks up arbitrary amounts of whitespace. So maybe "separate them with whitespace" instead of "a whitespace character". Or maybe we should be strict in what we suggest and liberal in what we parse. ;) > +test_expect_success 'with core.alternateRefsPrefixes' ' > + test_config -C fork core.alternateRefsPrefixes "refs/tags" && > + cat >expect <<-EOF && > + $(git rev-parse one) .have > + $(git rev-parse three) .have > + $(git rev-parse two) .have > + EOF > + printf "0000" | git receive-pack fork | extract_haves >actual && > + test_cmp expect actual Looks sane, though the same pipe comment applies as before. > test_done > diff --git a/transport.c b/transport.c > index e7d2cdf00b..9323e5c3cd 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) %(refname)"); > + > + if (!git_config_get_value("core.alternateRefsPrefixes", &value)) { > + argv_array_push(&cmd->args, "--"); > + argv_array_split(&cmd->args, value); > + } > } The implementation ended up delightfully simple. -Peff
On Thu, Sep 20, 2018 at 03:47:34PM -0400, Jeff King wrote: > On Thu, Sep 20, 2018 at 02:04:13PM -0400, 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. > > To be clear: this isn't something we plan to use at GitHub at all. It > just seemed like a nice "in between" the current inflexible state and > the "incredibly flexible but not trivial to use" command from patch 2. > > Note that unlike core.alternateRefsCommand, there are no security issues > here with reading this from the alternate, although: > > - it's a little awkward to read the config from the alternate > > - since these are clearly related config, it probably makes sense for > them to be consistent Another note is that the thing we are planning on using ("core.alternateRefsCommand") could also be implemented as a hook, e.g., .git/hooks/gather-alternate-refs. That said, I think that this makes more sense when the alternate is doing the configuring, not the ohter way around. > > 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) %(refname)" \ > > ' > > I think it's more likely that advertising only heads would make sense. > The pathological repos I see are usually a sane number of branches and > then an absurd number of tags. I agree with you. I used "refs/tags" as the prefix here since I'd like different output than when "core.alternateRefsPrefixes" isn't configured at all. Since we have a tag for each commit (we use test_commit to do so), and refs/heads/{a,b,c,master}, we'd get the same output whether we configured the prefix to be refs/heads, or didn't configure it at all. Since using 'git for-each-ref' sorts in order of refname, a prefix of "refs/tags" sorts in order of tagname, so we'll get different output because of it. That said, I think that this test is a little fragile as-is, since it'll break if we change the ordering of 'git for-each-ref'. Maybe we should `| sort >actual.haves`? > Not that it's super important, but I wonder if we should give a > motivating example like this in the documentation. In which case we'd > probably want to give the most plausible one. Maybe. I don't feel strongly about it, though. > > 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'. > > Good idea. > > > diff --git a/Documentation/config.txt b/Documentation/config.txt > > index b908bc5825..d768c57310 100644 > > --- a/Documentation/config.txt > > +++ b/Documentation/config.txt > > @@ -622,6 +622,12 @@ core.alternateRefsCommand:: > > linkgit:git-for-each-ref[1]. The first argument is the path of the alternate. > > Output must be of the form: `%(objectname) SPC %(refname)`. > > > > +core.alternateRefsPrefixes:: > > + When listing references from an alternate, list only references that begin > > + with the given prefix. To list multiple prefixes, separate them with a > > + whitespace character. If `core.alternateRefsCommand` is set, setting > > + `core.alternateRefsPrefixes` has no effect. > > I can't remember all of the rules for how for-each-ref matches prefixes, > but I remember that it's subtly different than git-branch (and that's > why ref-filter.c has two matching modes). Do we need to spell out the > rules here (or at least say "it matches like for-each-ref")? Good idea. I'll do that. > Also, a minor nit, but I think the argv_array_split() helper you're > using soaks up arbitrary amounts of whitespace. So maybe "separate them > with whitespace" instead of "a whitespace character". Or maybe we should > be strict in what we suggest and liberal in what we parse. ;) Yeah, I think that chaning "a whitespace character" -> "with whitespace" is the easier thing to do ;-). > > +test_expect_success 'with core.alternateRefsPrefixes' ' > > + test_config -C fork core.alternateRefsPrefixes "refs/tags" && > > + cat >expect <<-EOF && > > + $(git rev-parse one) .have > > + $(git rev-parse three) .have > > + $(git rev-parse two) .have > > + EOF > > + printf "0000" | git receive-pack fork | extract_haves >actual && > > + test_cmp expect actual > > Looks sane, though the same pipe comment applies as before. Thanks. I applied that suggestion in both locations when reading your last mail. > > test_done > > diff --git a/transport.c b/transport.c > > index e7d2cdf00b..9323e5c3cd 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) %(refname)"); > > + > > + if (!git_config_get_value("core.alternateRefsPrefixes", &value)) { > > + argv_array_push(&cmd->args, "--"); > > + argv_array_split(&cmd->args, value); > > + } > > } > > The implementation ended up delightfully simple. Thanks :-). It made me quite happy, too. Thanks, Taylor
On Thu, Sep 20, 2018 at 2:04 PM Taylor Blau <ttaylorr@github.com> 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. > [...] > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh > @@ -44,4 +44,15 @@ test_expect_success 'with core.alternateRefsCommand' ' > +test_expect_success 'with core.alternateRefsPrefixes' ' > + test_config -C fork core.alternateRefsPrefixes "refs/tags" && > + cat >expect <<-EOF && > + $(git rev-parse one) .have > + $(git rev-parse three) .have > + $(git rev-parse two) .have > + EOF It's probably a matter of taste as to which is more readable, but this entire "cat <<EOF" block could be replaced with a simple one-liner: printf "%s .have\n" $(git rev-parse one three two) >expect && Same comment applies to previous patch, as well. > + printf "0000" | git receive-pack fork | extract_haves >actual && > + test_cmp expect actual > +'
On Fri, Sep 21, 2018 at 03:19:20AM -0400, Eric Sunshine wrote: > On Thu, Sep 20, 2018 at 2:04 PM Taylor Blau <ttaylorr@github.com> 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. > > [...] > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > > --- > > diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh > > @@ -44,4 +44,15 @@ test_expect_success 'with core.alternateRefsCommand' ' > > +test_expect_success 'with core.alternateRefsPrefixes' ' > > + test_config -C fork core.alternateRefsPrefixes "refs/tags" && > > + cat >expect <<-EOF && > > + $(git rev-parse one) .have > > + $(git rev-parse three) .have > > + $(git rev-parse two) .have > > + EOF > > It's probably a matter of taste as to which is more readable, but this > entire "cat <<EOF" block could be replaced with a simple one-liner: > > printf "%s .have\n" $(git rev-parse one three two) >expect && > > Same comment applies to previous patch, as well. That's a good idea. I amended both patches to replace the 'cat <<-EOF ...' block with your suggestion above. It's tempting to introduce it as: expect_haves() { printf "%s .have\n" $(git rev-parse -- $@) } And call it as: expect_haves one three two >expect But I'm not sure whether I think that this is better or worse than writing it twice inline. I think that the test is small enough that it doesn't really matter either way, but I think that I've convinced myself while composing this email that expect_haves() is an OK idea. If you feel strongly that it isn't, please let me know, and I'll write them inline before sending v2. Thanks, Taylor
Eric Sunshine <sunshine@sunshineco.com> writes: > On Thu, Sep 20, 2018 at 2:04 PM Taylor Blau <ttaylorr@github.com> 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. >> [...] >> Signed-off-by: Taylor Blau <me@ttaylorr.com> >> --- >> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh >> @@ -44,4 +44,15 @@ test_expect_success 'with core.alternateRefsCommand' ' >> +test_expect_success 'with core.alternateRefsPrefixes' ' >> + test_config -C fork core.alternateRefsPrefixes "refs/tags" && >> + cat >expect <<-EOF && >> + $(git rev-parse one) .have >> + $(git rev-parse three) .have >> + $(git rev-parse two) .have >> + EOF > > It's probably a matter of taste as to which is more readable, but this > entire "cat <<EOF" block could be replaced with a simple one-liner: > > printf "%s .have\n" $(git rev-parse one three two) >expect && > > Same comment applies to previous patch, as well. If the expected pattern is expected to stay to be just a sequence of "<oid> .have" and nothing else for the foreseeable future, I think it is a good idea. > >> + printf "0000" | git receive-pack fork | extract_haves >actual && >> + test_cmp expect actual >> +'
Taylor Blau <me@ttaylorr.com> writes: > ...' block with your suggestion above. It's tempting to introduce it as: > > expect_haves() { > printf "%s .have\n" $(git rev-parse -- $@) > } > > And call it as: > > expect_haves one three two >expect > > But I'm not sure whether I think that this is better or worse than > writing it twice inline. If the expected pattern is expected to stay to be just a sequence of "<oid> .have" and nothing else for the foreseeable future, I think it is a good idea to introduce such a helper function. Spelling it out at the use site, e.g. printf "%s .have\n" $(git rev-parse a b c) >expect will become cumbersome once the set of objects you need to show starts growing. expect_haves a b c >expect would be shorter, of course. And as long as we expect to have ONLY "<oid> .have" lines and nothing else, there is no downside that the details of the format is hidden away inside the helper.
On Fri, Sep 21, 2018 at 09:45:11AM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > ...' block with your suggestion above. It's tempting to introduce it as: > > > > expect_haves() { > > printf "%s .have\n" $(git rev-parse -- $@) > > } > > > > And call it as: > > > > expect_haves one three two >expect > > > > But I'm not sure whether I think that this is better or worse than > > writing it twice inline. > > If the expected pattern is expected to stay to be just a sequence of > "<oid> .have" and nothing else for the foreseeable future, I think > it is a good idea to introduce such a helper function. Spelling it > out at the use site, e.g. > > printf "%s .have\n" $(git rev-parse a b c) >expect > > will become cumbersome once the set of objects you need to show > starts growing. That's a good reason, and I hadn't thought of it. > expect_haves a b c >expect > > would be shorter, of course. And as long as we expect to have ONLY > "<oid> .have" lines and nothing else, there is no downside that the > details of the format is hidden away inside the helper. Yeah, I don't expect this to to change much at all, so I think that 'expect_haves()' is good. Thanks, Taylor
diff --git a/Documentation/config.txt b/Documentation/config.txt index b908bc5825..d768c57310 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -622,6 +622,12 @@ core.alternateRefsCommand:: linkgit:git-for-each-ref[1]. The first argument is the path of the alternate. Output must be of the form: `%(objectname) SPC %(refname)`. +core.alternateRefsPrefixes:: + When listing references from an alternate, list only references that begin + with the given prefix. To list multiple prefixes, separate them with a + whitespace character. 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 09fb3f39a1..df2830e9f6 100755 --- a/t/t5410-receive-pack.sh +++ b/t/t5410-receive-pack.sh @@ -44,4 +44,15 @@ test_expect_success 'with core.alternateRefsCommand' ' test_cmp expect actual ' +test_expect_success 'with core.alternateRefsPrefixes' ' + test_config -C fork core.alternateRefsPrefixes "refs/tags" && + cat >expect <<-EOF && + $(git rev-parse one) .have + $(git rev-parse three) .have + $(git rev-parse two) .have + EOF + printf "0000" | git receive-pack fork | extract_haves >actual && + test_cmp expect actual +' + test_done diff --git a/transport.c b/transport.c index e7d2cdf00b..9323e5c3cd 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) %(refname)"); + + 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) %(refname)" \ ' 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 | 6 ++++++ t/t5410-receive-pack.sh | 11 +++++++++++ transport.c | 5 +++++ 3 files changed, 22 insertions(+)