{"thread":{"id":"25788","subject":"pager config for external commands","startedAt":"2010-11-19T15:26:59Z","lastAt":"2010-11-19T17:29:01Z","messageCount":4,"participants":["Jeffrey Middleton","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"156181","messageId":"AANLkTimMSp8XVzoaax2mW-jYP26B+zF2QSJFcy4TUi60@mail.gmail.com","threadId":"25788","inReplyTo":"AANLkTimtjR0O3K8iGOVVVaFJS2+2wHcHhWf45tFYXjRQ@mail.gmail.com","subject":"pager config for external commands","fromName":"Jeffrey Middleton","fromEmail":"jefromi@gmail.com","sentAt":"2010-11-19T15:26:59Z","receivedAt":"2010-11-19T15:26:59Z","isPatch":false,"sender":{"key":"jefromi@gmail.com","avatar":null},"body":"External commands don't appear to support the pager. config\nsetting. It's implemented for builtins via the call to\ncheck_pager_config and associated code in run_builtin, but there's\nnothing like that in execv_dashed_external. Is there a reason not to\nimplement this for external commands? I can't see one, since the\n--no-pager option does apply to them.\n\nThanks,\nJeffrey\n"},{"id":"156188","messageId":"20101119160058.GA7967@sigill.intra.peff.net","threadId":"25788","inReplyTo":"AANLkTimMSp8XVzoaax2mW-jYP26B+zF2QSJFcy4TUi60@mail.gmail.com","subject":"Re: pager config for external commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-11-19T16:00:58Z","receivedAt":"2010-11-19T16:00:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 19, 2010 at 09:26:59AM -0600, Jeffrey Middleton wrote:\n\n> External commands don't appear to support the pager. config\n> setting. It's implemented for builtins via the call to\n> check_pager_config and associated code in run_builtin, but there's\n> nothing like that in execv_dashed_external. Is there a reason not to\n> implement this for external commands? I can't see one, since the\n> --no-pager option does apply to them.\n\nSee 4e10738 (Allow per-command pager config, 2008-07-03) for some\ndiscussion.\n\nBasically the problem is that we just call execv(\"git-\"). If it\nworks, then we are running an external, but we no longer have an\nopportunity to start the pager. If it doesn't, then we don't necessarily\nwant to commit our pager choice. We may be running an alias, or we may\nsimply barf with \"no such command\".\n\nLater on, we started to use run_command instead of execv, but I don't\nthink that changes the situation.\n\nI think what we really want to do is split out the \"look up in PATH\" bit\nfrom run_command, actually find out if we have an external command, and\nthen commit to running it if it exists. If you're interested in working\non that, I think it would be great.\n\n-Peff\n"},{"id":"156194","messageId":"AANLkTikE6=At-BV22myZtM4nuNL6z4hoJ-kS=O1LkBXo@mail.gmail.com","threadId":"25788","inReplyTo":"20101119160058.GA7967@sigill.intra.peff.net","subject":"Re: pager config for external commands","fromName":"Jeffrey Middleton","fromEmail":"jefromi@gmail.com","sentAt":"2010-11-19T17:16:53Z","receivedAt":"2010-11-19T17:16:53Z","isPatch":false,"sender":{"key":"jefromi@gmail.com","avatar":null},"body":"On Fri, Nov 19, 2010 at 10:00 AM, Jeff King wrote:\n> Basically the problem is that we just call execv(\"git-\"). If it\n> works, then we are running an external, but we no longer have an\n> opportunity to start the pager. If it doesn't, then we don't necessarily\n> want to commit our pager choice. We may be running an alias, or we may\n> simply barf with \"no such command\".\n\nOkay, that makes sense, I think. But execv_dashed_external does\ncurrently commit the pager choice, just without looking it up from the\nconfig. This means that, for example, git --paginate\n does invoke the pager. Is that intended? I don't\nthink I see any tests covering the combination of pager. for\nanything but builtins.\n\nAlso, instead of having git implement \"look up in PATH\" internally,\nwould it make sense to simply commit the pager choice, try the execv,\nthen uncommit the pager choice if the command was not found?\n\nJeffrey\n"},{"id":"156196","messageId":"20101119172901.GA9747@sigill.intra.peff.net","threadId":"25788","inReplyTo":"AANLkTikE6=At-BV22myZtM4nuNL6z4hoJ-kS=O1LkBXo@mail.gmail.com","subject":"Re: pager config for external commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-11-19T17:29:01Z","receivedAt":"2010-11-19T17:29:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 19, 2010 at 11:16:53AM -0600, Jeffrey Middleton wrote:\n\n> Okay, that makes sense, I think. But execv_dashed_external does\n> currently commit the pager choice, just without looking it up from the\n> config. This means that, for example, git --paginate\n> does invoke the pager. Is that intended? I don't\n> think I see any tests covering the combination of pager. for\n> anything but builtins.\n\nHmm. You're right, I should have looked at the current code instead of\ndigging in the history. :)\n\nLooks like it was part of some pager work Jonathan did this summer. I\nthink there may have been issues in the past, too, with actually looking\nat config before exec'ing the external, but those seem cleared up, too.\n\nSo in theory you just need this:\n\ndiff --git a/git.c b/git.c\nindex 0409ac9..bb2c726 100644\n--- a/git.c\n+++ b/git.c\n@@ -438,6 +438,8 @@ static void execv_dashed_external(const char **argv)\n \tconst char *tmp;\n \tint status;\n \n+\tif (use_pager == -1)\n+\t\tuse_pager = check_pager_config(argv[0]);\n \tcommit_pager_choice();\n \n \tstrbuf_addf(&cmd, \"git-%s\", argv[0]);\n\nBut beyond seeing that it does in fact turn on the pager for an external\ncommand, I've done no testing.\n\nAs far as \"git -p bogus\", I guess we have accepted that it will start\na pager. These days we at least send stderr to the pager, too, so the\nerror message won't go unseen.\n\n> Also, instead of having git implement \"look up in PATH\" internally,\n> would it make sense to simply commit the pager choice, try the execv,\n> then uncommit the pager choice if the command was not found?\n\nNo. The reason we push commit_pager_choice off to the last minute is\nthat you can't uncommit it gracefully. It actually execs the pager,\nwhich may then do things to the terminal outside of our control.\n\n-Peff\n"}]}