{"thread":{"id":"46531","subject":"[PATCH 0/6] clean up parsing of maybe_bool","startedAt":"2017-08-07T18:21:20Z","lastAt":"2017-08-08T17:21:13Z","messageCount":13,"participants":["Martin Ågren","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"325714","messageId":"cover.1502128418.git.martin.agren@gmail.com","threadId":"46531","inReplyTo":null,"subject":"[PATCH 0/6] clean up parsing of maybe_bool","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-07T18:20:44Z","receivedAt":"2017-08-07T18:21:20Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When we want to parse a boolean config item without dying on error, we\ncall git_config_maybe_bool() which takes two arguments: the value to be\nparsed (obviously) and a `name` which is completely ignored. Junio has\nsuggested to drop `name` and rename the function [1]. That effort even\nstarted shortly after that, by introducing git_parse_maybe_bool(). The\nnew function currently only has a single user outside config.c.\n\nPatch 5 of this series deprecates the old function and updates all\ncallers to use git_parse_maybe_bool() instead. Patch 6 is a final\ncleanup: one of the converted callers suddenly had an unused argument.\n\nPatches 3 and 4 prepare for the switch. In particular, patch 4 ensures\nthat the two functions are actually equivalent. In doing so, it affects\n`git push --signed=..` which was very slightly inconsistent to the rest\nof Git.\n\nPatch 2 adds a failing test in preparation for patch 4. Patch 1 corrects\nthe documentation not to say \"git push --sign=..\" to make it a bit more\nobvious that the opposite typo is not being made in patches 2 and 4.\n\ngit_parse_maybe_bool is used in sb/diff-color-move, which is in \"next\".\nThis series makes --color-moved and diff.colormoved consistent with\nother booleans. That should be a good thing, but cc Stefan to be sure.\n\nMartin\n\n[1] https://public-inbox.org/git/xmqq7fotd71o.fsf@gitster.dls.corp.google.com/\n\nMartin Ågren (6):\n Doc/git-{push,send-pack}: correct --sign= to --signed=\n t5334: document that git push --signed=1 does not work\n config: introduce git_parse_maybe_bool_text\n config: make git_{config,parse}_maybe_bool equivalent\n treewide: deprecate git_config_maybe_bool, use git_parse_maybe_bool\n parse_decoration_style: drop unused argument `var`\n\n Documentation/git-push.txt | 4 ++--\n Documentation/git-send-pack.txt | 4 ++--\n Documentation/technical/api-config.txt | 4 ++++\n t/t5534-push-signed.sh | 7 +++++++\n builtin/log.c | 10 +++++-----\n builtin/merge.c | 4 ++--\n builtin/pull.c | 4 ++--\n builtin/push.c | 2 +-\n builtin/remote.c | 2 +-\n builtin/send-pack.c | 2 +-\n config.c | 15 ++++++++++-----\n pager.c | 2 +-\n submodule-config.c | 6 +++---\n 13 files changed, 41 insertions(+), 25 deletions(-)\n\n-- \n2.14.0.5.g0f7b1ed27\n\n"},{"id":"325715","messageId":"8721e94741955eb32b3e8f08ff7f7319c79f93bf.1502128418.git.martin.agren@gmail.com","threadId":"46531","inReplyTo":"cover.1502128418.git.martin.agren@gmail.com","subject":"[PATCH 1/6] Doc/git-{push,send-pack}: correct --sign= to --signed=","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-07T18:20:45Z","receivedAt":"2017-08-07T18:21:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Since we're about to touch the behavior of --signed=, do this as a\npreparatory step.\n\nThe documentation mentions --sign=, and it works. But that's just\nbecause it's an unambiguous abbreviation of --signed, which is how it is\nactually implemented. This was added in commit 30261094 (\"push: support\nsigning pushes iff the server supports it\", 2015-08-19). Back when that\nseries was developed [1] [2], there were suggestions about both --sign=\nand --signed=. The final implementation settled on --signed=, but some\nof the documentation and commit messages ended up using --sign=.\n\nThe option is referred to as --signed= in Documentation/config.txt\n(under push.gpgSign).\n\nOne could argue that we have promised --sign for two years now, so we\nshould implement it as an alias for --signed. (Then we might also\ndeprecate the latter, something which was considered already then.) That\nwould be a slightly more intrusive change.\n\nThis minor issue would only be a problem once we want to implement some\nother option --signfoo, but the earlier we do this step, the better.\n\n[1] v1-thread:\nhttps://public-inbox.org/git/1439492451-11233-1-git-send-email-dborowitz@google.com/T/#u\n\n[2] v2-thread:\nhttps://public-inbox.org/git/1439998007-28719-1-git-send-email-dborowitz@google.com/T/#m6533a6c4707a30b0d81e86169ff8559460cbf6eb\n\nSigned-off-by: Martin Ågren \n---\n Documentation/git-push.txt | 4 ++--\n Documentation/git-send-pack.txt | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 0a639664f..3e76e99f3 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=]\n \t [--repo=] [-f | --force] [-d | --delete] [--prune] [-v | --verbose]\n \t [-u | --set-upstream] [--push-option=]\n-\t [--[no-]signed|--sign=(true|false|if-asked)]\n+\t [--[no-]signed|--signed=(true|false|if-asked)]\n \t [--force-with-lease[=[:]]]\n \t [--no-verify] [ [...]]\n \n@@ -141,7 +141,7 @@ already exists on the remote side.\n \tinformation, see `push.followTags` in linkgit:git-config[1].\n \n --[no-]signed::\n---sign=(true|false|if-asked)::\n+--signed=(true|false|if-asked)::\n \tGPG-sign the push request to update refs on the receiving\n \tside, to allow it to be checked by the hooks and/or be\n \tlogged. If `false` or `--no-signed`, no signing will be\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex 966abb0df..f51c64939 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=]\n \t\t[--verbose] [--thin] [--atomic]\n-\t\t[--[no-]signed|--sign=(true|false|if-asked)]\n+\t\t[--[no-]signed|--signed=(true|false|if-asked)]\n \t\t[:] [...]\n \n DESCRIPTION\n@@ -71,7 +71,7 @@ be in a separate packet, and the list must end with a flush packet.\n \trefs.\n \n --[no-]signed::\n---sign=(true|false|if-asked)::\n+--signed=(true|false|if-asked)::\n \tGPG-sign the push request to update refs on the receiving\n \tside, to allow it to be checked by the hooks and/or be\n \tlogged. If `false` or `--no-signed`, no signing will be\n-- \n2.14.0.5.g0f7b1ed27\n\n"},{"id":"325716","messageId":"4ac17762106cde543c4c60968946b68263830d61.1502128418.git.martin.agren@gmail.com","threadId":"46531","inReplyTo":"cover.1502128418.git.martin.agren@gmail.com","subject":"[PATCH 2/6] t5334: document that git push --signed=1 does not work","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-07T18:20:46Z","receivedAt":"2017-08-07T18:21:32Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When accepting booleans as command-line or config options throughout\nGit, there are several documented synonyms for true and false.\nHowever, one particular user is slightly broken: `git push --signed=..`\ndoes not understand the integer synonyms for true and false.\n\nThis is hardly wanted. The --signed option has a different notion of\nboolean than all other arguments and config options, including the\nconfig option corresponding to it, push.gpgSign.\n\nAdd a test documenting the failure to handle --signed=1.\n\nSigned-off-by: Martin Ågren \n---\n t/t5534-push-signed.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex 464ffdd14..5dce55e1d 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -71,6 +71,13 @@ test_expect_success 'push --signed fails with a receiver without push certificat\n \ttest_i18ngrep \"the receiving end does not support\" err\n '\n \n+test_expect_failure 'push --signed=1 is accepted' '\n+\tprepare_dst &&\n+\tmkdir -p dst/.git/hooks &&\n+\ttest_must_fail git push --signed=1 dst noop ff +noff 2>err &&\n+\ttest_i18ngrep \"the receiving end does not support\" err\n+'\n+\n test_expect_success GPG 'no certificate for a signed push with no update' '\n \tprepare_dst &&\n \tmkdir -p dst/.git/hooks &&\n-- \n2.14.0.5.g0f7b1ed27\n\n"},{"id":"325717","messageId":"c244a7bfd521f99d79d0eae67395ff1dd6cd01d1.1502128418.git.martin.agren@gmail.com","threadId":"46531","inReplyTo":"cover.1502128418.git.martin.agren@gmail.com","subject":"[PATCH 3/6] config: introduce git_parse_maybe_bool_text","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-07T18:20:47Z","receivedAt":"2017-08-07T18:21:37Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit 9a549d43 (\"config.c: rename git_config_maybe_bool_text and export\nit as git_parse_maybe_bool\", 2015-08-19) intended git_parse_maybe_bool\nto be a replacement for git_config_maybe_bool, which could then be\nretired. That is not obvious from the commit message, but that is what\nthe background on the mailing list suggests [1].\n\nHowever, git_{config,parse}_maybe_bool do not handle all input the same.\nBefore the rename, that was by design and there is a caller in config.c\nwhich requires git_parse_maybe_bool to behave exactly as it does.\n\nPrepare for the next patch by renaming git_parse_maybe_bool to ..._text\nand reimplementing the first one as a simple call to the second one. Let\nthe existing users in config.c use ..._text, since it does what they\nneed.\n\n[1] https://public-inbox.org/git/xmqq7fotd71o.fsf@gitster.dls.corp.google.com/\n\nSigned-off-by: Martin Ågren \n---\n config.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 231f9a750..7df57cec0 100644\n--- a/config.c\n+++ b/config.c\n@@ -928,7 +928,7 @@ ssize_t git_config_ssize_t(const char *name, const char *value)\n \treturn ret;\n }\n \n-int git_parse_maybe_bool(const char *value)\n+static int git_parse_maybe_bool_text(const char *value)\n {\n \tif (!value)\n \t\treturn 1;\n@@ -945,9 +945,14 @@ int git_parse_maybe_bool(const char *value)\n \treturn -1;\n }\n \n+int git_parse_maybe_bool(const char *value)\n+{\n+\treturn git_parse_maybe_bool_text(value);\n+}\n+\n int git_config_maybe_bool(const char *name, const char *value)\n {\n-\tint v = git_parse_maybe_bool(value);\n+\tint v = git_parse_maybe_bool_text(value);\n \tif (0 <= v)\n \t\treturn v;\n \tif (git_parse_int(value, &v))\n@@ -957,7 +962,7 @@ int git_config_maybe_bool(const char *name, const char *value)\n \n int git_config_bool_or_int(const char *name, const char *value, int *is_bool)\n {\n-\tint v = git_parse_maybe_bool(value);\n+\tint v = git_parse_maybe_bool_text(value);\n \tif (0 <= v) {\n \t\t*is_bool = 1;\n \t\treturn v;\n-- \n2.14.0.5.g0f7b1ed27\n\n"},{"id":"325718","messageId":"74e20299473958b2279ee62d311cbf906b709952.1502128418.git.martin.agren@gmail.com","threadId":"46531","inReplyTo":"cover.1502128418.git.martin.agren@gmail.com","subject":"[PATCH 4/6] config: make git_{config,parse}_maybe_bool equivalent","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-07T18:20:48Z","receivedAt":"2017-08-07T18:21:42Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Both of these act on a string `value` which they parse as a boolean. The\n\"parse\"-variant was introduced as a replacement for the \"config\"-variant\nwhich for historical reasons takes an unused argument `name`. That it\nwas intended as a replacement is not obvious from commit 9a549d43\n(\"config.c: rename git_config_maybe_bool_text and export it as\ngit_parse_maybe_bool\", 2015-08-19), but that is what the background on\nthe mailing list suggests [1].\n\nHowever, these two functions do not parse `value` in exactly the same\nway. In particular, git_config_maybe_bool accepts integers (0 for false,\nnon-0 for true). This means there are two slightly different definitions\nof \"maybe_bool\" in the code-base, and that every time a call to\ngit_config_maybe_bool is changed to use git_parse_maybe_bool, it risks\nbreaking someone's workflow.\n\nMove the implementation of \"config\" into \"parse\" and make the latter a\ntrivial wrapper.\n\nThis also fixes the only user of git_parse_maybe_bool, `git push\n--signed=..`.\n\n[1] https://public-inbox.org/git/xmqq7fotd71o.fsf@gitster.dls.corp.google.com/\n\nSigned-off-by: Martin Ågren \n---\n t/t5534-push-signed.sh | 2 +-\n config.c | 10 +++++-----\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex 5dce55e1d..1cea758f7 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -71,7 +71,7 @@ test_expect_success 'push --signed fails with a receiver without push certificat\n \ttest_i18ngrep \"the receiving end does not support\" err\n '\n \n-test_expect_failure 'push --signed=1 is accepted' '\n+test_expect_success 'push --signed=1 is accepted' '\n \tprepare_dst &&\n \tmkdir -p dst/.git/hooks &&\n \ttest_must_fail git push --signed=1 dst noop ff +noff 2>err &&\ndiff --git a/config.c b/config.c\nindex 7df57cec0..d87376a5d 100644\n--- a/config.c\n+++ b/config.c\n@@ -946,11 +946,6 @@ static int git_parse_maybe_bool_text(const char *value)\n }\n \n int git_parse_maybe_bool(const char *value)\n-{\n-\treturn git_parse_maybe_bool_text(value);\n-}\n-\n-int git_config_maybe_bool(const char *name, const char *value)\n {\n \tint v = git_parse_maybe_bool_text(value);\n \tif (0 <= v)\n@@ -960,6 +955,11 @@ 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+\treturn git_parse_maybe_bool(value);\n+}\n+\n int git_config_bool_or_int(const char *name, const char *value, int *is_bool)\n {\n \tint v = git_parse_maybe_bool_text(value);\n-- \n2.14.0.5.g0f7b1ed27\n\n"},{"id":"325719","messageId":"4309125367e7bad34dac2347bc27c2ec59fda36e.1502128418.git.martin.agren@gmail.com","threadId":"46531","inReplyTo":"cover.1502128418.git.martin.agren@gmail.com","subject":"[PATCH 5/6] treewide: deprecate git_config_maybe_bool, use git_parse_maybe_bool","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-07T18:20:49Z","receivedAt":"2017-08-07T18:21:48Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The only difference between these is that the former takes an argument\n`name` which it ignores completely. Still, the callers are quite careful\nto provide reasonable values for it.\n\nOnce in-flight topics have landed, we should be able to remove\ngit_config_maybe_bool. In the meantime, document it as deprecated in the\ntechnical documentation. While at it, document git_parse_maybe_bool.\n\nSigned-off-by: Martin Ågren \n---\n Documentation/technical/api-config.txt | 4 ++++\n builtin/log.c | 4 ++--\n builtin/merge.c | 4 ++--\n builtin/pull.c | 4 ++--\n builtin/push.c | 2 +-\n builtin/remote.c | 2 +-\n builtin/send-pack.c | 2 +-\n config.c | 2 +-\n pager.c | 2 +-\n submodule-config.c | 6 +++---\n 10 files changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 20741f345..7a83a3a6e 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -187,6 +187,10 @@ Same as `git_config_bool`, except that integers are returned as-is, and\n an `is_bool` flag is unset.\n \n `git_config_maybe_bool`::\n+Deprecated. Use `git_parse_maybe_bool` instead. They are exactly the\n+same, except this function takes an unused argument `name`.\n+\n+`git_parse_maybe_bool`::\n Same as `git_config_bool`, except that it returns -1 on error rather\n than dying.\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex c6362cf92..9182f0ee3 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -60,7 +60,7 @@ static int auto_decoration_style(void)\n \n static int parse_decoration_style(const char *var, const char *value)\n {\n-\tswitch (git_config_maybe_bool(var, value)) {\n+\tswitch (git_parse_maybe_bool(value)) {\n \tcase 1:\n \t\treturn DECORATE_SHORT_REFS;\n \tcase 0:\n@@ -821,7 +821,7 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"format.from\")) {\n-\t\tint b = git_config_maybe_bool(var, value);\n+\t\tint b = git_parse_maybe_bool(value);\n \t\tfree(from);\n \t\tif (b < 0)\n \t\t\tfrom = xstrdup(value);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 900bafdb4..6a5122921 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -566,7 +566,7 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \telse if (!strcmp(k, \"merge.renormalize\"))\n \t\toption_renormalize = git_config_bool(k, v);\n \telse if (!strcmp(k, \"merge.ff\")) {\n-\t\tint boolval = git_config_maybe_bool(k, v);\n+\t\tint boolval = git_parse_maybe_bool(v);\n \t\tif (0 <= boolval) {\n \t\t\tfast_forward = boolval ? FF_ALLOW : FF_NO;\n \t\t} else if (v && !strcmp(v, \"only\")) {\n@@ -940,7 +940,7 @@ static int default_edit_option(void)\n \t\treturn 0;\n \n \tif (e) {\n-\t\tint v = git_config_maybe_bool(name, e);\n+\t\tint v = git_parse_maybe_bool(e);\n \t\tif (v < 0)\n \t\t\tdie(_(\"Bad value '%s' in environment '%s'\"), e, name);\n \t\treturn v;\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 9b86e519b..7fe281414 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -39,7 +39,7 @@ enum rebase_type {\n static enum rebase_type parse_config_rebase(const char *key, const char *value,\n \t\tint fatal)\n {\n-\tint v = git_config_maybe_bool(\"pull.rebase\", value);\n+\tint v = git_parse_maybe_bool(value);\n \n \tif (!v)\n \t\treturn REBASE_FALSE;\n@@ -274,7 +274,7 @@ static const char *config_get_ff(void)\n \tif (git_config_get_value(\"pull.ff\", &value))\n \t\treturn NULL;\n \n-\tswitch (git_config_maybe_bool(\"pull.ff\", value)) {\n+\tswitch (git_parse_maybe_bool(value)) {\n \tcase 0:\n \t\treturn \"--no-ff\";\n \tcase 1:\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 03846e837..2ac810422 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -481,7 +481,7 @@ static int git_push_config(const char *k, const char *v, void *cb)\n \t} else if (!strcmp(k, \"push.gpgsign\")) {\n \t\tconst char *value;\n \t\tif (!git_config_get_value(\"push.gpgsign\", &value)) {\n-\t\t\tswitch (git_config_maybe_bool(\"push.gpgsign\", value)) {\n+\t\t\tswitch (git_parse_maybe_bool(value)) {\n \t\t\tcase 0:\n \t\t\t\tset_push_cert_flags(flags, SEND_PACK_PUSH_CERT_NEVER);\n \t\t\t\tbreak;\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 6273c0c23..a995ea86c 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -301,7 +301,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)\n \t\t\t}\n \t\t\tstring_list_append(&info->merge, xstrdup(value));\n \t\t} else {\n-\t\t\tint v = git_config_maybe_bool(orig_key, value);\n+\t\t\tint v = git_parse_maybe_bool(value);\n \t\t\tif (v >= 0)\n \t\t\t\tinfo->rebase = v;\n \t\t\telse if (!strcmp(value, \"preserve\"))\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 633e0c3cd..fc4f0bb5f 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -105,7 +105,7 @@ static int send_pack_config(const char *k, const char *v, void *cb)\n \tif (!strcmp(k, \"push.gpgsign\")) {\n \t\tconst char *value;\n \t\tif (!git_config_get_value(\"push.gpgsign\", &value)) {\n-\t\t\tswitch (git_config_maybe_bool(\"push.gpgsign\", value)) {\n+\t\t\tswitch (git_parse_maybe_bool(value)) {\n \t\t\tcase 0:\n \t\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_NEVER;\n \t\t\t\tbreak;\ndiff --git a/config.c b/config.c\nindex d87376a5d..9b474d9a3 100644\n--- a/config.c\n+++ b/config.c\n@@ -1851,7 +1851,7 @@ int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *de\n {\n \tconst char *value;\n \tif (!git_configset_get_value(cs, key, &value)) {\n-\t\t*dest = git_config_maybe_bool(key, value);\n+\t\t*dest = git_parse_maybe_bool(value);\n \t\tif (*dest == -1)\n \t\t\treturn -1;\n \t\treturn 0;\ndiff --git a/pager.c b/pager.c\nindex 4dd9e1b26..92b23e6cd 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -194,7 +194,7 @@ static int pager_command_config(const char *var, const char *value, void *vdata)\n \tconst char *cmd;\n \n \tif (skip_prefix(var, \"pager.\", &cmd) && !strcmp(cmd, data->cmd)) {\n-\t\tint b = git_config_maybe_bool(var, value);\n+\t\tint b = git_parse_maybe_bool(value);\n \t\tif (b >= 0)\n \t\t\tdata->want = b;\n \t\telse {\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 5fe2d0787..11a32584d 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -232,7 +232,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,\n static int parse_fetch_recurse(const char *opt, const char *arg,\n \t\t\t int die_on_error)\n {\n-\tswitch (git_config_maybe_bool(opt, arg)) {\n+\tswitch (git_parse_maybe_bool(arg)) {\n \tcase 1:\n \t\treturn RECURSE_SUBMODULES_ON;\n \tcase 0:\n@@ -277,7 +277,7 @@ int option_fetch_parse_recurse_submodules(const struct option *opt,\n static int parse_update_recurse(const char *opt, const char *arg,\n \t\t\t\tint die_on_error)\n {\n-\tswitch (git_config_maybe_bool(opt, arg)) {\n+\tswitch (git_parse_maybe_bool(arg)) {\n \tcase 1:\n \t\treturn RECURSE_SUBMODULES_ON;\n \tcase 0:\n@@ -297,7 +297,7 @@ int parse_update_recurse_submodules_arg(const char *opt, const char *arg)\n static int parse_push_recurse(const char *opt, const char *arg,\n \t\t\t int die_on_error)\n {\n-\tswitch (git_config_maybe_bool(opt, arg)) {\n+\tswitch (git_parse_maybe_bool(arg)) {\n \tcase 1:\n \t\t/* There's no simple \"on\" value when pushing */\n \t\tif (die_on_error)\n-- \n2.14.0.5.g0f7b1ed27\n\n"},{"id":"325720","messageId":"efb46bffdac9dcf1fa91ee5a800ce370ceadfd55.1502128418.git.martin.agren@gmail.com","threadId":"46531","inReplyTo":"cover.1502128418.git.martin.agren@gmail.com","subject":"[PATCH 6/6] parse_decoration_style: drop unused argument `var`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-07T18:20:50Z","receivedAt":"2017-08-07T18:21:53Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The previous commit left it unused.\n\nSigned-off-by: Martin Ågren \n---\n builtin/log.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 9182f0ee3..483d15a94 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -58,7 +58,7 @@ static int auto_decoration_style(void)\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+static int parse_decoration_style(const char *value)\n {\n \tswitch (git_parse_maybe_bool(value)) {\n \tcase 1:\n@@ -82,7 +82,7 @@ static int decorate_callback(const struct option *opt, const char *arg, int unse\n \tif (unset)\n \t\tdecoration_style = 0;\n \telse if (arg)\n-\t\tdecoration_style = parse_decoration_style(\"command line\", arg);\n+\t\tdecoration_style = parse_decoration_style(arg);\n \telse\n \t\tdecoration_style = DECORATE_SHORT_REFS;\n \n@@ -409,7 +409,7 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"log.date\"))\n \t\treturn git_config_string(&default_date_mode, var, value);\n \tif (!strcmp(var, \"log.decorate\")) {\n-\t\tdecoration_style = parse_decoration_style(var, value);\n+\t\tdecoration_style = parse_decoration_style(value);\n \t\tif (decoration_style < 0)\n \t\t\tdecoration_style = 0; /* maybe warn? */\n \t\treturn 0;\n-- \n2.14.0.5.g0f7b1ed27\n\n"},{"id":"325725","messageId":"CAGZ79kaEA7DRm4inO=XPCVvA3mJioV_yckAVEUbCo5DEwoxbUQ@mail.gmail.com","threadId":"46531","inReplyTo":"cover.1502128418.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/6] clean up parsing of maybe_bool","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-07T18:44:06Z","receivedAt":"2017-08-07T18:44:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 7, 2017 at 11:20 AM, Martin Ågren wrote:\n> When we want to parse a boolean config item without dying on error, we\n> call git_config_maybe_bool() which takes two arguments: the value to be\n> parsed (obviously) and a `name` which is completely ignored. Junio has\n> suggested to drop `name` and rename the function [1]. That effort even\n> started shortly after that, by introducing git_parse_maybe_bool(). The\n> new function currently only has a single user outside config.c.\n>\n> Patch 5 of this series deprecates the old function and updates all\n> callers to use git_parse_maybe_bool() instead. Patch 6 is a final\n> cleanup: one of the converted callers suddenly had an unused argument.\n>\n> Patches 3 and 4 prepare for the switch. In particular, patch 4 ensures\n> that the two functions are actually equivalent. In doing so, it affects\n> `git push --signed=..` which was very slightly inconsistent to the rest\n> of Git.\n>\n> Patch 2 adds a failing test in preparation for patch 4. Patch 1 corrects\n> the documentation not to say \"git push --sign=..\" to make it a bit more\n> obvious that the opposite typo is not being made in patches 2 and 4.\n>\n> git_parse_maybe_bool is used in sb/diff-color-move, which is in \"next\".\n> This series makes --color-moved and diff.colormoved consistent with\n> other booleans. That should be a good thing, but cc Stefan to be sure.\n\nThe series looks fine to me overall, though patch 5 is overly gentle IMHO.\nWe could have removed it right there as Junio is very good at resolving\nconflicts or producing dirty merges for such a situation.\nBut delaying it until no other series' are in flight is fine with me, too.\n\nLooking back at sb/diff-color-move and the code where\ngit_parse_maybe_bool is used, I do not think this will be an issue.\n\nThanks,\nStefan\n"},{"id":"325740","messageId":"xmqqd187p1lm.fsf@gitster.mtv.corp.google.com","threadId":"46531","inReplyTo":"4ac17762106cde543c4c60968946b68263830d61.1502128418.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/6] t5334: document that git push --signed=1 does not work","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-07T20:25:09Z","receivedAt":"2017-08-07T20:25:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> When accepting booleans as command-line or config options throughout\n> Git, there are several documented synonyms for true and false.\n> However, one particular user is slightly broken: `git push --signed=..`\n> does not understand the integer synonyms for true and false.\n>\n> This is hardly wanted. The --signed option has a different notion of\n> boolean than all other arguments and config options, including the\n> config option corresponding to it, push.gpgSign.\n>\n> Add a test documenting the failure to handle --signed=1.\n\nMy knee-jerk reaction is that parse_maybe_bool() is broken in that\nit should do everything that config_maybe_bool() does. I wonder why\nwe do not call git_parse_int() like git_config_maybe_bool() does?\n\n... Ahh, that is because bool_or_int() wants to know that \"1\" is an\nint and not a bool, and parse_maybe_bool() is designed to cater to\nthe need of that thing, in addition to config_maybe_bool(). And the\n\"--signed=\" thing wants parse_bool_or_string(), not bool_or_int(),\nso we should treat \"1\" as true and \"0\" as false. There is no way to\ndo so in the current code and that is why you have the new _text()\nthing in patches 3-4/6.\n\nMakes sense.\n\n\n> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\n> index 464ffdd14..5dce55e1d 100755\n> --- a/t/t5534-push-signed.sh\n> +++ b/t/t5534-push-signed.sh\n> @@ -71,6 +71,13 @@ test_expect_success 'push --signed fails with a receiver without push certificat\n> \ttest_i18ngrep \"the receiving end does not support\" err\n> '\n> \n> +test_expect_failure 'push --signed=1 is accepted' '\n> +\tprepare_dst &&\nn> +\tmkdir -p dst/.git/hooks &&\n> +\ttest_must_fail git push --signed=1 dst noop ff +noff 2>err &&\n> +\ttest_i18ngrep \"the receiving end does not support\" err\n> +'\n> +\n> test_expect_success GPG 'no certificate for a signed push with no update' '\n> \tprepare_dst &&\n> \tmkdir -p dst/.git/hooks &&\n"},{"id":"325745","messageId":"xmqq1sonozdx.fsf@gitster.mtv.corp.google.com","threadId":"46531","inReplyTo":"CAGZ79kaEA7DRm4inO=XPCVvA3mJioV_yckAVEUbCo5DEwoxbUQ@mail.gmail.com","subject":"Re: [PATCH 0/6] clean up parsing of maybe_bool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-07T21:12:58Z","receivedAt":"2017-08-07T21:13:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The series looks fine to me overall, though patch 5 is overly gentle IMHO.\n> We could have removed it right there as Junio is very good at resolving\n> conflicts or producing dirty merges for such a situation.\n> But delaying it until no other series' are in flight is fine with me, too.\n\nIf you remove the old one, it would cause compilation error due to\nremoval of the declaration of the old one when other series that are\nin flight adds new callsites to it. Which makes life a bit easier\nfor the integrators when it is trivial to convert these callsites to\nuse the new one. If the way the old one and the new one are called\nare vastly different, of course, leaving the compatibility layer\nthat no longer is used after the series will make it easier to live\nwith other topics in flight, on the other hand.\n\nI am fine with either in this case, but I probably would have opted\nfor removal at the end of this series if I were doing this series,\nbecause\n\n-\tgit_config_maybe_bool(K,V)\n+\tgit_parse_maybe_bool(V)\n\nthat may have to happen during evil merges would have been trivial.\n\nThanks.\n"},{"id":"325780","messageId":"CAN0heSqbi-2VzhXtFO--v2+kMKuUbt4Z3QgtjW6WLEFP+VeoTg@mail.gmail.com","threadId":"46531","inReplyTo":"xmqq1sonozdx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] clean up parsing of maybe_bool","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-08T04:02:06Z","receivedAt":"2017-08-08T04:02:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 7 August 2017 at 23:12, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> The series looks fine to me overall, though patch 5 is overly gentle IMHO.\n>> We could have removed it right there as Junio is very good at resolving\n>> conflicts or producing dirty merges for such a situation.\n>> But delaying it until no other series' are in flight is fine with me, too.\n>\n[...]\n>\n> I am fine with either in this case, but I probably would have opted\n> for removal at the end of this series if I were doing this series,\n> because\n>\n> - git_config_maybe_bool(K,V)\n> + git_parse_maybe_bool(V)\n>\n> that may have to happen during evil merges would have been trivial.\n\nThanks, both of you. I could wait a couple of days to see if there are\nother things to address, then send a v2 with a more aggressive patch 5?\n\nMartin\n"},{"id":"325816","messageId":"xmqqbmnqc7od.fsf@gitster.mtv.corp.google.com","threadId":"46531","inReplyTo":"CAN0heSqbi-2VzhXtFO--v2+kMKuUbt4Z3QgtjW6WLEFP+VeoTg@mail.gmail.com","subject":"Re: [PATCH 0/6] clean up parsing of maybe_bool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-08T17:04:34Z","receivedAt":"2017-08-08T17:04:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Thanks, both of you. I could wait a couple of days to see if there are\n> other things to address, then send a v2 with a more aggressive patch 5?\n\nSounds like a plan. If there aren't anything else, I personally do\nnot mind using what is already on the list without v2, though. That\nwould be less work/churn for me ;-)\n\nThanks.\n"},{"id":"325820","messageId":"CAN0heSrRP-FsGjyhp5WNM7u+SAgBtRb-dkPcX52Ay6j3hakGLg@mail.gmail.com","threadId":"46531","inReplyTo":"xmqqbmnqc7od.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] clean up parsing of maybe_bool","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-08T17:21:08Z","receivedAt":"2017-08-08T17:21:13Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 8 August 2017 at 19:04, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> Thanks, both of you. I could wait a couple of days to see if there are\n>> other things to address, then send a v2 with a more aggressive patch 5?\n>\n> Sounds like a plan. If there aren't anything else, I personally do\n> not mind using what is already on the list without v2, though. That\n> would be less work/churn for me ;-)\n\nSure. I'll try to remember to send a patch to kill git_config_maybe_bool\nsome time from now. (At which point you'll get more work.. ;) )\n"}]}