clean up parsing of maybe_bool

When we want to parse a boolean config item without dying on error, we call git_config_maybe_bool() which takes two arguments: the value to be parsed (obviously) and a name which is completely ignored. Junio has suggested to drop name and rename the function [1]. That effort even started shortly after that, by introducing git_parse_maybe_bool(). The new function currently only has a single user outside config.c.

Patch 5 of this series deprecates the old function and updates all callers to use git_parse_maybe_bool() instead. Patch 6 is a final
cleanup: one of the converted callers suddenly had an unused argument.

Patches 3 and 4 prepare for the switch. In particular, patch 4 ensures that the two functions are actually equivalent. In doing so, it affects git push --signed=.. which was very slightly inconsistent to the rest of Git.

Patch 2 adds a failing test in preparation for patch 4. Patch 1 corrects the documentation not to say "git push --sign=.." to make it a bit more obvious that the opposite typo is not being made in patches 2 and 4.

git_parse_maybe_bool is used in sb/diff-color-move, which is in "next". This series makes --color-moved and diff.colormoved consistent with other booleans. That should be a good thing, but cc Stefan to be sure.

Martin

[1] https://public-inbox.org/git/[email protected]/

Martin Ågren (6):
Doc/git-{push,send-pack}: correct --sign= to --signed=
t5334: document that git push --signed=1 does not work
config: introduce git_parse_maybe_bool_text
config: make git_{config,parse}_maybe_bool equivalent
treewide: deprecate git_config_maybe_bool, use git_parse_maybe_bool
parse_decoration_style: drop unused argument var

 Documentation/git-push.txt             |  4 ++--
 Documentation/git-send-pack.txt        |  4 ++--
 Documentation/technical/api-config.txt |  4 ++++
 t/t5534-push-signed.sh                 |  7 +++++++
 builtin/log.c                          | 10 +++++-----
 builtin/merge.c                        |  4 ++--
 builtin/pull.c                         |  4 ++--
 builtin/push.c                         |  2 +-
 builtin/remote.c                       |  2 +-
 builtin/send-pack.c                    |  2 +-
 config.c                               | 15 ++++++++++-----
 pager.c                                |  2 +-
 submodule-config.c                     |  6 +++---
 13 files changed, 41 insertions(+), 25 deletions(-)

2.14.0.5.g0f7b1ed27

Since we're about to touch the behavior of --signed=, do this as a preparatory step.

The documentation mentions --sign=, and it works. But that's just because it's an unambiguous abbreviation of --signed, which is how it is actually implemented. This was added in commit 30261094 ("push: support signing pushes iff the server supports it", 2015-08-19). Back when that series was developed [1] [2], there were suggestions about both --sign= and --signed=. The final implementation settled on --signed=, but some of the documentation and commit messages ended up using --sign=.

The option is referred to as --signed= in Documentation/config.txt (under push.gpgSign).

One could argue that we have promised --sign for two years now, so we should implement it as an alias for --signed. (Then we might also deprecate the latter, something which was considered already then.) That would be a slightly more intrusive change.

This minor issue would only be a problem once we want to implement some other option --signfoo, but the earlier we do this step, the better.

[1] v1-thread:
https://public-inbox.org/git/[email protected]/T/#u

[2] v2-thread:
https://public-inbox.org/git/[email protected]/T/#m6533a6c4707a30b0d81e86169ff8559460cbf6eb

---
 Documentation/git-push.txt      | 4 ++--
 Documentation/git-send-pack.txt | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

index 0a639664f..3e76e99f3 100644
@@ -12,7 +12,7 @@ SYNOPSIS
 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
 	   [--repo=<repository>] [-f | --force] [-d | --delete] [--prune] [-v | --verbose]
 	   [-u | --set-upstream] [--push-option=<string>]
-	   [--[no-]signed|--sign=(true|false|if-asked)]
+	   [--[no-]signed|--signed=(true|false|if-asked)]
 	   [--force-with-lease[=<refname>[:<expect>]]]
 	   [--no-verify] [<repository> [<refspec>...]]
 

@@ -141,7 +141,7 @@ already exists on the remote side.
 	information, see `push.followTags` in linkgit:git-config[1].
 
 --[no-]signed::
