threads / patch / 58981

patchdiff: use strncmp to check for "diff." prefix

Subject: [PATCH] diff: use strncmp to check for "diff." prefix

## tl;dr

2 messages between Dec 19, 2022 and Dec 19, 2022. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Rose via GitGitGadget· Dec 19, 2022, 16:33 UTC · lore
From: Seija Kijin <doremylover123@gmail.com>

This would be a lot more efficient than checking for the full string, as so many of the accepted values begin with "diff."

This allows the computer to skip most checks if var did not start with diff at all.

It would also let us add more "diff." strings in the future without having to duplicate these 5 character so many times.

Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
    diff: use strncmp to check for "diff." prefix
    
    This would be a lot more efficient than checking for the full string, as
    so many of the accepted values begin with "diff."
    
    This allows the computer to skip most checks if var did not start with
    diff at all.
    
    It would also let us add more "diff." strings in the future without
    having to duplicate these 5 character so many times.
    
    Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1403%2FAtariDreams%2Fdiff-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1403/AtariDreams/diff-v1
Pull-Request: https://github.com/git/git/pull/1403
 diff.c | 158 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 87 insertions(+), 71 deletions(-)
Show changes to diff.c +87 −71
diff --git a/diff.c b/diff.c
index 4dfe824c858..e0fddbe6009 100644
--- a/diff.c
+++ b/diff.c
@@ -345,82 +345,98 @@ static unsigned parse_color_moved_ws(const char *arg)
 
 int git_diff_ui_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
+	if (!strcmp(var, "color.diff")) {
 		diff_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
-	if (!strcmp(var, "diff.colormoved")) {
-		int cm = parse_color_moved(value);
-		if (cm < 0)
-			return -1;
-		diff_color_moved_default = cm;
-		return 0;
-	}
-	if (!strcmp(var, "diff.colormovedws")) {
-		unsigned cm = parse_color_moved_ws(value);
-		if (cm & COLOR_MOVED_WS_ERROR)
-			return -1;
-		diff_color_moved_ws_default = cm;
-		return 0;
-	}
-	if (!strcmp(var, "diff.context")) {
-		diff_context_default = git_config_int(var, value);
-		if (diff_context_default < 0)
-			return -1;
-		return 0;
-	}
-	if (!strcmp(var, "diff.interhunkcontext")) {
-		diff_interhunk_context_default = git_config_int(var, value);
-		if (diff_interhunk_context_default < 0)
-			return -1;
-		return 0;
-	}
-	if (!strcmp(var, "diff.renames")) {
-		diff_detect_rename_default = git_config_rename(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "diff.autorefreshindex")) {
-		diff_auto_refresh_index = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "diff.mnemonicprefix")) {
-		diff_mnemonic_prefix = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "diff.noprefix")) {
-		diff_no_prefix = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "diff.relative")) {
-		diff_relative = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "diff.statgraphwidth")) {
-		diff_stat_graph_width = git_config_int(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "diff.external"))
-		return git_config_string(&external_diff_cmd_cfg, var, value);
-	if (!strcmp(var, "diff.wordregex"))
-		return git_config_string(&diff_word_regex_cfg, var, value);
-	if (!strcmp(var, "diff.orderfile"))
-		return git_config_pathname(&diff_order_file_cfg, var, value);
 
