{"thread":{"id":"59222","subject":"[PATCH] dir: remove unneeded local variables from match_pathname()","startedAt":"2023-02-10T04:52:31Z","lastAt":"2023-02-10T21:52:07Z","messageCount":2,"participants":["Masahiro Yamada","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471890","messageId":"20230210045119.25190-1-masahiroy@kernel.org","threadId":"59222","inReplyTo":null,"subject":"[PATCH] dir: remove unneeded local variables from match_pathname()","fromName":"Masahiro Yamada","fromEmail":"masahiroy@kernel.org","sentAt":"2023-02-10T04:51:19Z","receivedAt":"2023-02-10T04:52:31Z","isPatch":true,"sender":{"key":"masahiroy@kernel.org","avatar":"https://gravatar.com/avatar/de07e07e05be0542e420cb9b79a723c58893c54989cddfee4a69e05e5dac4a9f?d=mp&s=160"},"body":"The local variables are unneeded - you can simply advance the 'pathname'\npointer.\n\nIMHO, the variable 'name' is somewhat confusing. It is a relative path\nto 'base', not a file name. It may contain slashes.\n\nSigned-off-by: Masahiro Yamada <masahiroy@kernel.org>\n---\n\n dir.c | 21 ++++++++++-----------\n 1 file changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 4e99f0c868..06c6e7d79e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1251,9 +1251,6 @@ int match_pathname(const char *pathname, int pathlen,\n \t\t   const char *base, int baselen,\n \t\t   const char *pattern, int prefix, int patternlen)\n {\n-\tconst char *name;\n-\tint namelen;\n-\n \t/*\n \t * match with FNM_PATHNAME; the pattern has base implicitly\n \t * in front of it.\n@@ -1273,35 +1270,37 @@ int match_pathname(const char *pathname, int pathlen,\n \t    fspathncmp(pathname, base, baselen))\n \t\treturn 0;\n \n-\tnamelen = baselen ? pathlen - baselen - 1 : pathlen;\n-\tname = pathname + pathlen - namelen;\n+\tif (baselen > 0) {\n+\t\tpathname += baselen + 1;\n+\t\tpathlen -= baselen + 1;\n+\t}\n \n \tif (prefix) {\n \t\t/*\n \t\t * if the non-wildcard part is longer than the\n \t\t * remaining pathname, surely it cannot match.\n \t\t */\n-\t\tif (prefix > namelen)\n+\t\tif (prefix > pathlen)\n \t\t\treturn 0;\n \n-\t\tif (fspathncmp(pattern, name, prefix))\n+\t\tif (fspathncmp(pattern, pathname, prefix))\n \t\t\treturn 0;\n \t\tpattern += prefix;\n \t\tpatternlen -= prefix;\n-\t\tname    += prefix;\n-\t\tnamelen -= prefix;\n+\t\tpathname += prefix;\n+\t\tpathlen -= prefix;\n \n \t\t/*\n \t\t * If the whole pattern did not have a wildcard,\n \t\t * then our prefix match is all we need; we\n \t\t * do not need to call fnmatch at all.\n \t\t */\n-\t\tif (!patternlen && !namelen)\n+\t\tif (!patternlen && !pathlen)\n \t\t\treturn 1;\n \t}\n \n \treturn fnmatch_icase_mem(pattern, patternlen,\n-\t\t\t\t name, namelen,\n+\t\t\t\t pathname, pathlen,\n \t\t\t\t WM_PATHNAME) == 0;\n }\n \n-- \n2.34.1\n\n"},{"id":"471948","messageId":"xmqqfsbdgpe1.fsf@gitster.g","threadId":"59222","inReplyTo":"20230210045119.25190-1-masahiroy@kernel.org","subject":"Re: [PATCH] dir: remove unneeded local variables from match_pathname()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-10T21:51:50Z","receivedAt":"2023-02-10T21:52:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Masahiro Yamada <masahiroy@kernel.org> writes:\n\n> The local variables are unneeded - you can simply advance the 'pathname'\n> pointer.\n\nIt probably is somewhat subjective if it makes the resulting code\neasier or harder to read with these extra variables, even though\n\"are unneeded\" may technically be correct and the compilers may\nproduce identical binaries with or without the patch.\n\nIn the context of the original, this used to be in a loop where\nelements of an array was matched against a constant pathname\nvariable, and it was necessary to use <name, namelen>, separate\nvariables, to point to the \"remainder\" of \"pathname\".  It would not\nhave made any sense not to use separate variables in that loop.\n\nWhen the body of the loop was split into this helper function in\nb5592632 (exclude: split pathname matching code into a separate\nfunction, 2012-10-15), we could have removed these variables and\ninstead clobbered <pathname, pathlen>, but apparently we did not.  I\nsuspect that the original author found it easier to reason about the\nbehaviour of the function to keep the incoming parameter anchored at\nthe constant location, and use separate variables to point at the\ntail part of the string that are to be worked on, which I tend to\ndisagree, but I do not have a strong preference.\n\nHaving said all that, I consider this to fall into \"once the code is\nwritten one way, it is not worth the patch noise to go and change it\nto a different way.\" category.\n\nThanks.\n"}]}