{"thread":{"id":"25771","subject":"[PATCH 1/2] log.decorate: accept 0/1 bool values","startedAt":"2010-11-17T17:00:45Z","lastAt":"2010-11-18T17:14:43Z","messageCount":6,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"156055","messageId":"20101117170045.GA4108@sigill.intra.peff.net","threadId":"25771","inReplyTo":null,"subject":"[PATCH 1/2] log.decorate: accept 0/1 bool values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-11-17T17:00:45Z","receivedAt":"2010-11-17T17:00:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We explicitly document \"0\" and \"1\" as synonyms for \"false\"\nand \"true\" in boolean config options. However, we don't\nactually handle those values in git_config_maybe_bool.\n\nIn most cases this works fine, as we call git_config_bool,\nwhich in turn calls git_config_bool_or_int, which in turn\ncalls git_config_maybe_bool. Values of 0/1 are considered\n\"not bool\", but their integer values end up being converted\nto the corresponding boolean values.\n\nHowever, the log.decorate code looks for maybe_bool\nexplicitly, so that it can fall back to the \"short\" and\n\"full\" strings. It does not handle 0/1 at all, and considers\nthem invalid values.\n\nWe cannot simply add 0/1 support to git_config_maybe_bool.\nThat would confuse git_config_bool_or_int, which may want to\ndistinguish the integer values \"0\" and \"1\" from bools.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI don't know that anyone explicitly cares about distinguishing \"0\" from\n\"false\", but we do expose this interface via \"git config --bool-or-int\",\nand we test for it explicitly in t1300. So I kept the existing behavior\nintact in that instance.\n\nNote that another way to solve this would be to check explicitly for\n\"short\" or \"full\" and then just call git_config_bool itself. That works\nin this case, but not for an open-ended case (i.e., where you can take\neither a bool, or any arbitrary string). And I needed that case for my\npatch 2/2. :)\n\n config.c       |   16 ++++++++++++++--\n t/t4202-log.sh |    9 +++++++++\n 2 files changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 4b0a820..9918b93 100644\n--- a/config.c\n+++ b/config.c\n@@ -410,7 +410,7 @@ unsigned long git_config_ulong(const char *name, const char *value)\n \treturn ret;\n }\n \n-int git_config_maybe_bool(const char *name, const char *value)\n+static int git_config_maybe_bool_text(const char *name, const char *value)\n {\n \tif (!value)\n \t\treturn 1;\n@@ -427,9 +427,21 @@ int git_config_maybe_bool(const char *name, const char *value)\n \treturn -1;\n }\n \n+int git_config_maybe_bool(const char *name, const char *value)\n+{\n+\tint v = git_config_maybe_bool_text(name, value);\n+\tif (0 <= v)\n+\t\treturn v;\n+\tif (!strcmp(value, \"0\"))\n+\t\treturn 0;\n+\tif (!strcmp(value, \"1\"))\n+\t\treturn 1;\n+\treturn -1;\n+}\n+\n int git_config_bool_or_int(const char *name, const char *value, int *is_bool)\n {\n-\tint v = git_config_maybe_bool(name, value);\n+\tint v = git_config_maybe_bool_text(name, value);\n \tif (0 <= v) {\n \t\t*is_bool = 1;\n \t\treturn v;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 2e51356..2043bb8 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -422,6 +422,15 @@ test_expect_success 'log.decorate configuration' '\n \ttest_cmp expect.full actual &&\n \n \tgit config --unset-all log.decorate &&\n+\tgit config log.decorate 1 &&\n+\tgit log --oneline >actual &&\n+\ttest_cmp expect.short actual &&\n+\tgit log --oneline --decorate=full >actual &&\n+\ttest_cmp expect.full actual &&\n+\tgit log --oneline --decorate=no >actual &&\n+\ttest_cmp expect.none actual &&\n+\n+\tgit config --unset-all log.decorate &&\n \tgit config log.decorate short &&\n \tgit log --oneline >actual &&\n \ttest_cmp expect.short actual &&\n-- \n1.7.3.2.362.g308e9\n"},{"id":"156056","messageId":"20101117170411.GA4163@sigill.intra.peff.net","threadId":"25771","inReplyTo":"20101117170045.GA4108@sigill.intra.peff.net","subject":"[PATCH 2/2] allow command-specific pagers in pager.<cmd>","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-11-17T17:04:12Z","receivedAt":"2010-11-17T17:04:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A user may want different pager settings or even a\ndifferent pager for various subcommands (e.g., because they\nuse different less settings for \"log\" vs \"diff\", or because\nthey have a pager that interprets only log output but not\nother commands).\n\nThis patch extends the pager.<cmd> syntax to support not\nonly boolean to-page-or-not-to-page, but also to specify a\npager just for a specific command.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is a repost of\n\n  http://article.gmane.org/gmane.comp.version-control.git/158051\n\nThe patch text is the same, but without patch 1 in this series, there is\na regression (anyone with \"pager.foo = 0\" would stop interpreting \"0\" as\na bool, and start interpreting it as the command \"0\").\n\n Documentation/config.txt |   12 +++++++-----\n git.c                    |   21 ++++++++++++++++-----\n t/t7006-pager.sh         |   29 +++++++++++++++++++++++++++++\n 3 files changed, 52 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 538ebb5..b834c4e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1531,11 +1531,13 @@ pack.packSizeLimit::\n \tsupported.\n \n pager.<cmd>::\n-\tAllows turning on or off pagination of the output of a\n-\tparticular git subcommand when writing to a tty.  If\n-\t`\\--paginate` or `\\--no-pager` is specified on the command line,\n-\tit takes precedence over this option.  To disable pagination for\n-\tall commands, set `core.pager` or `GIT_PAGER` to `cat`.\n+\tIf the value is boolean, turns on or off pagination of the\n+\toutput of a particular git subcommand when writing to a tty.\n+\tOtherwise, turns on pagination for the subcommand using the\n+\tpager specified by the value of `pager.<cmd>`.  If `\\--paginate`\n+\tor `\\--no-pager` is specified on the command line, it takes\n+\tprecedence over this option.  To disable pagination for all\n+\tcommands, set `core.pager` or `GIT_PAGER` to `cat`.\n \n pretty.<name>::\n \tAlias for a --pretty= format string, as specified in\ndiff --git a/git.c b/git.c\nindex 0409ac9..81221cf 100644\n--- a/git.c\n+++ b/git.c\n@@ -19,14 +19,22 @@ static struct startup_info git_startup_info;\n static int use_pager = -1;\n struct pager_config {\n \tconst char *cmd;\n-\tint val;\n+\tint want;\n+\tchar *value;\n };\n \n static int pager_command_config(const char *var, const char *value, void *data)\n {\n \tstruct pager_config *c = data;\n-\tif (!prefixcmp(var, \"pager.\") && !strcmp(var + 6, c->cmd))\n-\t\tc->val = git_config_bool(var, value);\n+\tif (!prefixcmp(var, \"pager.\") && !strcmp(var + 6, c->cmd)) {\n+\t\tint b = git_config_maybe_bool(var, value);\n+\t\tif (b >= 0)\n+\t\t\tc->want = b;\n+\t\telse {\n+\t\t\tc->want = 1;\n+\t\t\tc->value = xstrdup(value);\n+\t\t}\n+\t}\n \treturn 0;\n }\n \n@@ -35,9 +43,12 @@ int check_pager_config(const char *cmd)\n {\n \tstruct pager_config c;\n \tc.cmd = cmd;\n-\tc.val = -1;\n+\tc.want = -1;\n+\tc.value = NULL;\n \tgit_config(pager_command_config, &c);\n-\treturn c.val;\n+\tif (c.value)\n+\t\tpager_program = c.value;\n+\treturn c.want;\n }\n \n static void commit_pager_choice(void) {\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex fb744e3..49a6261 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -435,4 +435,33 @@ test_core_pager_subdir    expect_success 'git -p shortlog'\n test_core_pager_subdir    expect_success test_must_fail \\\n \t\t\t\t\t 'git -p apply </dev/null'\n \n+test_expect_success TTY 'command-specific pager' '\n+\tunset PAGER GIT_PAGER;\n+\techo \"foo:initial\" >expect &&\n+\t>actual &&\n+\tgit config --unset core.pager &&\n+\tgit config pager.log \"sed s/^/foo:/ >actual\" &&\n+\ttest_terminal git log --format=%s -1 &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success TTY 'command-specific pager overrides core.pager' '\n+\tunset PAGER GIT_PAGER;\n+\techo \"foo:initial\" >expect &&\n+\t>actual &&\n+\tgit config core.pager \"exit 1\"\n+\tgit config pager.log \"sed s/^/foo:/ >actual\" &&\n+\ttest_terminal git log --format=%s -1 &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success TTY 'command-specific pager overridden by environment' '\n+\tGIT_PAGER=\"sed s/^/foo:/ >actual\" && export GIT_PAGER &&\n+\t>actual &&\n+\techo \"foo:initial\" >expect &&\n+\tgit config pager.log \"exit 1\" &&\n+\ttest_terminal git log --format=%s -1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.3.2.362.g308e9\n"},{"id":"156062","messageId":"7v4obfg2td.fsf@alter.siamese.dyndns.org","threadId":"25771","inReplyTo":"20101117170045.GA4108@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] log.decorate: accept 0/1 bool values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-17T19:36:30Z","receivedAt":"2010-11-17T19:36:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We cannot simply add 0/1 support to git_config_maybe_bool.\n> That would confuse git_config_bool_or_int, which may want to\n> distinguish the integer values \"0\" and \"1\" from bools.\n\n... e.g. number one spelled as \"1\" do mean one not true in contexts like\nstatus.submodulesummary, where \"true\" means \"unlimited\" and \"1\" means\n\"limit to one\".  It surely is necessary to avoid breakage there.\n\nA caller that uses git_config_maybe_bool() expects to see 0/1 when the\ninput looks like a boolean, and your patch looks like the right thing to\ndo.\n\nThe repertoire of parsers that involve elements that are possibly boolean\nare now:\n\n - git_config_bool(): takes \"0/false/no/...\" or \"$n/true/yes/...\" (where\n   any non-zero number $n is taken as true [*1*]), or more traditional\n\n   [section]\n       var\n\n   without any equal sign, which means true.  Errors out if the input is\n   not a boolean.\n\n - git_config_maybe_bool(): similar, and returns -1 (not a bool), 0\n   (false) and 1 (true).  \"0\" is false, \"1\" is true.  But all other\n   numbers are not boolean;\n\n - git_config_bool_or_int(): the caller can tell if the value was boolean.\n   \"0\" and \"1\" are integers, and not boolean.  Errors out if the input is\n   not bool nor int.\n\nPerhaps we would want to add Documentation/technical/api-config.txt\nsomeday?   Hint, hint....\n\n\n[Footnote]\n\n*1* I doubt anybody is insane enough to have an existing configuration\nfile that spells \"true\" with \"2\" (or \"100\"), so it might be ok to tighten\nthe rule after the fact a bit here to take only \"1\" as true from purist's\npoint of view, but I do not think the upside outweighs possible breakage.\n"},{"id":"156063","messageId":"20101117195259.GA13008@sigill.intra.peff.net","threadId":"25771","inReplyTo":"7v4obfg2td.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] log.decorate: accept 0/1 bool values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-11-17T19:52:59Z","receivedAt":"2010-11-17T19:52:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 17, 2010 at 11:36:30AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > We cannot simply add 0/1 support to git_config_maybe_bool.\n> > That would confuse git_config_bool_or_int, which may want to\n> > distinguish the integer values \"0\" and \"1\" from bools.\n> \n> ... e.g. number one spelled as \"1\" do mean one not true in contexts like\n> status.submodulesummary, where \"true\" means \"unlimited\" and \"1\" means\n> \"limit to one\".  It surely is necessary to avoid breakage there.\n\nMakes sense. I didn't even bother checking, as the fact that we export\nit as \"git config --bool-or-int\" was enough for me that we need to keep\nthe behavior the same.\n\n> A caller that uses git_config_maybe_bool() expects to see 0/1 when the\n> input looks like a boolean, and your patch looks like the right thing to\n> do.\n\nIt's possible we could have another type of caller, but I suspect they\nwould want git_config_bool_or_int instead. The exception would be one who\nwants a bool, an int, _or_ an arbitrary string. If somebody writes such\na thing, they are free to expose the new git_config_maybe_bool_text.\n\n> The repertoire of parsers that involve elements that are possibly boolean\n> are now:\n> \n>  - git_config_bool(): takes \"0/false/no/...\" or \"$n/true/yes/...\" (where\n>    any non-zero number $n is taken as true [*1*]), or more traditional\n> \n>    [section]\n>        var\n> \n>    without any equal sign, which means true.  Errors out if the input is\n>    not a boolean.\n> \n>  - git_config_maybe_bool(): similar, and returns -1 (not a bool), 0\n>    (false) and 1 (true).  \"0\" is false, \"1\" is true.  But all other\n>    numbers are not boolean;\n\nYeah, I did notice that. Technically it is a regression in my 2/2, in\nthat \"pager.foo = 2\" used to mean \"use the pager for foo\" and now means\n\"use a pager called 2\". But I am willing to write that off as insanity,\nespecially since we never documented that behavior (and in fact we\nexplicitly document the allowable values as yes/no, 0/1, true/false, and\non/off).\n\nI don't think it is worth closing the hole for no reason on other config\noptions, but I am certainly fine with breaking it in the case of\npager.*.\n\n> Perhaps we would want to add Documentation/technical/api-config.txt\n> someday?   Hint, hint....\n\nI'll put it on my todo, right after refactoring the awful mess of the\nconfig code itself. :)\n\n-Peff\n"},{"id":"156112","messageId":"7vmxp6d0te.fsf@alter.siamese.dyndns.org","threadId":"25771","inReplyTo":"20101117195259.GA13008@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] log.decorate: accept 0/1 bool values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-18T17:00:13Z","receivedAt":"2010-11-18T17:00:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I don't think it is worth closing the hole for no reason on other config\n> options, but I am certainly fine with breaking it in the case of\n> pager.*.\n\nHmm, I guess that is fine, but will we hear \"Why does it behave\ndifferently only for pager.*\" down the line, just like the issue your\npatch 1/2 addressed, which was \"Why does it behave differently only for\nlog.decorate?\"\n"},{"id":"156113","messageId":"20101118171443.GA21474@sigill.intra.peff.net","threadId":"25771","inReplyTo":"7vmxp6d0te.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] log.decorate: accept 0/1 bool values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-11-18T17:14:43Z","receivedAt":"2010-11-18T17:14:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 18, 2010 at 09:00:13AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I don't think it is worth closing the hole for no reason on other config\n> > options, but I am certainly fine with breaking it in the case of\n> > pager.*.\n> \n> Hmm, I guess that is fine, but will we hear \"Why does it behave\n> differently only for pager.*\" down the line, just like the issue your\n> patch 1/2 addressed, which was \"Why does it behave differently only for\n> log.decorate?\"\n\nPerhaps, but the difference is that for log.decorate I had no answer\n(because it was not behaving as indicated by the documentation), whereas\nwith this I can with a good conscious say \"go away, you are relying\nridiculous undocumented behavior\".\n\nThat being said, it is much easier than I expected to make it Just Work\nthe same way, so perhaps we should do this on top:\n\n-- >8 --\nSubject: [PATCH] handle arbitrary ints in git_config_maybe_bool\n\nThis function recently gained the ability to recognize the\ndocumented \"0\" and \"1\" values as false/true. However, unlike\nregular git_config_bool, it did not treat arbitrary numbers\nas true. While this is undocumented and probably ridiculous\nfor somebody to rely on, it is safer to behave exactly as\ngit_config_bool would. Because git_config_maybe_bool can be\nused to retrofit new non-bool values onto existings bool\noptions, not behaving in exactly the same way is technically\na regression.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIt even saves 2 lines, so it _must_ be better. :)\n\n config.c |    8 +++-----\n 1 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex d3fa953..299ae80 100644\n--- a/config.c\n+++ b/config.c\n@@ -429,13 +429,11 @@ static int git_config_maybe_bool_text(const char *name, const char *value)\n \n int git_config_maybe_bool(const char *name, const char *value)\n {\n-\tint v = git_config_maybe_bool_text(name, value);\n+\tlong v = git_config_maybe_bool_text(name, value);\n \tif (0 <= v)\n \t\treturn v;\n-\tif (!strcmp(value, \"0\"))\n-\t\treturn 0;\n-\tif (!strcmp(value, \"1\"))\n-\t\treturn 1;\n+\tif (git_parse_long(value, &v))\n+\t\treturn !!v;\n \treturn -1;\n }\n \n-- \n1.7.3.2.510.g24900\n"}]}