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/xmqqefzraqbu.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > 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/20170125195425.q4fpvc4ten5mfjgl@xxxxxxxxxxxxxxxxxxxxx/
