On Thu, Sep 20, 2018 at 03:37:51PM -0400, Jeff King wrote:
> On Thu, Sep 20, 2018 at 02:04:11PM -0400, Taylor Blau wrote:
>
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 112041f407..b908bc5825 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -616,6 +616,12 @@ core.preferSymlinkRefs::
> > This is sometimes needed to work with old scripts that
> > expect HEAD to be a symbolic link.
> >
> > +core.alternateRefsCommand::
> > + When listing references from an alternate (e.g., in the case of
> > ".have"), use
> > + the shell to execute the specified command instead of
> > + linkgit:git-for-each-ref[1]. The first argument is the path of the
> > alternate.
> > + Output must be of the form: `%(objectname) SPC %(refname)`.
>
> We discussed off-list the notion that this could just be the objectname,
> since the ".have" mechanism doesn't care about the actual refnames.
>
> There's a little prior discussion from the list:
>
> https://public-inbox.org/git/[email protected]/
>
> My "rev-list --alternate-refs" patches _do_ use the refnames, since you
> could do something like "--source" that cares about them. But there's
> some awkwardness there, because the names are in a different namespace
> than the rest of the refs. If we were to just say "nope, you do not get
> to see the names of the alternates" then that awkwardness goes away. But
> it also loses some information that could _possibly_ be of use to a
> caller.
>
> Back in that earlier discussion I did not have a strong opinion, but
> here we are cementing that decision into a user-visible interface. So it
> probably makes sense to revisit and decide once and for all.
Interesting, and thanks for the link to the prior discussion. I think
that I agree mostly with your rationale in [1], which boils down (for
me) to:
- Other callers (like 'rev-list --alternate-refs') might care about
them. Even if we don't have those patches in Git today, it's worth
keeping their use case(s) in mind.
- I didn't measure either, but I can't imagine that we're paying a
huge price for this. So, it might be easy enough to keep saying,
"please write output as '%(objectname) SP %(refname)'", even if we
end up throwing out the refname, anyway.
> > +test_description='git receive-pack test'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'setup' '
> > + test_commit one &&
> > + git update-ref refs/heads/a HEAD &&
> > + test_commit two &&
> > + git update-ref refs/heads/b HEAD &&
> > + test_commit three &&
> > + git update-ref refs/heads/c HEAD &&
> > + git clone --bare . fork &&
> > + git clone fork pusher &&
> > + (
> > + cd fork &&
> > + git config receive.advertisealternates true &&
> > + git update-ref -d refs/heads/a &&
> > + git update-ref -d refs/heads/b &&
> > + git update-ref -d refs/heads/c &&
> > + git update-ref -d refs/heads/master &&
> > + git update-ref -d refs/tags/one &&
> > + git update-ref -d refs/tags/two &&
> > + git update-ref -d refs/tags/three &&
>
> Probably not worth nit-picking process count, but this could done with a
> single "update-ref --stdin".
Sure, I don't think that 7 `update-ref`'s vs 2 (`cat` + `git update-ref
--stdin`) will make or break the series, but I can happily shorten it as
you suggest ;-).
> > + printf "../../.git/objects" >objects/info/alternates
>
> Also a nitpick, but I think "echo" would be more usual here (we handle
> the lack of a trailing newline just fine, but any use of printf makes me
> wonder if something tricky is going on with line endings).
'echo' indeed seems to be the way to go. This 'printf' preference is a
Git LFS-ism ;-).
> > +test_expect_success 'with core.alternateRefsCommand' '
> > + test_config -C fork core.alternateRefsCommand \
> > + "git --git-dir=\"\$1\" for-each-ref \
> > + --format=\"%(objectname) %(refname)\" \
> > + refs/heads/a refs/heads/c;:" &&
>
> This is cute and all, but might it be more readable to use
> write_script() to stick it into its own script?
Good idea, I'll do that.
> > + cat >expect <<-EOF &&
> > + $(git rev-parse a) .have
> > + $(git rev-parse c) .have
> > + EOF
> > + printf "0000" | git receive-pack fork | extract_haves >actual &&
>
> There's been a push lately to avoid having git on the left-hand side of
> a fork, since we might otherwise miss its exit code (including things
> like asan/valgrind errors). So maybe:
>
> ... receive-pack fork >actual &&
> extract_haves <actual >actual.haves &&
> test_cmp expect actual.haves
>
> or similar?
Sure, I agree that it's a good idea to not miss the exit code (since we
don't have pipefail on), etc. I adopted your suggestion into my local
copy.
> > diff --git a/transport.c b/transport.c
> > index 24ae3f375d..e7d2cdf00b 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -1328,10 +1328,21 @@ char *transport_anonymize_url(const char *url)
> > static void fill_alternate_refs_command(struct child_process *cmd,
> > const char *repo_path)
> > {
> > - cmd->git_cmd = 1;
> > - 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)");
> > + const char *value;
> > +
> > + if (!git_config_get_value("core.alternateRefsCommand", &value)) {
> > + cmd->use_shell = 1;
> > +
> > + argv_array_push(&cmd->args, value);
> > + argv_array_push(&cmd->args, repo_path);
>
> Setting use_shell allows the shell trickery in your test, and matches
> the modern way we run config-based commands. Good.
>
> > + } else {
> > + cmd->git_cmd = 1;
> > +
> > + 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)");
> > + }
> > +
> > cmd->env = local_repo_env;
> > cmd->out = -1;
>
> And we still clear local_repo_env for the custom command, which is good
> to avoid confusion like $GIT_DIR being set when the custom command does
> "cd $1 && git ...". Good.
Thanks,
Taylor
[1]:
https://public-inbox.org/git/[email protected]/
