Re: [PATCH 2/7] git.c: let builtins opt for handling `pager.foo` themselves

From: Martin Ågren <martin.agren@gmail.com>
Date: Jul 11, 2017, 13:46 UTC
Message-ID: <CAN0heSqLX1WNk3KC9U3f=qem0SYV-g=-HU+XHepi_CbuHatCOg@mail.gmail.com>
In-Reply-To: <20170711102451.27r3yaa2mgjgk4qv@sigill.intra.peff.net>

On 11 July 2017 at 12:24, Jeff King <peff@peff.net> wrote:

Show 10 quoted lines

> On Mon, Jul 10, 2017 at 11:55:15PM +0200, Martin Ågren wrote:
>
>> To allow individual builtins to make more informed decisions about when
>> to respect `pager.foo`, introduce a flag IGNORE_PAGER_CONFIG. If the flag
>> is set, do not check `pager.foo`. This applies to two code-paths -- one
>> in run_builtin() and one in execv_dashed_external().
>
> Can this ever trigger in execv_dashed_external()? We should only get
> there if get_builtin() returned NULL in the first place. Otherwise, we'd
> run and exited via handle_builtin().

I can trigger it with this:

$ git -c pager.tag="echo paging" -c pager.tag.list=no -c alias.t=tag t -l

where the alias is what triggers it and the two pager-configurations demonstrate the effect.

Show 19 quoted lines

> So I think this hunk:
>
>> @@ -543,11 +550,14 @@ static void execv_dashed_external(const char **argv)
>>  {
>>       struct child_process cmd = CHILD_PROCESS_INIT;
>>       int status;
>> +     struct cmd_struct *builtin;
>>
>>       if (get_super_prefix())
>>               die("%s doesn't support --super-prefix", argv[0]);
>>
>> -     if (use_pager == -1)
>> +     builtin = get_builtin(argv[0]);
>> +     if (use_pager == -1 &&
>> +         !(builtin && builtin->option & IGNORE_PAGER_CONFIG))
>>               use_pager = check_pager_config(argv[0]);
>>       commit_pager_choice();
>
> ...can just go away.

If I remove this, the call I gave above will page although it shouldn't, and it doesn't if I keep this hunk. There's this in run_argv: "If we tried alias and futzed with our environment, it no longer is safe to invoke builtins directly in general. We have to spawn them as dashed externals." There's also a NEEDSWORK.

Although, thinking about it, I'm not sure why when I remove this hunk, the child process doesn't set up the paging correctly. Maybe something related to my using "-c", or something about the launching of child processes. Those are both areas where I lack knowledge. Will look into it.

Martin