{"thread":{"id":"60594","subject":"[PATCH 0/9] bonus config cleanups","startedAt":"2023-12-07T07:23:39Z","lastAt":"2023-12-12T01:37:03Z","messageCount":16,"participants":["Jeff King","Patrick Steinhardt","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"485426","messageId":"20231207072338.GA1277727@coredump.intra.peff.net","threadId":"60594","inReplyTo":null,"subject":"[PATCH 0/9] bonus config cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:23:38Z","receivedAt":"2023-12-07T07:23:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While looking carefully at various config callbacks for the series at:\n\n https://lore.kernel.org/git/20231207071030.GA1275835@coredump.intra.peff.net/\n\nI noticed a bunch of other small bugs/cleanups. I split these into their\nown series here, which should be applied on top (it could go straight to\n\"master\", but there is a small conflict in patch 6, as the option it\ntouches was fixed in the other series). I'm happy to prepare it as an\nindependent series if we prefer.\n\n [1/9]: config: reject bogus values for core.checkstat\n [2/9]: git_xmerge_config(): prefer error() to die()\n [3/9]: imap-send: don't use git_die_config() inside callback\n [4/9]: config: use config_error_nonbool() instead of custom messages\n [5/9]: diff: give more detailed messages for bogus diff.* config\n [6/9]: config: use git_config_string() for core.checkRoundTripEncoding\n [7/9]: push: drop confusing configset/callback redundancy\n [8/9]: gpg-interface: drop pointless config_error_nonbool() checks\n [9/9]: sequencer: simplify away extra git_config_string() call\n\n builtin/push.c | 31 +++++++++++++------------------\n builtin/send-pack.c | 27 ++++++++++++---------------\n config.c | 11 +++++------\n convert.h | 2 +-\n diff.c | 8 ++++++--\n environment.c | 2 +-\n gpg-interface.c | 15 +++------------\n imap-send.c | 2 +-\n merge-ll.c | 2 +-\n sequencer.c | 21 ++++++++-------------\n xdiff-interface.c | 7 ++++---\n 11 files changed, 55 insertions(+), 73 deletions(-)\n\n-Peff\n"},{"id":"485427","messageId":"20231207072404.GA1277973@coredump.intra.peff.net","threadId":"60594","inReplyTo":"20231207072338.GA1277727@coredump.intra.peff.net","subject":"[PATCH 1/9] config: reject bogus values for core.checkstat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:24:04Z","receivedAt":"2023-12-07T07:24:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you feed nonsense config like:\n\n git -c core.checkstat=foobar status\n\nwe'll silently ignore the unknown value, rather than reporting an error.\nThis goes all the way back to c08e4d5b5c (Enable minimal stat checking,\n2013-01-22).\n\nDetecting and complaining now is technically a backwards-incompatible\nchange, but I don't think anybody has any reason to use an invalid value\nhere. There are no historical values we'd want to allow for backwards\ncompatibility or anything like that. We are better off loudly telling\nthe user that their config may not be doing what they expect.\n\nSigned-off-by: Jeff King \n---\n config.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 18085c7e38..d997c55e33 100644\n--- a/config.c\n+++ b/config.c\n@@ -1392,6 +1392,9 @@ static int git_default_core_config(const char *var, const char *value,\n \t\t\tcheck_stat = 1;\n \t\telse if (!strcasecmp(value, \"minimal\"))\n \t\t\tcheck_stat = 0;\n+\t\telse\n+\t\t\treturn error(_(\"invalid value for '%s': '%s'\"),\n+\t\t\t\t var, value);\n \t}\n \n \tif (!strcmp(var, \"core.quotepath\")) {\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485428","messageId":"20231207072449.GB1277973@coredump.intra.peff.net","threadId":"60594","inReplyTo":"20231207072338.GA1277727@coredump.intra.peff.net","subject":"[PATCH 2/9] git_xmerge_config(): prefer error() to die()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:24:49Z","receivedAt":"2023-12-07T07:24:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parsing merge config, a few code paths die on error. It's\npreferable for us to call error() here, because the resulting error\nmessage from the config parsing code contains much more detail.\n\nFor example, before:\n\n fatal: unknown style 'bogus' given for 'merge.conflictstyle'\n\nand after:\n\n error: unknown style 'bogus' given for 'merge.conflictstyle'\n fatal: bad config variable 'merge.conflictstyle' in file '.git/config' at line 7\n\nSince we're touching these lines, I also marked them for translation.\nThere's no reason they shouldn't behave like most other config-parsing\nerrors.\n\nSigned-off-by: Jeff King \n---\nBefore anyone mentions config_error_nonbool(), yes, the first hunk here\ngets simplified to that in a later patch.\n\n xdiff-interface.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex adcea109fa..05d6475a09 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -1,4 +1,5 @@\n #include \"git-compat-util.h\"\n+#include \"gettext.h\"\n #include \"config.h\"\n #include \"hex.h\"\n #include \"object-store-ll.h\"\n@@ -313,7 +314,7 @@ int git_xmerge_config(const char *var, const char *value,\n {\n \tif (!strcmp(var, \"merge.conflictstyle\")) {\n \t\tif (!value)\n-\t\t\tdie(\"'%s' is not a boolean\", var);\n+\t\t\treturn error(_(\"'%s' is not a boolean\"), var);\n \t\tif (!strcmp(value, \"diff3\"))\n \t\t\tgit_xmerge_style = XDL_MERGE_DIFF3;\n \t\telse if (!strcmp(value, \"zdiff3\"))\n@@ -325,8 +326,8 @@ int git_xmerge_config(const char *var, const char *value,\n \t\t * git-completion.bash when you add new merge config\n \t\t */\n \t\telse\n-\t\t\tdie(\"unknown style '%s' given for '%s'\",\n-\t\t\t value, var);\n+\t\t\treturn error(_(\"unknown style '%s' given for '%s'\"),\n+\t\t\t\t value, var);\n \t\treturn 0;\n \t}\n \treturn git_default_config(var, value, ctx, cb);\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485429","messageId":"20231207072458.GC1277973@coredump.intra.peff.net","threadId":"60594","inReplyTo":"20231207072338.GA1277727@coredump.intra.peff.net","subject":"[PATCH 3/9] imap-send: don't use git_die_config() inside callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:24:58Z","receivedAt":"2023-12-07T07:24:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The point of git_die_config() is to let configset users mention the\nfile/line info for invalid config, like:\n\n if (!git_config_get_int(\"foo.bar\", &value)) {\n\tif (!is_ok(value))\n\t\tgit_die_config(\"foo.bar\");\n }\n\nUsing it from within a config callback is unnecessary, because we can\nsimply return an error, at which point the config machinery will mention\nthe file/line of the offending variable. Worse, using git_die_config()\ncan actually produce the wrong location when the key is found in\nmultiple spots. For instance, with config like:\n\n [imap]\n host\n host = foo\n\nwe'll report the line number of the \"host = foo\" line, but the problem\nis on the implicit-bool \"host\" line.\n\nWe can fix it by just returning an error code.\n\nSigned-off-by: Jeff King \n---\n imap-send.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 996651e4f8..5b0fe4f95a 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val,\n \t\tserver.port = git_config_int(var, val, ctx->kvi);\n \telse if (!strcmp(\"imap.host\", var)) {\n \t\tif (!val) {\n-\t\t\tgit_die_config(\"imap.host\", \"Missing value for 'imap.host'\");\n+\t\t\treturn error(\"Missing value for 'imap.host'\");\n \t\t} else {\n \t\t\tif (starts_with(val, \"imap:\"))\n \t\t\t\tval += 5;\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485430","messageId":"20231207072516.GD1277973@coredump.intra.peff.net","threadId":"60594","inReplyTo":"20231207072338.GA1277727@coredump.intra.peff.net","subject":"[PATCH 4/9] config: use config_error_nonbool() instead of custom messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:25:16Z","receivedAt":"2023-12-07T07:25:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A few config callbacks use their own custom messages to report an\nunexpected implicit bool like:\n\n [merge \"foo\"]\n driver\n\nThese should just use config_error_nonbool(), so the user sees\nconsistent messages.\n\nSigned-off-by: Jeff King \n---\n imap-send.c | 2 +-\n merge-ll.c | 2 +-\n xdiff-interface.c | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 5b0fe4f95a..9c4df7bbc8 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val,\n \t\tserver.port = git_config_int(var, val, ctx->kvi);\n \telse if (!strcmp(\"imap.host\", var)) {\n \t\tif (!val) {\n-\t\t\treturn error(\"Missing value for 'imap.host'\");\n+\t\t\treturn config_error_nonbool(var);\n \t\t} else {\n \t\t\tif (starts_with(val, \"imap:\"))\n \t\t\t\tval += 5;\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 8fcf2d3710..1df58ebaac 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -301,7 +301,7 @@ static int read_merge_config(const char *var, const char *value,\n \n \tif (!strcmp(\"driver\", key)) {\n \t\tif (!value)\n-\t\t\treturn error(\"%s: lacks value\", var);\n+\t\t\treturn config_error_nonbool(var);\n \t\t/*\n \t\t * merge..driver specifies the command line:\n \t\t *\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 05d6475a09..d788689d01 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -314,7 +314,7 @@ int git_xmerge_config(const char *var, const char *value,\n {\n \tif (!strcmp(var, \"merge.conflictstyle\")) {\n \t\tif (!value)\n-\t\t\treturn error(_(\"'%s' is not a boolean\"), var);\n+\t\t\treturn config_error_nonbool(var);\n \t\tif (!strcmp(value, \"diff3\"))\n \t\t\tgit_xmerge_style = XDL_MERGE_DIFF3;\n \t\telse if (!strcmp(value, \"zdiff3\"))\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485431","messageId":"20231207072523.GE1277973@coredump.intra.peff.net","threadId":"60594","inReplyTo":"20231207072338.GA1277727@coredump.intra.peff.net","subject":"[PATCH 5/9] diff: give more detailed messages for bogus diff.* config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:25:23Z","receivedAt":"2023-12-07T07:25:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The config callbacks for a few diff.* variables simply return -1 when we\nencounter an error. The message you get mentions the offending location,\nlike:\n\n fatal: bad config variable 'diff.algorithm' in file '.git/config' at line 7\n\nbut is vague about \"bad\" (as it must be, since the message comes from\nthe generic config code). Most callbacks add their own messages here, so\nlet's do the same. E.g.:\n\n error: unknown value for config 'diff.algorithm': foo\n fatal: bad config variable 'diff.algorithm' in file '.git/config' at line 7\n\nI've written the string in a way that should be reusable for\ntranslators, and matches another similar message in transport.c (there\ndoesn't yet seem to be a popular generic message to reuse here, so\nhopefully this will get the ball rolling).\n\nNote that in the case of diff.algorithm, our parse_algorithm_value()\nhelper does detect a NULL value string. But it's still worth detecting\nit ourselves here, since we can give a more specific error message (and\nwhich is the usual one for unexpected implicit-bool values).\n\nSigned-off-by: Jeff King \n---\n diff.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 5b213a4b44..a2def45644 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -445,9 +445,12 @@ int git_diff_ui_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"diff.algorithm\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n \t\tdiff_algorithm = parse_algorithm_value(value);\n \t\tif (diff_algorithm < 0)\n-\t\t\treturn -1;\n+\t\t\treturn error(_(\"unknown value for config '%s': %s\"),\n+\t\t\t\t var, value);\n \t\treturn 0;\n \t}\n \n@@ -486,7 +489,8 @@ int git_diff_basic_config(const char *var, const char *value,\n \t\t\treturn config_error_nonbool(var);\n \t\tval = parse_ws_error_highlight(value);\n \t\tif (val < 0)\n-\t\t\treturn -1;\n+\t\t\treturn error(_(\"unknown value for config '%s': %s\"),\n+\t\t\t\t var, value);\n \t\tws_error_highlight_default = val;\n \t\treturn 0;\n \t}\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485432","messageId":"20231207072611.GF1277973@coredump.intra.peff.net","threadId":"60594","inReplyTo":"20231207072338.GA1277727@coredump.intra.peff.net","subject":"[PATCH 6/9] config: use git_config_string() for core.checkRoundTripEncoding","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:26:11Z","receivedAt":"2023-12-07T07:26:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since this code path was recently converted to check for a NULL value,\nit now behaves exactly like git_config_string(). We can shorten the code\na bit by using that helper.\n\nNote that git_config_string() takes a const pointer, but our storage\nvariable is non-const. We're better off making this \"const\", though,\nsince the default value points to a string literal (and thus it would be\nan error if anybody tried to write to it).\n\nSigned-off-by: Jeff King \n---\nAn observant reader may notice that this means duplicate config like:\n\n [core]\n checkRoundTripEncoding = foo\n checkRoundTripEncoding = bar\n\nwill leak the string for \"foo\". That is true before this patch, too, and\nis true of all callers of git_config_string(). I'm going to punt on that\nfor now, and look into it as a separate series.\n\n config.c | 8 ++------\n convert.h | 2 +-\n environment.c | 2 +-\n 3 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex d997c55e33..00a11b5d98 100644\n--- a/config.c\n+++ b/config.c\n@@ -1551,12 +1551,8 @@ static int git_default_core_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"core.checkroundtripencoding\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tcheck_roundtrip_encoding = xstrdup(value);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(var, \"core.checkroundtripencoding\"))\n+\t\treturn git_config_string(&check_roundtrip_encoding, var, value);\n \n \tif (!strcmp(var, \"core.notesref\")) {\n \t\tif (!value)\ndiff --git a/convert.h b/convert.h\nindex d925589444..ab8b4fa68d 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -92,7 +92,7 @@ void convert_attrs(struct index_state *istate,\n \t\t struct conv_attrs *ca, const char *path);\n \n extern enum eol core_eol;\n-extern char *check_roundtrip_encoding;\n+extern const char *check_roundtrip_encoding;\n const char *get_cached_convert_stats_ascii(struct index_state *istate,\n \t\t\t\t\t const char *path);\n const char *get_wt_convert_stats_ascii(const char *path);\ndiff --git a/environment.c b/environment.c\nindex 9e37bf58c0..90632a39bc 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -64,7 +64,7 @@ const char *excludes_file;\n enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;\n enum eol core_eol = EOL_UNSET;\n int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;\n-char *check_roundtrip_encoding = \"SHIFT-JIS\";\n+const char *check_roundtrip_encoding = \"SHIFT-JIS\";\n enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;\n enum rebase_setup_type autorebase = AUTOREBASE_NEVER;\n enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485433","messageId":"20231207072622.GG1277973@coredump.intra.peff.net","threadId":"60594","inReplyTo":"20231207072338.GA1277727@coredump.intra.peff.net","subject":"[PATCH 7/9] push: drop confusing configset/callback redundancy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:26:22Z","receivedAt":"2023-12-07T07:26:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We parse push config by calling git_config() with our git_push_config()\ncallback. But inside that callback, when we see \"push.gpgsign\", we\nignore the value passed into the callback and instead make a new call to\ngit_config_get_value().\n\nThis is unnecessary at best, and slightly wrong at worst (if there are\nmultiple instances, get_value() only returns one; both methods end up\nwith last-one-wins, but we'd fail to report errors if earlier\nincarnations were bogus).\n\nThe call was added by 68c757f219 (push: add a config option push.gpgSign\nfor default signed pushes, 2015-08-19). That commit doesn't give any\nreason to deviate from the usual strategy here; it was probably just\nsomebody unfamiliar with our config API and its conventions.\n\nIt also added identical code to builtin/send-pack.c, which also handles\npush.gpgsign.\n\nAnd then the same issue spread to its neighbor in b33a15b081 (push: add\nrecurseSubmodules config option, 2015-11-17), presumably via\ncargo-culting.\n\nThis patch fixes all three to just directly use the value provided to\nthe callback. While I was adjusting the code to do so, I noticed that\npush.gpgsign is overly careful about a NULL value. After\ngit_parse_maybe_bool() has returned anything besides 1, we know that the\nvalue cannot be NULL (if it were, it would be an implicit \"true\", and\nmany callers of maybe_bool rely on that). Here that lets us shorten \"if\n(v && !strcasecmp(v, ...))\" to just \"if (!strcasecmp(v, ...))\".\n\nSigned-off-by: Jeff King \n---\n builtin/push.c | 31 +++++++++++++------------------\n builtin/send-pack.c | 27 ++++++++++++---------------\n 2 files changed, 25 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 2e708383c2..58a9273e90 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -526,26 +526,21 @@ static int git_push_config(const char *k, const char *v,\n \t\t\t*flags |= TRANSPORT_PUSH_AUTO_UPSTREAM;\n \t\treturn 0;\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_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;\n-\t\t\tcase 1:\n-\t\t\t\tset_push_cert_flags(flags, SEND_PACK_PUSH_CERT_ALWAYS);\n-\t\t\t\tbreak;\n-\t\t\tdefault:\n-\t\t\t\tif (value && !strcasecmp(value, \"if-asked\"))\n-\t\t\t\t\tset_push_cert_flags(flags, SEND_PACK_PUSH_CERT_IF_ASKED);\n-\t\t\t\telse\n-\t\t\t\t\treturn error(_(\"invalid value for '%s'\"), k);\n-\t\t\t}\n+\t\tswitch (git_parse_maybe_bool(v)) {\n+\t\tcase 0:\n+\t\t\tset_push_cert_flags(flags, SEND_PACK_PUSH_CERT_NEVER);\n+\t\t\tbreak;\n+\t\tcase 1:\n+\t\t\tset_push_cert_flags(flags, SEND_PACK_PUSH_CERT_ALWAYS);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tif (!strcasecmp(v, \"if-asked\"))\n+\t\t\t\tset_push_cert_flags(flags, SEND_PACK_PUSH_CERT_IF_ASKED);\n+\t\t\telse\n+\t\t\t\treturn error(_(\"invalid value for '%s'\"), k);\n \t\t}\n \t} else if (!strcmp(k, \"push.recursesubmodules\")) {\n-\t\tconst char *value;\n-\t\tif (!git_config_get_value(\"push.recursesubmodules\", &value))\n-\t\t\trecurse_submodules = parse_push_recurse_submodules_arg(k, value);\n+\t\trecurse_submodules = parse_push_recurse_submodules_arg(k, v);\n \t} else if (!strcmp(k, \"submodule.recurse\")) {\n \t\tint val = git_config_bool(k, v) ?\n \t\t\tRECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex cd6d9e4112..00e6c90477 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -135,21 +135,18 @@ static int send_pack_config(const char *k, const char *v,\n \t\t\t const struct config_context *ctx, void *cb)\n {\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_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;\n-\t\t\tcase 1:\n-\t\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;\n-\t\t\t\tbreak;\n-\t\t\tdefault:\n-\t\t\t\tif (value && !strcasecmp(value, \"if-asked\"))\n-\t\t\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_IF_ASKED;\n-\t\t\t\telse\n-\t\t\t\t\treturn error(_(\"invalid value for '%s'\"), k);\n-\t\t\t}\n+\t\tswitch (git_parse_maybe_bool(v)) {\n+\t\tcase 0:\n+\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_NEVER;\n+\t\t\tbreak;\n+\t\tcase 1:\n+\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tif (!strcasecmp(v, \"if-asked\"))\n+\t\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_IF_ASKED;\n+\t\t\telse\n+\t\t\t\treturn error(_(\"invalid value for '%s'\"), k);\n \t\t}\n \t}\n \treturn git_default_config(k, v, ctx, cb);\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485434","messageId":"20231207072631.GH1277973@coredump.intra.peff.net","threadId":"60594","inReplyTo":"20231207072338.GA1277727@coredump.intra.peff.net","subject":"[PATCH 8/9] gpg-interface: drop pointless config_error_nonbool() checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:26:31Z","receivedAt":"2023-12-07T07:26:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Config callbacks which use git_config_string() or git_config_pathname()\nhave no need to check for a NULL value. This is handled automatically\nby those helpers.\n\nSigned-off-by: Jeff King \n---\n gpg-interface.c | 15 +++------------\n 1 file changed, 3 insertions(+), 12 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 48f43c5a21..25c42cb9fd 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -762,23 +762,14 @@ static int git_gpg_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"gpg.ssh.defaultkeycommand\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n+\tif (!strcmp(var, \"gpg.ssh.defaultkeycommand\"))\n \t\treturn git_config_string(&ssh_default_key_command, var, value);\n-\t}\n \n-\tif (!strcmp(var, \"gpg.ssh.allowedsignersfile\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n+\tif (!strcmp(var, \"gpg.ssh.allowedsignersfile\"))\n \t\treturn git_config_pathname(&ssh_allowed_signers, var, value);\n-\t}\n \n-\tif (!strcmp(var, \"gpg.ssh.revocationfile\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n+\tif (!strcmp(var, \"gpg.ssh.revocationfile\"))\n \t\treturn git_config_pathname(&ssh_revocation_file, var, value);\n-\t}\n \n \tif (!strcmp(var, \"gpg.program\") || !strcmp(var, \"gpg.openpgp.program\"))\n \t\tfmtname = \"openpgp\";\n-- \n2.43.0.664.ga12c899002\n\n"},{"id":"485435","messageId":"20231207072642.GI1277973@coredump.intra.peff.net","threadId":"60594","inReplyTo":"20231207072338.GA1277727@coredump.intra.peff.net","subject":"[PATCH 9/9] sequencer: simplify away extra git_config_string() call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-07T07:26:42Z","receivedAt":"2023-12-07T07:26:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In our config callback, we call git_config_string() to copy the incoming\nvalue string into a local string. But we don't modify or store that\nstring; we just look at it and then free it. We can make the code\nsimpler by just looking at the value passed into the callback.\n\nNote that we do need to check for NULL, which is the one bit of logic\ngit_config_string() did for us. And I could even see an argument that we\nare abstracting any error-checking of the value behind the\ngit_config_string() layer. But in practice no other callbacks behave\nthis way; it is standard to check for NULL and then just look at the\nstring directly.\n\nSigned-off-by: Jeff King \n---\n sequencer.c | 21 ++++++++-------------\n 1 file changed, 8 insertions(+), 13 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed..74c3b1243e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -238,34 +238,29 @@ static int git_sequencer_config(const char *k, const char *v,\n \t\t\t\tconst struct config_context *ctx, void *cb)\n {\n \tstruct replay_opts *opts = cb;\n-\tint status;\n \n \tif (!strcmp(k, \"commit.cleanup\")) {\n-\t\tconst char *s;\n+\t\tif (!v)\n+\t\t\treturn config_error_nonbool(k);\n \n-\t\tstatus = git_config_string(&s, k, v);\n-\t\tif (status)\n-\t\t\treturn status;\n-\n-\t\tif (!strcmp(s, \"verbatim\")) {\n+\t\tif (!strcmp(v, \"verbatim\")) {\n \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;\n \t\t\topts->explicit_cleanup = 1;\n-\t\t} else if (!strcmp(s, \"whitespace\")) {\n+\t\t} else if (!strcmp(v, \"whitespace\")) {\n \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;\n \t\t\topts->explicit_cleanup = 1;\n-\t\t} else if (!strcmp(s, \"strip\")) {\n+\t\t} else if (!strcmp(v, \"strip\")) {\n \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;\n \t\t\topts->explicit_cleanup = 1;\n-\t\t} else if (!strcmp(s, \"scissors\")) {\n+\t\t} else if (!strcmp(v, \"scissors\")) {\n \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SCISSORS;\n \t\t\topts->explicit_cleanup = 1;\n \t\t} else {\n \t\t\twarning(_(\"invalid commit message cleanup mode '%s'\"),\n-\t\t\t\t s);\n+\t\t\t\t v);\n \t\t}\n \n-\t\tfree((char *)s);\n-\t\treturn status;\n+\t\treturn 0;\n \t}\n \n \tif (!strcmp(k, \"commit.gpgsign\")) {\n-- \n2.43.0.664.ga12c899002\n"},{"id":"485444","messageId":"ZXGJE-pkb3BjlO-d@tanuki","threadId":"60594","inReplyTo":"20231207072458.GC1277973@coredump.intra.peff.net","subject":"Re: [PATCH 3/9] imap-send: don't use git_die_config() inside callback","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T08:57:55Z","receivedAt":"2023-12-07T08:58:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote:\n[snip]\n> diff --git a/imap-send.c b/imap-send.c\n> index 996651e4f8..5b0fe4f95a 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val,\n> \t\tserver.port = git_config_int(var, val, ctx->kvi);\n> \telse if (!strcmp(\"imap.host\", var)) {\n> \t\tif (!val) {\n> -\t\t\tgit_die_config(\"imap.host\", \"Missing value for 'imap.host'\");\n> +\t\t\treturn error(\"Missing value for 'imap.host'\");\n\nNit: while at it we might also mark this error for translation. Not\nworth a reroll on its own though.\n\nPatrick\n"},{"id":"485445","messageId":"ZXGJGv2i1WQRjVOl@tanuki","threadId":"60594","inReplyTo":"20231207072338.GA1277727@coredump.intra.peff.net","subject":"Re: [PATCH 0/9] bonus config cleanups","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T08:58:02Z","receivedAt":"2023-12-07T08:58:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 07, 2023 at 02:23:38AM -0500, Jeff King wrote:\n> While looking carefully at various config callbacks for the series at:\n> \n> https://lore.kernel.org/git/20231207071030.GA1275835@coredump.intra.peff.net/\n> \n> I noticed a bunch of other small bugs/cleanups. I split these into their\n> own series here, which should be applied on top (it could go straight to\n> \"master\", but there is a small conflict in patch 6, as the option it\n> touches was fixed in the other series). I'm happy to prepare it as an\n> independent series if we prefer.\n\nThe whole patch series looks good to me, thanks!\n\nPatrick\n\n> [1/9]: config: reject bogus values for core.checkstat\n> [2/9]: git_xmerge_config(): prefer error() to die()\n> [3/9]: imap-send: don't use git_die_config() inside callback\n> [4/9]: config: use config_error_nonbool() instead of custom messages\n> [5/9]: diff: give more detailed messages for bogus diff.* config\n> [6/9]: config: use git_config_string() for core.checkRoundTripEncoding\n> [7/9]: push: drop confusing configset/callback redundancy\n> [8/9]: gpg-interface: drop pointless config_error_nonbool() checks\n> [9/9]: sequencer: simplify away extra git_config_string() call\n> \n> builtin/push.c | 31 +++++++++++++------------------\n> builtin/send-pack.c | 27 ++++++++++++---------------\n> config.c | 11 +++++------\n> convert.h | 2 +-\n> diff.c | 8 ++++++--\n> environment.c | 2 +-\n> gpg-interface.c | 15 +++------------\n> imap-send.c | 2 +-\n> merge-ll.c | 2 +-\n> sequencer.c | 21 ++++++++-------------\n> xdiff-interface.c | 7 ++++---\n> 11 files changed, 55 insertions(+), 73 deletions(-)\n> \n> -Peff\n> \n"},{"id":"485492","messageId":"ZXOdp+uHCe+Daz4O@nand.local","threadId":"60594","inReplyTo":"20231207072404.GA1277973@coredump.intra.peff.net","subject":"Re: [PATCH 1/9] config: reject bogus values for core.checkstat","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-08T22:50:15Z","receivedAt":"2023-12-08T22:50:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Dec 07, 2023 at 02:24:04AM -0500, Jeff King wrote:\n> If you feed nonsense config like:\n>\n> git -c core.checkstat=foobar status\n>\n> we'll silently ignore the unknown value, rather than reporting an error.\n> This goes all the way back to c08e4d5b5c (Enable minimal stat checking,\n> 2013-01-22).\n>\n> Detecting and complaining now is technically a backwards-incompatible\n> change, but I don't think anybody has any reason to use an invalid value\n> here. There are no historical values we'd want to allow for backwards\n> compatibility or anything like that. We are better off loudly telling\n> the user that their config may not be doing what they expect.\n\nI think this is a good instance of \"yes, this is a backwards\nincompatible change, but the behavior we're breaking is so obviously\nbroken already that it's not worth maintaining compatibility.\"\n\nWell reasoned, I am definitely in favor here.\n\nThanks,\nTaylor\n"},{"id":"485494","messageId":"ZXOfrKYsmOjOHGmj@nand.local","threadId":"60594","inReplyTo":"ZXGJE-pkb3BjlO-d@tanuki","subject":"Re: [PATCH 3/9] imap-send: don't use git_die_config() inside callback","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-12-08T22:58:52Z","receivedAt":"2023-12-08T22:58:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Dec 07, 2023 at 09:57:55AM +0100, Patrick Steinhardt wrote:\n> On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote:\n> [snip]\n> > diff --git a/imap-send.c b/imap-send.c\n> > index 996651e4f8..5b0fe4f95a 100644\n> > --- a/imap-send.c\n> > +++ b/imap-send.c\n> > @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val,\n> > \t\tserver.port = git_config_int(var, val, ctx->kvi);\n> > \telse if (!strcmp(\"imap.host\", var)) {\n> > \t\tif (!val) {\n> > -\t\t\tgit_die_config(\"imap.host\", \"Missing value for 'imap.host'\");\n> > +\t\t\treturn error(\"Missing value for 'imap.host'\");\n>\n> Nit: while at it we might also mark this error for translation. Not\n> worth a reroll on its own though.\n\nThis string goes away entirely in the next patch, so I don't think we\nneed to mark it here.\n\nThanks,\nTaylor\n"},{"id":"485512","messageId":"ZXa9pz65O2B6Gwjp@tanuki","threadId":"60594","inReplyTo":"ZXOfrKYsmOjOHGmj@nand.local","subject":"Re: [PATCH 3/9] imap-send: don't use git_die_config() inside callback","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T07:43:35Z","receivedAt":"2023-12-11T07:43:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 08, 2023 at 05:58:52PM -0500, Taylor Blau wrote:\n> On Thu, Dec 07, 2023 at 09:57:55AM +0100, Patrick Steinhardt wrote:\n> > On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote:\n> > [snip]\n> > > diff --git a/imap-send.c b/imap-send.c\n> > > index 996651e4f8..5b0fe4f95a 100644\n> > > --- a/imap-send.c\n> > > +++ b/imap-send.c\n> > > @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val,\n> > > \t\tserver.port = git_config_int(var, val, ctx->kvi);\n> > > \telse if (!strcmp(\"imap.host\", var)) {\n> > > \t\tif (!val) {\n> > > -\t\t\tgit_die_config(\"imap.host\", \"Missing value for 'imap.host'\");\n> > > +\t\t\treturn error(\"Missing value for 'imap.host'\");\n> >\n> > Nit: while at it we might also mark this error for translation. Not\n> > worth a reroll on its own though.\n> \n> This string goes away entirely in the next patch, so I don't think we\n> need to mark it here.\n> \n> Thanks,\n> Taylor\n\nAh, true. Never mind in that case.\n\nPatrick\n"},{"id":"485554","messageId":"20231212013702.GG376323@coredump.intra.peff.net","threadId":"60594","inReplyTo":"ZXOfrKYsmOjOHGmj@nand.local","subject":"Re: [PATCH 3/9] imap-send: don't use git_die_config() inside callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-12T01:37:02Z","receivedAt":"2023-12-12T01:37:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 08, 2023 at 05:58:52PM -0500, Taylor Blau wrote:\n\n> On Thu, Dec 07, 2023 at 09:57:55AM +0100, Patrick Steinhardt wrote:\n> > On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote:\n> > [snip]\n> > > diff --git a/imap-send.c b/imap-send.c\n> > > index 996651e4f8..5b0fe4f95a 100644\n> > > --- a/imap-send.c\n> > > +++ b/imap-send.c\n> > > @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val,\n> > > \t\tserver.port = git_config_int(var, val, ctx->kvi);\n> > > \telse if (!strcmp(\"imap.host\", var)) {\n> > > \t\tif (!val) {\n> > > -\t\t\tgit_die_config(\"imap.host\", \"Missing value for 'imap.host'\");\n> > > +\t\t\treturn error(\"Missing value for 'imap.host'\");\n> >\n> > Nit: while at it we might also mark this error for translation. Not\n> > worth a reroll on its own though.\n> \n> This string goes away entirely in the next patch, so I don't think we\n> need to mark it here.\n\nRight. It's a little confusing because it is converted along with some\nother spots in the next patch. But in one of those other spots, we\nearlier switched it (in patch 2) from die() to error(), and we _did_\nmark it for translation as we did so.\n\nI did it there because in patch 2 we touch multiple messages, and the\nother ones don't end up as config_error_nonbool(), so we do want them\ntranslated.\n\nI'm not sure if there would have been an easier ordering to the series.\nI could have pulled the \"mark for translation\" bits from patch 2 into\ntheir own patch (after this one makes some of the messages go away), but\nthen I'd expect somebody would review patch 2 and say \"why not mark them\nfor translation while we're here?\". :)\n\n-Peff\n"}]}