From a81383badc50633fe459d8d2d10ee7af6c00abb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Mon, 7 Aug 2017 20:20:45 +0200 Subject: [PATCH 1/6] Doc/git-{push,send-pack}: correct --sign= to --signed= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 1624a35888..766673434a 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=] [--repo=] [-f | --force] [-d | --delete] [--prune] [-v | --verbose] [-u | --set-upstream] [--push-option=] - [--[no-]signed|--sign=(true|false|if-asked)] + [--[no-]signed|--signed=(true|false|if-asked)] [--force-with-lease[=[:]]] [--no-verify] [ [...]] @@ -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 a831dd0288..d32841f7a0 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=] [--verbose] [--thin] [--atomic] - [--[no-]signed|--sign=(true|false|if-asked)] + [--[no-]signed|--signed=(true|false|if-asked)] [:] [...] 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 From c4b71a77828ffe7154c2e0bbc5e8a34e843f2f3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Mon, 7 Aug 2017 20:20:46 +0200 Subject: [PATCH 2/6] t5334: document that git push --signed=1 does not work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 ecb8d446a5..591a26278d 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 && From 9be04d64c9b45a37cba161ba2eff2e784f87f91a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Mon, 7 Aug 2017 20:20:47 +0200 Subject: [PATCH 3/6] config: introduce git_parse_maybe_bool_text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- config.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/config.c b/config.c index c6b874a7bf..ce763c824c 100644 --- a/config.c +++ b/config.c @@ -709,7 +709,7 @@ unsigned long git_config_ulong(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; @@ -726,9 +726,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)) @@ -738,7 +743,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; From 4666741823239ed45ce9a63914dfd3c1601cf868 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Mon, 7 Aug 2017 20:20:48 +0200 Subject: [PATCH 4/6] config: make git_{config,parse}_maybe_bool equivalent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- config.c | 10 +++++----- t/t5534-push-signed.sh | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/config.c b/config.c index ce763c824c..25d798f4cf 100644 --- a/config.c +++ b/config.c @@ -727,11 +727,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) @@ -741,6 +736,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); diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh index 591a26278d..f88487bea2 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 && From 8957661378b073b49875059d6426614facb0d7f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Mon, 7 Aug 2017 20:20:49 +0200 Subject: [PATCH 5/6] treewide: deprecate git_config_maybe_bool, use git_parse_maybe_bool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 | 4 ++-- 10 files changed, 17 insertions(+), 13 deletions(-) diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt index 20741f345e..7a83a3a6e2 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 55d20cc2d8..c9564da84f 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -54,7 +54,7 @@ struct line_opt_callback_data { 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: @@ -809,7 +809,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 a96d4fb501..f9fe5c9d78 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")) { @@ -959,7 +959,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 3ecb881b0b..1c0eb27c37 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -36,7 +36,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; @@ -271,7 +271,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 5c22e9f2e5..1410b66d0e 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -480,7 +480,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 5339ed6ad1..ecd7b3a8cc 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -300,7 +300,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 1ff5a67538..a3d68d60ab 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -104,7 +104,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 25d798f4cf..434a7daf19 100644 --- a/config.c +++ b/config.c @@ -1617,7 +1617,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 ae79643363..40347d4e22 100644 --- a/pager.c +++ b/pager.c @@ -226,7 +226,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 93453909cf..0fcdb39267 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -213,7 +213,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: @@ -237,7 +237,7 @@ int parse_fetch_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) From f094b89a4d75216736830b5d2286ebc6846a25a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Mon, 7 Aug 2017 20:20:50 +0200 Subject: [PATCH 6/6] parse_decoration_style: drop unused argument `var` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit left it unused. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- builtin/log.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index c9564da84f..8a46cec399 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -52,7 +52,7 @@ struct line_opt_callback_data { struct string_list args; }; -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: @@ -76,7 +76,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; @@ -401,7 +401,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;