{"thread":{"id":"55636","subject":"[PATCH] clean: remove unnecessary variable","startedAt":"2021-05-06T19:33:26Z","lastAt":"2021-05-06T21:35:59Z","messageCount":4,"participants":["Matheus Tavares","Jeff King","Matheus Tavares Bernardino","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"423812","messageId":"4f88ac58e46782c7cf335e13fabd14c8b4438267.1620329445.git.matheus.bernardino@usp.br","threadId":"55636","inReplyTo":null,"subject":"[PATCH] clean: remove unnecessary variable","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-06T19:33:15Z","receivedAt":"2021-05-06T19:33:26Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"The variable `matches` used to hold the return of a `dir_path_match()`\ncall that was removed in 95c11ecc73 (\"Fix error-prone fill_directory()\nAPI; make it only return matches\", 2020-04-01). Now `matches` will\nalways hold 0, which is the value it's initialized with; and the\ncondition `matches != MATCHED_EXACTLY` will always evaluate to true. So\nlet's remove this unnecessary variable.\n\nInterestingly, it seems that `matches != MATCHED_EXACTLY` was already\nunnecessary before 95c11ecc73. That's because `remove_directories` is\nalways set to 1 when we have pathspecs; So, in the condition\n`!remove_directories && matches != MATCHED_EXACTLY`, we would either:\n\n- have pathspecs (or have been given `-d`) and ignore `matches` because\n `remove_directories` is 1; or\n\n- not have pathspecs (nor `-d`) and end up just checking that\n `0 != MATCHED_EXACTLY`, as `matches` would never get reassigned\n after its zero initialization (because there is no pathspec to match).\n\nSigned-off-by: Matheus Tavares \n---\n builtin/clean.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 995053b791..f6d7e8119c 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -1003,7 +1003,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \tfor (i = 0; i < dir.nr; i++) {\n \t\tstruct dir_entry *ent = dir.entries[i];\n-\t\tint matches = 0;\n \t\tstruct stat st;\n \t\tconst char *rel;\n \n@@ -1013,8 +1012,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tif (lstat(ent->name, &st))\n \t\t\tdie_errno(\"Cannot lstat '%s'\", ent->name);\n \n-\t\tif (S_ISDIR(st.st_mode) && !remove_directories &&\n-\t\t matches != MATCHED_EXACTLY)\n+\t\tif (S_ISDIR(st.st_mode) && !remove_directories)\n \t\t\tcontinue;\n \n \t\trel = relative_path(ent->name, prefix, &buf);\n-- \n2.30.1\n\n"},{"id":"423813","messageId":"YJRLim10cznG5G3d@coredump.intra.peff.net","threadId":"55636","inReplyTo":"4f88ac58e46782c7cf335e13fabd14c8b4438267.1620329445.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] clean: remove unnecessary variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-06T20:03:22Z","receivedAt":"2021-05-06T20:03:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 06, 2021 at 04:33:15PM -0300, Matheus Tavares wrote:\n\n> The variable `matches` used to hold the return of a `dir_path_match()`\n> call that was removed in 95c11ecc73 (\"Fix error-prone fill_directory()\n> API; make it only return matches\", 2020-04-01). Now `matches` will\n> always hold 0, which is the value it's initialized with; and the\n> condition `matches != MATCHED_EXACTLY` will always evaluate to true. So\n> let's remove this unnecessary variable.\n>\n> Interestingly, it seems that `matches != MATCHED_EXACTLY` was already\n> unnecessary before 95c11ecc73. That's because `remove_directories` is\n> always set to 1 when we have pathspecs; So, in the condition\n> `!remove_directories && matches != MATCHED_EXACTLY`, we would either:\n> \n> - have pathspecs (or have been given `-d`) and ignore `matches` because\n> `remove_directories` is 1; or\n> \n> - not have pathspecs (nor `-d`) and end up just checking that\n> `0 != MATCHED_EXACTLY`, as `matches` would never get reassigned\n> after its zero initialization (because there is no pathspec to match).\n\nThanks for this digging and the extra analysis. We can see from the\npatch that this variable can't possibly be doing anything, but it is\nalways a comfort to see authors researching the source of the oddity and\nexplaining what they found.\n\nI'm adding Elijah to the cc as an area expert, just in case he has any\nother insight.\n\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 995053b791..f6d7e8119c 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -1003,7 +1003,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> \n> \tfor (i = 0; i < dir.nr; i++) {\n> \t\tstruct dir_entry *ent = dir.entries[i];\n> -\t\tint matches = 0;\n> \t\tstruct stat st;\n> \t\tconst char *rel;\n> \n> @@ -1013,8 +1012,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> \t\tif (lstat(ent->name, &st))\n> \t\t\tdie_errno(\"Cannot lstat '%s'\", ent->name);\n> \n> -\t\tif (S_ISDIR(st.st_mode) && !remove_directories &&\n> -\t\t matches != MATCHED_EXACTLY)\n> +\t\tif (S_ISDIR(st.st_mode) && !remove_directories)\n> \t\t\tcontinue;\n> \n> \t\trel = relative_path(ent->name, prefix, &buf);\n\nDefinitely not necessary, but on a patch like this I'll sometimes\nmanually specify \"-U4\" (and I always have diff.interhunkcontext set to\n\"1\") to show the complete code between the declaration and use. It makes\nit even more obvious that the result is correct (though obviously\napplying and compiling shows it, too). #gitlifehacks\n\n-Peff\n"},{"id":"423820","messageId":"CAHd-oW6oN7=8DwzEFmCXdSWRcvzpqGEKFDkTMzN3uP1M9Cphrg@mail.gmail.com","threadId":"55636","inReplyTo":"YJRLim10cznG5G3d@coredump.intra.peff.net","subject":"Re: [PATCH] clean: remove unnecessary variable","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-05-06T21:14:22Z","receivedAt":"2021-05-06T21:14:38Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, May 6, 2021 at 5:03 PM Jeff King wrote:\n>\n> Definitely not necessary, but on a patch like this I'll sometimes\n> manually specify \"-U4\" (and I always have diff.interhunkcontext set to\n> \"1\") to show the complete code between the declaration and use. It makes\n> it even more obvious that the result is correct (though obviously\n> applying and compiling shows it, too). #gitlifehacks\n\nThat's a good idea, thanks for the advice :)\n"},{"id":"423823","messageId":"CABPp-BGj5AmWrQLfU7u1MZbcjGiytCMkAXwutEZG1tjfJYWDTQ@mail.gmail.com","threadId":"55636","inReplyTo":"YJRLim10cznG5G3d@coredump.intra.peff.net","subject":"Re: [PATCH] clean: remove unnecessary variable","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-05-06T21:35:45Z","receivedAt":"2021-05-06T21:35:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, May 6, 2021 at 1:03 PM Jeff King wrote:\n>\n> On Thu, May 06, 2021 at 04:33:15PM -0300, Matheus Tavares wrote:\n>\n> > The variable `matches` used to hold the return of a `dir_path_match()`\n> > call that was removed in 95c11ecc73 (\"Fix error-prone fill_directory()\n> > API; make it only return matches\", 2020-04-01). Now `matches` will\n> > always hold 0, which is the value it's initialized with; and the\n> > condition `matches != MATCHED_EXACTLY` will always evaluate to true. So\n> > let's remove this unnecessary variable.\n> >\n> > Interestingly, it seems that `matches != MATCHED_EXACTLY` was already\n> > unnecessary before 95c11ecc73. That's because `remove_directories` is\n> > always set to 1 when we have pathspecs; So, in the condition\n> > `!remove_directories && matches != MATCHED_EXACTLY`, we would either:\n> >\n> > - have pathspecs (or have been given `-d`) and ignore `matches` because\n> > `remove_directories` is 1; or\n> >\n> > - not have pathspecs (nor `-d`) and end up just checking that\n> > `0 != MATCHED_EXACTLY`, as `matches` would never get reassigned\n> > after its zero initialization (because there is no pathspec to match).\n>\n> Thanks for this digging and the extra analysis. We can see from the\n> patch that this variable can't possibly be doing anything, but it is\n> always a comfort to see authors researching the source of the oddity and\n> explaining what they found.\n>\n> I'm adding Elijah to the cc as an area expert, just in case he has any\n> other insight.\n\nThanks for catching this Matheus, and digging in a bit on the\nanalysis. This change looks good to me:\n\nReviewed-by: Elijah Newren \n\n\n>\n> > diff --git a/builtin/clean.c b/builtin/clean.c\n> > index 995053b791..f6d7e8119c 100644\n> > --- a/builtin/clean.c\n> > +++ b/builtin/clean.c\n> > @@ -1003,7 +1003,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> >\n> > for (i = 0; i < dir.nr; i++) {\n> > struct dir_entry *ent = dir.entries[i];\n> > - int matches = 0;\n> > struct stat st;\n> > const char *rel;\n> >\n> > @@ -1013,8 +1012,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> > if (lstat(ent->name, &st))\n> > die_errno(\"Cannot lstat '%s'\", ent->name);\n> >\n> > - if (S_ISDIR(st.st_mode) && !remove_directories &&\n> > - matches != MATCHED_EXACTLY)\n> > + if (S_ISDIR(st.st_mode) && !remove_directories)\n> > continue;\n> >\n> > rel = relative_path(ent->name, prefix, &buf);\n>\n> Definitely not necessary, but on a patch like this I'll sometimes\n> manually specify \"-U4\" (and I always have diff.interhunkcontext set to\n> \"1\") to show the complete code between the declaration and use. It makes\n> it even more obvious that the result is correct (though obviously\n> applying and compiling shows it, too). #gitlifehacks\n>\n> -Peff\n"}]}