-	if (!strcmp(var, "diff.ignoresubmodules"))
-		handle_ignore_submodules_arg(&default_diff_options, value);
-
-	if (!strcmp(var, "diff.submodule")) {
-		if (parse_submodule_params(&default_diff_options, value))
-			warning(_("Unknown value for 'diff.submodule' config variable: '%s'"),
-				value);
-		return 0;
-	}
+	if (!strncmp(var, "diff.", 5)) {
+		const char *tmp = var + 5;
+		if (!strcmp(var, "color")) {
+			diff_use_color_default =
+				git_config_colorbool(var, value);
+			return 0;
+		}
+		if (!strcmp(tmp, "colormoved")) {
+			int cm = parse_color_moved(value);
+			if (cm < 0)
+				return -1;
+			diff_color_moved_default = cm;
+			return 0;
+		}
+		if (!strcmp(tmp, "colormovedws")) {
+			unsigned cm = parse_color_moved_ws(value);
+			if (cm & COLOR_MOVED_WS_ERROR)
+				return -1;
+			diff_color_moved_ws_default = cm;
+			return 0;
+		}
+		if (!strcmp(tmp, "context")) {
+			diff_context_default = git_config_int(var, value);
+			if (diff_context_default < 0)
+				return -1;
+			return 0;
+		}
+		if (!strcmp(tmp, "interhunkcontext")) {
+			diff_interhunk_context_default =
+				git_config_int(var, value);
+			if (diff_interhunk_context_default < 0)
+				return -1;
+			return 0;
+		}
+		if (!strcmp(tmp, "renames")) {
+			diff_detect_rename_default =
+				git_config_rename(var, value);
+			return 0;
+		}
+		if (!strcmp(tmp, "autorefreshindex")) {
+			diff_auto_refresh_index = git_config_bool(var, value);
+			return 0;
+		}
+		if (!strcmp(tmp, "mnemonicprefix")) {
+			diff_mnemonic_prefix = git_config_bool(var, value);
+			return 0;
+		}
+		if (!strcmp(tmp, "noprefix")) {
+			diff_no_prefix = git_config_bool(var, value);
+			return 0;
+		}
+		if (!strcmp(tmp, "relative")) {
+			diff_relative = git_config_bool(var, value);
+			return 0;
+		}
+		if (!strcmp(tmp, "statgraphwidth")) {
+			diff_stat_graph_width = git_config_int(var, value);
+			return 0;
+		}
+		if (!strcmp(tmp, "external"))
+			return git_config_string(&external_diff_cmd_cfg, var,
+						 value);
+		if (!strcmp(tmp, "wordregex"))
+			return git_config_string(&diff_word_regex_cfg, var,
+						 value);
+		if (!strcmp(tmp, "orderfile"))
+			return git_config_pathname(&diff_order_file_cfg, var,
+						   value);
+
+		if (!strcmp(tmp, "ignoresubmodules"))
+			handle_ignore_submodules_arg(&default_diff_options,
+						     value);
+
+		if (!strcmp(tmp, "submodule")) {
+			if (parse_submodule_params(&default_diff_options,
+						   value))
+				warning(_("Unknown value for 'diff.submodule' config variable: '%s'"),
+					value);
+			return 0;
+		}
 
-	if (!strcmp(var, "diff.algorithm")) {
-		diff_algorithm = parse_algorithm_value(value);
-		if (diff_algorithm < 0)
-			return -1;
-		return 0;
+		if (!strcmp(tmp, "algorithm")) {
+			diff_algorithm = parse_algorithm_value(value);
+			if (diff_algorithm < 0)
+				return -1;
+			return 0;
+		}
 	}
 
 	if (git_color_config(var, value, cb) < 0)

base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7

-- 
gitgitgadget
Ævar Arnfjörð Bjarmason· Dec 19, 2022, 18:26 UTC · re: Rose via GitGitGadget · lore

Re: [PATCH] diff: use strncmp to check for "diff." prefix

On Mon, Dec 19 2022, Rose via GitGitGadget wrote:
Show 13 quoted lines
> From: Seija Kijin <doremylover123@gmail.com>
>
> This would be a lot more efficient than
> checking for the full string, as
> so many of the accepted values
> begin with "diff."
>
> This allows the computer to skip most checks
> if var did not start with diff at all.
>
> It would also let us add more "diff." strings
> in the future without having to duplicate
> these 5 character so many times.

This is missing a "why", is this one-off really needed for optimization purposes? If not it doesn't seem worth the churn.

That we duplicate the "diff." part of "diff.renames" (as opposed to "renames" is also a feature in my opinion, it makes things easier to grep for.

It would really just be preferrable to have the compiler do this work for us. A while ago I experimented with that, i.e. if/else if/else chains of strcmp(): https://godbolt.org/z/rrnYWW7nj

As that example shows at least GCC doesn't do anything interesting with it as optimizations are concerned, i.e. you'd really want something like this instead (but for the compiler to do it for you): https://godbolt.org/z/7hsPo59TM

But, as you can be seen at https://godbolt.org/z/zWK46rGK5 it *will* do that if we just use memcmp().

So, doing that really seems preferrable, perhaps with some macro that would provide the "n" to memcmp, so we wouldn't need to hardcode the length.

But all of that would be predicated on whether the optimization is actually worth it.

If we are micro-optimizing config reading we should be moving the relevant code to the configset API, where we have the parsed keys as a hash, rather than trying to micro-optimize the callback API, where we'll call the callback N times, just to brute-force ask it if it's interested in some small subset of the config space.

← back to recent threads