{"thread":{"id":"45452","subject":"[PATCH] log: if --decorate is not given, default to --decorate=auto","startedAt":"2017-03-21T05:54:13Z","lastAt":"2017-03-24T19:13:32Z","messageCount":13,"participants":["Alex Henrie","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"314777","messageId":"20170321055203.26488-1-alexhenrie24@gmail.com","threadId":"45452","inReplyTo":null,"subject":"[PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2017-03-21T05:52:03Z","receivedAt":"2017-03-21T05:54:13Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Git's branching and merging system can be confusing, especially to new\nusers. When teaching people Git, I tell them to set log.decorate=auto.\nThis preference greatly improves the user's awareness of the local and\nremote branches. So for the sake of user friendliness, I'd like to\nchange the default from log.decorate=no to log.decorate=auto.\n\nSigned-off-by: Alex Henrie \n---\n builtin/log.c | 9 ++++++++-\n t/t4202-log.sh | 2 +-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 281af8c1e..ddb4515dc 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -52,6 +52,11 @@ struct line_opt_callback_data {\n \tstruct string_list args;\n };\n \n+static int auto_decoration_style()\n+{\n+\treturn (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;\n+}\n+\n static int parse_decoration_style(const char *var, const char *value)\n {\n \tswitch (git_config_maybe_bool(var, value)) {\n@@ -67,7 +72,7 @@ static int parse_decoration_style(const char *var, const char *value)\n \telse if (!strcmp(value, \"short\"))\n \t\treturn DECORATE_SHORT_REFS;\n \telse if (!strcmp(value, \"auto\"))\n-\t\treturn (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;\n+\t\treturn auto_decoration_style();\n \treturn -1;\n }\n \n@@ -405,6 +410,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\tif (decoration_style < 0)\n \t\t\tdecoration_style = 0; /* maybe warn? */\n \t\treturn 0;\n+\t} else {\n+\t\tdecoration_style = auto_decoration_style();\n \t}\n \tif (!strcmp(var, \"log.showroot\")) {\n \t\tdefault_show_root = git_config_bool(var, value);\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 48b55bfd2..3aa8daba3 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -520,7 +520,7 @@ test_expect_success 'log --graph with merge' '\n '\n \n test_expect_success 'log.decorate configuration' '\n-\tgit log --oneline >expect.none &&\n+\tgit log --oneline --no-decorate >expect.none &&\n \tgit log --oneline --decorate >expect.short &&\n \tgit log --oneline --decorate=full >expect.full &&\n \n-- \n2.12.0\n\n"},{"id":"314836","messageId":"xmqqinn2sfdw.fsf@gitster.mtv.corp.google.com","threadId":"45452","inReplyTo":"20170321055203.26488-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T17:34:19Z","receivedAt":"2017-03-21T18:27:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> Git's branching and merging system can be confusing, especially to new\n> users. When teaching people Git, I tell them to set log.decorate=auto.\n> This preference greatly improves the user's awareness of the local and\n> remote branches. So for the sake of user friendliness, I'd like to\n> change the default from log.decorate=no to log.decorate=auto.\n>\n> Signed-off-by: Alex Henrie \n\nI guess this is an analog of making color.ui default to \"auto\" we\ndid earlier. After having shipped with a more conservative default\nwhile letting willing users experiment/experience to help us get the\nkinks out of the \"auto\" setting, it probably is about time to flip\nthe default.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 48b55bfd2..3aa8daba3 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -520,7 +520,7 @@ test_expect_success 'log --graph with merge' '\n> '\n> \n> test_expect_success 'log.decorate configuration' '\n> -\tgit log --oneline >expect.none &&\n> +\tgit log --oneline --no-decorate >expect.none &&\n> \tgit log --oneline --decorate >expect.short &&\n> \tgit log --oneline --decorate=full >expect.full &&\n\nThis ensures that an explicit --no-decorate from the command line\ndoes give \"none\" output, which we failed to do so far, and is a good\nchange. Don't we also need a _new_ test to ensure that \"auto\" kicks\nin without any explicit request? Knowing the implementation that\npager-in-use triggers the \"auto\" behaviour, perhaps testing the\noutput from \"git -p log\" would be sufficient?\n\n"},{"id":"314878","messageId":"xmqqzigemfij.fsf@gitster.mtv.corp.google.com","threadId":"45452","inReplyTo":"xmqqinn2sfdw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T22:28:04Z","receivedAt":"2017-03-21T22:34:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> test_expect_success 'log.decorate configuration' '\n>> -\tgit log --oneline >expect.none &&\n>> +\tgit log --oneline --no-decorate >expect.none &&\n>> \tgit log --oneline --decorate >expect.short &&\n>> \tgit log --oneline --decorate=full >expect.full &&\n>\n> This ensures that an explicit --no-decorate from the command line\n> does give \"none\" output, which we failed to do so far, and is a good\n> change. Don't we also need a _new_ test to ensure that \"auto\" kicks\n> in without any explicit request? Knowing the implementation that\n> pager-in-use triggers the \"auto\" behaviour, perhaps testing the\n> output from \"git -p log\" would be sufficient?\n\nBTW,\n\n> \n> +static int auto_decoration_style()\n> +{\n> +\treturn (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;\n> +}\n\nFYI, I fixed this to\n\n\tstatic int auto_decoration_style(void)\n\nwhile queuing to make it compile.\n"},{"id":"314892","messageId":"CAMMLpeQZXzPtN+mmtRS33vbT6hFz8p0qqjFMhz9tUHk747=Z3Q@mail.gmail.com","threadId":"45452","inReplyTo":"xmqqzigemfij.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2017-03-22T05:47:23Z","receivedAt":"2017-03-22T05:48:41Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"2017-03-21 16:28 GMT-06:00 Junio C Hamano :\n> Junio C Hamano writes:\n>\n>>> test_expect_success 'log.decorate configuration' '\n>>> - git log --oneline >expect.none &&\n>>> + git log --oneline --no-decorate >expect.none &&\n>>> git log --oneline --decorate >expect.short &&\n>>> git log --oneline --decorate=full >expect.full &&\n>>\n>> This ensures that an explicit --no-decorate from the command line\n>> does give \"none\" output, which we failed to do so far, and is a good\n>> change. Don't we also need a _new_ test to ensure that \"auto\" kicks\n>> in without any explicit request? Knowing the implementation that\n>> pager-in-use triggers the \"auto\" behaviour, perhaps testing the\n>> output from \"git -p log\" would be sufficient?\n>\n> BTW,\n>\n>>\n>> +static int auto_decoration_style()\n>> +{\n>> + return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;\n>> +}\n>\n> FYI, I fixed this to\n>\n> static int auto_decoration_style(void)\n>\n> while queuing to make it compile.\n\nNo problem. Do I need to submit a second version of the patch with a\ntest for `git -p log`?\n\n-Alex\n"},{"id":"314919","messageId":"xmqqmvcdmeuv.fsf@gitster.mtv.corp.google.com","threadId":"45452","inReplyTo":"CAMMLpeQZXzPtN+mmtRS33vbT6hFz8p0qqjFMhz9tUHk747=Z3Q@mail.gmail.com","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T16:54:32Z","receivedAt":"2017-03-22T17:02:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> 2017-03-21 16:28 GMT-06:00 Junio C Hamano :\n>> Junio C Hamano writes:\n>>\n>>>> test_expect_success 'log.decorate configuration' '\n>>>> - git log --oneline >expect.none &&\n>>>> + git log --oneline --no-decorate >expect.none &&\n>>>> git log --oneline --decorate >expect.short &&\n>>>> git log --oneline --decorate=full >expect.full &&\n>>>\n>>> This ensures that an explicit --no-decorate from the command line\n>>> does give \"none\" output, which we failed to do so far, and is a good\n>>> change. Don't we also need a _new_ test to ensure that \"auto\" kicks\n>>> in without any explicit request? Knowing the implementation that\n>>> pager-in-use triggers the \"auto\" behaviour, perhaps testing the\n>>> output from \"git -p log\" would be sufficient?\n>>\n>> BTW,\n>>\n>>>\n>>> +static int auto_decoration_style()\n>>> +{\n>>> + return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;\n>>> +}\n>>\n>> FYI, I fixed this to\n>>\n>> static int auto_decoration_style(void)\n>>\n>> while queuing to make it compile.\n>\n> No problem. Do I need to submit a second version of the patch with a\n> test for `git -p log`?\n\nYou do want to protect this \"without an option, we default to\n'auto'\" feature from future breakage, no?\n\nThanks.\n"},{"id":"315078","messageId":"CAMMLpeR-zCpL5Gx=BoK8G9_wL2TBe-wD3VnsAShAuVVzS=Nirg@mail.gmail.com","threadId":"45452","inReplyTo":"xmqqmvcdmeuv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2017-03-23T15:07:15Z","receivedAt":"2017-03-23T15:07:57Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"2017-03-22 10:54 GMT-06:00 Junio C Hamano :\n> Alex Henrie writes:\n>> No problem. Do I need to submit a second version of the patch with a\n>> test for `git -p log`?\n>\n> You do want to protect this \"without an option, we default to\n> 'auto'\" feature from future breakage, no?\n\nYes, but I need to know whether you want a v2 of this patch with all\nof the changes including the new test, or a second patch that depends\non the first patch and only adds the new test.\n\n-Alex\n"},{"id":"315104","messageId":"xmqqfui4f0pp.fsf@gitster.mtv.corp.google.com","threadId":"45452","inReplyTo":"CAMMLpeR-zCpL5Gx=BoK8G9_wL2TBe-wD3VnsAShAuVVzS=Nirg@mail.gmail.com","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T15:54:10Z","receivedAt":"2017-03-23T15:54:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> 2017-03-22 10:54 GMT-06:00 Junio C Hamano :\n>> Alex Henrie writes:\n>>> No problem. Do I need to submit a second version of the patch with a\n>>> test for `git -p log`?\n>>\n>> You do want to protect this \"without an option, we default to\n>> 'auto'\" feature from future breakage, no?\n>\n> Yes, but I need to know whether you want a v2 of this patch with all\n> of the changes including the new test, or a second patch that depends\n> on the first patch and only adds the new test.\n\nSorry, I misunderstood the question.\n\nIn general, we prefer to have tests that protects the updated\nbehaviour in the same patch that makes code changes that brings in\nthe new behaviour, i.e. a single v2 patch with new test would be\nmore appropriate in this case.\n\nWhen people work on a large bugfix, especially one that needs\nmultiple steps, we sometimes see a patch that adds new tests that\ndescribe the desired behaviour as failing tests first, and then\nsubsequent patches to the code to update the behaviour flip\n\"test_expect_failure\" to \"test_expect_success\" as they fix the\nbehaviour. But for a small change like this one, that approach is\ninappropriate.\n\nWhen a patch that was reviewed, deemed good enough and has been\nalready merged to the 'next' branch later turns out that it needs\nfurther work (like \"we do need some tests\"), we do such necessary\nupdates as separate follow-up patches, simply because we promise\nthat 'next' won't be rewound and are not allowed to replace patches.\nBut this one is not yet in 'next', so we can freely take a\nreplacement patch.\n\nHope this message makes it clear enough?\n\nThanks.\n\n"},{"id":"315113","messageId":"CAMMLpeRtzYM3a639DcNbhqAmz3dNkRebvQps-gusEKx+eQqPgQ@mail.gmail.com","threadId":"45452","inReplyTo":"xmqqfui4f0pp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2017-03-23T16:52:34Z","receivedAt":"2017-03-23T16:53:18Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"2017-03-23 9:54 GMT-06:00 Junio C Hamano :\n> Alex Henrie writes:\n>\n>> 2017-03-22 10:54 GMT-06:00 Junio C Hamano :\n>>> Alex Henrie writes:\n>>>> No problem. Do I need to submit a second version of the patch with a\n>>>> test for `git -p log`?\n>>>\n>>> You do want to protect this \"without an option, we default to\n>>> 'auto'\" feature from future breakage, no?\n>>\n>> Yes, but I need to know whether you want a v2 of this patch with all\n>> of the changes including the new test, or a second patch that depends\n>> on the first patch and only adds the new test.\n>\n> Sorry, I misunderstood the question.\n>\n> In general, we prefer to have tests that protects the updated\n> behaviour in the same patch that makes code changes that brings in\n> the new behaviour, i.e. a single v2 patch with new test would be\n> more appropriate in this case.\n>\n> When people work on a large bugfix, especially one that needs\n> multiple steps, we sometimes see a patch that adds new tests that\n> describe the desired behaviour as failing tests first, and then\n> subsequent patches to the code to update the behaviour flip\n> \"test_expect_failure\" to \"test_expect_success\" as they fix the\n> behaviour. But for a small change like this one, that approach is\n> inappropriate.\n>\n> When a patch that was reviewed, deemed good enough and has been\n> already merged to the 'next' branch later turns out that it needs\n> further work (like \"we do need some tests\"), we do such necessary\n> updates as separate follow-up patches, simply because we promise\n> that 'next' won't be rewound and are not allowed to replace patches.\n> But this one is not yet in 'next', so we can freely take a\n> replacement patch.\n>\n> Hope this message makes it clear enough?\n\nYes, that makes sense. I assume that when you talk about 'next', you\nmean 'master'?\n\nUnfortunately, I think I found a bug. Even when using `git -p`, the\nfunction pager_in_use() always returns false if the output is not a\nTTY. So, `isatty(1) || pager_in_use()` and `color_stdout_is_tty ||\n(pager_in_use() && pager_use_color)` are redundant.\n\nIf we want to use `git -p log` in a test, we'll have to change the\nbehavior of pager_in_use(). Alternatively, we could use\n`GIT_PAGER_IN_USE=1 git log` instead.\n\n-Alex\n"},{"id":"315135","messageId":"xmqq8tnveuqv.fsf@gitster.mtv.corp.google.com","threadId":"45452","inReplyTo":"CAMMLpeRtzYM3a639DcNbhqAmz3dNkRebvQps-gusEKx+eQqPgQ@mail.gmail.com","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T18:03:04Z","receivedAt":"2017-03-23T18:03:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> Yes, that makes sense. I assume that when you talk about 'next', you\n> mean 'master'?\n\nNo, I do mean 'next'. See \"A note from the maintainer\" post that\nare sent to the list every once in a while (i.e. after a new release\nis tagged) for the project structure.\n\n https://public-inbox.org/git/xmqqy3vztypi.fsf@gitster.mtv.corp.google.com/\n\n> If we want to use `git -p log` in a test, we'll have to change the\n> behavior of pager_in_use(). Alternatively, we could use\n> `GIT_PAGER_IN_USE=1 git log` instead.\n\nTesting \"git -p\" is not the goal; testing that decorate defaults to\nauto during an interactive session is. We could run tests under pty\nlike t7006 does using lib-terminal.sh if we really want to emulate\nan interactive session. \n\nExporting GIT_PAGER_IN_USE may be sufficient for the purpose of\nconvincing the command to be in an interactive session for this\ntest, even though it feels a bit too brittle to depend on the\ninternal implementation detail.\n\nThanks.\n"},{"id":"315228","messageId":"CAMMLpeSawRe7SLmFPzZ+EerQvtcyvObYyv=iWMwhiRvjNPreHw@mail.gmail.com","threadId":"45452","inReplyTo":"xmqq8tnveuqv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2017-03-24T05:45:18Z","receivedAt":"2017-03-24T05:45:52Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"2017-03-23 12:03 GMT-06:00 Junio C Hamano :\n> Alex Henrie writes:\n>\n>> Yes, that makes sense. I assume that when you talk about 'next', you\n>> mean 'master'?\n>\n> No, I do mean 'next'. See \"A note from the maintainer\" post that\n> are sent to the list every once in a while (i.e. after a new release\n> is tagged) for the project structure.\n>\n> https://public-inbox.org/git/xmqqy3vztypi.fsf@gitster.mtv.corp.google.com/\n\nThat document was very helpful, thanks.\n\n>> If we want to use `git -p log` in a test, we'll have to change the\n>> behavior of pager_in_use(). Alternatively, we could use\n>> `GIT_PAGER_IN_USE=1 git log` instead.\n>\n> Testing \"git -p\" is not the goal; testing that decorate defaults to\n> auto during an interactive session is. We could run tests under pty\n> like t7006 does using lib-terminal.sh if we really want to emulate\n> an interactive session.\n>\n> Exporting GIT_PAGER_IN_USE may be sufficient for the purpose of\n> convincing the command to be in an interactive session for this\n> test, even though it feels a bit too brittle to depend on the\n> internal implementation detail.\n\nOkay, I've now written a lib-terminal test for git log. I'll send the\nrevised patch momentarily.\n\n-Alex\n"},{"id":"315321","messageId":"20170324185543.uq4clqyvbdkahbzo@sigill.intra.peff.net","threadId":"45452","inReplyTo":"CAMMLpeRtzYM3a639DcNbhqAmz3dNkRebvQps-gusEKx+eQqPgQ@mail.gmail.com","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T18:55:44Z","receivedAt":"2017-03-24T18:55:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 23, 2017 at 10:52:34AM -0600, Alex Henrie wrote:\n\n> Unfortunately, I think I found a bug. Even when using `git -p`, the\n> function pager_in_use() always returns false if the output is not a\n> TTY. So, `isatty(1) || pager_in_use()` and `color_stdout_is_tty ||\n> (pager_in_use() && pager_use_color)` are redundant.\n> \n> If we want to use `git -p log` in a test, we'll have to change the\n> behavior of pager_in_use(). Alternatively, we could use\n> `GIT_PAGER_IN_USE=1 git log` instead.\n\nI see you ended up with a test that uses test_terminal, which is much\nbetter (and your patch looks good to me).\n\nBut I was concerned that there might be a bug in pager_in_use(), so I\ndug into it a little. I think the code there is correct; it's just\nrelaying the environment variable's value. Likewise, on the setting\nside, I think the code is correct. We set the environment variable\nwhenever we start the pager in setup_pager().\n\nI think what is confusing is that \"git -p\" does _not_ mean\n\"unconditionally use a pager\". It means \"start a pager if stdout is a\nterminal, even if this command is not usually paged\". So something like\n\"git -p log >actual\" is correct not to trigger pager_in_use().\n\nI also double-checked the documentation, which covers this case\naccurately. So I think all is well, and there's nothing to fix. You\nmight want an option for \"even though stdout is not a tty pretend like a\npager is in use\", but that is exactly what GIT_PAGER_IN_USE=1 is for.\n\n-Peff\n"},{"id":"315322","messageId":"20170324185912.rvzi46kr5ldxnehm@sigill.intra.peff.net","threadId":"45452","inReplyTo":"20170324185543.uq4clqyvbdkahbzo@sigill.intra.peff.net","subject":"[PATCH] pager_in_use: use git_env_bool()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T18:59:12Z","receivedAt":"2017-03-24T18:59:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2017 at 02:55:43PM -0400, Jeff King wrote:\n\n> But I was concerned that there might be a bug in pager_in_use(), so I\n> dug into it a little. I think the code there is correct; [...]\n\nI did see this small cleanup opportunity, though.\n\n-- >8 --\nSubject: [PATCH] pager_in_use: use git_env_bool()\n\nThe pager_in_use() function predates git_env_bool(), but\nends up doing the same thing. Let's make use of the latter,\nwhich is shorter and less repetitive.\n\nSigned-off-by: Jeff King \n---\n pager.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 73ca8bc3b..c113d898a 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -135,9 +135,7 @@ void setup_pager(void)\n \n int pager_in_use(void)\n {\n-\tconst char *env;\n-\tenv = getenv(\"GIT_PAGER_IN_USE\");\n-\treturn env ? git_config_bool(\"GIT_PAGER_IN_USE\", env) : 0;\n+\treturn git_env_bool(\"GIT_PAGER_IN_USE\", 0);\n }\n \n /*\n-- \n2.12.1.843.g1937c56c2\n\n"},{"id":"315324","messageId":"xmqqr31m7ajw.fsf@gitster.mtv.corp.google.com","threadId":"45452","inReplyTo":"20170324185543.uq4clqyvbdkahbzo@sigill.intra.peff.net","subject":"Re: [PATCH] log: if --decorate is not given, default to --decorate=auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-24T19:13:23Z","receivedAt":"2017-03-24T19:13:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I see you ended up with a test that uses test_terminal, which is much\n> better (and your patch looks good to me).\n>\n> But I was concerned that there might be a bug in pager_in_use(), so I\n> dug into it a little. I think the code there is correct; it's just\n> relaying the environment variable's value. Likewise, on the setting\n> side, I think the code is correct. We set the environment variable\n> whenever we start the pager in setup_pager().\n>\n> I think what is confusing is that \"git -p\" does _not_ mean\n> \"unconditionally use a pager\". It means \"start a pager if stdout is a\n> terminal, even if this command is not usually paged\". So something like\n> \"git -p log >actual\" is correct not to trigger pager_in_use().\n>\n> I also double-checked the documentation, which covers this case\n> accurately. So I think all is well, and there's nothing to fix. You\n> might want an option for \"even though stdout is not a tty pretend like a\n> pager is in use\", but that is exactly what GIT_PAGER_IN_USE=1 is for.\n\nThanks ;-).\n"}]}