{"thread":{"id":"57418","subject":"[PATCH 1/2] clean: avoid looking for nested repository when appropriate","startedAt":"2022-02-15T22:16:28Z","lastAt":"2022-02-16T04:18:05Z","messageCount":4,"participants":["Patrick Marlier","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"448489","messageId":"20220215221615.20683-1-patrick.marlier@gmail.com","threadId":"57418","inReplyTo":null,"subject":"[PATCH 1/2] clean: avoid looking for nested repository when appropriate","fromName":"Patrick Marlier","fromEmail":"patrick.marlier@gmail.com","sentAt":"2022-02-15T22:16:14Z","receivedAt":"2022-02-15T22:16:28Z","isPatch":true,"sender":{"key":"patrick.marlier@gmail.com","avatar":null},"body":"avoiding the unnecessary checks for is_nonbare_repository_dir() via setting DIR_NO_GITLINKS\n---\n builtin/clean.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 3ff02bbbff..18b37e3fd9 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -955,9 +955,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\t \" refusing to clean\"));\n \t}\n \n-\tif (force > 1)\n+\tif (force > 1) {\n \t\trm_flags = 0;\n-\telse\n+\t\tdir.flags |= DIR_NO_GITLINKS;\n+\t} else\n \t\tdir.flags |= DIR_SKIP_NESTED_GIT;\n \n \tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n-- \n2.35.1\n\n"},{"id":"448490","messageId":"20220215221615.20683-2-patrick.marlier@gmail.com","threadId":"57418","inReplyTo":"20220215221615.20683-1-patrick.marlier@gmail.com","subject":"[PATCH 2/2] clean: avoid to differentiate untracked and ignored when appropriate","fromName":"Patrick Marlier","fromEmail":"patrick.marlier@gmail.com","sentAt":"2022-02-15T22:16:15Z","receivedAt":"2022-02-15T22:16:33Z","isPatch":true,"sender":{"key":"patrick.marlier@gmail.com","avatar":null},"body":"---\n builtin/clean.c | 4 +++-\n t/t7300-clean.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 18b37e3fd9..1b1454d052 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -978,7 +978,9 @@ 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+\tif (remove_directories && ignored && !exclude_list.nr && force > 1)\n+\t\t; /* No need to recurse to look for ignored files */\n+\telse if (remove_directories && !ignored_only) {\n \t\t/*\n \t\t * We need to know about ignored files too:\n \t\t *\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 0399701e62..684eba914b 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -788,4 +788,28 @@ test_expect_success 'traverse into directories that may have ignored entries' '\n \t)\n '\n \n+test_expect_success 'avoid traversing into untracked directories' '\n+\ttest_when_finished rm -f output error trace.* &&\n+\ttest_create_repo avoid-traversing-untracked-hierarchy &&\n+\t(\n+\t\tcd avoid-traversing-untracked-hierarchy &&\n+\n+\t\tmkdir -p untracked/subdir/with/b &&\n+\t\tmkdir -p untracked/subdir/with/a &&\n+\t\t>untracked/subdir/with/a/random-file.txt &&\n+\n+\t\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n+\t\tgit clean -ffdx\n+\t) &&\n+\n+\t# Make sure we only visited into the top-level directory, and did\n+\t# not traverse into the \"untracked\" subdirectory since it was excluded\n+\tgrep data.*read_directo.*directories-visited trace.output |\n+\t\tcut -d \"|\" -f 9 >trace.relevant &&\n+\tcat >trace.expect <<-EOF &&\n+\t ..directories-visited:1\n+\tEOF\n+\ttest_cmp trace.expect trace.relevant\n+'\n+\n test_done\n-- \n2.35.1\n\n"},{"id":"448536","messageId":"CABPp-BEimfkjKugBGkUkbcfCnsvgBEXdPq_wSVCNk-O7-nOV=w@mail.gmail.com","threadId":"57418","inReplyTo":"20220215221615.20683-1-patrick.marlier@gmail.com","subject":"Re: [PATCH 1/2] clean: avoid looking for nested repository when appropriate","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-16T04:09:52Z","receivedAt":"2022-02-16T04:10:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Tue, Feb 15, 2022 at 2:16 PM Patrick Marlier\n wrote:\n>\n> avoiding the unnecessary checks for is_nonbare_repository_dir() via setting DIR_NO_GITLINKS\n\nLooks great, but a few details about commit messages that we like to see:\n\n * Please wrap commit messages at 72 characters\n * Describe your changes in imperative mood (i.e. \"Avoid the\nunnecessary\" rather than \"avoiding the unnecessary\")\n * Use complete sentences for everything other than the subject.\n\nSo, perhaps:\n\n\"\"\"\nclean: avoid looking for nested repositories when unnecessary\n\nWith `git clean --ff` we will be deleting nested untracked repositories,\nso there is no need to differentiate them from other untracked files.\nUse the DIR_NO_GITLINKS flag in dir.flags to signify this and avoid the\nis_nonbare_repository_dir() checks.\n\"\"\"\n\n> ---\n> builtin/clean.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 3ff02bbbff..18b37e3fd9 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -955,9 +955,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> \" refusing to clean\"));\n> }\n>\n> - if (force > 1)\n> + if (force > 1) {\n> rm_flags = 0;\n> - else\n> + dir.flags |= DIR_NO_GITLINKS;\n> + } else\n> dir.flags |= DIR_SKIP_NESTED_GIT;\n>\n> dir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n> --\n> 2.35.1\n"},{"id":"448537","messageId":"CABPp-BFnrd4=DM-xQtw+j=LRkA4fwYp1EZ2j3cBVjZRsYPU-9Q@mail.gmail.com","threadId":"57418","inReplyTo":"20220215221615.20683-2-patrick.marlier@gmail.com","subject":"Re: [PATCH 2/2] clean: avoid to differentiate untracked and ignored when appropriate","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-16T04:16:48Z","receivedAt":"2022-02-16T04:18:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Feb 15, 2022 at 2:16 PM Patrick Marlier\n wrote:\n\nCould I suggest a bit longer commit message? Perhaps:\n\n\"\"\"\nclean: avoid traversing into untracked dirs when unnecessary\n\nWhen deleting all untracked and ignored files and any nested\nrepositories (such as with `git clean -ffdx`), we do not need to recurse\ninto an untracked directory to see if any of the entries under it are\nignored or a nested repository. Special case this condition to avoid\nunnecessary recursion.\n\"\"\"\n\n> ---\n> builtin/clean.c | 4 +++-\n> t/t7300-clean.sh | 24 ++++++++++++++++++++++++\n> 2 files changed, 27 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 18b37e3fd9..1b1454d052 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -978,7 +978,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> remove_directories = 1;\n> }\n>\n> - if (remove_directories && !ignored_only) {\n> + if (remove_directories && ignored && !exclude_list.nr && force > 1)\n> + ; /* No need to recurse to look for ignored files */\n> + else if (remove_directories && !ignored_only) {\n> /*\n> * We need to know about ignored files too:\n> *\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index 0399701e62..684eba914b 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -788,4 +788,28 @@ test_expect_success 'traverse into directories that may have ignored entries' '\n> )\n> '\n>\n> +test_expect_success 'avoid traversing into untracked directories' '\n> + test_when_finished rm -f output error trace.* &&\n> + test_create_repo avoid-traversing-untracked-hierarchy &&\n\nI think you may have been copying from an example earlier in the file\nthat I like wrote. As someone else recently pointed out to me,\nt/test-lib-functions.sh points out that test_create_repo is deprecated\nand we should just use \"git init \" instead.\n\n> + (\n> + cd avoid-traversing-untracked-hierarchy &&\n> +\n> + mkdir -p untracked/subdir/with/b &&\n> + mkdir -p untracked/subdir/with/a &&\n> + >untracked/subdir/with/a/random-file.txt &&\n> +\n> + GIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n> + git clean -ffdx\n> + ) &&\n> +\n> + # Make sure we only visited into the top-level directory, and did\n> + # not traverse into the \"untracked\" subdirectory since it was excluded\n> + grep data.*read_directo.*directories-visited trace.output |\n> + cut -d \"|\" -f 9 >trace.relevant &&\n> + cat >trace.expect <<-EOF &&\n> + ..directories-visited:1\n> + EOF\n> + test_cmp trace.expect trace.relevant\n> +'\n> +\n> test_done\n> --\n> 2.35.1\n\nLooks good, other than those minor items. Thanks for working on this!\n"}]}