pager config for external commands

pager config for external commands

[Jeffrey Middleton]

External commands don't appear to support the pager.<cmd> config
setting. It's implemented for builtins via the call to
check_pager_config and associated code in run_builtin, but there's
nothing like that in execv_dashed_external. Is there a reason not to
implement this for external commands? I can't see one, since the
--no-pager option does apply to them.

Thanks,
Jeffrey

Re: pager config for external commands

[Jeff King]

On Fri, Nov 19, 2010 at 09:26:59AM -0600, Jeffrey Middleton wrote:

> External commands don't appear to support the pager.<cmd> config
> setting. It's implemented for builtins via the call to
> check_pager_config and associated code in run_builtin, but there's
> nothing like that in execv_dashed_external. Is there a reason not to
> implement this for external commands? I can't see one, since the
> --no-pager option does apply to them.

See 4e10738 (Allow per-command pager config, 2008-07-03) for some
discussion.

Basically the problem is that we just call execv("git-<cmd>"). If it
works, then we are running an external, but we no longer have an
opportunity to start the pager. If it doesn't, then we don't necessarily
want to commit our pager choice. We may be running an alias, or we may
simply barf with "no such command".

Later on, we started to use run_command instead of execv, but I don't
think that changes the situation.

I think what we really want to do is split out the "look up in PATH" bit
from run_command, actually find out if we have an external command, and
then commit to running it if it exists. If you're interested in working
on that, I think it would be great.

-Peff

Re: pager config for external commands

[Jeffrey Middleton]

On Fri, Nov 19, 2010 at 10:00 AM, Jeff King <peff@peff.net> wrote:
> Basically the problem is that we just call execv("git-<cmd>"). If it
> works, then we are running an external, but we no longer have an
> opportunity to start the pager. If it doesn't, then we don't necessarily
> want to commit our pager choice. We may be running an alias, or we may
> simply barf with "no such command".

Okay, that makes sense, I think. But execv_dashed_external does
currently commit the pager choice, just without looking it up from the
config. This means that, for example, git --paginate
<nonexistent-command> does invoke the pager. Is that intended? I don't
think I see any tests covering the combination of pager.<cmd> for
anything but builtins.

Also, instead of having git implement "look up in PATH" internally,
would it make sense to simply commit the pager choice, try the execv,
then uncommit the pager choice if the command was not found?

Jeffrey

Re: pager config for external commands

[Jeff King]

On Fri, Nov 19, 2010 at 11:16:53AM -0600, Jeffrey Middleton wrote:

> Okay, that makes sense, I think. But execv_dashed_external does
> currently commit the pager choice, just without looking it up from the
> config. This means that, for example, git --paginate
> <nonexistent-command> does invoke the pager. Is that intended? I don't
> think I see any tests covering the combination of pager.<cmd> for
> anything but builtins.

Hmm. You're right, I should have looked at the current code instead of
digging in the history. :)

Looks like it was part of some pager work Jonathan did this summer. I
think there may have been issues in the past, too, with actually looking
at config before exec'ing the external, but those seem cleared up, too.

So in theory you just need this:

diff --git a/git.c b/git.c
index 0409ac9..bb2c726 100644
--- a/git.c
+++ b/git.c
@@ -438,6 +438,8 @@ static void execv_dashed_external(const char **argv)
 	const char *tmp;
 	int status;
 
+	if (use_pager == -1)
+		use_pager = check_pager_config(argv[0]);
 	commit_pager_choice();
 
 	strbuf_addf(&cmd, "git-%s", argv[0]);

But beyond seeing that it does in fact turn on the pager for an external
command, I've done no testing.

As far as "git -p bogus", I guess we have accepted that it will start
a pager. These days we at least send stderr to the pager, too, so the
error message won't go unseen.

> Also, instead of having git implement "look up in PATH" internally,
> would it make sense to simply commit the pager choice, try the execv,
> then uncommit the pager choice if the command was not found?

No. The reason we push commit_pager_choice off to the last minute is
that you can't uncommit it gracefully. It actually execs the pager,
which may then do things to the terminal outside of our control.

-Peff