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

Commit Message

Seija Kijin Dec. 19, 2022, 4:33 p.m. UTC

From: Seija Kijin <[e-mail]>

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 <[e-mail]>
---
    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 [e-mail]

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(-)


base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7

Comments

Ævar Arnfjörð Bjarmason Dec. 19, 2022, 6:26 p.m. UTC | #1

On Mon, Dec 19 2022, Rose via GitGitGadget wrote:

> From: Seija Kijin <[e-mail]>
>
> 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.

Patch

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)