git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand

From
Taylor Blau <me@ttaylorr.com>
Date
Sep 20, 2018, 20:00 UTC
Message-ID
<20180920200034.GA83799@syl>
In-Reply-To
<20180920193751.GC29603@sigill.intra.peff.net>
On Thu, Sep 20, 2018 at 03:37:51PM -0400, Jeff King wrote:
Show 34 quoted lines
> 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@gitster.mtv.corp.google.com/
>
> 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.
Show 26 quoted lines
> > +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 ;-).

Show 5 quoted lines
> > +		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 ;-).

Show 8 quoted lines
> > +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.
Show 15 quoted lines
> > +	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.

Show 37 quoted lines
> > 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@sigill.intra.peff.net/
Previous: Jeff KingNext: Jeff King
Message 5 of 94 in “Filter alternate references”
  1. 0/3 Filter alternate referencesTaylor Blau, Sep 20, 2018
  2. 1/3 transport.c: extract 'fill_alternate_refs_command'Taylor Blau, Sep 20, 2018
  3. 2/3 transport.c: introduce core.alternateRefsCommandTaylor Blau, Sep 20, 2018
  4. Jeff KingSep 20, 2018
  5. Taylor BlauSep 20, 2018
  6. Jeff KingSep 20, 2018
  7. Junio C HamanoSep 21, 2018
  8. Taylor BlauSep 21, 2018
  9. Taylor BlauSep 21, 2018
  10. Junio C HamanoSep 21, 2018
  11. Taylor BlauSep 26, 2018
  12. 3/3 transport.c: introduce core.alternateRefsPrefixesTaylor Blau, Sep 20, 2018
  13. Jeff KingSep 20, 2018
  14. Taylor BlauSep 20, 2018
  15. Eric SunshineSep 21, 2018
  16. Taylor BlauSep 21, 2018
  17. Junio C HamanoSep 21, 2018
  18. Taylor BlauSep 21, 2018
  19. Junio C HamanoSep 21, 2018
  20. Stefan BellerSep 20, 2018
  21. Taylor BlauSep 20, 2018
  22. Jeff KingSep 20, 2018
  23. Jeff KingSep 20, 2018
  24. 0/3 Filter alternate referencesTaylor Blau, Sep 21, 2018
  25. 1/3 transport.c: extract 'fill_alternate_refs_command'Taylor Blau, Sep 21, 2018
  26. 2/3 transport.c: introduce core.alternateRefsCommandTaylor Blau, Sep 21, 2018
  27. Eric SunshineSep 21, 2018
  28. Taylor BlauSep 26, 2018
  29. Junio C HamanoSep 21, 2018
  30. Jeff KingSep 21, 2018
  31. Junio C HamanoSep 21, 2018
  32. Jeff KingSep 21, 2018
  33. Taylor BlauSep 26, 2018
  34. Jeff KingSep 26, 2018
  35. Eric SunshineSep 21, 2018
  36. brian m. carlsonSep 22, 2018
  37. Jeff KingSep 22, 2018
  38. brian m. carlsonSep 23, 2018
  39. Taylor BlauSep 26, 2018
  40. Jeff KingSep 26, 2018
  41. Taylor BlauSep 26, 2018
  42. Jeff KingSep 26, 2018
  43. Taylor BlauSep 28, 2018
  44. 3/3 transport.c: introduce core.alternateRefsPrefixesTaylor Blau, Sep 21, 2018
  45. Junio C HamanoSep 21, 2018
  46. Jeff KingSep 21, 2018
  47. Junio C HamanoSep 21, 2018
  48. Jeff KingSep 21, 2018
  49. Stefan BellerSep 21, 2018
  50. Junio C HamanoSep 24, 2018
  51. Jeff KingSep 24, 2018
  52. Junio C HamanoSep 24, 2018
  53. Jeff KingSep 24, 2018
  54. Jeff KingSep 24, 2018
  55. Junio C HamanoSep 24, 2018
  56. Jeff KingSep 24, 2018
  57. Junio C HamanoSep 25, 2018
  58. Taylor BlauSep 25, 2018
  59. Junio C HamanoSep 25, 2018
  60. Taylor BlauSep 26, 2018
  61. Jeff KingSep 26, 2018
  62. 0/4 Filter alternate referencesTaylor Blau, Sep 28, 2018
  63. 1/4 transport: drop refnames from for_each_alternate_refJeff King, Sep 28, 2018
  64. Jeff KingSep 28, 2018
  65. Taylor BlauSep 28, 2018
  66. 2/4 transport.c: extract 'fill_alternate_refs_command'Taylor Blau, Sep 28, 2018
  67. Jeff KingSep 28, 2018
  68. 3/4 transport.c: introduce core.alternateRefsCommandTaylor Blau, Sep 28, 2018
  69. Jeff KingSep 28, 2018
  70. Taylor BlauSep 28, 2018
  71. Jeff KingSep 29, 2018
  72. Taylor BlauOct 2, 2018
  73. 4/4 transport.c: introduce core.alternateRefsPrefixesTaylor Blau, Sep 28, 2018
  74. Jeff KingSep 28, 2018
  75. Taylor BlauSep 28, 2018
  76. Jeff KingSep 29, 2018
  77. Taylor BlauOct 2, 2018
  78. Taylor BlauOct 2, 2018
  79. 0/4 Filter alternate referencesTaylor Blau, Oct 2, 2018
  80. 1/4 transport: drop refnames from for_each_alternate_refTaylor Blau, Oct 2, 2018
  81. 3/4 transport.c: introduce core.alternateRefsCommandTaylor Blau, Oct 2, 2018
  82. Jeff KingOct 2, 2018
  83. Taylor BlauOct 4, 2018
  84. 2/4 transport.c: extract 'fill_alternate_refs_command'Taylor Blau, Oct 2, 2018
  85. 4/4 transport.c: introduce core.alternateRefsPrefixesTaylor Blau, Oct 2, 2018
  86. Ramsay JonesOct 2, 2018
  87. 4/4 transport.c: introduce core.alternateRefsPrefixesTaylor Blau, Oct 2, 2018
  88. 0/4 Filter alternate referencesTaylor Blau, Oct 8, 2018
  89. 1/4 transport: drop refnames from for_each_alternate_refTaylor Blau, Oct 8, 2018
  90. 2/4 transport.c: extract 'fill_alternate_refs_command'Taylor Blau, Oct 8, 2018
  91. 3/4 transport.c: introduce core.alternateRefsCommandTaylor Blau, Oct 8, 2018
  92. 4/4 transport.c: introduce core.alternateRefsPrefixesTaylor Blau, Oct 8, 2018
  93. Jeff KingOct 9, 2018
  94. Taylor BlauOct 9, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.