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

From: Jeff King <peff@peff.net>
Date: Jul 11, 2017, 10:24 UTC
Message-ID: <20170711102451.27r3yaa2mgjgk4qv@sigill.intra.peff.net>
In-Reply-To: <62a5a588d880d1a41d6cdd54cd92ee577a0451fa.1499723297.git.martin.agren@gmail.com>

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().

So I think this hunk:

Show 15 quoted lines

> @@ -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. And that highlights the issue with externals; we don't have any internal database that says "these ones handle their own pager". We don't even have a list of all the possibilities, since users can drop whatever they like into their $PATH.

So we'd have to make a (backwards-incompatible) decision that pager.* doesn't work for external commands, and they must manually trigger the pager themselves. I'm undecided on whether that's reasonable or not (certainly it's what git-stash would want, but it may be hurting other commands).

Anyway, I think that's out of scope for your series, which is just trying to make the builtins work better.

-Peff