Re: [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external

[Martin Ågren]

On 31 July 2017 at 05:45, Jeff King  wrote:
> On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:
>
>> One could address this in run_argv(), by making the second call to
>> execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
>> would be started as "git foo". (Possibly after unrolling and cleaning up
>> the "while (1)"-loop.) That seems like the wrong fix for this particular
>> issue, but might be a wanted change on its own -- or maybe not --, since
>> it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
>> only for builtins...).
>
> We shouldn't need to relay them. They get added to the environment by
> the initial "git" invocation, and then are available everywhere (in
> fact, it would be wrong to relay them for multi-valued config).

Thanks for explaining. I did some very sloppy reading of the comment
in git.c that we "cannot take flags in between the 'git' and the
''" which I somehow misunderstood completely as "we cannot pass
that sort of information to git-". Silly. Thanks for taking the
time to explain what I should have found out myself...

So yeah, I meant the above and not this:

> Or did
> you mean that we could potentially allow:
>
>   [alias]
>   foo = "-c baz some-builtin"
>
> That's interesting, but I think the fact that it only works with
> builtins makes it a bad idea. And you can always do:
>
>   [alias]
>   foo = "!git -c baz some-builtin"
>
> which is equivalent.

Re: [PATCH v2 10/10] git.c: ignore pager.* when launching builtin as dashed external

[Jeff King]

On Mon, Jul 17, 2017 at 10:10:52PM +0200, Martin Ågren wrote:

> When running, e.g., `git -c alias.bar=foo bar`, we expand the alias and
> execute `git-foo` as a dashed external. This is true even if git foo is
> a builtin. That is on purpose, and is motivated in a comment which was
> added in commit 441981bc ("git: simplify environment save/restore
> logic", 2016-01-26).
> 
> Shortly before we launch a dashed external, and unless we have already
> found out whether we should use a pager, we check `pager.foo`. This was
> added in commit 92058e4d ("support pager.* for external commands",
> 2011-08-18). If the dashed external is a builtin, this does not match
> that commit's intention and is arguably wrong, since it would be cleaner
> if we let the "dashed external builtin" handle `pager.foo`.
> 
> This has not mattered in practice, but a recent patch taught `git-tag`
> to ignore `pager.tag` under certain circumstances. But, when started
> using an alias, it doesn't get the chance to do so, as outlined above.
> That recent patch added a test to document this breakage.
> 
> Do not check `pager.foo` before launching a builtin as a dashed
> external, i.e., if we recognize the name of the external as a builtin.
> Change the test to use `test_expect_success`.

I think floating this change to the end like this has made it much
easier to see why we must do it. The end result looks good to me.

> Signed-off-by: Martin Ågren 
> ---
> One could address this in run_argv(), by making the second call to
> execv_dashed_external() conditional on "!is_builtin()" whereas a builtin
> would be started as "git foo". (Possibly after unrolling and cleaning up
> the "while (1)"-loop.) That seems like the wrong fix for this particular
> issue, but might be a wanted change on its own -- or maybe not --, since
> it would mean one could relay, e.g., "-c baz" to "git -c baz foo" (but
> only for builtins...).

We shouldn't need to relay them. They get added to the environment by
the initial "git" invocation, and then are available everywhere (in
fact, it would be wrong to relay them for multi-valued config). Or did
you mean that we could potentially allow:

  [alias]
  foo = "-c baz some-builtin"

That's interesting, but I think the fact that it only works with
builtins makes it a bad idea. And you can always do:

  [alias]
  foo = "!git -c baz some-builtin"

which is equivalent.

-Peff