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

From: Jeff King <peff@peff.net>
Date: Oct 2, 2018, 23:40 UTC
Message-ID: <20181002234056.GA18516@sigill.intra.peff.net>
In-Reply-To: <aadb27c0106d8f1a49dd35e7a040131aecaef2c1.1538446827.git.me@ttaylorr.com>

On Mon, Oct 01, 2018 at 07:23:58PM -0700, Taylor Blau wrote:

Show 15 quoted lines

> +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 contain one
> +	hex object id per line (i.e., the same as produce by `git for-each-ref
> +	--format='%(objectname)'`).
> ++
> +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
> +heads, configure `core.alternateRefsCommand` to the path of a script which runs
> +`git --git-dir="$1" for-each-ref --format='%(objectname)' refs/heads`.
> ++
> +Note that the configured value is executed in a shell, and thus
> +linkgit:git-for-each-ref[1] by itself does not work, as scripts have to handle
> +the path argument specially.

This last paragraph is trying to fix the wrong-impression that we discussed in the last round. But I'm not sure it doesn't make things more confusing. ;)

Specifically, the problem isn't the shell. The issue is that we pass the repo path as an argument to the command. So either:

  - it's a real command that we run, in which case git-for-each-ref does
    not take a repo path argument and so doesn't work; or

  - it's a shell snippet, in which case the argument is appended to the
    snippet (and here's where you can get into a rabbit hole of
    explaining how our shell invocation works, and we should avoid that)

Can we just say:

  Note that you cannot generally put `git for-each-ref` directly into
  the config value, as it does not take a repository path as an argument
  (but you can wrap the command above in a shell script).

> [...]

The rest of the patch looks good to me, along with the other three (modulo the "expect" fixup you already sent).

-Peff