[PATCH 0/6] clean up parsing of maybe_bool

[PATCH 0/6] clean up parsing of maybe_bool

[Martin Ågren]

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/xmqq7fotd71o.fsf@gitster.dls.corp.google.com/

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

[PATCH 1/6] Doc/git-{push,send-pack}: correct --sign= to --signed=

[Martin Ågren]

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/1439492451-11233-1-git-send-email-dborowitz@google.com/T/#u

[2] v2-thread:
https://public-inbox.org/git/1439998007-28719-1-git-send-email-dborowitz@google.com/T/#m6533a6c4707a30b0d81e86169ff8559460cbf6eb

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-push.txt      | 4 ++--
 Documentation/git-send-pack.txt | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 0a639664f..3e76e99f3 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -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
diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 966abb0df..f51c64939 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -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

[PATCH 2/6] t5334: document that git push --signed=1 does not work

[Martin Ågren]

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t5534-push-signed.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 464ffdd14..5dce55e1d 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -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

[PATCH 3/6] config: introduce git_parse_maybe_bool_text

[Martin Ågren]

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/xmqq7fotd71o.fsf@gitster.dls.corp.google.com/

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 config.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 231f9a750..7df57cec0 100644
--- a/config.c
+++ b/config.c
@@ -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

[PATCH 4/6] config: make git_{config,parse}_maybe_bool equivalent

[Martin Ågren]

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/xmqq7fotd71o.fsf@gitster.dls.corp.google.com/

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t5534-push-signed.sh |  2 +-
 config.c               | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 5dce55e1d..1cea758f7 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -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 &&
diff --git a/config.c b/config.c
index 7df57cec0..d87376a5d 100644
--- a/config.c
+++ b/config.c
@@ -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

[PATCH 5/6] treewide: deprecate git_config_maybe_bool, use git_parse_maybe_bool

[Martin Ågren]

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 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(-)

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 20741f345..7a83a3a6e 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -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.
 
diff --git a/builtin/log.c b/builtin/log.c
index c6362cf92..9182f0ee3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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);
diff --git a/builtin/merge.c b/builtin/merge.c
index 900bafdb4..6a5122921 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -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;
diff --git a/builtin/pull.c b/builtin/pull.c
index 9b86e519b..7fe281414 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -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:
diff --git a/builtin/push.c b/builtin/push.c
index 03846e837..2ac810422 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -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;
diff --git a/builtin/remote.c b/builtin/remote.c
index 6273c0c23..a995ea86c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -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"))
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 633e0c3cd..fc4f0bb5f 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -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;
diff --git a/config.c b/config.c
index d87376a5d..9b474d9a3 100644
--- a/config.c
+++ b/config.c
@@ -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;
diff --git a/pager.c b/pager.c
index 4dd9e1b26..92b23e6cd 100644
--- a/pager.c
+++ b/pager.c
@@ -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 {
diff --git a/submodule-config.c b/submodule-config.c
index 5fe2d0787..11a32584d 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -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

[PATCH 6/6] parse_decoration_style: drop unused argument `var`

[Martin Ågren]

The previous commit left it unused.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/log.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 9182f0ee3..483d15a94 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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

Re: [PATCH 0/6] clean up parsing of maybe_bool

[Stefan Beller]

On Mon, Aug 7, 2017 at 11:20 AM, Martin Ågren <martin.agren@gmail.com> wrote:
> 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.

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

Re: [PATCH 2/6] t5334: document that git push --signed=1 does not work

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> 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.

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.


> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
> index 464ffdd14..5dce55e1d 100755
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -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 &&
n> +	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 &&

Re: [PATCH 0/6] clean up parsing of maybe_bool

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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.

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.

Re: [PATCH 0/6] clean up parsing of maybe_bool

[Martin Ågren]

On 7 August 2017 at 23:12, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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.
>
[...]
>
> 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, 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

Re: [PATCH 0/6] clean up parsing of maybe_bool

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> 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?

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.

Re: [PATCH 0/6] clean up parsing of maybe_bool

[Martin Ågren]

On 8 August 2017 at 19:04, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>
>> 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?
>
> 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 ;-)

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.. ;) )