---sign=(true|false|if-asked)::
+--signed=(true|false|if-asked)::
 	GPG-sign the push request to update refs on the receiving
 	side, to allow it to be checked by the hooks and/or be
 	logged.  If `false` or `--no-signed`, no signing will be

index 966abb0df..f51c64939 100644
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
 		[--verbose] [--thin] [--atomic]
-		[--[no-]signed|--sign=(true|false|if-asked)]
+		[--[no-]signed|--signed=(true|false|if-asked)]
 		[<host>:]<directory> [<ref>...]
 
 DESCRIPTION

@@ -71,7 +71,7 @@ be in a separate packet, and the list must end with a flush packet.
 	refs.
 
 --[no-]signed::
---sign=(true|false|if-asked)::
+--signed=(true|false|if-asked)::
 	GPG-sign the push request to update refs on the receiving
 	side, to allow it to be checked by the hooks and/or be
 	logged.  If `false` or `--no-signed`, no signing will be

2.14.0.5.g0f7b1ed27

When accepting booleans as command-line or config options throughout Git, there are several documented synonyms for true and false. However, one particular user is slightly broken: git push --signed=.. does not understand the integer synonyms for true and false.

This is hardly wanted. The --signed option has a different notion of boolean than all other arguments and config options, including the config option corresponding to it, push.gpgSign.

Add a test documenting the failure to handle --signed=1.

---
 t/t5534-push-signed.sh | 7 +++++++
 1 file changed, 7 insertions(+)

index 464ffdd14..5dce55e1d 100755
@@ -71,6 +71,13 @@ test_expect_success 'push --signed fails with a receiver without push certificat
 	test_i18ngrep "the receiving end does not support" err
 '
 
+test_expect_failure 'push --signed=1 is accepted' '
+	prepare_dst &&
+	mkdir -p dst/.git/hooks &&
+	test_must_fail git push --signed=1 dst noop ff +noff 2>err &&
+	test_i18ngrep "the receiving end does not support" err
+'
+
 test_expect_success GPG 'no certificate for a signed push with no update' '
 	prepare_dst &&
 	mkdir -p dst/.git/hooks &&

2.14.0.5.g0f7b1ed27

My knee-jerk reaction is that parse_maybe_bool() is broken in that it should do everything that config_maybe_bool() does. I wonder why we do not call git_parse_int() like git_config_maybe_bool() does?

... Ahh, that is because bool_or_int() wants to know that "1" is an int and not a bool, and parse_maybe_bool() is designed to cater to the need of that thing, in addition to config_maybe_bool(). And the "--signed=" thing wants parse_bool_or_string(), not bool_or_int(), so we should treat "1" as true and "0" as false. There is no way to do so in the current code and that is why you have the new _text() thing in patches 3-4/6.

Makes sense.

Commit 9a549d43 ("config.c: rename git_config_maybe_bool_text and export it as git_parse_maybe_bool", 2015-08-19) intended git_parse_maybe_bool to be a replacement for git_config_maybe_bool, which could then be retired. That is not obvious from the commit message, but that is what the background on the mailing list suggests [1].

However, git_{config,parse}_maybe_bool do not handle all input the same. Before the rename, that was by design and there is a caller in config.c which requires git_parse_maybe_bool to behave exactly as it does.

Prepare for the next patch by renaming git_parse_maybe_bool to ..._text and reimplementing the first one as a simple call to the second one. Let the existing users in config.c use ..._text, since it does what they need.

[1] https://public-inbox.org/git/[email protected]/

---
 config.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

index 231f9a750..7df57cec0 100644
@@ -928,7 +928,7 @@ ssize_t git_config_ssize_t(const char *name, const char *value)
 	return ret;
 }
 
-int git_parse_maybe_bool(const char *value)
+static int git_parse_maybe_bool_text(const char *value)
 {
 	if (!value)
 		return 1;

@@ -945,9 +945,14 @@ int git_parse_maybe_bool(const char *value)
 	return -1;
 }
 
