From patchwork Thu Dec 7 07:24:04 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13482782 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3CF27137 for ; Wed, 6 Dec 2023 23:24:06 -0800 (PST) Received: (qmail 10000 invoked by uid 109); 7 Dec 2023 07:24:05 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 07 Dec 2023 07:24:05 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1039 invoked by uid 111); 7 Dec 2023 07:24:09 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 07 Dec 2023 02:24:09 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 7 Dec 2023 02:24:04 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 1/9] config: reject bogus values for core.checkstat Message-ID: <20231207072404.GA1277973@coredump.intra.peff.net> References: <20231207072338.GA1277727@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net> If you feed nonsense config like: git -c core.checkstat=foobar status we'll silently ignore the unknown value, rather than reporting an error. This goes all the way back to c08e4d5b5c (Enable minimal stat checking, 2013-01-22). Detecting and complaining now is technically a backwards-incompatible change, but I don't think anybody has any reason to use an invalid value here. There are no historical values we'd want to allow for backwards compatibility or anything like that. We are better off loudly telling the user that their config may not be doing what they expect. Signed-off-by: Jeff King --- config.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/config.c b/config.c index 18085c7e38..d997c55e33 100644 --- a/config.c +++ b/config.c @@ -1392,6 +1392,9 @@ static int git_default_core_config(const char *var, const char *value, check_stat = 1; else if (!strcasecmp(value, "minimal")) check_stat = 0; + else + return error(_("invalid value for '%s': '%s'"), + var, value); } if (!strcmp(var, "core.quotepath")) { From patchwork Thu Dec 7 07:24:49 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13482783 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 50E9ED5E for ; Wed, 6 Dec 2023 23:24:51 -0800 (PST) Received: (qmail 10009 invoked by uid 109); 7 Dec 2023 07:24:50 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 07 Dec 2023 07:24:50 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1045 invoked by uid 111); 7 Dec 2023 07:24:54 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 07 Dec 2023 02:24:54 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 7 Dec 2023 02:24:49 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 2/9] git_xmerge_config(): prefer error() to die() Message-ID: <20231207072449.GB1277973@coredump.intra.peff.net> References: <20231207072338.GA1277727@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net> When parsing merge config, a few code paths die on error. It's preferable for us to call error() here, because the resulting error message from the config parsing code contains much more detail. For example, before: fatal: unknown style 'bogus' given for 'merge.conflictstyle' and after: error: unknown style 'bogus' given for 'merge.conflictstyle' fatal: bad config variable 'merge.conflictstyle' in file '.git/config' at line 7 Since we're touching these lines, I also marked them for translation. There's no reason they shouldn't behave like most other config-parsing errors. Signed-off-by: Jeff King --- Before anyone mentions config_error_nonbool(), yes, the first hunk here gets simplified to that in a later patch. xdiff-interface.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/xdiff-interface.c b/xdiff-interface.c index adcea109fa..05d6475a09 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -1,4 +1,5 @@ #include "git-compat-util.h" +#include "gettext.h" #include "config.h" #include "hex.h" #include "object-store-ll.h" @@ -313,7 +314,7 @@ int git_xmerge_config(const char *var, const char *value, { if (!strcmp(var, "merge.conflictstyle")) { if (!value) - die("'%s' is not a boolean", var); + return error(_("'%s' is not a boolean"), var); if (!strcmp(value, "diff3")) git_xmerge_style = XDL_MERGE_DIFF3; else if (!strcmp(value, "zdiff3")) @@ -325,8 +326,8 @@ int git_xmerge_config(const char *var, const char *value, * git-completion.bash when you add new merge config */ else - die("unknown style '%s' given for '%s'", - value, var); + return error(_("unknown style '%s' given for '%s'"), + value, var); return 0; } return git_default_config(var, value, ctx, cb); From patchwork Thu Dec 7 07:24:58 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13482784 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A8A9C137 for ; Wed, 6 Dec 2023 23:24:59 -0800 (PST) Received: (qmail 10012 invoked by uid 109); 7 Dec 2023 07:24:59 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 07 Dec 2023 07:24:59 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1080 invoked by uid 111); 7 Dec 2023 07:25:03 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 07 Dec 2023 02:25:03 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 7 Dec 2023 02:24:58 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 3/9] imap-send: don't use git_die_config() inside callback Message-ID: <20231207072458.GC1277973@coredump.intra.peff.net> References: <20231207072338.GA1277727@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net> The point of git_die_config() is to let configset users mention the file/line info for invalid config, like: if (!git_config_get_int("foo.bar", &value)) { if (!is_ok(value)) git_die_config("foo.bar"); } Using it from within a config callback is unnecessary, because we can simply return an error, at which point the config machinery will mention the file/line of the offending variable. Worse, using git_die_config() can actually produce the wrong location when the key is found in multiple spots. For instance, with config like: [imap] host host = foo we'll report the line number of the "host = foo" line, but the problem is on the implicit-bool "host" line. We can fix it by just returning an error code. Signed-off-by: Jeff King --- imap-send.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imap-send.c b/imap-send.c index 996651e4f8..5b0fe4f95a 100644 --- a/imap-send.c +++ b/imap-send.c @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val, server.port = git_config_int(var, val, ctx->kvi); else if (!strcmp("imap.host", var)) { if (!val) { - git_die_config("imap.host", "Missing value for 'imap.host'"); + return error("Missing value for 'imap.host'"); } else { if (starts_with(val, "imap:")) val += 5; From patchwork Thu Dec 7 07:25:16 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13482785 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 926B0137 for ; Wed, 6 Dec 2023 23:25:17 -0800 (PST) Received: (qmail 10017 invoked by uid 109); 7 Dec 2023 07:25:17 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 07 Dec 2023 07:25:16 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1084 invoked by uid 111); 7 Dec 2023 07:25:20 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 07 Dec 2023 02:25:20 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 7 Dec 2023 02:25:16 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 4/9] config: use config_error_nonbool() instead of custom messages Message-ID: <20231207072516.GD1277973@coredump.intra.peff.net> References: <20231207072338.GA1277727@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net> A few config callbacks use their own custom messages to report an unexpected implicit bool like: [merge "foo"] driver These should just use config_error_nonbool(), so the user sees consistent messages. Signed-off-by: Jeff King --- imap-send.c | 2 +- merge-ll.c | 2 +- xdiff-interface.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/imap-send.c b/imap-send.c index 5b0fe4f95a..9c4df7bbc8 100644 --- a/imap-send.c +++ b/imap-send.c @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val, server.port = git_config_int(var, val, ctx->kvi); else if (!strcmp("imap.host", var)) { if (!val) { - return error("Missing value for 'imap.host'"); + return config_error_nonbool(var); } else { if (starts_with(val, "imap:")) val += 5; diff --git a/merge-ll.c b/merge-ll.c index 8fcf2d3710..1df58ebaac 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -301,7 +301,7 @@ static int read_merge_config(const char *var, const char *value, if (!strcmp("driver", key)) { if (!value) - return error("%s: lacks value", var); + return config_error_nonbool(var); /* * merge..driver specifies the command line: * diff --git a/xdiff-interface.c b/xdiff-interface.c index 05d6475a09..d788689d01 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -314,7 +314,7 @@ int git_xmerge_config(const char *var, const char *value, { if (!strcmp(var, "merge.conflictstyle")) { if (!value) - return error(_("'%s' is not a boolean"), var); + return config_error_nonbool(var); if (!strcmp(value, "diff3")) git_xmerge_style = XDL_MERGE_DIFF3; else if (!strcmp(value, "zdiff3")) From patchwork Thu Dec 7 07:25:23 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13482786 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E5B9B137 for ; Wed, 6 Dec 2023 23:25:24 -0800 (PST) Received: (qmail 10020 invoked by uid 109); 7 Dec 2023 07:25:24 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 07 Dec 2023 07:25:24 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1088 invoked by uid 111); 7 Dec 2023 07:25:28 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 07 Dec 2023 02:25:28 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 7 Dec 2023 02:25:23 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 5/9] diff: give more detailed messages for bogus diff.* config Message-ID: <20231207072523.GE1277973@coredump.intra.peff.net> References: <20231207072338.GA1277727@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net> The config callbacks for a few diff.* variables simply return -1 when we encounter an error. The message you get mentions the offending location, like: fatal: bad config variable 'diff.algorithm' in file '.git/config' at line 7 but is vague about "bad" (as it must be, since the message comes from the generic config code). Most callbacks add their own messages here, so let's do the same. E.g.: error: unknown value for config 'diff.algorithm': foo fatal: bad config variable 'diff.algorithm' in file '.git/config' at line 7 I've written the string in a way that should be reusable for translators, and matches another similar message in transport.c (there doesn't yet seem to be a popular generic message to reuse here, so hopefully this will get the ball rolling). Note that in the case of diff.algorithm, our parse_algorithm_value() helper does detect a NULL value string. But it's still worth detecting it ourselves here, since we can give a more specific error message (and which is the usual one for unexpected implicit-bool values). Signed-off-by: Jeff King --- diff.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/diff.c b/diff.c index 5b213a4b44..a2def45644 100644 --- a/diff.c +++ b/diff.c @@ -445,9 +445,12 @@ int git_diff_ui_config(const char *var, const char *value, } if (!strcmp(var, "diff.algorithm")) { + if (!value) + return config_error_nonbool(var); diff_algorithm = parse_algorithm_value(value); if (diff_algorithm < 0) - return -1; + return error(_("unknown value for config '%s': %s"), + var, value); return 0; } @@ -486,7 +489,8 @@ int git_diff_basic_config(const char *var, const char *value, return config_error_nonbool(var); val = parse_ws_error_highlight(value); if (val < 0) - return -1; + return error(_("unknown value for config '%s': %s"), + var, value); ws_error_highlight_default = val; return 0; } From patchwork Thu Dec 7 07:26:11 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13482787 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 044EB137 for ; Wed, 6 Dec 2023 23:26:12 -0800 (PST) Received: (qmail 10026 invoked by uid 109); 7 Dec 2023 07:26:12 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 07 Dec 2023 07:26:12 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1092 invoked by uid 111); 7 Dec 2023 07:26:16 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 07 Dec 2023 02:26:16 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 7 Dec 2023 02:26:11 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 6/9] config: use git_config_string() for core.checkRoundTripEncoding Message-ID: <20231207072611.GF1277973@coredump.intra.peff.net> References: <20231207072338.GA1277727@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net> Since this code path was recently converted to check for a NULL value, it now behaves exactly like git_config_string(). We can shorten the code a bit by using that helper. Note that git_config_string() takes a const pointer, but our storage variable is non-const. We're better off making this "const", though, since the default value points to a string literal (and thus it would be an error if anybody tried to write to it). Signed-off-by: Jeff King --- An observant reader may notice that this means duplicate config like: [core] checkRoundTripEncoding = foo checkRoundTripEncoding = bar will leak the string for "foo". That is true before this patch, too, and is true of all callers of git_config_string(). I'm going to punt on that for now, and look into it as a separate series. config.c | 8 ++------ convert.h | 2 +- environment.c | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/config.c b/config.c index d997c55e33..00a11b5d98 100644 --- a/config.c +++ b/config.c @@ -1551,12 +1551,8 @@ static int git_default_core_config(const char *var, const char *value, return 0; } - if (!strcmp(var, "core.checkroundtripencoding")) { - if (!value) - return config_error_nonbool(var); - check_roundtrip_encoding = xstrdup(value); - return 0; - } + if (!strcmp(var, "core.checkroundtripencoding")) + return git_config_string(&check_roundtrip_encoding, var, value); if (!strcmp(var, "core.notesref")) { if (!value) diff --git a/convert.h b/convert.h index d925589444..ab8b4fa68d 100644 --- a/convert.h +++ b/convert.h @@ -92,7 +92,7 @@ void convert_attrs(struct index_state *istate, struct conv_attrs *ca, const char *path); extern enum eol core_eol; -extern char *check_roundtrip_encoding; +extern const char *check_roundtrip_encoding; const char *get_cached_convert_stats_ascii(struct index_state *istate, const char *path); const char *get_wt_convert_stats_ascii(const char *path); diff --git a/environment.c b/environment.c index 9e37bf58c0..90632a39bc 100644 --- a/environment.c +++ b/environment.c @@ -64,7 +64,7 @@ const char *excludes_file; enum auto_crlf auto_crlf = AUTO_CRLF_FALSE; enum eol core_eol = EOL_UNSET; int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN; -char *check_roundtrip_encoding = "SHIFT-JIS"; +const char *check_roundtrip_encoding = "SHIFT-JIS"; enum branch_track git_branch_track = BRANCH_TRACK_REMOTE; enum rebase_setup_type autorebase = AUTOREBASE_NEVER; enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED; From patchwork Thu Dec 7 07:26:22 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13482788 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2739BD44 for ; Wed, 6 Dec 2023 23:26:24 -0800 (PST) Received: (qmail 10032 invoked by uid 109); 7 Dec 2023 07:26:23 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 07 Dec 2023 07:26:23 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1095 invoked by uid 111); 7 Dec 2023 07:26:27 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 07 Dec 2023 02:26:27 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 7 Dec 2023 02:26:22 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 7/9] push: drop confusing configset/callback redundancy Message-ID: <20231207072622.GG1277973@coredump.intra.peff.net> References: <20231207072338.GA1277727@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net> We parse push config by calling git_config() with our git_push_config() callback. But inside that callback, when we see "push.gpgsign", we ignore the value passed into the callback and instead make a new call to git_config_get_value(). This is unnecessary at best, and slightly wrong at worst (if there are multiple instances, get_value() only returns one; both methods end up with last-one-wins, but we'd fail to report errors if earlier incarnations were bogus). The call was added by 68c757f219 (push: add a config option push.gpgSign for default signed pushes, 2015-08-19). That commit doesn't give any reason to deviate from the usual strategy here; it was probably just somebody unfamiliar with our config API and its conventions. It also added identical code to builtin/send-pack.c, which also handles push.gpgsign. And then the same issue spread to its neighbor in b33a15b081 (push: add recurseSubmodules config option, 2015-11-17), presumably via cargo-culting. This patch fixes all three to just directly use the value provided to the callback. While I was adjusting the code to do so, I noticed that push.gpgsign is overly careful about a NULL value. After git_parse_maybe_bool() has returned anything besides 1, we know that the value cannot be NULL (if it were, it would be an implicit "true", and many callers of maybe_bool rely on that). Here that lets us shorten "if (v && !strcasecmp(v, ...))" to just "if (!strcasecmp(v, ...))". Signed-off-by: Jeff King --- builtin/push.c | 31 +++++++++++++------------------ builtin/send-pack.c | 27 ++++++++++++--------------- 2 files changed, 25 insertions(+), 33 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index 2e708383c2..58a9273e90 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -526,26 +526,21 @@ static int git_push_config(const char *k, const char *v, *flags |= TRANSPORT_PUSH_AUTO_UPSTREAM; return 0; } else if (!strcmp(k, "push.gpgsign")) { - const char *value; - if (!git_config_get_value("push.gpgsign", &value)) { - switch (git_parse_maybe_bool(value)) { - case 0: - set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_NEVER); - break; - case 1: - set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_ALWAYS); - break; - default: - if (value && !strcasecmp(value, "if-asked")) - set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_IF_ASKED); - else - return error(_("invalid value for '%s'"), k); - } + switch (git_parse_maybe_bool(v)) { + case 0: + set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_NEVER); + break; + case 1: + set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_ALWAYS); + break; + default: + if (!strcasecmp(v, "if-asked")) + set_push_cert_flags(flags, SEND_PACK_PUSH_CERT_IF_ASKED); + else + return error(_("invalid value for '%s'"), k); } } else if (!strcmp(k, "push.recursesubmodules")) { - const char *value; - if (!git_config_get_value("push.recursesubmodules", &value)) - recurse_submodules = parse_push_recurse_submodules_arg(k, value); + recurse_submodules = parse_push_recurse_submodules_arg(k, v); } else if (!strcmp(k, "submodule.recurse")) { int val = git_config_bool(k, v) ? RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF; diff --git a/builtin/send-pack.c b/builtin/send-pack.c index cd6d9e4112..00e6c90477 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -135,21 +135,18 @@ static int send_pack_config(const char *k, const char *v, const struct config_context *ctx, void *cb) { if (!strcmp(k, "push.gpgsign")) { - const char *value; - if (!git_config_get_value("push.gpgsign", &value)) { - switch (git_parse_maybe_bool(value)) { - case 0: - args.push_cert = SEND_PACK_PUSH_CERT_NEVER; - break; - case 1: - args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS; - break; - default: - if (value && !strcasecmp(value, "if-asked")) - args.push_cert = SEND_PACK_PUSH_CERT_IF_ASKED; - else - return error(_("invalid value for '%s'"), k); - } + switch (git_parse_maybe_bool(v)) { + case 0: + args.push_cert = SEND_PACK_PUSH_CERT_NEVER; + break; + case 1: + args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS; + break; + default: + if (!strcasecmp(v, "if-asked")) + args.push_cert = SEND_PACK_PUSH_CERT_IF_ASKED; + else + return error(_("invalid value for '%s'"), k); } } return git_default_config(k, v, ctx, cb); From patchwork Thu Dec 7 07:26:31 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13482789 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2212C137 for ; Wed, 6 Dec 2023 23:26:32 -0800 (PST) Received: (qmail 10037 invoked by uid 109); 7 Dec 2023 07:26:32 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 07 Dec 2023 07:26:32 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1099 invoked by uid 111); 7 Dec 2023 07:26:36 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 07 Dec 2023 02:26:36 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 7 Dec 2023 02:26:31 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 8/9] gpg-interface: drop pointless config_error_nonbool() checks Message-ID: <20231207072631.GH1277973@coredump.intra.peff.net> References: <20231207072338.GA1277727@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net> Config callbacks which use git_config_string() or git_config_pathname() have no need to check for a NULL value. This is handled automatically by those helpers. Signed-off-by: Jeff King --- gpg-interface.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 48f43c5a21..25c42cb9fd 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -762,23 +762,14 @@ static int git_gpg_config(const char *var, const char *value, return 0; } - if (!strcmp(var, "gpg.ssh.defaultkeycommand")) { - if (!value) - return config_error_nonbool(var); + if (!strcmp(var, "gpg.ssh.defaultkeycommand")) return git_config_string(&ssh_default_key_command, var, value); - } - if (!strcmp(var, "gpg.ssh.allowedsignersfile")) { - if (!value) - return config_error_nonbool(var); + if (!strcmp(var, "gpg.ssh.allowedsignersfile")) return git_config_pathname(&ssh_allowed_signers, var, value); - } - if (!strcmp(var, "gpg.ssh.revocationfile")) { - if (!value) - return config_error_nonbool(var); + if (!strcmp(var, "gpg.ssh.revocationfile")) return git_config_pathname(&ssh_revocation_file, var, value); - } if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program")) fmtname = "openpgp"; From patchwork Thu Dec 7 07:26:42 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13482790 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0CA9C137 for ; Wed, 6 Dec 2023 23:26:43 -0800 (PST) Received: (qmail 10040 invoked by uid 109); 7 Dec 2023 07:26:43 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 07 Dec 2023 07:26:43 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1103 invoked by uid 111); 7 Dec 2023 07:26:47 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 07 Dec 2023 02:26:47 -0500 Authentication-Results: peff.net; auth=none Date: Thu, 7 Dec 2023 02:26:42 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 9/9] sequencer: simplify away extra git_config_string() call Message-ID: <20231207072642.GI1277973@coredump.intra.peff.net> References: <20231207072338.GA1277727@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231207072338.GA1277727@coredump.intra.peff.net> In our config callback, we call git_config_string() to copy the incoming value string into a local string. But we don't modify or store that string; we just look at it and then free it. We can make the code simpler by just looking at the value passed into the callback. Note that we do need to check for NULL, which is the one bit of logic git_config_string() did for us. And I could even see an argument that we are abstracting any error-checking of the value behind the git_config_string() layer. But in practice no other callbacks behave this way; it is standard to check for NULL and then just look at the string directly. Signed-off-by: Jeff King --- sequencer.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/sequencer.c b/sequencer.c index d584cac8ed..74c3b1243e 100644 --- a/sequencer.c +++ b/sequencer.c @@ -238,34 +238,29 @@ static int git_sequencer_config(const char *k, const char *v, const struct config_context *ctx, void *cb) { struct replay_opts *opts = cb; - int status; if (!strcmp(k, "commit.cleanup")) { - const char *s; + if (!v) + return config_error_nonbool(k); - status = git_config_string(&s, k, v); - if (status) - return status; - - if (!strcmp(s, "verbatim")) { + if (!strcmp(v, "verbatim")) { opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE; opts->explicit_cleanup = 1; - } else if (!strcmp(s, "whitespace")) { + } else if (!strcmp(v, "whitespace")) { opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE; opts->explicit_cleanup = 1; - } else if (!strcmp(s, "strip")) { + } else if (!strcmp(v, "strip")) { opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL; opts->explicit_cleanup = 1; - } else if (!strcmp(s, "scissors")) { + } else if (!strcmp(v, "scissors")) { opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SCISSORS; opts->explicit_cleanup = 1; } else { warning(_("invalid commit message cleanup mode '%s'"), - s); + v); } - free((char *)s); - return status; + return 0; } if (!strcmp(k, "commit.gpgsign")) {