git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Dec 7, 2023, 07:26 UTC
Message-ID
<20231207072642.GI1277973@coredump.intra.peff.net>
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 <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
Previous: Jeff KingNext: Patrick Steinhardt
Message 15 of 16 in “bonus config cleanups”
  1. 0/9 bonus config cleanupsJeff King, Dec 7, 2023
  2. 1/9 config: reject bogus values for core.checkstatJeff King, Dec 7, 2023
  3. Taylor BlauDec 8, 2023
  4. 2/9 git_xmerge_config(): prefer error() to die()Jeff King, Dec 7, 2023
  5. 3/9 imap-send: don't use git_die_config() inside callbackJeff King, Dec 7, 2023
  6. Patrick SteinhardtDec 7, 2023
  7. Taylor BlauDec 8, 2023
  8. Patrick SteinhardtDec 11, 2023
  9. Jeff KingDec 12, 2023
  10. 4/9 config: use config_error_nonbool() instead of custom messagesJeff King, Dec 7, 2023
  11. 5/9 diff: give more detailed messages for bogus diff.* configJeff King, Dec 7, 2023
  12. 6/9 config: use git_config_string() for core.checkRoundTripEncodingJeff King, Dec 7, 2023
  13. 7/9 push: drop confusing configset/callback redundancyJeff King, Dec 7, 2023
  14. 8/9 gpg-interface: drop pointless config_error_nonbool() checksJeff King, Dec 7, 2023
  15. 9/9 sequencer: simplify away extra git_config_string() callJeff King, Dec 7, 2023
  16. Patrick SteinhardtDec 7, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.