[PATCH 0/9] bonus config cleanups

[PATCH 0/9] bonus config cleanups

[Jeff King]

While looking carefully at various config callbacks for the series at:

  https://lore.kernel.org/git/20231207071030.GA1275835@coredump.intra.peff.net/

I noticed a bunch of other small bugs/cleanups. I split these into their
own series here, which should be applied on top (it could go straight to
"master", but there is a small conflict in patch 6, as the option it
touches was fixed in the other series). I'm happy to prepare it as an
independent series if we prefer.

  [1/9]: config: reject bogus values for core.checkstat
  [2/9]: git_xmerge_config(): prefer error() to die()
  [3/9]: imap-send: don't use git_die_config() inside callback
  [4/9]: config: use config_error_nonbool() instead of custom messages
  [5/9]: diff: give more detailed messages for bogus diff.* config
  [6/9]: config: use git_config_string() for core.checkRoundTripEncoding
  [7/9]: push: drop confusing configset/callback redundancy
  [8/9]: gpg-interface: drop pointless config_error_nonbool() checks
  [9/9]: sequencer: simplify away extra git_config_string() call

 builtin/push.c      | 31 +++++++++++++------------------
 builtin/send-pack.c | 27 ++++++++++++---------------
 config.c            | 11 +++++------
 convert.h           |  2 +-
 diff.c              |  8 ++++++--
 environment.c       |  2 +-
 gpg-interface.c     | 15 +++------------
 imap-send.c         |  2 +-
 merge-ll.c          |  2 +-
 sequencer.c         | 21 ++++++++-------------
 xdiff-interface.c   |  7 ++++---
 11 files changed, 55 insertions(+), 73 deletions(-)

-Peff

[PATCH 1/9] config: reject bogus values for core.checkstat

[Jeff King]

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 <peff@peff.net>
---
 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")) {
-- 
2.43.0.664.ga12c899002

Re: [PATCH 1/9] config: reject bogus values for core.checkstat

[Taylor Blau]

On Thu, Dec 07, 2023 at 02:24:04AM -0500, Jeff King wrote:
> 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.

I think this is a good instance of "yes, this is a backwards
incompatible change, but the behavior we're breaking is so obviously
broken already that it's not worth maintaining compatibility."

Well reasoned, I am definitely in favor here.

Thanks,
Taylor

[PATCH 2/9] git_xmerge_config(): prefer error() to die()

[Jeff King]

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 <peff@peff.net>
---
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);
-- 
2.43.0.664.ga12c899002

[PATCH 3/9] imap-send: don't use git_die_config() inside callback

[Jeff King]

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 <peff@peff.net>
---
 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;
-- 
2.43.0.664.ga12c899002

Re: [PATCH 3/9] imap-send: don't use git_die_config() inside callback

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote:
[snip]
> 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'");

Nit: while at it we might also mark this error for translation. Not
worth a reroll on its own though.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/9] imap-send: don't use git_die_config() inside callback

[Taylor Blau]

On Thu, Dec 07, 2023 at 09:57:55AM +0100, Patrick Steinhardt wrote:
> On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote:
> [snip]
> > 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'");
>
> Nit: while at it we might also mark this error for translation. Not
> worth a reroll on its own though.

This string goes away entirely in the next patch, so I don't think we
need to mark it here.

Thanks,
Taylor

Re: [PATCH 3/9] imap-send: don't use git_die_config() inside callback

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On Fri, Dec 08, 2023 at 05:58:52PM -0500, Taylor Blau wrote:
> On Thu, Dec 07, 2023 at 09:57:55AM +0100, Patrick Steinhardt wrote:
> > On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote:
> > [snip]
> > > 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'");
> >
> > Nit: while at it we might also mark this error for translation. Not
> > worth a reroll on its own though.
> 
> This string goes away entirely in the next patch, so I don't think we
> need to mark it here.
> 
> Thanks,
> Taylor

Ah, true. Never mind in that case.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/9] imap-send: don't use git_die_config() inside callback

[Jeff King]

On Fri, Dec 08, 2023 at 05:58:52PM -0500, Taylor Blau wrote:

> On Thu, Dec 07, 2023 at 09:57:55AM +0100, Patrick Steinhardt wrote:
> > On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote:
> > [snip]
> > > 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'");
> >
> > Nit: while at it we might also mark this error for translation. Not
> > worth a reroll on its own though.
> 
> This string goes away entirely in the next patch, so I don't think we
> need to mark it here.

Right. It's a little confusing because it is converted along with some
other spots in the next patch. But in one of those other spots, we
earlier switched it (in patch 2) from die() to error(), and we _did_
mark it for translation as we did so.

I did it there because in patch 2 we touch multiple messages, and the
other ones don't end up as config_error_nonbool(), so we do want them
translated.

I'm not sure if there would have been an easier ordering to the series.
I could have pulled the "mark for translation" bits from patch 2 into
their own patch (after this one makes some of the messages go away), but
then I'd expect somebody would review patch 2 and say "why not mark them
for translation while we're here?". :)

-Peff

[PATCH 4/9] config: use config_error_nonbool() instead of custom messages

[Jeff King]

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 <peff@peff.net>
---
 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.<name>.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"))
-- 
2.43.0.664.ga12c899002

[PATCH 5/9] diff: give more detailed messages for bogus diff.* config

[Jeff King]

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 <peff@peff.net>
---
 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;
 	}
-- 
2.43.0.664.ga12c899002

[PATCH 6/9] config: use git_config_string() for core.checkRoundTripEncoding

[Jeff King]

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 <peff@peff.net>
---
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;
-- 
2.43.0.664.ga12c899002

[PATCH 7/9] push: drop confusing configset/callback redundancy

[Jeff King]

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 <peff@peff.net>
---
 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);
-- 
2.43.0.664.ga12c899002

[PATCH 8/9] gpg-interface: drop pointless config_error_nonbool() checks

[Jeff King]

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 <peff@peff.net>
---
 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";
-- 
2.43.0.664.ga12c899002

[PATCH 9/9] sequencer: simplify away extra git_config_string() call

[Jeff King]

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 <peff@peff.net>
---
 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")) {
-- 
2.43.0.664.ga12c899002

Re: [PATCH 0/9] bonus config cleanups

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On Thu, Dec 07, 2023 at 02:23:38AM -0500, Jeff King wrote:
> While looking carefully at various config callbacks for the series at:
> 
>   https://lore.kernel.org/git/20231207071030.GA1275835@coredump.intra.peff.net/
> 
> I noticed a bunch of other small bugs/cleanups. I split these into their
> own series here, which should be applied on top (it could go straight to
> "master", but there is a small conflict in patch 6, as the option it
> touches was fixed in the other series). I'm happy to prepare it as an
> independent series if we prefer.

The whole patch series looks good to me, thanks!

Patrick

>   [1/9]: config: reject bogus values for core.checkstat
>   [2/9]: git_xmerge_config(): prefer error() to die()
>   [3/9]: imap-send: don't use git_die_config() inside callback
>   [4/9]: config: use config_error_nonbool() instead of custom messages
>   [5/9]: diff: give more detailed messages for bogus diff.* config
>   [6/9]: config: use git_config_string() for core.checkRoundTripEncoding
>   [7/9]: push: drop confusing configset/callback redundancy
>   [8/9]: gpg-interface: drop pointless config_error_nonbool() checks
>   [9/9]: sequencer: simplify away extra git_config_string() call
> 
>  builtin/push.c      | 31 +++++++++++++------------------
>  builtin/send-pack.c | 27 ++++++++++++---------------
>  config.c            | 11 +++++------
>  convert.h           |  2 +-
>  diff.c              |  8 ++++++--
>  environment.c       |  2 +-
>  gpg-interface.c     | 15 +++------------
>  imap-send.c         |  2 +-
>  merge-ll.c          |  2 +-
>  sequencer.c         | 21 ++++++++-------------
>  xdiff-interface.c   |  7 ++++---
>  11 files changed, 55 insertions(+), 73 deletions(-)
> 
> -Peff
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]