{"thread":{"id":"53656","subject":"[PATCH 0/4] Clean cleanups","startedAt":"2020-06-11T06:59:39Z","lastAt":"2020-06-18T06:20:35Z","messageCount":7,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Brian Malehorn"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"399499","messageId":"pull.806.git.git.1591858774.gitgitgadget@gmail.com","threadId":"53656","inReplyTo":null,"subject":"[PATCH 0/4] Clean cleanups","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T06:59:29Z","receivedAt":"2020-06-11T06:59:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This fixes the performance regression reported at [1], and also performs a\nfew other cleanups found while investigating the issue.\n\n[1] \nhttps://lore.kernel.org/git/CAJB88a23uU2WfB0mnB9NfNbtgmABhNOWNOEMBt7rRVu7uL_C9A@mail.gmail.com/\n\nElijah Newren (4):\n dir: fix a few confusing comments\n dir, clean: avoid disallowed behavior\n clean: consolidate handling of ignored parameters\n clean: optimize and document cases where we recurse into\n subdirectories\n\n builtin/clean.c | 49 ++++++++++++++++++++++++++++++++++++++-----------\n dir.c | 15 ++++++++++-----\n 2 files changed, 48 insertions(+), 16 deletions(-)\n\n\nbase-commit: 20514004ddf1a3528de8933bc32f284e175e1012\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-806%2Fnewren%2Fclean-cleanups-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-806/newren/clean-cleanups-v1\nPull-Request: https://github.com/git/git/pull/806\n-- \ngitgitgadget\n"},{"id":"399500","messageId":"d8e050701bde53084a337ec0bde40af8ff392ae5.1591858774.git.gitgitgadget@gmail.com","threadId":"53656","inReplyTo":"pull.806.git.git.1591858774.gitgitgadget@gmail.com","subject":"[PATCH 1/4] dir: fix a few confusing comments","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T06:59:30Z","receivedAt":"2020-06-11T06:59:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n dir.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d97e9558489..6fb2f8ecdd7 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1820,7 +1820,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t * to recurse into untracked/ignored directories if either of the\n \t * following bits is set:\n \t * - DIR_SHOW_IGNORED_TOO (because then we need to determine if\n-\t * there are ignored directories below)\n+\t * there are ignored entries below)\n \t * - DIR_HIDE_EMPTY_DIRECTORIES (because we have to determine if\n \t * the directory is empty)\n \t */\n@@ -1838,10 +1838,11 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\treturn path_excluded;\n \n \t/*\n-\t * If we have we don't want to know the all the paths under an\n-\t * untracked or ignored directory, we still need to go into the\n-\t * directory to determine if it is empty (because an empty directory\n-\t * should be path_none instead of path_excluded or path_untracked).\n+\t * Even if we don't want to know all the paths under an untracked or\n+\t * ignored directory, we may still need to go into the directory to\n+\t * determine if it is empty (because with DIR_HIDE_EMPTY_DIRECTORIES,\n+\t * an empty directory should be path_none instead of path_excluded or\n+\t * path_untracked).\n \t */\n \tcheck_only = ((dir->flags & DIR_HIDE_EMPTY_DIRECTORIES) &&\n \t\t !(dir->flags & DIR_SHOW_IGNORED_TOO));\n-- \ngitgitgadget\n\n"},{"id":"399501","messageId":"d59f877dd5cb5515f6af5ac6ce308263051b926b.1591858774.git.gitgitgadget@gmail.com","threadId":"53656","inReplyTo":"pull.806.git.git.1591858774.gitgitgadget@gmail.com","subject":"[PATCH 2/4] dir, clean: avoid disallowed behavior","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T06:59:31Z","receivedAt":"2020-06-11T06:59:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\ndir.h documented quite clearly that DIR_SHOW_IGNORED and\nDIR_SHOW_IGNORED_TOO are mutually exclusive, with a big comment to this\neffect by the definition of both enum values. However, a command like\n git clean -fx $DIR\nwould set both values for dir.flags. I _think_ it happened to work\nbecause:\n * As dir.h points out, DIR_KEEP_UNTRACKED_CONTENTS only takes effect\n if DIR_SHOW_IGNORED_TOO is set.\n * As coded, I believe DIR_SHOW_IGNORED would just happen to take\n precedence over DIR_SHOW_IGNORED_TOO in the code as currently\n constructed.\nWhich is a long way of saying \"we just got lucky\".\n\nFix clean.c to avoid setting these mutually exclusive values at the same\ntime, and add a check to dir.c that will throw a BUG() to prevent anyone\nelse from making this mistake.\n\nSigned-off-by: Elijah Newren \n---\n builtin/clean.c | 2 +-\n dir.c | 4 ++++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 4ca12bc0c0b..3ca940f83a2 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -954,7 +954,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tremove_directories = 1;\n \t}\n \n-\tif (remove_directories)\n+\tif (remove_directories && !ignored_only)\n \t\tdir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;\n \n \tif (read_cache() < 0)\ndiff --git a/dir.c b/dir.c\nindex 6fb2f8ecdd7..a3ad9702d64 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -193,6 +193,10 @@ int fill_directory(struct dir_struct *dir,\n \tconst char *prefix;\n \tsize_t prefix_len;\n \n+\tunsigned exclusive_flags = DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO;\n+\tif ((dir->flags & exclusive_flags) == exclusive_flags)\n+\t\tBUG(\"DIR_SHOW_IGNORED and DIR_SHOW_IGNORED_TOO are exclusive\");\n+\n \t/*\n \t * Calculate common prefix for the pathspec, and\n \t * use that to optimize the directory walk\n-- \ngitgitgadget\n\n"},{"id":"399502","messageId":"e6c700b3b4fa064c6a78e6d3ab262cf105b0e1e4.1591858774.git.gitgitgadget@gmail.com","threadId":"53656","inReplyTo":"pull.806.git.git.1591858774.gitgitgadget@gmail.com","subject":"[PATCH 3/4] clean: consolidate handling of ignored parameters","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T06:59:32Z","receivedAt":"2020-06-11T06:59:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nI spent a long time trying to figure out how and whether the code worked\nwith different values of ignore, ignore_only, and remove_directories.\nAfter lots of time setting up lots of testcases, sifting through lots of\nprint statements, and walking through the debugger, I finally realized\nthat one piece of code related to how it was all setup was found in\nclean.c rather than dir.c. Make a change that would have made it easier\nfor me to do the extra testing by putting this handling in one spot.\n\nSigned-off-by: Elijah Newren \n---\n builtin/clean.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 3ca940f83a2..1be437bd5a3 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -924,12 +924,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t 0);\n \n \tmemset(&dir, 0, sizeof(dir));\n-\tif (ignored_only)\n-\t\tdir.flags |= DIR_SHOW_IGNORED;\n-\n-\tif (ignored && ignored_only)\n-\t\tdie(_(\"-x and -X cannot be used together\"));\n-\n \tif (!interactive && !dry_run && !force) {\n \t\tif (config_set)\n \t\t\tdie(_(\"clean.requireForce set to true and neither -i, -n, nor -f given; \"\n@@ -946,6 +940,13 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n \n+\tif (ignored && ignored_only)\n+\t\tdie(_(\"-x and -X cannot be used together\"));\n+\tif (!ignored)\n+\t\tsetup_standard_excludes(&dir);\n+\tif (ignored_only)\n+\t\tdir.flags |= DIR_SHOW_IGNORED;\n+\n \tif (argc) {\n \t\t/*\n \t\t * Remaining args implies pathspecs specified, and we should\n@@ -960,9 +961,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n \n-\tif (!ignored)\n-\t\tsetup_standard_excludes(&dir);\n-\n \tpl = add_pattern_list(&dir, EXC_CMDL, \"--exclude option\");\n \tfor (i = 0; i < exclude_list.nr; i++)\n \t\tadd_pattern(exclude_list.items[i].string, \"\", 0, pl, -(i+1));\n-- \ngitgitgadget\n\n"},{"id":"399503","messageId":"b4cb448e8d76ae16f6fa38a773244bb4a8499938.1591858774.git.gitgitgadget@gmail.com","threadId":"53656","inReplyTo":"pull.806.git.git.1591858774.gitgitgadget@gmail.com","subject":"[PATCH 4/4] clean: optimize and document cases where we recurse into subdirectories","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T06:59:33Z","receivedAt":"2020-06-11T06:59:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCommit 6b1db43109 (\"clean: teach clean -d to preserve ignored paths\",\n2017-05-23) added the following code block (among others) to git-clean:\n if (remove_directories)\n dir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;\nThe reason for these flags is well documented in the commit message, but\nisn't obvious just from looking at the code. Add some explanations to\nthe code to make it clearer.\n\nFurther, it appears git-2.26 did not correctly handle this combination\nof flags from git-clean. With both these flags and without\nDIR_SHOW_IGNORED_TOO_MODE_MATCHING set, git is supposed to recurse into\nall untracked AND ignored directories. git-2.26.0 clearly was not doing\nthat. I don't know the full reasons for that or whether git < 2.27.0\nhad additional unknown bugs because of that misbehavior, because I don't\nfeel it's worth digging into. As per the huge changes and craziness\ndocumented in commit 8d92fb2927 (\"dir: replace exponential algorithm\nwith a linear one\", 2020-04-01), the old algorithm was a mess and was\nthrown out. What I can say is that git-2.27.0 correctly recurses into\nuntracked AND ignored directories with that combination.\n\nHowever, in clean's case we don't need to recurse into ignored\ndirectories; that is just a waste of time. Thus, when git-2.27.0\nstarted correctly handling those flags, we got a performance regression\nreport. Rather than relying on other bugs in fill_directory()'s former\nlogic to provide the behavior of skipping ignored directories, make use\nof the DIR_SHOW_IGNORED_TOO_MODE_MATCHING value specifically added in\ncommit eec0f7f2b7 (\"status: add option to show ignored files\ndifferently\", 2017-10-30) for this purpose.\n\nReported-by: Brian Malehorn \nSigned-off-by: Elijah Newren \n---\n builtin/clean.c | 33 +++++++++++++++++++++++++++++++--\n 1 file changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 1be437bd5a3..5a9c29a558b 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -955,8 +955,37 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tremove_directories = 1;\n \t}\n \n-\tif (remove_directories && !ignored_only)\n-\t\tdir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;\n+\tif (remove_directories && !ignored_only) {\n+\t\t/*\n+\t\t * We need to know about ignored files too:\n+\t\t *\n+\t\t * If (ignored), then we will delete ignored files as well.\n+\t\t *\n+\t\t * If (!ignored), then even though we not are doing\n+\t\t * anything with ignored files, we need to know about them\n+\t\t * so that we can avoid deleting a directory of untracked\n+\t\t * files that also contains an ignored file within it.\n+\t\t *\n+\t\t * For the (!ignored) case, since we only need to avoid\n+\t\t * deleting ignored files, we can set\n+\t\t * DIR_SHOW_IGNORED_TOO_MODE_MATCHING in order to avoid\n+\t\t * recursing into a directory which is itself ignored.\n+\t\t */\n+\t\tdir.flags |= DIR_SHOW_IGNORED_TOO;\n+\t\tif (!ignored)\n+\t\t\tdir.flags |= DIR_SHOW_IGNORED_TOO_MODE_MATCHING;\n+\n+\t\t/*\n+\t\t * Let the fill_directory() machinery know that we aren't\n+\t\t * just recursing to collect the ignored files; we want all\n+\t\t * the untracked ones so that we can delete them. (Note:\n+\t\t * we could also set DIR_KEEP_UNTRACKED_CONTENTS when\n+\t\t * ignored_only is true, since DIR_KEEP_UNTRACKED_CONTENTS\n+\t\t * only has effect in combination with DIR_SHOW_IGNORED_TOO. It makes\n+\t\t * the code clearer to exclude it, though.\n+\t\t */\n+\t\tdir.flags |= DIR_KEEP_UNTRACKED_CONTENTS;\n+\t}\n \n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n-- \ngitgitgadget\n"},{"id":"399637","messageId":"xmqqzh97zu5k.fsf@gitster.c.googlers.com","threadId":"53656","inReplyTo":"b4cb448e8d76ae16f6fa38a773244bb4a8499938.1591858774.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] clean: optimize and document cases where we recurse into subdirectories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-13T00:28:07Z","receivedAt":"2020-06-13T00:28:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> ...\n> However, in clean's case we don't need to recurse into ignored\n> directories; that is just a waste of time.\n\nNice.\n\n> Rather than relying on other bugs in fill_directory()'s former\n> logic to provide the behavior of skipping ignored directories, make use\n> of the DIR_SHOW_IGNORED_TOO_MODE_MATCHING value specifically added in\n> commit eec0f7f2b7 (\"status: add option to show ignored files\n> differently\", 2017-10-30) for this purpose.\n\nThanks.\n\n"},{"id":"400029","messageId":"CAJB88a0PkHmZTVQSi2tQZ7b2Adk46k0qEU4Xz8W+bYuHAeunuw@mail.gmail.com","threadId":"53656","inReplyTo":"xmqqzh97zu5k.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/4] clean: optimize and document cases where we recurse into subdirectories","fromName":"Brian Malehorn","fromEmail":"bmalehorn@gmail.com","sentAt":"2020-06-18T06:19:56Z","receivedAt":"2020-06-18T06:20:35Z","isPatch":true,"sender":{"key":"bmalehorn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3344958?v=4"},"body":"Catching up on this - thanks Elijah for jumping on this bug and fixing\nit immediately. I'll let you know if I see similar bugs in the future.\n\nThanks,\nBrian\n\nOn Fri, Jun 12, 2020 at 5:28 PM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > ...\n> > However, in clean's case we don't need to recurse into ignored\n> > directories; that is just a waste of time.\n>\n> Nice.\n>\n> > Rather than relying on other bugs in fill_directory()'s former\n> > logic to provide the behavior of skipping ignored directories, make use\n> > of the DIR_SHOW_IGNORED_TOO_MODE_MATCHING value specifically added in\n> > commit eec0f7f2b7 (\"status: add option to show ignored files\n> > differently\", 2017-10-30) for this purpose.\n>\n> Thanks.\n>\n"}]}