[PATCH 1/2] log.decorate: accept 0/1 bool values

[PATCH 1/2] log.decorate: accept 0/1 bool values

[Jeff King]

We explicitly document "0" and "1" as synonyms for "false"
and "true" in boolean config options. However, we don't
actually handle those values in git_config_maybe_bool.

In most cases this works fine, as we call git_config_bool,
which in turn calls git_config_bool_or_int, which in turn
calls git_config_maybe_bool. Values of 0/1 are considered
"not bool", but their integer values end up being converted
to the corresponding boolean values.

However, the log.decorate code looks for maybe_bool
explicitly, so that it can fall back to the "short" and
"full" strings. It does not handle 0/1 at all, and considers
them invalid values.

We cannot simply add 0/1 support to git_config_maybe_bool.
That would confuse git_config_bool_or_int, which may want to
distinguish the integer values "0" and "1" from bools.

Signed-off-by: Jeff King <peff@peff.net>
---
I don't know that anyone explicitly cares about distinguishing "0" from
"false", but we do expose this interface via "git config --bool-or-int",
and we test for it explicitly in t1300. So I kept the existing behavior
intact in that instance.

Note that another way to solve this would be to check explicitly for
"short" or "full" and then just call git_config_bool itself. That works
in this case, but not for an open-ended case (i.e., where you can take
either a bool, or any arbitrary string). And I needed that case for my
patch 2/2. :)

 config.c       |   16 ++++++++++++++--
 t/t4202-log.sh |    9 +++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 4b0a820..9918b93 100644
--- a/config.c
+++ b/config.c
@@ -410,7 +410,7 @@ unsigned long git_config_ulong(const char *name, const char *value)
 	return ret;
 }
 
-int git_config_maybe_bool(const char *name, const char *value)
+static int git_config_maybe_bool_text(const char *name, const char *value)
 {
 	if (!value)
 		return 1;
@@ -427,9 +427,21 @@ int git_config_maybe_bool(const char *name, const char *value)
 	return -1;
 }
 
