threads / discuss / 25788
Subject: pager config for external commands
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
Jeff KingRe: pager config for external commands
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
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
Jeff KingRe: pager config for external commands
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