diff: use strncmp to check for "diff." prefix
PATCH 1/1 diff: use strncmp to check for "diff." prefix
From: Seija Kijin <[email protected]>
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.
---
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.
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(-)
diff.c +87 -71
@@ -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
Signature · 1 line
gitgitgadget
Quoted 7 lines · Rose via GitGitGadget
From: Seija Kijin <[email protected]>
This would be a lot more efficient than checking for the full string, as
so many of the accepted values
begin with "diff."
Quoted 6 lines
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.