+int git_config_maybe_bool(const char *name, const char *value)
+{
+	int v = git_config_maybe_bool_text(name, value);
+	if (0 <= v)
+		return v;
+	if (!strcmp(value, "0"))
+		return 0;
+	if (!strcmp(value, "1"))
+		return 1;
+	return -1;
+}
+
 int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 {
-	int v = git_config_maybe_bool(name, value);
+	int v = git_config_maybe_bool_text(name, value);
 	if (0 <= v) {
 		*is_bool = 1;
 		return v;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 2e51356..2043bb8 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -422,6 +422,15 @@ test_expect_success 'log.decorate configuration' '
 	test_cmp expect.full actual &&
 
 	git config --unset-all log.decorate &&
+	git config log.decorate 1 &&
+	git log --oneline >actual &&
+	test_cmp expect.short actual &&
+	git log --oneline --decorate=full >actual &&
+	test_cmp expect.full actual &&
+	git log --oneline --decorate=no >actual &&
+	test_cmp expect.none actual &&
+
+	git config --unset-all log.decorate &&
 	git config log.decorate short &&
 	git log --oneline >actual &&
 	test_cmp expect.short actual &&
-- 
1.7.3.2.362.g308e9

[PATCH 2/2] allow command-specific pagers in pager.<cmd>

[Jeff King]

A user may want different pager settings or even a
different pager for various subcommands (e.g., because they
use different less settings for "log" vs "diff", or because
they have a pager that interprets only log output but not
other commands).

This patch extends the pager.<cmd> syntax to support not
only boolean to-page-or-not-to-page, but also to specify a
pager just for a specific command.

Signed-off-by: Jeff King <peff@peff.net>
---
This is a repost of

  http://article.gmane.org/gmane.comp.version-control.git/158051

The patch text is the same, but without patch 1 in this series, there is
a regression (anyone with "pager.foo = 0" would stop interpreting "0" as
a bool, and start interpreting it as the command "0").

 Documentation/config.txt |   12 +++++++-----
 git.c                    |   21 ++++++++++++++++-----
 t/t7006-pager.sh         |   29 +++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 538ebb5..b834c4e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1531,11 +1531,13 @@ pack.packSizeLimit::
 	supported.
 
 pager.<cmd>::
-	Allows turning on or off pagination of the output of a
-	particular git subcommand when writing to a tty.  If
-	`\--paginate` or `\--no-pager` is specified on the command line,
-	it takes precedence over this option.  To disable pagination for
-	all commands, set `core.pager` or `GIT_PAGER` to `cat`.
+	If the value is boolean, turns on or off pagination of the
+	output of a particular git subcommand when writing to a tty.
+	Otherwise, turns on pagination for the subcommand using the
+	pager specified by the value of `pager.<cmd>`.  If `\--paginate`
+	or `\--no-pager` is specified on the command line, it takes
+	precedence over this option.  To disable pagination for all
+	commands, set `core.pager` or `GIT_PAGER` to `cat`.
 
 pretty.<name>::
 	Alias for a --pretty= format string, as specified in
diff --git a/git.c b/git.c
index 0409ac9..81221cf 100644
--- a/git.c
+++ b/git.c
@@ -19,14 +19,22 @@ static struct startup_info git_startup_info;
 static int use_pager = -1;
 struct pager_config {
 	const char *cmd;
-	int val;
+	int want;
+	char *value;
 };
 
 static int pager_command_config(const char *var, const char *value, void *data)
 {
 	struct pager_config *c = data;
-	if (!prefixcmp(var, "pager.") && !strcmp(var + 6, c->cmd))
-		c->val = git_config_bool(var, value);
+	if (!prefixcmp(var, "pager.") && !strcmp(var + 6, c->cmd)) {
+		int b = git_config_maybe_bool(var, value);
+		if (b >= 0)
+			c->want = b;
+		else {
+			c->want = 1;
+			c->value = xstrdup(value);
+		}
+	}
 	return 0;
 }
 
@@ -35,9 +43,12 @@ int check_pager_config(const char *cmd)
 {
 	struct pager_config c;
 	c.cmd = cmd;
-	c.val = -1;
+	c.want = -1;
+	c.value = NULL;
 	git_config(pager_command_config, &c);
-	return c.val;
+	if (c.value)
+		pager_program = c.value;
+	return c.want;
 }
 
 static void commit_pager_choice(void) {
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index fb744e3..49a6261 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -435,4 +435,33 @@ test_core_pager_subdir    expect_success 'git -p shortlog'
 test_core_pager_subdir    expect_success test_must_fail \
 					 'git -p apply </dev/null'
 
+test_expect_success TTY 'command-specific pager' '
+	unset PAGER GIT_PAGER;
+	echo "foo:initial" >expect &&
+	>actual &&
+	git config --unset core.pager &&
+	git config pager.log "sed s/^/foo:/ >actual" &&
+	test_terminal git log --format=%s -1 &&
+	test_cmp expect actual
+'
+
+test_expect_success TTY 'command-specific pager overrides core.pager' '
+	unset PAGER GIT_PAGER;
+	echo "foo:initial" >expect &&
+	>actual &&
+	git config core.pager "exit 1"
+	git config pager.log "sed s/^/foo:/ >actual" &&
+	test_terminal git log --format=%s -1 &&
+	test_cmp expect actual
+'
+
+test_expect_success TTY 'command-specific pager overridden by environment' '
+	GIT_PAGER="sed s/^/foo:/ >actual" && export GIT_PAGER &&
+	>actual &&
+	echo "foo:initial" >expect &&
+	git config pager.log "exit 1" &&
+	test_terminal git log --format=%s -1 &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.3.2.362.g308e9

Re: [PATCH 1/2] log.decorate: accept 0/1 bool values

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We cannot simply add 0/1 support to git_config_maybe_bool.
> That would confuse git_config_bool_or_int, which may want to
> distinguish the integer values "0" and "1" from bools.

... e.g. number one spelled as "1" do mean one not true in contexts like
status.submodulesummary, where "true" means "unlimited" and "1" means
"limit to one".  It surely is necessary to avoid breakage there.

A caller that uses git_config_maybe_bool() expects to see 0/1 when the
input looks like a boolean, and your patch looks like the right thing to
do.

The repertoire of parsers that involve elements that are possibly boolean
are now:

 - git_config_bool(): takes "0/false/no/..." or "$n/true/yes/..." (where
   any non-zero number $n is taken as true [*1*]), or more traditional

   [section]
       var

   without any equal sign, which means true.  Errors out if the input is
   not a boolean.

 - git_config_maybe_bool(): similar, and returns -1 (not a bool), 0
   (false) and 1 (true).  "0" is false, "1" is true.  But all other
   numbers are not boolean;

 - git_config_bool_or_int(): the caller can tell if the value was boolean.
   "0" and "1" are integers, and not boolean.  Errors out if the input is
   not bool nor int.

Perhaps we would want to add Documentation/technical/api-config.txt
someday?   Hint, hint....


[Footnote]

*1* I doubt anybody is insane enough to have an existing configuration
file that spells "true" with "2" (or "100"), so it might be ok to tighten
the rule after the fact a bit here to take only "1" as true from purist's
point of view, but I do not think the upside outweighs possible breakage.

Re: [PATCH 1/2] log.decorate: accept 0/1 bool values

[Jeff King]

On Wed, Nov 17, 2010 at 11:36:30AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > We cannot simply add 0/1 support to git_config_maybe_bool.
> > That would confuse git_config_bool_or_int, which may want to
> > distinguish the integer values "0" and "1" from bools.
> 
> ... e.g. number one spelled as "1" do mean one not true in contexts like
> status.submodulesummary, where "true" means "unlimited" and "1" means
> "limit to one".  It surely is necessary to avoid breakage there.

Makes sense. I didn't even bother checking, as the fact that we export
it as "git config --bool-or-int" was enough for me that we need to keep
the behavior the same.

> A caller that uses git_config_maybe_bool() expects to see 0/1 when the
> input looks like a boolean, and your patch looks like the right thing to
> do.

It's possible we could have another type of caller, but I suspect they
would want git_config_bool_or_int instead. The exception would be one who
wants a bool, an int, _or_ an arbitrary string. If somebody writes such
a thing, they are free to expose the new git_config_maybe_bool_text.

> The repertoire of parsers that involve elements that are possibly boolean
> are now:
> 
>  - git_config_bool(): takes "0/false/no/..." or "$n/true/yes/..." (where
>    any non-zero number $n is taken as true [*1*]), or more traditional
> 
>    [section]
>        var
> 
>    without any equal sign, which means true.  Errors out if the input is
>    not a boolean.
> 
>  - git_config_maybe_bool(): similar, and returns -1 (not a bool), 0
>    (false) and 1 (true).  "0" is false, "1" is true.  But all other
>    numbers are not boolean;

Yeah, I did notice that. Technically it is a regression in my 2/2, in
that "pager.foo = 2" used to mean "use the pager for foo" and now means
"use a pager called 2". But I am willing to write that off as insanity,
especially since we never documented that behavior (and in fact we
explicitly document the allowable values as yes/no, 0/1, true/false, and
on/off).

I don't think it is worth closing the hole for no reason on other config
options, but I am certainly fine with breaking it in the case of
pager.*.

> Perhaps we would want to add Documentation/technical/api-config.txt
> someday?   Hint, hint....

I'll put it on my todo, right after refactoring the awful mess of the
config code itself. :)

-Peff

Re: [PATCH 1/2] log.decorate: accept 0/1 bool values

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't think it is worth closing the hole for no reason on other config
> options, but I am certainly fine with breaking it in the case of
> pager.*.

Hmm, I guess that is fine, but will we hear "Why does it behave
differently only for pager.*" down the line, just like the issue your
patch 1/2 addressed, which was "Why does it behave differently only for
log.decorate?"

Re: [PATCH 1/2] log.decorate: accept 0/1 bool values

[Jeff King]

On Thu, Nov 18, 2010 at 09:00:13AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I don't think it is worth closing the hole for no reason on other config
> > options, but I am certainly fine with breaking it in the case of
> > pager.*.
> 
> Hmm, I guess that is fine, but will we hear "Why does it behave
> differently only for pager.*" down the line, just like the issue your
> patch 1/2 addressed, which was "Why does it behave differently only for
> log.decorate?"

Perhaps, but the difference is that for log.decorate I had no answer
(because it was not behaving as indicated by the documentation), whereas
with this I can with a good conscious say "go away, you are relying
ridiculous undocumented behavior".

That being said, it is much easier than I expected to make it Just Work
the same way, so perhaps we should do this on top:

-- >8 --
Subject: [PATCH] handle arbitrary ints in git_config_maybe_bool

This function recently gained the ability to recognize the
documented "0" and "1" values as false/true. However, unlike
regular git_config_bool, it did not treat arbitrary numbers
as true. While this is undocumented and probably ridiculous
for somebody to rely on, it is safer to behave exactly as
git_config_bool would. Because git_config_maybe_bool can be
used to retrofit new non-bool values onto existings bool
options, not behaving in exactly the same way is technically
a regression.

Signed-off-by: Jeff King <peff@peff.net>
---
It even saves 2 lines, so it _must_ be better. :)

 config.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/config.c b/config.c
index d3fa953..299ae80 100644
--- a/config.c
+++ b/config.c
@@ -429,13 +429,11 @@ static int git_config_maybe_bool_text(const char *name, const char *value)
 
 int git_config_maybe_bool(const char *name, const char *value)
 {
-	int v = git_config_maybe_bool_text(name, value);
+	long v = git_config_maybe_bool_text(name, value);
 	if (0 <= v)
 		return v;
-	if (!strcmp(value, "0"))
-		return 0;
-	if (!strcmp(value, "1"))
-		return 1;
+	if (git_parse_long(value, &v))
+		return !!v;
 	return -1;
 }
 
-- 
1.7.3.2.510.g24900