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

[Jeff King <peff@peff.net>]

On Thu, Sep 27, 2018 at 09:25:42PM -0700, Taylor Blau wrote:

> Let the repository that has alternates configure this command to avoid
> trusting the alternate to provide us a safe command to run in the shell.
> To behave differently on each alternate (e.g., only list tags from
> alternate A, only heads from B) provide the path of the alternate as the
> first argument.

Well, you also need to pass the path so it knows which repo to look at.
Which I think is the primary reason we do it, but behaving differently
for each alternate is another option.

> +core.alternateRefsCommand::
> +	When advertising tips of available history from an alternate, use the shell to
> +	execute the specified command instead of linkgit:git-for-each-ref[1]. The
> +	first argument is the absolute path of the alternate. Output must be of the
> +	form: `%(objectname)`, where multiple tips are separated by newlines.

I wonder if people may be confused about the %(objectname) syntax, since
it's specific to for-each-ref.  Now that we've simplified the output
format to a single value, perhaps we should define it more directly.
E.g., like:

  The output should contain one hex object id per line (i.e., the same
  as produced by `git for-each-ref --format='%(objectname)'`).

Now that we've dropped the refname requirement from the output, it is
more clear that this really does not have to be about refs at all.  In
the most technical sense, what we really allow in the output is any
object id X for which the alternate promises it has all objects
reachable from X. Ref tips are a convenient and efficient way of
providing that, but they are not the only possibility (and likewise, it
is fine to omit duplicates or even tips that are ancestors of other
tips).

I think that's probably getting _too_ technical, though. It probably
makes sense to just keep thinking of these as "what are the ref tips".

> +This is useful when a repository only wishes to advertise some of its
> +alternate's references as ".have"'s. For example, to only advertise branch

Maybe put ".have" into backticks for formatting?

> +heads, configure `core.alternateRefsCommand` to the path of a script which runs
> +`git --git-dir="$1" for-each-ref --format='%(objectname)' refs/heads`.

Does that script actually work? Because of the way we invoke shell
commands with arguments, I think we'd end up with:

  git --git-dir="$1" for-each-ref --format='%(objectname)' refs/heads "$@"

Possibly for-each-ref would ignore the extra path argument (thinking
it's a ref pattern that just doesn't match), but it's definitely not
what you intended. You'd have to write:

  f() { git --git-dir=$1 ...etc; } f

in the usual way. That's a minor pain, but it's what makes the more
direct:

  /my/script

work.

The other alternative is to pass $GIT_DIR in the environment on behalf
of the program. Then writing:

  git for-each-ref --format='%(objectname)' refs/heads

would Just Work. But it's a bit subtle, since it is not immediately
obvious that the command is meant to run in a different repository.

> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> new file mode 100755
> index 0000000000..503dde35a4
> --- /dev/null
> +++ b/t/t5410-receive-pack.sh
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +
> +test_description='git receive-pack test'

The name of this test file and the description are pretty vague. Can we
say something like "test handling of receive-pack with alternate-refs
config"?

> +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 update-ref --stdin <<-\EOF &&
> +		delete refs/heads/a
> +		delete refs/heads/b
> +		delete refs/heads/c
> +		delete refs/heads/master
> +		delete refs/tags/one
> +		delete refs/tags/two
> +		delete refs/tags/three
> +		EOF
> +		echo "../../.git/objects" >objects/info/alternates
> +	)
> +'

This setup is kind of convoluted. You're deleting those refs in the
fork, I think, because we don't want them to suppress the duplicate
.have lines from the alternate. Might it be easier to just create the
.have lines we're interested in after the fact?

I think we can also use "clone -s" to make the setup of the alternate a
little simpler.

I don't see the "pusher" repo being used for anything here. Leftover
cruft from when you were using "git push" to test?

So all together, perhaps something like:

  # we have a fork which points back to us as an alternate
  test_commit base &&
  git clone -s . fork &&

  # the alternate has two refs with new tips, in two separate hierarchies
  git checkout -b public/branch master &&
  test_commit public &&
  git checkout -b private/branch master &&
  test_commit private

And then...

> +test_expect_success 'with core.alternateRefsCommand' '
> +	write_script fork/alternate-refs <<-\EOF &&
> +		git --git-dir="$1" for-each-ref \
> +			--format="%(objectname)" \
> +			refs/heads/a \
> +			refs/heads/c
> +	EOF

...this can just look for refs/heads/public/, and...

> +	test_config -C fork core.alternateRefsCommand alternate-refs &&
> +	git rev-parse a c >expect &&

...we verify that we saw public/branch but not private/branch.

It's not that much shorter, but I had trouble understanding from the
setup why we needed to delete all those refs (and why we cared about
those tags in the first place).

> diff --git a/transport.c b/transport.c
> index 2825debac5..e271b66603 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)

The code change itself looks good to me.

-Peff