{"thread":{"id":"39576","subject":"[PATCH v7 1/5] setup: add gentle version of read_gitfile","startedAt":"2015-06-09T18:24:34Z","lastAt":"2015-06-15T03:08:44Z","messageCount":9,"participants":["Erik Elfström","Eric Sunshine","erik elfström"],"isPatch":true,"patchVersion":7,"patchTotal":5},"messages":[{"id":"263397","messageId":"1433874279-30720-1-git-send-email-erik.elfstrom@gmail.com","threadId":"39576","inReplyTo":null,"subject":"[PATCH v7 0/5] Improving performance of git clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-06-09T18:24:34Z","receivedAt":"2015-06-09T18:24:34Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Here is a reroll of this series (after much delay).\n\nChanges in v7:\n* changed order of file size and file open error check in read_gitfile\n* resolved conflicts with nd/multiple-work-trees. This removed the\n  need for is_git_directory_gently that was added in v6 and simplified\n  some error cases.\n\nErik Elfström (5):\n  setup: add gentle version of read_gitfile\n  setup: sanity check file size in read_gitfile_gently\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       |  30 +++++++++--\n cache.h               |  12 ++++-\n setup.c               |  91 +++++++++++++++++++++++++-------\n t/perf/p7300-clean.sh |  31 +++++++++++\n t/t7300-clean.sh      | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 280 insertions(+), 24 deletions(-)\n create mode 100755 t/perf/p7300-clean.sh\n\n-- \n2.4.3.373.gc496bfb\n"},{"id":"263393","messageId":"1433874279-30720-2-git-send-email-erik.elfstrom@gmail.com","threadId":"39576","inReplyTo":"1433874279-30720-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v7 1/5] setup: add gentle version of read_gitfile","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-06-09T18:24:35Z","receivedAt":"2015-06-09T18:24:35Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"read_gitfile will die on most error cases. This makes it unsuitable\nfor speculative calls. Extract the core logic and provide a gentle\nversion that returns NULL on failure.\n\nThe first usecase of the new gentle version will be to probe for\nsubmodules during git clean.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n---\n cache.h | 11 ++++++++-\n setup.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------\n 2 files changed, 75 insertions(+), 20 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 571c98f..25578cb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -446,7 +446,16 @@ extern int get_common_dir(struct strbuf *sb, const char *gitdir);\n extern const char *get_git_namespace(void);\n extern const char *strip_namespace(const char *namespaced_ref);\n extern const char *get_git_work_tree(void);\n-extern const char *read_gitfile(const char *path);\n+\n+#define READ_GITFILE_ERR_STAT_FAILED 1\n+#define READ_GITFILE_ERR_NOT_A_FILE 2\n+#define READ_GITFILE_ERR_OPEN_FAILED 3\n+#define READ_GITFILE_ERR_READ_FAILED 4\n+#define READ_GITFILE_ERR_INVALID_FORMAT 5\n+#define READ_GITFILE_ERR_NO_PATH 6\n+#define READ_GITFILE_ERR_NOT_A_REPO 7\n+extern const char *read_gitfile_gently(const char *path, int *return_error_code);\n+#define read_gitfile(path) read_gitfile_gently((path), NULL)\n extern const char *resolve_gitdir(const char *suspect);\n extern void set_git_work_tree(const char *tree);\n \ndiff --git a/setup.c b/setup.c\nindex 863ddfd..4748b63 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -406,35 +406,53 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n /*\n  * Try to read the location of the git directory from the .git file,\n  * return path to git directory if found.\n+ *\n+ * On failure, if return_error_code is not NULL, return_error_code\n+ * will be set to an error code and NULL will be returned. If\n+ * return_error_code is NULL the function will die instead (for most\n+ * cases).\n  */\n-const char *read_gitfile(const char *path)\n+const char *read_gitfile_gently(const char *path, int *return_error_code)\n {\n-\tchar *buf;\n-\tchar *dir;\n+\tint error_code = 0;\n+\tchar *buf = NULL;\n+\tchar *dir = NULL;\n \tconst char *slash;\n \tstruct stat st;\n \tint fd;\n \tssize_t len;\n \n-\tif (stat(path, &st))\n-\t\treturn NULL;\n-\tif (!S_ISREG(st.st_mode))\n-\t\treturn NULL;\n+\tif (stat(path, &st)) {\n+\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n+\t\tgoto cleanup_return;\n+\t}\n+\tif (!S_ISREG(st.st_mode)) {\n+\t\terror_code = READ_GITFILE_ERR_NOT_A_FILE;\n+\t\tgoto cleanup_return;\n+\t}\n \tfd = open(path, O_RDONLY);\n-\tif (fd < 0)\n-\t\tdie_errno(\"Error opening '%s'\", path);\n+\tif (fd < 0) {\n+\t\terror_code = READ_GITFILE_ERR_OPEN_FAILED;\n+\t\tgoto cleanup_return;\n+\t}\n \tbuf = xmalloc(st.st_size + 1);\n \tlen = read_in_full(fd, buf, st.st_size);\n \tclose(fd);\n-\tif (len != st.st_size)\n-\t\tdie(\"Error reading %s\", path);\n+\tif (len != st.st_size) {\n+\t\terror_code = READ_GITFILE_ERR_READ_FAILED;\n+\t\tgoto cleanup_return;\n+\t}\n \tbuf[len] = '\\0';\n-\tif (!starts_with(buf, \"gitdir: \"))\n-\t\tdie(\"Invalid gitfile format: %s\", path);\n+\tif (!starts_with(buf, \"gitdir: \")) {\n+\t\terror_code = READ_GITFILE_ERR_INVALID_FORMAT;\n+\t\tgoto cleanup_return;\n+\t}\n \twhile (buf[len - 1] == '\\n' || buf[len - 1] == '\\r')\n \t\tlen--;\n-\tif (len < 9)\n-\t\tdie(\"No path in gitfile: %s\", path);\n+\tif (len < 9) {\n+\t\terror_code = READ_GITFILE_ERR_NO_PATH;\n+\t\tgoto cleanup_return;\n+\t}\n \tbuf[len] = '\\0';\n \tdir = buf + 8;\n \n@@ -448,14 +466,42 @@ const char *read_gitfile(const char *path)\n \t\tfree(buf);\n \t\tbuf = dir;\n \t}\n-\n-\tif (!is_git_directory(dir))\n-\t\tdie(\"Not a git repository: %s\", dir);\n-\n+\tif (!is_git_directory(dir)) {\n+\t\terror_code = READ_GITFILE_ERR_NOT_A_REPO;\n+\t\tgoto cleanup_return;\n+\t}\n \tupdate_linked_gitdir(path, dir);\n \tpath = real_path(dir);\n \n+cleanup_return:\n \tfree(buf);\n+\n+\tif (return_error_code)\n+\t\t*return_error_code = error_code;\n+\n+\tif (error_code) {\n+\t\tif (return_error_code)\n+\t\t\treturn NULL;\n+\n+\t\tswitch (error_code) {\n+\t\tcase READ_GITFILE_ERR_STAT_FAILED:\n+\t\tcase READ_GITFILE_ERR_NOT_A_FILE:\n+\t\t\treturn NULL;\n+\t\tcase READ_GITFILE_ERR_OPEN_FAILED:\n+\t\t\tdie_errno(\"Error opening '%s'\", path);\n+\t\tcase READ_GITFILE_ERR_READ_FAILED:\n+\t\t\tdie(\"Error reading %s\", path);\n+\t\tcase READ_GITFILE_ERR_INVALID_FORMAT:\n+\t\t\tdie(\"Invalid gitfile format: %s\", path);\n+\t\tcase READ_GITFILE_ERR_NO_PATH:\n+\t\t\tdie(\"No path in gitfile: %s\", path);\n+\t\tcase READ_GITFILE_ERR_NOT_A_REPO:\n+\t\t\tdie(\"Not a git repository: %s\", dir);\n+\t\tdefault:\n+\t\t\tassert(0);\n+\t\t}\n+\t}\n+\n \treturn path;\n }\n \n-- \n2.4.3.373.gc496bfb\n"},{"id":"263394","messageId":"1433874279-30720-3-git-send-email-erik.elfstrom@gmail.com","threadId":"39576","inReplyTo":"1433874279-30720-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v7 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-06-09T18:24:36Z","receivedAt":"2015-06-09T18:24:36Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"read_gitfile_gently will allocate a buffer to fit the entire file that\nshould be read. Add a sanity check of the file size before opening to\navoid allocating a potentially huge amount of memory if we come across\na large file that someone happened to name \".git\". The limit is set to\na sufficiently unreasonable size that should never be exceeded by a\ngenuine .git file.\n\nSigned-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n---\n cache.h | 1 +\n setup.c | 7 +++++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 25578cb..858d9b3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -454,6 +454,7 @@ extern const char *get_git_work_tree(void);\n #define READ_GITFILE_ERR_INVALID_FORMAT 5\n #define READ_GITFILE_ERR_NO_PATH 6\n #define READ_GITFILE_ERR_NOT_A_REPO 7\n+#define READ_GITFILE_ERR_TOO_LARGE 8\n extern const char *read_gitfile_gently(const char *path, int *return_error_code);\n #define read_gitfile(path) read_gitfile_gently((path), NULL)\n extern const char *resolve_gitdir(const char *suspect);\ndiff --git a/setup.c b/setup.c\nindex 4748b63..e76955e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -414,6 +414,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n  */\n const char *read_gitfile_gently(const char *path, int *return_error_code)\n {\n+\tstatic const int one_MB = 1 << 20;\n \tint error_code = 0;\n \tchar *buf = NULL;\n \tchar *dir = NULL;\n@@ -430,6 +431,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \t\terror_code = READ_GITFILE_ERR_NOT_A_FILE;\n \t\tgoto cleanup_return;\n \t}\n+\tif (st.st_size > one_MB) {\n+\t\terror_code = READ_GITFILE_ERR_TOO_LARGE;\n+\t\tgoto cleanup_return;\n+\t}\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0) {\n \t\terror_code = READ_GITFILE_ERR_OPEN_FAILED;\n@@ -489,6 +494,8 @@ cleanup_return:\n \t\t\treturn NULL;\n \t\tcase READ_GITFILE_ERR_OPEN_FAILED:\n \t\t\tdie_errno(\"Error opening '%s'\", path);\n+\t\tcase READ_GITFILE_ERR_TOO_LARGE:\n+\t\t\tdie(\"Too large to be a .git file: '%s'\", path);\n \t\tcase READ_GITFILE_ERR_READ_FAILED:\n \t\t\tdie(\"Error reading %s\", path);\n \t\tcase READ_GITFILE_ERR_INVALID_FORMAT:\n-- \n2.4.3.373.gc496bfb\n"},{"id":"263395","messageId":"1433874279-30720-4-git-send-email-erik.elfstrom@gmail.com","threadId":"39576","inReplyTo":"1433874279-30720-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v7 3/5] t7300: add tests to document behavior of clean and nested git","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-06-09T18:24:37Z","receivedAt":"2015-06-09T18:24:37Z","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 | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 142 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 99be5d9..fbfdf2d 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -455,6 +455,148 @@ test_expect_success 'nested git work tree' '\n \t! test -d bar\n '\n \n+test_expect_failure 'should clean things that almost look like git but are not' '\n+\trm -fr almost_git almost_bare_git almost_submodule &&\n+\tmkdir -p almost_git/.git/objects &&\n+\tmkdir -p almost_git/.git/refs &&\n+\tcat >almost_git/.git/HEAD <<-\\EOF &&\n+\tgarbage\n+\tEOF\n+\tcp -r almost_git/.git/ almost_bare_git &&\n+\tmkdir almost_submodule/ &&\n+\tcat >almost_submodule/.git <<-\\EOF &&\n+\tgarbage\n+\tEOF\n+\ttest_when_finished \"rm -rf almost_*\" &&\n+\t## This will fail due to die(\"Invalid gitfile format: %s\", path); in\n+\t## setup.c:read_gitfile.\n+\tgit clean -f -d &&\n+\ttest_path_is_missing almost_git &&\n+\ttest_path_is_missing almost_bare_git &&\n+\ttest_path_is_missing almost_submodule\n+'\n+\n+test_expect_success 'should not clean submodules' '\n+\trm -fr repo to_clean sub1 sub2 &&\n+\tmkdir repo to_clean &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\t\ttest_commit msg hello.world\n+\t) &&\n+\tgit submodule add ./repo/.git sub1 &&\n+\tgit commit -m \"sub1\" &&\n+\tgit branch before_sub2 &&\n+\tgit submodule add ./repo/.git sub2 &&\n+\tgit commit -m \"sub2\" &&\n+\tgit checkout before_sub2 &&\n+\t>to_clean/should_clean.this &&\n+\tgit clean -f -d &&\n+\ttest_path_is_file repo/.git/index &&\n+\ttest_path_is_file repo/hello.world &&\n+\ttest_path_is_file sub1/.git &&\n+\ttest_path_is_file sub1/hello.world &&\n+\ttest_path_is_file sub2/.git &&\n+\ttest_path_is_file sub2/hello.world &&\n+\ttest_path_is_missing to_clean\n+'\n+\n+test_expect_failure 'should avoid cleaning possible submodules' '\n+\trm -fr to_clean possible_sub1 &&\n+\tmkdir to_clean possible_sub1 &&\n+\ttest_when_finished \"rm -rf possible_sub*\" &&\n+\techo \"gitdir: foo\" >possible_sub1/.git &&\n+\t>possible_sub1/hello.world &&\n+\tchmod 0 possible_sub1/.git &&\n+\t>to_clean/should_clean.this &&\n+\tgit clean -f -d &&\n+\ttest_path_is_file possible_sub1/.git &&\n+\ttest_path_is_file possible_sub1/hello.world &&\n+\ttest_path_is_missing to_clean\n+'\n+\n+test_expect_failure 'nested (empty) git should be kept' '\n+\trm -fr empty_repo to_clean &&\n+\tgit init empty_repo &&\n+\tmkdir to_clean &&\n+\t>to_clean/should_clean.this &&\n+\tgit clean -f -d &&\n+\ttest_path_is_file empty_repo/.git/HEAD &&\n+\ttest_path_is_missing to_clean\n+'\n+\n+test_expect_success 'nested bare repositories should be cleaned' '\n+\trm -fr bare1 bare2 subdir &&\n+\tgit init --bare bare1 &&\n+\tgit clone --local --bare . bare2 &&\n+\tmkdir subdir &&\n+\tcp -r bare2 subdir/bare3 &&\n+\tgit clean -f -d &&\n+\ttest_path_is_missing bare1 &&\n+\ttest_path_is_missing bare2 &&\n+\ttest_path_is_missing subdir\n+'\n+\n+test_expect_success 'nested (empty) bare repositories should be cleaned even when in .git' '\n+\trm -fr strange_bare &&\n+\tmkdir strange_bare &&\n+\tgit init --bare strange_bare/.git &&\n+\tgit clean -f -d &&\n+\ttest_path_is_missing strange_bare\n+'\n+\n+test_expect_failure 'nested (non-empty) bare repositories should be cleaned even when in .git' '\n+\trm -fr strange_bare &&\n+\tmkdir strange_bare &&\n+\tgit clone --local --bare . strange_bare/.git &&\n+\tgit clean -f -d &&\n+\ttest_path_is_missing strange_bare\n+'\n+\n+test_expect_success 'giving path in nested git work tree will remove it' '\n+\trm -fr repo &&\n+\tmkdir repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\t\tmkdir -p bar/baz &&\n+\t\ttest_commit msg bar/baz/hello.world\n+\t) &&\n+\tgit clean -f -d repo/bar/baz &&\n+\ttest_path_is_file repo/.git/HEAD &&\n+\ttest_path_is_dir repo/bar/ &&\n+\ttest_path_is_missing repo/bar/baz\n+'\n+\n+test_expect_success 'giving path to nested .git will not remove it' '\n+\trm -fr repo &&\n+\tmkdir repo untracked &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\t\ttest_commit msg hello.world\n+\t) &&\n+\tgit clean -f -d repo/.git &&\n+\ttest_path_is_file repo/.git/HEAD &&\n+\ttest_path_is_dir repo/.git/refs &&\n+\ttest_path_is_dir repo/.git/objects &&\n+\ttest_path_is_dir untracked/\n+'\n+\n+test_expect_success 'giving path to nested .git/ will remove contents' '\n+\trm -fr repo untracked &&\n+\tmkdir repo untracked &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit init &&\n+\t\ttest_commit msg hello.world\n+\t) &&\n+\tgit clean -f -d repo/.git/ &&\n+\ttest_path_is_dir repo/.git &&\n+\ttest_dir_is_empty repo/.git &&\n+\ttest_path_is_dir untracked/\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.3.373.gc496bfb\n"},{"id":"263396","messageId":"1433874279-30720-5-git-send-email-erik.elfstrom@gmail.com","threadId":"39576","inReplyTo":"1433874279-30720-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v7 4/5] p7300: add performance tests for clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-06-09T18:24:38Z","receivedAt":"2015-06-09T18:24:38Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"The tests are run in dry-run mode to avoid having to restore the test\ndirectories for each timed iteration. Using dry-run is an acceptable\ncompromise since we are mostly interested in the initial computation\nof what to clean and not so much in the cleaning it self.\n\nSigned-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n---\n t/perf/p7300-clean.sh | 31 +++++++++++++++++++++++++++++++\n 1 file changed, 31 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..ec94cdd\n--- /dev/null\n+++ b/t/perf/p7300-clean.sh\n@@ -0,0 +1,31 @@\n+#!/bin/sh\n+\n+test_description=\"Test git-clean performance\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_default_repo\n+test_checkout_worktree\n+\n+test_expect_success 'setup untracked directory with many sub dirs' '\n+\trm -rf 500_sub_dirs 100000_sub_dirs clean_test_dir &&\n+\tmkdir 500_sub_dirs 100000_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 200)\n+\tdo\n+\t\tcp -r 500_sub_dirs 100000_sub_dirs/dir$i || return $?\n+\tdone\n+'\n+\n+test_perf 'clean many untracked sub dirs, check for nested git' '\n+\tgit clean -n -q -f -d 100000_sub_dirs/\n+'\n+\n+test_perf 'clean many untracked sub dirs, ignore nested git' '\n+\tgit clean -n -q -f -f -d 100000_sub_dirs/\n+'\n+\n+test_done\n-- \n2.4.3.373.gc496bfb\n"},{"id":"263407","messageId":"1433874279-30720-6-git-send-email-erik.elfstrom@gmail.com","threadId":"39576","inReplyTo":"1433874279-30720-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v7 5/5] clean: improve performance when removing lots of directories","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-06-09T18:24:39Z","receivedAt":"2015-06-09T18:24:39Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"\"git clean\" uses resolve_gitlink_ref() to check for the presence of\nnested git repositories, but it has the drawback of creating a\nref_cache entry for every directory that should potentially be\ncleaned. The linear search through the ref_cache list causes a massive\nperformance hit for large number of directories.\n\nModify clean.c:remove_dirs to use setup.c:is_git_directory and\nsetup.c:read_gitfile_gently instead.\n\nBoth these functions will open files and parse contents when they find\nsomething that looks like a git repository. This is ok from a\nperformance standpoint since finding repository candidates should be\ncomparatively rare.\n\nUsing is_git_directory and read_gitfile_gently should give a more\nstandardized check for what is and what isn't a git repository but\nalso gives three behavioral changes.\n\nThe first change is that we will now detect and avoid cleaning empty\nnested git repositories (only init run). This is desirable.\n\nSecond, we will no longer die when cleaning a file named \".git\" with\ngarbage content (it will be cleaned instead). This is also desirable.\n\nThe last change is that we will detect and avoid cleaning empty bare\nrepositories that have been placed in a directory named \".git\". This\nis not desirable but should have no real user impact since we already\nfail to clean non-empty bare repositories in the same scenario. This\nis thus deemed acceptable.\n\nOn top of this we add some extra precautions. If read_gitfile_gently\nfails to open the git file, read the git file or verify the path in\nthe git file we assume that the path with the git file is a valid\nrepository and avoid cleaning.\n\nUpdate t7300 to reflect these changes in behavior.\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  | 30 ++++++++++++++++++++++++++----\n t/t7300-clean.sh | 10 ++++------\n 2 files changed, 30 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 6dcb72e..df53def 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,31 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+/*\n+ * Return 1 if the given path is the root of a git repository or\n+ * submodule else 0. Will not return 1 for bare repositories with the\n+ * exception of creating a bare repository in \"foo/.git\" and calling\n+ * is_git_repository(\"foo\").\n+ */\n+static int is_git_repository(struct strbuf *path)\n+{\n+\tint ret = 0;\n+\tint gitfile_error;\n+\tsize_t orig_path_len = path->len;\n+\tassert(orig_path_len != 0);\n+\tif (path->buf[orig_path_len - 1] != '/')\n+\t\tstrbuf_addch(path, '/');\n+\tstrbuf_addstr(path, \".git\");\n+\tif (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))\n+\t\tret = 1;\n+\tif (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||\n+\t    gitfile_error == READ_GITFILE_ERR_READ_FAILED)\n+\t\tret = 1;  /* This could be a real .git file, take the\n+\t\t\t   * safe option and avoid cleaning */\n+\tstrbuf_setlen(path, orig_path_len);\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 +179,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 fbfdf2d..32e96da 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 'should clean things that almost look like git but are not' '\n+test_expect_success 'should clean things that almost look like git but are not' '\n \trm -fr almost_git almost_bare_git almost_submodule &&\n \tmkdir -p almost_git/.git/objects &&\n \tmkdir -p almost_git/.git/refs &&\n@@ -468,8 +468,6 @@ test_expect_failure 'should clean things that almost look like git but are not'\n \tgarbage\n \tEOF\n \ttest_when_finished \"rm -rf almost_*\" &&\n-\t## This will fail due to die(\"Invalid gitfile format: %s\", path); in\n-\t## setup.c:read_gitfile.\n \tgit clean -f -d &&\n \ttest_path_is_missing almost_git &&\n \ttest_path_is_missing almost_bare_git &&\n@@ -501,7 +499,7 @@ test_expect_success 'should not clean submodules' '\n \ttest_path_is_missing to_clean\n '\n \n-test_expect_failure 'should avoid cleaning possible submodules' '\n+test_expect_success 'should avoid cleaning possible submodules' '\n \trm -fr to_clean possible_sub1 &&\n \tmkdir to_clean possible_sub1 &&\n \ttest_when_finished \"rm -rf possible_sub*\" &&\n@@ -515,7 +513,7 @@ test_expect_failure 'should avoid cleaning possible submodules' '\n \ttest_path_is_missing to_clean\n '\n \n-test_expect_failure 'nested (empty) git should be kept' '\n+test_expect_success 'nested (empty) git should be kept' '\n \trm -fr empty_repo to_clean &&\n \tgit init empty_repo &&\n \tmkdir to_clean &&\n@@ -537,7 +535,7 @@ test_expect_success 'nested bare repositories should be cleaned' '\n \ttest_path_is_missing subdir\n '\n \n-test_expect_success 'nested (empty) bare repositories should be cleaned even when in .git' '\n+test_expect_failure 'nested (empty) bare repositories should be cleaned even when in .git' '\n \trm -fr strange_bare &&\n \tmkdir strange_bare &&\n \tgit init --bare strange_bare/.git &&\n-- \n2.4.3.373.gc496bfb\n"},{"id":"263779","messageId":"CAPig+cTQKxD9WRV3spVce=SfeECo4XR_ujZ1sGecuhTL-3DeRw@mail.gmail.com","threadId":"39576","inReplyTo":"1433874279-30720-3-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v7 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-14T03:42:58Z","receivedAt":"2015-06-14T03:42:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 9, 2015 at 2:24 PM, Erik Elfström <erik.elfstrom@gmail.com> wrote:\n> read_gitfile_gently will allocate a buffer to fit the entire file that\n> should be read. Add a sanity check of the file size before opening to\n> avoid allocating a potentially huge amount of memory if we come across\n> a large file that someone happened to name \".git\". The limit is set to\n> a sufficiently unreasonable size that should never be exceeded by a\n> genuine .git file.\n>\n> Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>\n> ---\n> diff --git a/setup.c b/setup.c\n> index 4748b63..e76955e 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -414,6 +414,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir)\n>   */\n>  const char *read_gitfile_gently(const char *path, int *return_error_code)\n>  {\n> +       static const int one_MB = 1 << 20;\n\nThis variable name doesn't convey much about its purpose, and\nintroduces a bit of maintenance burden if the limit is some day\nchanged. Perhaps \"sane_size_limit\" or something even more descriptive\n(and/or terse) would be better.\n\n>         int error_code = 0;\n>         char *buf = NULL;\n>         char *dir = NULL;\n> @@ -430,6 +431,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n>                 error_code = READ_GITFILE_ERR_NOT_A_FILE;\n>                 goto cleanup_return;\n>         }\n> +       if (st.st_size > one_MB) {\n> +               error_code = READ_GITFILE_ERR_TOO_LARGE;\n> +               goto cleanup_return;\n> +       }\n>         fd = open(path, O_RDONLY);\n>         if (fd < 0) {\n>                 error_code = READ_GITFILE_ERR_OPEN_FAILED;\n> @@ -489,6 +494,8 @@ cleanup_return:\n>                         return NULL;\n>                 case READ_GITFILE_ERR_OPEN_FAILED:\n>                         die_errno(\"Error opening '%s'\", path);\n> +               case READ_GITFILE_ERR_TOO_LARGE:\n> +                       die(\"Too large to be a .git file: '%s'\", path);\n>                 case READ_GITFILE_ERR_READ_FAILED:\n>                         die(\"Error reading %s\", path);\n>                 case READ_GITFILE_ERR_INVALID_FORMAT:\n> --\n> 2.4.3.373.gc496bfb\n"},{"id":"263817","messageId":"CAMpP7NZ2tqZ5cv=9=gJqDZ5m0LOZtsHGvt-ntZcp6cBA6X5tqg@mail.gmail.com","threadId":"39576","inReplyTo":"CAPig+cTQKxD9WRV3spVce=SfeECo4XR_ujZ1sGecuhTL-3DeRw@mail.gmail.com","subject":"Re: [PATCH v7 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-06-14T17:21:44Z","receivedAt":"2015-06-14T17:21:44Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Sun, Jun 14, 2015 at 5:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> This variable name doesn't convey much about its purpose, and\n> introduces a bit of maintenance burden if the limit is some day\n> changed. Perhaps \"sane_size_limit\" or something even more descriptive\n> (and/or terse) would be better.\n>\n\nWould you be happy with this change?\n\n-       static const int one_MB = 1 << 20;\n+       static const int max_file_size = 1 << 20;  /* 1MB */\n"},{"id":"263834","messageId":"CAPig+cRouQOgd29qQZSUo0t+NBPfeKWD+4j42pArLW3TpND=bQ@mail.gmail.com","threadId":"39576","inReplyTo":"CAMpP7NZ2tqZ5cv=9=gJqDZ5m0LOZtsHGvt-ntZcp6cBA6X5tqg@mail.gmail.com","subject":"Re: [PATCH v7 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-15T03:08:44Z","receivedAt":"2015-06-15T03:08:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jun 14, 2015 at 1:21 PM, erik elfström <erik.elfstrom@gmail.com> wrote:\n> On Sun, Jun 14, 2015 at 5:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>\n>> This variable name doesn't convey much about its purpose, and\n>> introduces a bit of maintenance burden if the limit is some day\n>> changed. Perhaps \"sane_size_limit\" or something even more descriptive\n>> (and/or terse) would be better.\n>\n> Would you be happy with this change?\n>\n> -       static const int one_MB = 1 << 20;\n> +       static const int max_file_size = 1 << 20;  /* 1MB */\n\nYep, that's a much nicer variable name. Thanks.\n\nI also note that 'const int' shows up pretty frequently in the git\nsource code, but 'static const int' is used only very rarely.\n"}]}