{"thread":{"id":"58981","subject":"[PATCH] diff: use strncmp to check for \"diff.\" prefix","startedAt":"2022-12-19T16:33:51Z","lastAt":"2022-12-19T18:36:40Z","messageCount":2,"participants":["Rose via GitGitGadget","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469300","messageId":"pull.1403.git.git.1671467624143.gitgitgadget@gmail.com","threadId":"58981","inReplyTo":null,"subject":"[PATCH] diff: use strncmp to check for \"diff.\" prefix","fromName":"Rose via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-19T16:33:43Z","receivedAt":"2022-12-19T16:33:51Z","isPatch":true,"sender":{"key":"ckelsch@jgrcpa.com","avatar":null},"body":"From: Seija Kijin <doremylover123@gmail.com>\n\nThis would be a lot more efficient than\nchecking for the full string, as\nso many of the accepted values\nbegin with \"diff.\"\n\nThis allows the computer to skip most checks\nif var did not start with diff at all.\n\nIt would also let us add more \"diff.\" strings\nin the future without having to duplicate\nthese 5 character so many times.\n\nSigned-off-by: Seija Kijin <doremylover123@gmail.com>\n---\n    diff: use strncmp to check for \"diff.\" prefix\n    \n    This would be a lot more efficient than checking for the full string, as\n    so many of the accepted values begin with \"diff.\"\n    \n    This allows the computer to skip most checks if var did not start with\n    diff at all.\n    \n    It would also let us add more \"diff.\" strings in the future without\n    having to duplicate these 5 character so many times.\n    \n    Signed-off-by: Seija Kijin doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1403%2FAtariDreams%2Fdiff-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1403/AtariDreams/diff-v1\nPull-Request: https://github.com/git/git/pull/1403\n\n diff.c | 158 +++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 87 insertions(+), 71 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 4dfe824c858..e0fddbe6009 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -345,82 +345,98 @@ static unsigned parse_color_moved_ws(const char *arg)\n \n int git_diff_ui_config(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(var, \"diff.color\") || !strcmp(var, \"color.diff\")) {\n+\tif (!strcmp(var, \"color.diff\")) {\n \t\tdiff_use_color_default = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(var, \"diff.colormoved\")) {\n-\t\tint cm = parse_color_moved(value);\n-\t\tif (cm < 0)\n-\t\t\treturn -1;\n-\t\tdiff_color_moved_default = cm;\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"diff.colormovedws\")) {\n-\t\tunsigned cm = parse_color_moved_ws(value);\n-\t\tif (cm & COLOR_MOVED_WS_ERROR)\n-\t\t\treturn -1;\n-\t\tdiff_color_moved_ws_default = cm;\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"diff.context\")) {\n-\t\tdiff_context_default = git_config_int(var, value);\n-\t\tif (diff_context_default < 0)\n-\t\t\treturn -1;\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"diff.interhunkcontext\")) {\n-\t\tdiff_interhunk_context_default = git_config_int(var, value);\n-\t\tif (diff_interhunk_context_default < 0)\n-\t\t\treturn -1;\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"diff.renames\")) {\n-\t\tdiff_detect_rename_default = git_config_rename(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"diff.autorefreshindex\")) {\n-\t\tdiff_auto_refresh_index = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"diff.mnemonicprefix\")) {\n-\t\tdiff_mnemonic_prefix = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"diff.noprefix\")) {\n-\t\tdiff_no_prefix = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"diff.relative\")) {\n-\t\tdiff_relative = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"diff.statgraphwidth\")) {\n-\t\tdiff_stat_graph_width = git_config_int(var, value);\n-\t\treturn 0;\n-\t}\n-\tif (!strcmp(var, \"diff.external\"))\n-\t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n-\tif (!strcmp(var, \"diff.wordregex\"))\n-\t\treturn git_config_string(&diff_word_regex_cfg, var, value);\n-\tif (!strcmp(var, \"diff.orderfile\"))\n-\t\treturn git_config_pathname(&diff_order_file_cfg, var, value);\n \n-\tif (!strcmp(var, \"diff.ignoresubmodules\"))\n-\t\thandle_ignore_submodules_arg(&default_diff_options, value);\n-\n-\tif (!strcmp(var, \"diff.submodule\")) {\n-\t\tif (parse_submodule_params(&default_diff_options, value))\n-\t\t\twarning(_(\"Unknown value for 'diff.submodule' config variable: '%s'\"),\n-\t\t\t\tvalue);\n-\t\treturn 0;\n-\t}\n+\tif (!strncmp(var, \"diff.\", 5)) {\n+\t\tconst char *tmp = var + 5;\n+\t\tif (!strcmp(var, \"color\")) {\n+\t\t\tdiff_use_color_default =\n+\t\t\t\tgit_config_colorbool(var, value);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"colormoved\")) {\n+\t\t\tint cm = parse_color_moved(value);\n+\t\t\tif (cm < 0)\n+\t\t\t\treturn -1;\n+\t\t\tdiff_color_moved_default = cm;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"colormovedws\")) {\n+\t\t\tunsigned cm = parse_color_moved_ws(value);\n+\t\t\tif (cm & COLOR_MOVED_WS_ERROR)\n+\t\t\t\treturn -1;\n+\t\t\tdiff_color_moved_ws_default = cm;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"context\")) {\n+\t\t\tdiff_context_default = git_config_int(var, value);\n+\t\t\tif (diff_context_default < 0)\n+\t\t\t\treturn -1;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"interhunkcontext\")) {\n+\t\t\tdiff_interhunk_context_default =\n+\t\t\t\tgit_config_int(var, value);\n+\t\t\tif (diff_interhunk_context_default < 0)\n+\t\t\t\treturn -1;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"renames\")) {\n+\t\t\tdiff_detect_rename_default =\n+\t\t\t\tgit_config_rename(var, value);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"autorefreshindex\")) {\n+\t\t\tdiff_auto_refresh_index = git_config_bool(var, value);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"mnemonicprefix\")) {\n+\t\t\tdiff_mnemonic_prefix = git_config_bool(var, value);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"noprefix\")) {\n+\t\t\tdiff_no_prefix = git_config_bool(var, value);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"relative\")) {\n+\t\t\tdiff_relative = git_config_bool(var, value);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"statgraphwidth\")) {\n+\t\t\tdiff_stat_graph_width = git_config_int(var, value);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tif (!strcmp(tmp, \"external\"))\n+\t\t\treturn git_config_string(&external_diff_cmd_cfg, var,\n+\t\t\t\t\t\t value);\n+\t\tif (!strcmp(tmp, \"wordregex\"))\n+\t\t\treturn git_config_string(&diff_word_regex_cfg, var,\n+\t\t\t\t\t\t value);\n+\t\tif (!strcmp(tmp, \"orderfile\"))\n+\t\t\treturn git_config_pathname(&diff_order_file_cfg, var,\n+\t\t\t\t\t\t   value);\n+\n+\t\tif (!strcmp(tmp, \"ignoresubmodules\"))\n+\t\t\thandle_ignore_submodules_arg(&default_diff_options,\n+\t\t\t\t\t\t     value);\n+\n+\t\tif (!strcmp(tmp, \"submodule\")) {\n+\t\t\tif (parse_submodule_params(&default_diff_options,\n+\t\t\t\t\t\t   value))\n+\t\t\t\twarning(_(\"Unknown value for 'diff.submodule' config variable: '%s'\"),\n+\t\t\t\t\tvalue);\n+\t\t\treturn 0;\n+\t\t}\n \n-\tif (!strcmp(var, \"diff.algorithm\")) {\n-\t\tdiff_algorithm = parse_algorithm_value(value);\n-\t\tif (diff_algorithm < 0)\n-\t\t\treturn -1;\n-\t\treturn 0;\n+\t\tif (!strcmp(tmp, \"algorithm\")) {\n+\t\t\tdiff_algorithm = parse_algorithm_value(value);\n+\t\t\tif (diff_algorithm < 0)\n+\t\t\t\treturn -1;\n+\t\t\treturn 0;\n+\t\t}\n \t}\n \n \tif (git_color_config(var, value, cb) < 0)\n\nbase-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7\n-- \ngitgitgadget\n"},{"id":"469305","messageId":"221219.86r0wvxmyy.gmgdl@evledraar.gmail.com","threadId":"58981","inReplyTo":"pull.1403.git.git.1671467624143.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff: use strncmp to check for \"diff.\" prefix","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-19T18:26:11Z","receivedAt":"2022-12-19T18:36:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 19 2022, Rose via GitGitGadget wrote:\n\n> From: Seija Kijin <doremylover123@gmail.com>\n>\n> This would be a lot more efficient than\n> checking for the full string, as\n> so many of the accepted values\n> begin with \"diff.\"\n>\n> This allows the computer to skip most checks\n> if var did not start with diff at all.\n>\n> It would also let us add more \"diff.\" strings\n> in the future without having to duplicate\n> these 5 character so many times.\n\nThis is missing a \"why\", is this one-off really needed for optimization\npurposes? If not it doesn't seem worth the churn.\n\nThat we duplicate the \"diff.\" part of \"diff.renames\" (as opposed to\n\"renames\" is also a feature in my opinion, it makes things easier to\ngrep for.\n\nIt would really just be preferrable to have the compiler do this work\nfor us. A while ago I experimented with that, i.e. if/else if/else\nchains of strcmp(): https://godbolt.org/z/rrnYWW7nj\n\nAs that example shows at least GCC doesn't do anything interesting with\nit as optimizations are concerned, i.e. you'd really want something like\nthis instead (but for the compiler to do it for you):\nhttps://godbolt.org/z/7hsPo59TM\n\nBut, as you can be seen at https://godbolt.org/z/zWK46rGK5 it *will* do\nthat if we just use memcmp().\n\nSo, doing that really seems preferrable, perhaps with some macro that\nwould provide the \"n\" to memcmp, so we wouldn't need to hardcode the\nlength.\n\nBut all of that would be predicated on whether the optimization is\nactually worth it.\n\nIf we are micro-optimizing config reading we should be moving the\nrelevant code to the configset API, where we have the parsed keys as a\nhash, rather than trying to micro-optimize the callback API, where we'll\ncall the callback N times, just to brute-force ask it if it's interested\nin some small subset of the config space.\n"}]}