+int git_parse_maybe_bool(const char *value)
+{
+	return git_parse_maybe_bool_text(value);
+}
+
 int git_config_maybe_bool(const char *name, const char *value)
 {
-	int v = git_parse_maybe_bool(value);
+	int v = git_parse_maybe_bool_text(value);
 	if (0 <= v)
 		return v;
 	if (git_parse_int(value, &v))

@@ -957,7 +962,7 @@ int git_config_maybe_bool(const char *name, const char *value)
 
 int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 {
-	int v = git_parse_maybe_bool(value);
+	int v = git_parse_maybe_bool_text(value);
 	if (0 <= v) {
 		*is_bool = 1;
 		return v;

2.14.0.5.g0f7b1ed27

Both of these act on a string value which they parse as a boolean. The "parse"-variant was introduced as a replacement for the "config"-variant which for historical reasons takes an unused argument name. That it was intended as a replacement is not obvious from commit 9a549d43 ("config.c: rename git_config_maybe_bool_text and export it as git_parse_maybe_bool", 2015-08-19), but that is what the background on the mailing list suggests [1].

However, these two functions do not parse value in exactly the same way. In particular, git_config_maybe_bool accepts integers (0 for false, non-0 for true). This means there are two slightly different definitions of "maybe_bool" in the code-base, and that every time a call to git_config_maybe_bool is changed to use git_parse_maybe_bool, it risks breaking someone's workflow.

Move the implementation of "config" into "parse" and make the latter a trivial wrapper.

This also fixes the only user of git_parse_maybe_bool, git push --signed=...

[1] https://public-inbox.org/git/[email protected]/

---
 t/t5534-push-signed.sh |  2 +-
 config.c               | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

index 5dce55e1d..1cea758f7 100755
@@ -71,7 +71,7 @@ test_expect_success 'push --signed fails with a receiver without push certificat
 	test_i18ngrep "the receiving end does not support" err
 '
 
-test_expect_failure 'push --signed=1 is accepted' '
+test_expect_success 'push --signed=1 is accepted' '
 	prepare_dst &&
 	mkdir -p dst/.git/hooks &&
 	test_must_fail git push --signed=1 dst noop ff +noff 2>err &&

index 7df57cec0..d87376a5d 100644
@@ -946,11 +946,6 @@ static int git_parse_maybe_bool_text(const char *value)
 }
 
 int git_parse_maybe_bool(const char *value)
-{
-	return git_parse_maybe_bool_text(value);
-}
-
-int git_config_maybe_bool(const char *name, const char *value)
 {
 	int v = git_parse_maybe_bool_text(value);
 	if (0 <= v)

@@ -960,6 +955,11 @@ int git_config_maybe_bool(const char *name, const char *value)
 	return -1;
 }
 
+int git_config_maybe_bool(const char *name, const char *value)
+{
+	return git_parse_maybe_bool(value);
+}
+
 int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 {
 	int v = git_parse_maybe_bool_text(value);

2.14.0.5.g0f7b1ed27

The only difference between these is that the former takes an argument name which it ignores completely. Still, the callers are quite careful to provide reasonable values for it.

Once in-flight topics have landed, we should be able to remove git_config_maybe_bool. In the meantime, document it as deprecated in the technical documentation. While at it, document git_parse_maybe_bool.

---
 Documentation/technical/api-config.txt | 4 ++++
 builtin/log.c                          | 4 ++--
 builtin/merge.c                        | 4 ++--
 builtin/pull.c                         | 4 ++--
 builtin/push.c                         | 2 +-
 builtin/remote.c                       | 2 +-
 builtin/send-pack.c                    | 2 +-
 config.c                               | 2 +-
 pager.c                                | 2 +-
 submodule-config.c                     | 6 +++---
 10 files changed, 18 insertions(+), 14 deletions(-)

index 20741f345..7a83a3a6e 100644
@@ -187,6 +187,10 @@ Same as `git_config_bool`, except that integers are returned as-is, and
 an `is_bool` flag is unset.
 
 `git_config_maybe_bool`::
+Deprecated. Use `git_parse_maybe_bool` instead. They are exactly the
+same, except this function takes an unused argument `name`.
+
+`git_parse_maybe_bool`::
 Same as `git_config_bool`, except that it returns -1 on error rather
 than dying.
 

index c6362cf92..9182f0ee3 100644
@@ -60,7 +60,7 @@ static int auto_decoration_style(void)
 
 static int parse_decoration_style(const char *var, const char *value)
 {
-	switch (git_config_maybe_bool(var, value)) {
+	switch (git_parse_maybe_bool(value)) {
 	case 1:
 		return DECORATE_SHORT_REFS;
 	case 0:

@@ -821,7 +821,7 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (!strcmp(var, "format.from")) {
-		int b = git_config_maybe_bool(var, value);
+		int b = git_parse_maybe_bool(value);
 		free(from);
 		if (b < 0)
 			from = xstrdup(value);

index 900bafdb4..6a5122921 100644
@@ -566,7 +566,7 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 	else if (!strcmp(k, "merge.renormalize"))
 		option_renormalize = git_config_bool(k, v);
 	else if (!strcmp(k, "merge.ff")) {
-		int boolval = git_config_maybe_bool(k, v);
+		int boolval = git_parse_maybe_bool(v);
 		if (0 <= boolval) {
 			fast_forward = boolval ? FF_ALLOW : FF_NO;
 		} else if (v && !strcmp(v, "only")) {

@@ -940,7 +940,7 @@ static int default_edit_option(void)
 		return 0;
 
 	if (e) {
-		int v = git_config_maybe_bool(name, e);
+		int v = git_parse_maybe_bool(e);
 		if (v < 0)
 			die(_("Bad value '%s' in environment '%s'"), e, name);
 		return v;

index 9b86e519b..7fe281414 100644
@@ -39,7 +39,7 @@ enum rebase_type {
 static enum rebase_type parse_config_rebase(const char *key, const char *value,
 		int fatal)
 {
-	int v = git_config_maybe_bool("pull.rebase", value);
+	int v = git_parse_maybe_bool(value);
 
 	if (!v)
 		return REBASE_FALSE;

@@ -274,7 +274,7 @@ static const char *config_get_ff(void)
 	if (git_config_get_value("pull.ff", &value))
 		return NULL;
 
-	switch (git_config_maybe_bool("pull.ff", value)) {
+	switch (git_parse_maybe_bool(value)) {
 	case 0:
 		return "--no-ff";
 	case 1:

index 03846e837..2ac810422 100644
@@ -481,7 +481,7 @@ static int git_push_config(const char *k, const char *v, void *cb)
 	} else if (!strcmp(k, "push.gpgsign")) {
 		const char *value;
 		if (!git_config_get_value("push.gpgsign", &value)) {
-			switch (git_config_maybe_bool("push.gpgsign", value)) {
+			switch (git_parse_maybe_bool(value)) {
 			case 0:
 				set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_NEVER);
 				break;

index 6273c0c23..a995ea86c 100644
@@ -301,7 +301,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 			}
 			string_list_append(&info->merge, xstrdup(value));
 		} else {
-			int v = git_config_maybe_bool(orig_key, value);
+			int v = git_parse_maybe_bool(value);
 			if (v >= 0)
 				info->rebase = v;
 			else if (!strcmp(value, "preserve"))

index 633e0c3cd..fc4f0bb5f 100644
@@ -105,7 +105,7 @@ static int send_pack_config(const char *k, const char *v, void *cb)
 	if (!strcmp(k, "push.gpgsign")) {
 		const char *value;
 		if (!git_config_get_value("push.gpgsign", &value)) {
-			switch (git_config_maybe_bool("push.gpgsign", value)) {
+			switch (git_parse_maybe_bool(value)) {
 			case 0:
 				args.push_cert = SEND_PACK_PUSH_CERT_NEVER;
 				break;

index d87376a5d..9b474d9a3 100644
@@ -1851,7 +1851,7 @@ int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *de
 {
 	const char *value;
 	if (!git_configset_get_value(cs, key, &value)) {
-		*dest = git_config_maybe_bool(key, value);
+		*dest = git_parse_maybe_bool(value);
 		if (*dest == -1)
 			return -1;
 		return 0;

index 4dd9e1b26..92b23e6cd 100644
@@ -194,7 +194,7 @@ static int pager_command_config(const char *var, const char *value, void *vdata)
 	const char *cmd;
 
 	if (skip_prefix(var, "pager.", &cmd) && !strcmp(cmd, data->cmd)) {
-		int b = git_config_maybe_bool(var, value);
+		int b = git_parse_maybe_bool(value);
 		if (b >= 0)
 			data->want = b;
 		else {

index 5fe2d0787..11a32584d 100644
@@ -232,7 +232,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 static int parse_fetch_recurse(const char *opt, const char *arg,
 			       int die_on_error)
 {
-	switch (git_config_maybe_bool(opt, arg)) {
+	switch (git_parse_maybe_bool(arg)) {
 	case 1:
 		return RECURSE_SUBMODULES_ON;
 	case 0:

@@ -277,7 +277,7 @@ int option_fetch_parse_recurse_submodules(const struct option *opt,
 static int parse_update_recurse(const char *opt, const char *arg,
 				int die_on_error)
 {
-	switch (git_config_maybe_bool(opt, arg)) {
+	switch (git_parse_maybe_bool(arg)) {
 	case 1:
 		return RECURSE_SUBMODULES_ON;
 	case 0:

@@ -297,7 +297,7 @@ int parse_update_recurse_submodules_arg(const char *opt, const char *arg)
 static int parse_push_recurse(const char *opt, const char *arg,
 			       int die_on_error)
 {
-	switch (git_config_maybe_bool(opt, arg)) {
+	switch (git_parse_maybe_bool(arg)) {
 	case 1:
 		/* There's no simple "on" value when pushing */
 		if (die_on_error)

2.14.0.5.g0f7b1ed27

The previous commit left it unused.

---
 builtin/log.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 9182f0ee3..483d15a94 100644
@@ -58,7 +58,7 @@ static int auto_decoration_style(void)
 	return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
 }
 
-static int parse_decoration_style(const char *var, const char *value)
+static int parse_decoration_style(const char *value)
 {
 	switch (git_parse_maybe_bool(value)) {
 	case 1:

@@ -82,7 +82,7 @@ static int decorate_callback(const struct option *opt, const char *arg, int unse
 	if (unset)
 		decoration_style = 0;
 	else if (arg)
-		decoration_style = parse_decoration_style("command line", arg);
+		decoration_style = parse_decoration_style(arg);
 	else
 		decoration_style = DECORATE_SHORT_REFS;
 

@@ -409,7 +409,7 @@ static int git_log_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "log.date"))
 		return git_config_string(&default_date_mode, var, value);
 	if (!strcmp(var, "log.decorate")) {
-		decoration_style = parse_decoration_style(var, value);
+		decoration_style = parse_decoration_style(value);
 		if (decoration_style < 0)
 			decoration_style = 0; /* maybe warn? */
 		return 0;

2.14.0.5.g0f7b1ed27

The series looks fine to me overall, though patch 5 is overly gentle IMHO. We could have removed it right there as Junio is very good at resolving conflicts or producing dirty merges for such a situation.
But delaying it until no other series' are in flight is fine with me, too.

Looking back at sb/diff-color-move and the code where git_parse_maybe_bool is used, I do not think this will be an issue.

Thanks,
Stefan

If you remove the old one, it would cause compilation error due to removal of the declaration of the old one when other series that are in flight adds new callsites to it. Which makes life a bit easier for the integrators when it is trivial to convert these callsites to use the new one. If the way the old one and the new one are called are vastly different, of course, leaving the compatibility layer that no longer is used after the series will make it easier to live with other topics in flight, on the other hand.

I am fine with either in this case, but I probably would have opted for removal at the end of this series if I were doing this series, because

- git_config_maybe_bool(K,V)
+ git_parse_maybe_bool(V)

that may have to happen during evil merges would have been trivial.

Thanks.

[...]

Thanks, both of you. I could wait a couple of days to see if there are other things to address, then send a v2 with a more aggressive patch 5?

Martin

Sounds like a plan. If there aren't anything else, I personally do not mind using what is already on the list without v2, though. That would be less work/churn for me ;-)

Thanks.

Sure. I'll try to remember to send a patch to kill git_config_maybe_bool some time from now. (At which point you'll get more work.. ;) )