{"thread":{"id":"39050","subject":"[PATCH v2 0/3] Improving performance of git clean","startedAt":"2015-04-11T16:43:03Z","lastAt":"2015-04-17T19:13:20Z","messageCount":13,"participants":["Erik Elfström","Thomas Gummerer","erik elfström","Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"259236","messageId":"1428770587-9674-1-git-send-email-erik.elfstrom@gmail.com","threadId":"39050","inReplyTo":null,"subject":"[PATCH v2 0/3] Improving performance of git clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-11T16:43:03Z","receivedAt":"2015-04-11T16:43:03Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"v1 of the patch can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/266839/focus=266839\n\nchanges in v2:\n* fixed commit message,\n  \"p7300: added performance tests for clean\"\n  change to:\n  \"p7300: add performance tests for clean\"\n* simplified test code\n* removed non portable ls -A in test\n* removed non portable $(seq ) in test\n* fixed missing \" || return $?\" in test\n* fixed missing sub shell for 'cd' command in test\n* fixed broken && chains in test\n* added assert new clean.c:is_git_repository to guard against\n  negative array index\n* use size_t instead of int for strbuf->len\n\nfixes held back for cleanup patches:\n* fixed existing broken && chains\n* added assert in existing code to guard against\n  negative array index\n\nThanks to Eric Sunshine and Torsten Bögershausen for the very helpful\nreview!\n\n\nErik Elfström (3):\n  t7300: add tests to document behavior of clean and nested git\n  p7300: add performance tests for clean\n  clean: improve performance when removing lots of directories\n\n builtin/clean.c       | 24 ++++++++++++++---\n t/perf/p7300-clean.sh | 37 ++++++++++++++++++++++++++\n t/t7300-clean.sh      | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 129 insertions(+), 4 deletions(-)\n create mode 100755 t/perf/p7300-clean.sh\n\n-- \n2.4.0.rc0.37.ga3b75b3\n"},{"id":"259237","messageId":"1428770587-9674-2-git-send-email-erik.elfstrom@gmail.com","threadId":"39050","inReplyTo":"1428770587-9674-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v2 0/3] Improving performance of git clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-11T16:43:04Z","receivedAt":"2015-04-11T16:43:04Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"v1 of the patch can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/266839/focus=266839\n\nchanges in v2:\n* fixed commit message,\n  \"p7300: added performance tests for clean\"\n  change to:\n  \"p7300: add performance tests for clean\"\n* simplified test code\n* removed non portable ls -A in test\n* removed non portable $(seq ) in test\n* fixed missing \" || return $?\" in test\n* fixed missing sub shell for 'cd' command in test\n* fixed broken && chains in test\n* added assert new clean.c:is_git_repository to guard aginst\n  negative array index\n* use size_t instead of int for strbuf->len\n\nfixes held back for cleanup patches:\n* fixed existing broken && chains\n* added assert in existing code to guard gainst\n  negative array index\n\nThanks to Eric Sunshine and Torsten Bögershausen for the very helpful\nreview!\n\n\nErik Elfström (3):\n  t7300: add tests to document behavior of clean and nested git\n  p7300: add performance tests for clean\n  clean: improve performance when removing lots of directories\n\n builtin/clean.c       | 24 ++++++++++++++---\n t/perf/p7300-clean.sh | 37 ++++++++++++++++++++++++++\n t/t7300-clean.sh      | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 129 insertions(+), 4 deletions(-)\n create mode 100755 t/perf/p7300-clean.sh\n\n-- \n2.4.0.rc0.37.ga3b75b3\n"},{"id":"259239","messageId":"1428770587-9674-3-git-send-email-erik.elfstrom@gmail.com","threadId":"39050","inReplyTo":"1428770587-9674-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v2 1/3] t7300: add tests to document behavior of clean and nested git","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-11T16:43:05Z","receivedAt":"2015-04-11T16:43:05Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n---\n t/t7300-clean.sh | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 72 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 99be5d9..58e6b4a 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -455,6 +455,78 @@ test_expect_success 'nested git work tree' '\n \t! test -d bar\n '\n \n+test_expect_failure 'nested git (only init) should be kept' '\n+\trm -fr foo bar &&\n+\tgit init foo &&\n+\tmkdir bar &&\n+\t>bar/goodbye.people &&\n+\tgit clean -f -d &&\n+\ttest_path_is_file foo/.git/HEAD &&\n+\ttest_path_is_missing bar\n+'\n+\n+test_expect_failure 'nested git (bare) should be kept' '\n+\trm -fr foo bar &&\n+\tgit init --bare foo &&\n+\tmkdir bar &&\n+\t>bar/goodbye.people &&\n+\tgit clean -f -d &&\n+\ttest_path_is_file foo/HEAD &&\n+\ttest_path_is_missing bar\n+'\n+\n+test_expect_success 'giving path in nested git work tree will remove it' '\n+\trm -fr foo &&\n+\tmkdir foo &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init &&\n+\t\tmkdir -p bar/baz &&\n+\t\t(\n+\t\t\tcd bar/baz &&\n+\t\t\t>hello.world &&\n+\t\t\tgit add . &&\n+\t\t\tgit commit -a -m nested\n+\t\t)\n+\t) &&\n+\tgit clean -f -d foo/bar/baz &&\n+\ttest_path_is_file foo/.git/HEAD &&\n+\ttest_path_is_dir foo/bar/ &&\n+\ttest_path_is_missing foo/bar/baz\n+'\n+\n+test_expect_success 'giving path to nested .git will not remove it' '\n+\trm -fr foo &&\n+\tmkdir foo bar &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init &&\n+\t\t>hello.world &&\n+\t\tgit add . &&\n+\t\tgit commit -a -m nested\n+\t) &&\n+\tgit clean -f -d foo/.git &&\n+\ttest_path_is_file foo/.git/HEAD &&\n+\ttest_path_is_dir foo/.git/refs &&\n+\ttest_path_is_dir foo/.git/objects &&\n+\ttest_path_is_dir bar/\n+'\n+\n+test_expect_success 'giving path to nested .git/ will remove contents' '\n+\trm -fr foo bar &&\n+\tmkdir foo bar &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init &&\n+\t\t>hello.world &&\n+\t\tgit add . &&\n+\t\tgit commit -a -m nested\n+\t) &&\n+\tgit clean -f -d foo/.git/ &&\n+\ttest_path_is_dir foo/.git &&\n+\ttest_dir_is_empty foo/.git\n+'\n+\n test_expect_success 'force removal of nested git work tree' '\n \trm -fr foo bar baz &&\n \tmkdir -p foo bar baz/boo &&\n-- \n2.4.0.rc0.37.ga3b75b3\n"},{"id":"259240","messageId":"1428770587-9674-4-git-send-email-erik.elfstrom@gmail.com","threadId":"39050","inReplyTo":"1428770587-9674-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v2 2/3] p7300: add performance tests for clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-11T16:43:06Z","receivedAt":"2015-04-11T16:43:06Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n---\n t/perf/p7300-clean.sh | 37 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n create mode 100755 t/perf/p7300-clean.sh\n\ndiff --git a/t/perf/p7300-clean.sh b/t/perf/p7300-clean.sh\nnew file mode 100755\nindex 0000000..af50d5d\n--- /dev/null\n+++ b/t/perf/p7300-clean.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description=\"Test git-clean performance\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+test_expect_success 'setup untracked directory with many sub dirs' '\n+\trm -rf 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n+\tmkdir 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n+\tfor i in $(test_seq 1 500)\n+\tdo\n+\t\tmkdir 500_sub_dirs/dir$i || return $?\n+\tdone &&\n+\tfor i in $(test_seq 1 100)\n+\tdo\n+\t\tcp -r 500_sub_dirs 50000_sub_dirs/dir$i || return $?\n+\tdone\n+'\n+\n+test_perf 'clean many untracked sub dirs, check for nested git' '\n+\trm -rf clean_test_dir/50000_sub_dirs_cpy &&\n+\tcp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n+\tgit clean -q -f -d  clean_test_dir/ &&\n+\ttest_dir_is_empty clean_test_dir\n+'\n+\n+test_perf 'clean many untracked sub dirs, ignore nested git' '\n+\trm -rf clean_test_dir/50000_sub_dirs_cpy &&\n+\tcp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n+\tgit clean -q -f -f -d  clean_test_dir/ &&\n+\ttest_dir_is_empty clean_test_dir\n+'\n+\n+test_done\n-- \n2.4.0.rc0.37.ga3b75b3\n"},{"id":"259238","messageId":"1428770587-9674-5-git-send-email-erik.elfstrom@gmail.com","threadId":"39050","inReplyTo":"1428770587-9674-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v2 3/3] clean: improve performance when removing lots of directories","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-11T16:43:07Z","receivedAt":"2015-04-11T16:43:07Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Before this change, clean used resolve_gitlink_ref to check for the\npresence of nested git repositories. This had the drawback of creating\na ref_cache entry for every directory that should potentially be\ncleaned. The linear search through the ref_cache list caused a massive\nperformance hit for large number of directories.\n\nTeach clean.c:remove_dirs to use setup.c:is_git_directory\ninstead. is_git_directory will actually open HEAD and parse the HEAD\nref but this implies a nested git repository and should be rare when\ncleaning.\n\nUsing is_git_directory should give a more standardized check for what\nis and what isn't a git repository but also gives a slight behavioral\nchange. We will now detect and respect bare and empty nested git\nrepositories (only init run). Update t7300 to reflect this.\n\nThe time to clean an untracked directory containing 100000 sub\ndirectories went from 61s to 1.7s after this change.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n---\n builtin/clean.c  | 24 ++++++++++++++++++++----\n t/t7300-clean.sh |  4 ++--\n 2 files changed, 22 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 98c103f..b679913 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -10,7 +10,6 @@\n #include \"cache.h\"\n #include \"dir.h\"\n #include \"parse-options.h\"\n-#include \"refs.h\"\n #include \"string-list.h\"\n #include \"quote.h\"\n #include \"column.h\"\n@@ -148,6 +147,25 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int is_git_repository(struct strbuf *path)\n+{\n+\tint ret = 0;\n+\tif (is_git_directory(path->buf))\n+\t\tret = 1;\n+\telse {\n+\t\tsize_t orig_path_len = path->len;\n+\t\tassert(orig_path_len != 0);\n+\t\tif (path->buf[orig_path_len - 1] != '/')\n+\t\t\tstrbuf_addch(path, '/');\n+\t\tstrbuf_addstr(path, \".git\");\n+\t\tif (is_git_directory(path->buf))\n+\t\t\tret = 1;\n+\t\tstrbuf_setlen(path, orig_path_len);\n+\t}\n+\n+\treturn ret;\n+}\n+\n static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \t\tint dry_run, int quiet, int *dir_gone)\n {\n@@ -155,13 +173,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \tstruct strbuf quoted = STRBUF_INIT;\n \tstruct dirent *e;\n \tint res = 0, ret = 0, gone = 1, original_len = path->len, len;\n-\tunsigned char submodule_head[20];\n \tstruct string_list dels = STRING_LIST_INIT_DUP;\n \n \t*dir_gone = 1;\n \n-\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n-\t\t\t!resolve_gitlink_ref(path->buf, \"HEAD\", submodule_head)) {\n+\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) && is_git_repository(path)) {\n \t\tif (!quiet) {\n \t\t\tquote_path_relative(path->buf, prefix, &quoted);\n \t\t\tprintf(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 58e6b4a..da294fe 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -455,7 +455,7 @@ test_expect_success 'nested git work tree' '\n \t! test -d bar\n '\n \n-test_expect_failure 'nested git (only init) should be kept' '\n+test_expect_success 'nested git (only init) should be kept' '\n \trm -fr foo bar &&\n \tgit init foo &&\n \tmkdir bar &&\n@@ -465,7 +465,7 @@ test_expect_failure 'nested git (only init) should be kept' '\n \ttest_path_is_missing bar\n '\n \n-test_expect_failure 'nested git (bare) should be kept' '\n+test_expect_success 'nested git (bare) should be kept' '\n \trm -fr foo bar &&\n \tgit init --bare foo &&\n \tmkdir bar &&\n-- \n2.4.0.rc0.37.ga3b75b3\n"},{"id":"259241","messageId":"20150411175941.GA32735@hank","threadId":"39050","inReplyTo":"1428770587-9674-4-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v2 2/3] p7300: add performance tests for clean","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-04-11T17:59:41Z","receivedAt":"2015-04-11T17:59:41Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/11, Erik Elfström wrote:\n> Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n> ---\n>  t/perf/p7300-clean.sh | 37 +++++++++++++++++++++++++++++++++++++\n>  1 file changed, 37 insertions(+)\n>  create mode 100755 t/perf/p7300-clean.sh\n>\n> diff --git a/t/perf/p7300-clean.sh b/t/perf/p7300-clean.sh\n> new file mode 100755\n> index 0000000..af50d5d\n> --- /dev/null\n> +++ b/t/perf/p7300-clean.sh\n> @@ -0,0 +1,37 @@\n> +#!/bin/sh\n> +\n> +test_description=\"Test git-clean performance\"\n> +\n> +. ./perf-lib.sh\n> +\n> +test_perf_large_repo\n> +test_checkout_worktree\n> +\n> +test_expect_success 'setup untracked directory with many sub dirs' '\n> +\trm -rf 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n> +\tmkdir 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n> +\tfor i in $(test_seq 1 500)\n> +\tdo\n> +\t\tmkdir 500_sub_dirs/dir$i || return $?\n> +\tdone &&\n> +\tfor i in $(test_seq 1 100)\n> +\tdo\n> +\t\tcp -r 500_sub_dirs 50000_sub_dirs/dir$i || return $?\n> +\tdone\n> +'\n> +\n> +test_perf 'clean many untracked sub dirs, check for nested git' '\n> +\trm -rf clean_test_dir/50000_sub_dirs_cpy &&\n> +\tcp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n\nMaybe this would be a good place to use test_perf_cleanup, which I\nintroduced a while ago and you can find in the\ntg/perf-lib-test-perf-cleanup branch?  It probably won't influence the\nperformance a lot, but still better separate the code that actually\nneeds to be tested from the cleanup/preparation code.  Ditto in the\nother test.\n\n> +\tgit clean -q -f -d  clean_test_dir/ &&\n> +\ttest_dir_is_empty clean_test_dir\n> +'\n> +\n> +test_perf 'clean many untracked sub dirs, ignore nested git' '\n> +\trm -rf clean_test_dir/50000_sub_dirs_cpy &&\n> +\tcp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n> +\tgit clean -q -f -f -d  clean_test_dir/ &&\n> +\ttest_dir_is_empty clean_test_dir\n> +'\n> +\n> +test_done\n> --\n> 2.4.0.rc0.37.ga3b75b3\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"259263","messageId":"CAMpP7NZyHBMZ5omT+fPGfkRR56HQPNP3n6cXD7S8voVvqrG6eg@mail.gmail.com","threadId":"39050","inReplyTo":"20150411175941.GA32735@hank","subject":"Re: [PATCH v2 2/3] p7300: add performance tests for clean","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-12T15:31:49Z","receivedAt":"2015-04-12T15:31:49Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Sat, Apr 11, 2015 at 7:59 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:\n> On 04/11, Erik Elfström wrote:\n>> Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n>> ---\n>>  t/perf/p7300-clean.sh | 37 +++++++++++++++++++++++++++++++++++++\n>>  1 file changed, 37 insertions(+)\n>>  create mode 100755 t/perf/p7300-clean.sh\n>>\n>> diff --git a/t/perf/p7300-clean.sh b/t/perf/p7300-clean.sh\n>> new file mode 100755\n>> index 0000000..af50d5d\n>> --- /dev/null\n>> +++ b/t/perf/p7300-clean.sh\n>> @@ -0,0 +1,37 @@\n>> +#!/bin/sh\n>> +\n>> +test_description=\"Test git-clean performance\"\n>> +\n>> +. ./perf-lib.sh\n>> +\n>> +test_perf_large_repo\n>> +test_checkout_worktree\n>> +\n>> +test_expect_success 'setup untracked directory with many sub dirs' '\n>> +     rm -rf 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n>> +     mkdir 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n>> +     for i in $(test_seq 1 500)\n>> +     do\n>> +             mkdir 500_sub_dirs/dir$i || return $?\n>> +     done &&\n>> +     for i in $(test_seq 1 100)\n>> +     do\n>> +             cp -r 500_sub_dirs 50000_sub_dirs/dir$i || return $?\n>> +     done\n>> +'\n>> +\n>> +test_perf 'clean many untracked sub dirs, check for nested git' '\n>> +     rm -rf clean_test_dir/50000_sub_dirs_cpy &&\n>> +     cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n>\n> Maybe this would be a good place to use test_perf_cleanup, which I\n> introduced a while ago and you can find in the\n> tg/perf-lib-test-perf-cleanup branch?  It probably won't influence the\n> performance a lot, but still better separate the code that actually\n> needs to be tested from the cleanup/preparation code.  Ditto in the\n> other test.\n>\n\nYes, that would be a clear improvement. I was looking for something like\nthis, the copy takes more time than the clean currently.\n\nThe cleanup hook is maybe not exactly the right fit here though. I would\nneed to do one initial copy in the setup test and then a copy in the\ncleanup, something like this:\n\ntest_expect_success 'setup untracked directory with many sub dirs' '\n    ...\n    cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy\n'\n\ntest_perf_cleanup 'clean many untracked sub dirs, check for nested git' '\n    git clean -q -f -d  clean_test_dir/\n' '\n    test_dir_is_empty clean_test_dir &&\n    rm -rf clean_test_dir/50000_sub_dirs_cpy &&\n    cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy\n'\n\nThis works better than my original code but maybe we can do even better\nwith something like:\n\ntest_setup_perf_cleanup 'clean many untracked sub dirs, check for nested git' '\n    rm -rf clean_test_dir/50000_sub_dirs_cpy &&\n    cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy\n' '\n    git clean -q -f -d  clean_test_dir/\n' '\n    test_dir_is_empty clean_test_dir\n'\n\nHaving a setup phase avoids the initial copy in the setup test making\nthings a little easier to follow. I'm not sure its worth the extra complexity\nin perf-lib though (and I'm not sure I would be able to implement it either).\n\nAlso, what would the implications be if I were to use your new cleanup\nfunction that is not yet on master? Should I rebase on top of your topic\nor make a follow up patch to switch over?\n\n>> +     git clean -q -f -d  clean_test_dir/ &&\n>> +     test_dir_is_empty clean_test_dir\n>> +'\n>> +\n>> +test_perf 'clean many untracked sub dirs, ignore nested git' '\n>> +     rm -rf clean_test_dir/50000_sub_dirs_cpy &&\n>> +     cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n>> +     git clean -q -f -f -d  clean_test_dir/ &&\n>> +     test_dir_is_empty clean_test_dir\n>> +'\n>> +\n>> +test_done\n>> --\n>> 2.4.0.rc0.37.ga3b75b3\n>>\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"259264","messageId":"20150412165257.GA5793@hank","threadId":"39050","inReplyTo":"CAMpP7NZyHBMZ5omT+fPGfkRR56HQPNP3n6cXD7S8voVvqrG6eg@mail.gmail.com","subject":"Re: [PATCH v2 2/3] p7300: add performance tests for clean","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-04-12T16:52:57Z","receivedAt":"2015-04-12T16:52:57Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/12, erik elfström wrote:\n> On Sat, Apr 11, 2015 at 7:59 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:\n> > On 04/11, Erik Elfström wrote:\n> >> Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n> >> ---\n> >>  t/perf/p7300-clean.sh | 37 +++++++++++++++++++++++++++++++++++++\n> >>  1 file changed, 37 insertions(+)\n> >>  create mode 100755 t/perf/p7300-clean.sh\n> >>\n> >> diff --git a/t/perf/p7300-clean.sh b/t/perf/p7300-clean.sh\n> >> new file mode 100755\n> >> index 0000000..af50d5d\n> >> --- /dev/null\n> >> +++ b/t/perf/p7300-clean.sh\n> >> @@ -0,0 +1,37 @@\n> >> +#!/bin/sh\n> >> +\n> >> +test_description=\"Test git-clean performance\"\n> >> +\n> >> +. ./perf-lib.sh\n> >> +\n> >> +test_perf_large_repo\n> >> +test_checkout_worktree\n> >> +\n> >> +test_expect_success 'setup untracked directory with many sub dirs' '\n> >> +     rm -rf 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n> >> +     mkdir 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n> >> +     for i in $(test_seq 1 500)\n> >> +     do\n> >> +             mkdir 500_sub_dirs/dir$i || return $?\n> >> +     done &&\n> >> +     for i in $(test_seq 1 100)\n> >> +     do\n> >> +             cp -r 500_sub_dirs 50000_sub_dirs/dir$i || return $?\n> >> +     done\n> >> +'\n> >> +\n> >> +test_perf 'clean many untracked sub dirs, check for nested git' '\n> >> +     rm -rf clean_test_dir/50000_sub_dirs_cpy &&\n> >> +     cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n> >\n> > Maybe this would be a good place to use test_perf_cleanup, which I\n> > introduced a while ago and you can find in the\n> > tg/perf-lib-test-perf-cleanup branch?  It probably won't influence the\n> > performance a lot, but still better separate the code that actually\n> > needs to be tested from the cleanup/preparation code.  Ditto in the\n> > other test.\n> >\n>\n> Yes, that would be a clear improvement. I was looking for something like\n> this, the copy takes more time than the clean currently.\n>\n> The cleanup hook is maybe not exactly the right fit here though. I would\n> need to do one initial copy in the setup test and then a copy in the\n> cleanup, something like this:\n>\n> test_expect_success 'setup untracked directory with many sub dirs' '\n>     ...\n>     cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy\n> '\n>\n> test_perf_cleanup 'clean many untracked sub dirs, check for nested git' '\n>     git clean -q -f -d  clean_test_dir/\n> ' '\n>     test_dir_is_empty clean_test_dir &&\n>     rm -rf clean_test_dir/50000_sub_dirs_cpy &&\n>     cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy\n> '\n>\n> This works better than my original code but maybe we can do even better\n> with something like:\n>\n> test_setup_perf_cleanup 'clean many untracked sub dirs, check for nested git' '\n>     rm -rf clean_test_dir/50000_sub_dirs_cpy &&\n>     cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy\n> ' '\n>     git clean -q -f -d  clean_test_dir/\n> ' '\n>     test_dir_is_empty clean_test_dir\n> '\n>\n> Having a setup phase avoids the initial copy in the setup test making\n> things a little easier to follow. I'm not sure its worth the extra complexity\n> in perf-lib though (and I'm not sure I would be able to implement it either).\n\nYeah, you're right, that would look even better.  In order to avoid\nthe extra complexity we might actually go the other route that was\nproposed by Junio in the discussion back then\n(http://thread.gmane.org/gmane.comp.version-control.git/234874/focus=235241).\n\nUsing that and adding a setup option we could have something like:\n\ntest_perf --setup '\n\trm -rf clean_test_dir/50000_sub_dirs_cpy &&\n\tcp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy\n' --cleanup '\n\ttest_dir_is_empty clean_test_dir\n' 'clean many untracked sub dirs, check for nested git' '\n\tgit clean -q -f -d clean_test_dir\n'\n\n> Also, what would the implications be if I were to use your new cleanup\n> function that is not yet on master? Should I rebase on top of your topic\n> or make a follow up patch to switch over?\n\nIf the consensus is to go that route, I think you could just rebase\nthe topic onto my topic.  If we introduce the \"modern\" test style as\ndescribed before it might be worth merging this series first and then\nswitch over to the new style.  You should probably wait for other\nopinions before changing it though.\n\n> >> +     git clean -q -f -d  clean_test_dir/ &&\n> >> +     test_dir_is_empty clean_test_dir\n> >> +'\n> >> +\n> >> +test_perf 'clean many untracked sub dirs, ignore nested git' '\n> >> +     rm -rf clean_test_dir/50000_sub_dirs_cpy &&\n> >> +     cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n> >> +     git clean -q -f -f -d  clean_test_dir/ &&\n> >> +     test_dir_is_empty clean_test_dir\n> >> +'\n> >> +\n> >> +test_done\n> >> --\n> >> 2.4.0.rc0.37.ga3b75b3\n> >>\n> >> --\n> >> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> >> the body of a message to majordomo@vger.kernel.org\n> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n--\nThomas Gummerer\n"},{"id":"259388","messageId":"CAPig+cTnhCQHOvAkNyXxD+KKjs5bwSRTARvwZeLHqJ4tPq3FVg@mail.gmail.com","threadId":"39050","inReplyTo":"1428770587-9674-1-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v2 0/3] Improving performance of git clean","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-15T03:33:46Z","receivedAt":"2015-04-15T03:33:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 11, 2015 at 12:43 PM, Erik Elfström <erik.elfstrom@gmail.com> wrote:\n> v1 of the patch can be found here:\n> http://thread.gmane.org/gmane.comp.version-control.git/266839/focus=266839\n>\n> changes in v2:\n> * fixed commit message,\n>   \"p7300: added performance tests for clean\"\n>   change to:\n>   \"p7300: add performance tests for clean\"\n> * simplified test code\n> * removed non portable ls -A in test\n> * removed non portable $(seq ) in test\n> * fixed missing \" || return $?\" in test\n> * fixed missing sub shell for 'cd' command in test\n> * fixed broken && chains in test\n> * added assert new clean.c:is_git_repository to guard against\n>   negative array index\n> * use size_t instead of int for strbuf->len\n>\n> fixes held back for cleanup patches:\n> * fixed existing broken && chains\n> * added assert in existing code to guard against\n>   negative array index\n>\n> Thanks to Eric Sunshine and Torsten Bögershausen for the very helpful\n> review!\n\nThanks. This version looks much better.\n"},{"id":"259432","messageId":"xmqqpp75l1gd.fsf@gitster.dls.corp.google.com","threadId":"39050","inReplyTo":"1428770587-9674-5-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v2 3/3] clean: improve performance when removing lots of directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T17:56:50Z","receivedAt":"2015-04-15T17:56:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Elfström <erik.elfstrom@gmail.com> writes:\n\n> Before this change, clean used resolve_gitlink_ref to check for the\n> presence of nested git repositories. This had the drawback of creating\n> a ref_cache entry for every directory that should potentially be\n> cleaned. The linear search through the ref_cache list caused a massive\n> performance hit for large number of directories.\n\nI'd prefer to see the \"current state\" described in the current\ntense, e.g.\n\n    \"git clean\" uses resolve_gitlink_ref() to check for the presence of\n    nested git repositories, but it has the drawback of creating a\n    ref_cache entry for every directory that should potentially be\n    cleaned. The linear search through the ref_cache list causes a\n    massive performance hit for large number of directories.\n\n> Teach clean.c:remove_dirs to use setup.c:is_git_directory\n> instead. is_git_directory will actually open HEAD and parse the HEAD\n> ref but this implies a nested git repository and should be rare when\n> cleaning.\n\nI am not sure what you wanted to say in this paragraph.  What does\nit being rare have to do with it?  Even if it is not rare (i.e. the\ntop-level project you are working with has many submodules checked\nout without using the more recent \"a file .git pointing into\n.git/modules/ via 'gitdir: $overThere'\" mechanism), if we found a\nnested git repository, we treat it as special and exclude it from\ncleaning it out, which is a good thing, no?\n\nDoesn't this implementation get confused by modern submodule\ncheckouts and descend into and clean their working tree, though?\nModule M with path P would have a directory P in the working tree of\nthe top-level project, and P/.git is a regular file that will fail\n\"is_git_directory()\" test but records the location of the real\nsubmodule repository i.e. \".git/modules/M\" via the \"gitdir:\"\nmechanism.\n\n> Using is_git_directory should give a more standardized check for what\n> is and what isn't a git repository but also gives a slight behavioral\n> change. We will now detect and respect bare and empty nested git\n> repositories (only init run). Update t7300 to reflect this.\n>\n> The time to clean an untracked directory containing 100000 sub\n> directories went from 61s to 1.7s after this change.\n>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n> ---\n>  builtin/clean.c  | 24 ++++++++++++++++++++----\n>  t/t7300-clean.sh |  4 ++--\n>  2 files changed, 22 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 98c103f..b679913 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -10,7 +10,6 @@\n>  #include \"cache.h\"\n>  #include \"dir.h\"\n>  #include \"parse-options.h\"\n> -#include \"refs.h\"\n>  #include \"string-list.h\"\n>  #include \"quote.h\"\n>  #include \"column.h\"\n> @@ -148,6 +147,25 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)\n>  \treturn 0;\n>  }\n>  \n> +static int is_git_repository(struct strbuf *path)\n> +{\n> +\tint ret = 0;\n> +\tif (is_git_directory(path->buf))\n> +\t\tret = 1;\n> +\telse {\n> +\t\tsize_t orig_path_len = path->len;\n> +\t\tassert(orig_path_len != 0);\n> +\t\tif (path->buf[orig_path_len - 1] != '/')\n> +\t\t\tstrbuf_addch(path, '/');\n> +\t\tstrbuf_addstr(path, \".git\");\n> +\t\tif (is_git_directory(path->buf))\n> +\t\t\tret = 1;\n> +\t\tstrbuf_setlen(path, orig_path_len);\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n>  static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n>  \t\tint dry_run, int quiet, int *dir_gone)\n>  {\n> @@ -155,13 +173,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n>  \tstruct strbuf quoted = STRBUF_INIT;\n>  \tstruct dirent *e;\n>  \tint res = 0, ret = 0, gone = 1, original_len = path->len, len;\n> -\tunsigned char submodule_head[20];\n>  \tstruct string_list dels = STRING_LIST_INIT_DUP;\n>  \n>  \t*dir_gone = 1;\n>  \n> -\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n> -\t\t\t!resolve_gitlink_ref(path->buf, \"HEAD\", submodule_head)) {\n> +\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) && is_git_repository(path)) {\n>  \t\tif (!quiet) {\n>  \t\t\tquote_path_relative(path->buf, prefix, &quoted);\n>  \t\t\tprintf(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index 58e6b4a..da294fe 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -455,7 +455,7 @@ test_expect_success 'nested git work tree' '\n>  \t! test -d bar\n>  '\n>  \n> -test_expect_failure 'nested git (only init) should be kept' '\n> +test_expect_success 'nested git (only init) should be kept' '\n>  \trm -fr foo bar &&\n>  \tgit init foo &&\n>  \tmkdir bar &&\n> @@ -465,7 +465,7 @@ test_expect_failure 'nested git (only init) should be kept' '\n>  \ttest_path_is_missing bar\n>  '\n>  \n> -test_expect_failure 'nested git (bare) should be kept' '\n> +test_expect_success 'nested git (bare) should be kept' '\n>  \trm -fr foo bar &&\n>  \tgit init --bare foo &&\n>  \tmkdir bar &&\n"},{"id":"259580","messageId":"CAMpP7NbQ7pmcjT4pDn5dA5yvfgMgirbkoMD6ijjiFp7zmARPbA@mail.gmail.com","threadId":"39050","inReplyTo":"xmqqpp75l1gd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 3/3] clean: improve performance when removing lots of directories","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-17T18:15:40Z","receivedAt":"2015-04-17T18:15:40Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Wed, Apr 15, 2015 at 7:56 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Erik Elfström <erik.elfstrom@gmail.com> writes:\n>\n>> Before this change, clean used resolve_gitlink_ref to check for the\n>> presence of nested git repositories. This had the drawback of creating\n>> a ref_cache entry for every directory that should potentially be\n>> cleaned. The linear search through the ref_cache list caused a massive\n>> performance hit for large number of directories.\n>\n> I'd prefer to see the \"current state\" described in the current\n> tense, e.g.\n>\n>     \"git clean\" uses resolve_gitlink_ref() to check for the presence of\n>     nested git repositories, but it has the drawback of creating a\n>     ref_cache entry for every directory that should potentially be\n>     cleaned. The linear search through the ref_cache list causes a\n>     massive performance hit for large number of directories.\n>\n\nYes, that reads better.\n\n>> Teach clean.c:remove_dirs to use setup.c:is_git_directory\n>> instead. is_git_directory will actually open HEAD and parse the HEAD\n>> ref but this implies a nested git repository and should be rare when\n>> cleaning.\n>\n> I am not sure what you wanted to say in this paragraph.  What does\n> it being rare have to do with it?  Even if it is not rare (i.e. the\n> top-level project you are working with has many submodules checked\n> out without using the more recent \"a file .git pointing into\n> .git/modules/ via 'gitdir: $overThere'\" mechanism), if we found a\n> nested git repository, we treat it as special and exclude it from\n> cleaning it out, which is a good thing, no?\n>\n\nI was trying to motivate that the performance of is_git_directory is not a\nproblem for us even though it opens a file and parses it. I see now when I\nread it again that this is not very clear.\n\n> Doesn't this implementation get confused by modern submodule\n> checkouts and descend into and clean their working tree, though?\n> Module M with path P would have a directory P in the working tree of\n> the top-level project, and P/.git is a regular file that will fail\n> \"is_git_directory()\" test but records the location of the real\n> submodule repository i.e. \".git/modules/M\" via the \"gitdir:\"\n> mechanism.\n>\n\nYes, there is a problem here. I've added the test below and it fails after\nmy change by cleaning sub2 (sub1 is not cleaned). Are there more cases\nhere that I should test for?\n\n+test_expect_success 'should not clean submodules' '\n+       rm -fr repo to_clean sub1 sub2 &&\n+       mkdir -p repo to_clean &&\n+       (\n+               cd repo &&\n+               git init &&\n+               >hello.world\n+               git add . &&\n+               git commit -a -m nested\n+       ) &&\n+       git submodule add ./repo/.git sub1 &&\n+       git commit -m \"sub1\" &&\n+       git branch before_sub2 &&\n+       git submodule add ./repo/.git sub2 &&\n+       git commit -m \"sub2\" &&\n+       git checkout before_sub2 &&\n+       >to_clean/should_clean.this &&\n+       git clean -f -d &&\n+       test_path_is_file repo/.git/index &&\n+       test_path_is_file repo/hello.world &&\n+       test_path_is_file sub1/.git &&\n+       test_path_is_file sub1/hello.world &&\n+       test_path_is_file sub2/.git &&\n+       test_path_is_file sub2/hello.world &&\n+       test_path_is_missing to_clean\n+'\n\nBase on the previous discussion of the patch topic I can see 3 options\nfor how to fix this:\n\nOption 1:\n Plug the hole in my new is_git_repository function. A quick and dirty\n fix that passes the above test would be:\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex b679913..4f2fe95 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -153,14 +153,21 @@ static int is_git_repository(struct strbuf *path)\n        if (is_git_directory(path->buf))\n                ret = 1;\n        else {\n-               size_t orig_path_len = path->len;\n-               assert(orig_path_len != 0);\n-               if (path->buf[orig_path_len - 1] != '/')\n-                       strbuf_addch(path, '/');\n-               strbuf_addstr(path, \".git\");\n-               if (is_git_directory(path->buf))\n-                       ret = 1;\n-               strbuf_setlen(path, orig_path_len);\n+               struct stat st;\n+               const char *submodule_git_dir =\ngit_path_submodule(path->buf, \"\");\n+               lstat(submodule_git_dir, &st);\n+               if (S_ISDIR(st.st_mode))\n+                       ret = 1;\n+               else {\n+                       size_t orig_path_len = path->len;\n+                       assert(orig_path_len != 0);\n+                       if (path->buf[orig_path_len - 1] != '/')\n+                               strbuf_addch(path, '/');\n+                       strbuf_addstr(path, \".git\");\n+                       if (is_git_directory(path->buf))\n+                               ret = 1;\n+                       strbuf_setlen(path, orig_path_len);\n+               }\n        }\n\n        return ret;\n\nThere are probably more elegant solutions available here, suggestions\nwelcome.\n\nOption 2:\n Go with the current solution of using resolve_gitlink_ref but either\n A) avoid placing non-submodule paths in the ref_cache list, or\n B) remove non-submodule paths from the ref_cache list after we\n have detected that they are not submodules.\n\nOption 3:\n Speed up the ref_cache enough to make these dummy entires not matter.\n Nothing wrong with speeding up the lookup but it would still feel strange to\n populate it with junk entries.\n\nAre there more options available?\nWhat direction would you prefer?\n\n>> Using is_git_directory should give a more standardized check for what\n>> is and what isn't a git repository but also gives a slight behavioral\n>> change. We will now detect and respect bare and empty nested git\n>> repositories (only init run). Update t7300 to reflect this.\n>>\n>> The time to clean an untracked directory containing 100000 sub\n>> directories went from 61s to 1.7s after this change.\n>>\n>> Helped-by: Jeff King <peff@peff.net>\n>> Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n>> ---\n>>  builtin/clean.c  | 24 ++++++++++++++++++++----\n>>  t/t7300-clean.sh |  4 ++--\n>>  2 files changed, 22 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/builtin/clean.c b/builtin/clean.c\n>> index 98c103f..b679913 100644\n>> --- a/builtin/clean.c\n>> +++ b/builtin/clean.c\n>> @@ -10,7 +10,6 @@\n>>  #include \"cache.h\"\n>>  #include \"dir.h\"\n>>  #include \"parse-options.h\"\n>> -#include \"refs.h\"\n>>  #include \"string-list.h\"\n>>  #include \"quote.h\"\n>>  #include \"column.h\"\n>> @@ -148,6 +147,25 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)\n>>       return 0;\n>>  }\n>>\n>> +static int is_git_repository(struct strbuf *path)\n>> +{\n>> +     int ret = 0;\n>> +     if (is_git_directory(path->buf))\n>> +             ret = 1;\n>> +     else {\n>> +             size_t orig_path_len = path->len;\n>> +             assert(orig_path_len != 0);\n>> +             if (path->buf[orig_path_len - 1] != '/')\n>> +                     strbuf_addch(path, '/');\n>> +             strbuf_addstr(path, \".git\");\n>> +             if (is_git_directory(path->buf))\n>> +                     ret = 1;\n>> +             strbuf_setlen(path, orig_path_len);\n>> +     }\n>> +\n>> +     return ret;\n>> +}\n>> +\n>>  static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n>>               int dry_run, int quiet, int *dir_gone)\n>>  {\n>> @@ -155,13 +173,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n>>       struct strbuf quoted = STRBUF_INIT;\n>>       struct dirent *e;\n>>       int res = 0, ret = 0, gone = 1, original_len = path->len, len;\n>> -     unsigned char submodule_head[20];\n>>       struct string_list dels = STRING_LIST_INIT_DUP;\n>>\n>>       *dir_gone = 1;\n>>\n>> -     if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n>> -                     !resolve_gitlink_ref(path->buf, \"HEAD\", submodule_head)) {\n>> +     if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) && is_git_repository(path)) {\n>>               if (!quiet) {\n>>                       quote_path_relative(path->buf, prefix, &quoted);\n>>                       printf(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),\n>> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n>> index 58e6b4a..da294fe 100755\n>> --- a/t/t7300-clean.sh\n>> +++ b/t/t7300-clean.sh\n>> @@ -455,7 +455,7 @@ test_expect_success 'nested git work tree' '\n>>       ! test -d bar\n>>  '\n>>\n>> -test_expect_failure 'nested git (only init) should be kept' '\n>> +test_expect_success 'nested git (only init) should be kept' '\n>>       rm -fr foo bar &&\n>>       git init foo &&\n>>       mkdir bar &&\n>> @@ -465,7 +465,7 @@ test_expect_failure 'nested git (only init) should be kept' '\n>>       test_path_is_missing bar\n>>  '\n>>\n>> -test_expect_failure 'nested git (bare) should be kept' '\n>> +test_expect_success 'nested git (bare) should be kept' '\n>>       rm -fr foo bar &&\n>>       git init --bare foo &&\n>>       mkdir bar &&\n"},{"id":"259586","messageId":"20150417190002.GB32578@peff.net","threadId":"39050","inReplyTo":"CAMpP7NbQ7pmcjT4pDn5dA5yvfgMgirbkoMD6ijjiFp7zmARPbA@mail.gmail.com","subject":"Re: [PATCH v2 3/3] clean: improve performance when removing lots of directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-17T19:00:02Z","receivedAt":"2015-04-17T19:00:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 17, 2015 at 08:15:40PM +0200, erik elfström wrote:\n\n> > Doesn't this implementation get confused by modern submodule\n> > checkouts and descend into and clean their working tree, though?\n> > Module M with path P would have a directory P in the working tree of\n> > the top-level project, and P/.git is a regular file that will fail\n> > \"is_git_directory()\" test but records the location of the real\n> > submodule repository i.e. \".git/modules/M\" via the \"gitdir:\"\n> > mechanism.\n> >\n> \n> Yes, there is a problem here. I've added the test below and it fails after\n> my change by cleaning sub2 (sub1 is not cleaned). Are there more cases\n> here that I should test for?\n\nI wonder about the opposite case, too (finding more repos than we used\nto).\n\nIt looks like your patches will find bare repositories in the tree,\nwhereas the current code does not (it only cares about \".git\"). AFAIK,\nsubmodules will never exist as bare in the working tree. And I have seen\nrepositories which embed bare repos as test cases. Admittedly this is\nbecause I work on projects that are related to git itself, but I don't\nsee a reason to regress this case if the submodule code doesn't get any\nbenefit.\n\n> Base on the previous discussion of the patch topic I can see 3 options\n> for how to fix this:\n> \n> Option 1:\n>  Plug the hole in my new is_git_repository function. A quick and dirty\n>  fix that passes the above test would be:\n\nI think that makes sense. It would be nice if you could just call\nread_gitfile, but that function is very anxious to die on error. So the\nprerequisite step would probably be to refactor that into a\nread_gitfile_gently that returns an error code.\n\n-Peff\n\nPS Thank you for working on this. I have been quiet because I haven't\n   had a chance to look over your patches carefully yet, but overall\n   they look very promising.\n"},{"id":"259587","messageId":"xmqqpp72pnzj.fsf@gitster.dls.corp.google.com","threadId":"39050","inReplyTo":"20150417190002.GB32578@peff.net","subject":"Re: [PATCH v2 3/3] clean: improve performance when removing lots of directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-17T19:13:20Z","receivedAt":"2015-04-17T19:13:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Option 1:\n>>  Plug the hole in my new is_git_repository function. A quick and dirty\n>>  fix that passes the above test would be:\n>\n> I think that makes sense. It would be nice if you could just call\n> read_gitfile, but that function is very anxious to die on error. So the\n> prerequisite step would probably be to refactor that into a\n> read_gitfile_gently that returns an error code.\n\nI agree.\n\nI was looking at the repository discovery loop to see if it makes\nsense to update is-git-directory() to take a gitfile, but I do not\nthink it is a good idea (typically after is-git-directory() says\n\"yes\", we would append paths e.g. \"refs/heads/master\" after it to\npass the result to system calls like open()).  I agree that adding\nread-gitfile-gently and call it before running is-git-directory\nwould be a good solution for this change.\n\n> PS Thank you for working on this.\n\nThat too.\n\nThanks.\n"}]}