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
