{"thread":{"id":"34119","subject":"[PATCH RFC] show-branch: use pager","startedAt":"2013-06-13T06:43:31Z","lastAt":"2013-06-13T17:33:40Z","messageCount":5,"participants":["Øystein Walle","Jeff King","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"220693","messageId":"1371105811-3112-1-git-send-email-oystwa@gmail.com","threadId":"34119","inReplyTo":null,"subject":"[PATCH RFC] show-branch: use pager","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2013-06-13T06:43:31Z","receivedAt":"2013-06-13T06:43:31Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"This is for consistency with other porcelain commands such as 'log'.\n\nSigned-off-by: Øystein Walle <oystwa@gmail.com>\n---\nThe rationale for this patch I hope is consicely explained in the commit\nmessage. I was rather surprised it didn't use a pager as I've gotten used to it\nfor most commands.\n\nI marked this as an RFC because of Jeff King's comments in\ndaa0c3d97 where I got the impression this this might not be a good idea.\nHowever I haven't found any bugs and all the tests pass. It is more a huble\nsuggestion than anything but I thought I might as well send it as a patch.\n\nsetup_pager() is already pulled in via cache.h so there was no need to add any\n#include directive. I suppose this is as close to a one-liner as it gets :)\n\nBest regards,\nØsse\n\n builtin/show-branch.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 90fc6b1..bd3e10c 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -683,6 +683,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t};\n \n \tgit_config(git_show_branch_config, NULL);\n+\tsetup_pager();\n \n \t/* If nothing is specified, try the default first */\n \tif (ac == 1 && default_num) {\n-- \n1.8.2.2\n"},{"id":"220695","messageId":"20130613070118.GA23235@sigill.intra.peff.net","threadId":"34119","inReplyTo":"1371105811-3112-1-git-send-email-oystwa@gmail.com","subject":"Re: [PATCH RFC] show-branch: use pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-13T07:01:19Z","receivedAt":"2013-06-13T07:01:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 13, 2013 at 08:43:31AM +0200, Øystein Walle wrote:\n\n> This is for consistency with other porcelain commands such as 'log'.\n\nI do not use show-branch myself, but being consistent with the other\nporcelain commands makes sense to me.\n\n> I marked this as an RFC because of Jeff King's comments in\n> daa0c3d97 where I got the impression this this might not be a good idea.\n> However I haven't found any bugs and all the tests pass. It is more a huble\n> suggestion than anything but I thought I might as well send it as a patch.\n\nI don't think the problems described in daa0c3d97 should be an issue for\nus, as the purpose of that commit was to delay the color decision until\nthe last minute. That helps commands which load color config before\nhaving decided on whether to use a pager. In other words, it covers the\nexact situation you introduce here:\n\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index 90fc6b1..bd3e10c 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -683,6 +683,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n>  \t};\n>  \n>  \tgit_config(git_show_branch_config, NULL);\n> +\tsetup_pager();\n\nSo I think your patch is fine with respect to those problems.\n\nHowever, I do not see any need for show_branch to delay its pager setup\nat all. Commands like \"git diff\" and \"git log\" must do so, because they\ndo not know whether they want a pager or not until after parsing\ncommand-line arguments. But in this case we are always starting the\npager.\n\nWould it make more sense to just set the USE_PAGER flag in the\n\"show-branch\" entry in git.c (see the \"shortlog\" entry for an example)?\n\n-Peff\n"},{"id":"220698","messageId":"1371110238-6910-1-git-send-email-oystwa@gmail.com","threadId":"34119","inReplyTo":"20130613070118.GA23235@sigill.intra.peff.net","subject":"[PATCH v2] show-branch: use pager","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2013-06-13T07:57:18Z","receivedAt":"2013-06-13T07:57:18Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"This is for consistency with other porcelain commands such as 'log'.\n\nSigned-off-by: Øystein Walle <oystwa@gmail.com>\n---\nHi, Jeff\n\nThanks for the (fast!) feedback and good to hear it won't cause any trouble.\n\nI hadn't actually noticed this mechanism of setting up the pager before now but\nI fully agree. Here's an updated version. \n\nØsse\n\n git.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git.c b/git.c\nindex 4359086..6d1f6ca 100644\n--- a/git.c\n+++ b/git.c\n@@ -406,7 +406,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"send-pack\", cmd_send_pack, RUN_SETUP },\n \t\t{ \"shortlog\", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },\n \t\t{ \"show\", cmd_show, RUN_SETUP },\n-\t\t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n+\t\t{ \"show-branch\", cmd_show_branch, RUN_SETUP | USE_PAGER },\n \t\t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n \t\t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n-- \n1.8.2.2\n"},{"id":"220703","messageId":"vpq1u86pd8q.fsf@anie.imag.fr","threadId":"34119","inReplyTo":"1371110238-6910-1-git-send-email-oystwa@gmail.com","subject":"Re: [PATCH v2] show-branch: use pager","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-13T09:32:37Z","receivedAt":"2013-06-13T09:32:37Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Øystein Walle <oystwa@gmail.com> writes:\n\n> This is for consistency with other porcelain commands such as 'log'.\n\nI don't think consistency with other porcelain is a sufficient argument.\nMany commands purposely don't use the pager by default because they will\nnormally have a short output.\n\nUsers can already set \"pager.show-branch\" to get the behavior you\nintroduce in the patch. The question is more: will users prefer having\nthe pager by default for this particular command? I don't use\nshow-branch enough to answer by myself, but probably the answer is yes.\n\n(This is not an objection, just to make sure you have all the elements)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"220744","messageId":"7vobb9vrt7.fsf@alter.siamese.dyndns.org","threadId":"34119","inReplyTo":"1371105811-3112-1-git-send-email-oystwa@gmail.com","subject":"Re: [PATCH RFC] show-branch: use pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-13T17:33:40Z","receivedAt":"2013-06-13T17:33:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle <oystwa@gmail.com> writes:\n\n> This is for consistency with other porcelain commands such as 'log'.\n>\n> Signed-off-by: Øystein Walle <oystwa@gmail.com>\n> ---\n> The rationale for this patch I hope is consicely explained in the commit\n> message. I was rather surprised it didn't use a pager as I've gotten used to it\n> for most commands.\n>\n> I marked this as an RFC because of Jeff King's comments in\n> daa0c3d97 where I got the impression this this might not be a good idea.\n> However I haven't found any bugs and all the tests pass.\n\nThe tests are run largely without tty to allow them to run\nunattended, aren't they?\n\nI think it makes a lot of sense to use pager by default for the\nnormal show-branch output.  I however do not think pager should\napply to other modes (e.g. --independent, --merge-base).\n\nBut the use of these other modes are meant to be on the upstream\nside of a pipe or to be written out to a file, so a blanket call to\nsetup_pager() before you even discover what mode we are in would not\nhurt in practice.\n"}]}