{"thread":{"id":"39291","subject":"[PATCH v6 1/7] setup: add gentle version of is_git_directory","startedAt":"2015-05-10T20:00:34Z","lastAt":"2015-05-12T06:46:08Z","messageCount":9,"participants":["Erik Elfström","erik elfström"],"isPatch":true,"patchVersion":6,"patchTotal":7},"messages":[{"id":"260936","messageId":"1431288041-21077-1-git-send-email-erik.elfstrom@gmail.com","threadId":"39291","inReplyTo":null,"subject":"[PATCH v6 0/7] Improving performance of git clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-05-10T20:00:34Z","receivedAt":"2015-05-10T20:00:34Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Here is v6 of of this series. v5 can be found at:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/267823\n\nSorry for the slow progress on this, I've been busy with other things.\n\nChanges in v6:\n* added gentle version of is_git_directory and used it in\n read_gitfile_gently\n* use 1MB as size limit for read_gitfile_gently instead of\n PATH_MAX*4\n* fixed file descriptor leak in read_gitfile_gently\n* avoid cleaning if we can't open, read or validate the path in a git\n file (we used to die on these cases).\n* added one more testcase to cover the behavior mention above.\n* switched to default repo in performance test\n\n\nErik Elfström (7):\n setup: add gentle version of is_git_directory\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 RFC: Change error handling scheme in read_gitfile_gently\n\n builtin/clean.c | 32 ++++++++--\n cache.h | 16 +++++\n setup.c | 158 +++++++++++++++++++++++++++++++++++++++++++-------\n t/perf/p7300-clean.sh | 31 ++++++++++\n t/t7300-clean.sh | 144 +++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 355 insertions(+), 26 deletions(-)\n create mode 100755 t/perf/p7300-clean.sh\n\n-- \n2.4.0.60.gf7143f7\n"},{"id":"260935","messageId":"1431288041-21077-2-git-send-email-erik.elfstrom@gmail.com","threadId":"39291","inReplyTo":"1431288041-21077-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v6 1/7] setup: add gentle version of is_git_directory","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-05-10T20:00:35Z","receivedAt":"2015-05-10T20:00:35Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"This is a prerequisite for implementing a gentle version of\nread_gitfile.\n\nSigned-off-by: Erik Elfström \n---\n cache.h | 4 ++++\n setup.c | 44 +++++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 45 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b34447f..dd67695 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -431,7 +431,11 @@ extern int is_inside_git_dir(void);\n extern char *git_work_tree_cfg;\n extern int is_inside_work_tree(void);\n extern const char *get_git_dir(void);\n+\n+#define IS_GIT_DIRECTORY_ERR_PATH_TOO_LONG 1\n+extern int is_git_directory_gently(const char *path, int *return_err, struct strbuf *err_msg);\n extern int is_git_directory(const char *path);\n+\n extern char *get_object_directory(void);\n extern char *get_index_file(void);\n extern char *get_graft_file(void);\ndiff --git a/setup.c b/setup.c\nindex 979b13f..62ee88c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -224,6 +224,18 @@ void verify_non_filename(const char *prefix, const char *arg)\n \t \"'git [...] -- [...]'\", arg);\n }\n \n+__attribute((format (printf,4,5)))\n+static void set_error(int *return_err, struct strbuf *err_msg, int err,\n+\t\t const char *msg, ...)\n+{\n+\tva_list params;\n+\tva_start(params, msg);\n+\tif (err_msg)\n+\t\tstrbuf_vaddf(err_msg, msg, params);\n+\tva_end(params);\n+\tif (return_err)\n+\t\t*return_err = err;\n+}\n \n /*\n * Test if it looks like we're at a git directory.\n@@ -235,14 +247,28 @@ void verify_non_filename(const char *prefix, const char *arg)\n * - either a HEAD symlink or a HEAD file that is formatted as\n * a proper \"ref:\", or a regular file HEAD that has a properly\n * formatted sha1 object name.\n+ *\n+ * In the event of an error, return_err will be set to an error code\n+ * and err_msg will be set to an error message describing the error\n+ * and 0 will be returned. If no error reporting is required, pass\n+ * NULL for return_err and/or err_msg.\n */\n-int is_git_directory(const char *suspect)\n+int is_git_directory_gently(const char *suspect, int *return_err,\n+\t\t\t struct strbuf *err_msg)\n {\n \tchar path[PATH_MAX];\n \tsize_t len = strlen(suspect);\n \n-\tif (PATH_MAX <= len + strlen(\"/objects\"))\n-\t\tdie(\"Too long path: %.*s\", 60, suspect);\n+\tif (return_err)\n+\t\t*return_err = 0;\n+\n+\tif (PATH_MAX <= len + strlen(\"/objects\")) {\n+\t\tset_error(return_err, err_msg,\n+\t\t\t IS_GIT_DIRECTORY_ERR_PATH_TOO_LONG,\n+\t\t\t \"Too long path: %.*s\", 60, suspect);\n+\t\treturn 0;\n+\t}\n+\n \tstrcpy(path, suspect);\n \tif (getenv(DB_ENVIRONMENT)) {\n \t\tif (access(getenv(DB_ENVIRONMENT), X_OK))\n@@ -265,6 +291,18 @@ int is_git_directory(const char *suspect)\n \treturn 1;\n }\n \n+int is_git_directory(const char *suspect)\n+{\n+\tint err;\n+\tint ret;\n+\tstruct strbuf err_msg = STRBUF_INIT;\n+\tret = is_git_directory_gently(suspect, &err, &err_msg);\n+\tif (err)\n+\t\tdie(\"%s\", err_msg.buf);\n+\t/* No need to free err_msg, will only be touched in case of error */\n+\treturn ret;\n+}\n+\n int is_inside_git_dir(void)\n {\n \tif (inside_git_dir < 0)\n-- \n2.4.0.60.gf7143f7\n"},{"id":"260937","messageId":"1431288041-21077-3-git-send-email-erik.elfstrom@gmail.com","threadId":"39291","inReplyTo":"1431288041-21077-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v6 2/7] setup: add gentle version of read_gitfile","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-05-10T20:00:36Z","receivedAt":"2015-05-10T20:00:36Z","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 \nHelped-by: Jeff King \nSigned-off-by: Erik Elfström \n---\n cache.h | 12 ++++++++-\n setup.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++------------\n 2 files changed, 86 insertions(+), 18 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex dd67695..54c902b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -443,7 +443,17 @@ extern int set_git_dir(const char *path);\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_CANT_VERIFY_PATH 7\n+#define READ_GITFILE_ERR_NOT_A_REPO 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);\n extern void set_git_work_tree(const char *tree);\n \ndiff --git a/setup.c b/setup.c\nindex 62ee88c..b919ea6 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -373,35 +373,55 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\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+\tint is_git_dir;\n+\tstruct strbuf err_msg = STRBUF_INIT;\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@@ -416,11 +436,49 @@ const char *read_gitfile(const char *path)\n \t\tbuf = dir;\n \t}\n \n-\tif (!is_git_directory(dir))\n-\t\tdie(\"Not a git repository: %s\", dir);\n+\tis_git_dir = is_git_directory_gently(dir, &error_code, &err_msg);\n+\tif (error_code) {\n+\t\terror_code = READ_GITFILE_ERR_CANT_VERIFY_PATH;\n+\t\tgoto cleanup_return;\n+\t}\n+\tif (!is_git_dir) {\n+\t\terror_code = READ_GITFILE_ERR_NOT_A_REPO;\n+\t\tgoto cleanup_return;\n+\t}\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_CANT_VERIFY_PATH:\n+\t\t\tdie(\"%s\", err_msg.buf);\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+\tstrbuf_release(&err_msg);\n \treturn path;\n }\n \n-- \n2.4.0.60.gf7143f7\n"},{"id":"260938","messageId":"1431288041-21077-4-git-send-email-erik.elfstrom@gmail.com","threadId":"39291","inReplyTo":"1431288041-21077-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v6 3/7] setup: sanity check file size in read_gitfile_gently","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-05-10T20:00:37Z","receivedAt":"2015-05-10T20:00:37Z","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 \n---\n cache.h | 1 +\n setup.c | 8 ++++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 54c902b..7c8abcb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -452,6 +452,7 @@ extern const char *get_git_work_tree(void);\n #define READ_GITFILE_ERR_NO_PATH 6\n #define READ_GITFILE_ERR_CANT_VERIFY_PATH 7\n #define READ_GITFILE_ERR_NOT_A_REPO 8\n+#define READ_GITFILE_ERR_TOO_LARGE 9\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 b919ea6..bfaf4a6 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -381,6 +381,7 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\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@@ -404,6 +405,11 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \t\terror_code = READ_GITFILE_ERR_OPEN_FAILED;\n \t\tgoto cleanup_return;\n \t}\n+\tif (st.st_size > one_MB) {\n+\t\tclose(fd);\n+\t\terror_code = READ_GITFILE_ERR_TOO_LARGE;\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@@ -463,6 +469,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.0.60.gf7143f7\n"},{"id":"260939","messageId":"1431288041-21077-5-git-send-email-erik.elfstrom@gmail.com","threadId":"39291","inReplyTo":"1431288041-21077-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v6 4/7] t7300: add tests to document behavior of clean and nested git","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-05-10T20:00:38Z","receivedAt":"2015-05-10T20:00:38Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Signed-off-by: Erik Elfström \n---\n t/t7300-clean.sh | 146 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 146 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 99be5d9..23962e4 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -455,6 +455,152 @@ 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 possible_sub2 &&\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+\tcp -r possible_sub1 possible_sub2 &&\n+\tprintf \"%*s\\n\" 5000 | tr \" \" a >> possible_sub1/.git &&\n+\tchmod 0 possible_sub2/.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_file possible_sub2/.git &&\n+\ttest_path_is_file possible_sub2/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.0.60.gf7143f7\n"},{"id":"260940","messageId":"1431288041-21077-6-git-send-email-erik.elfstrom@gmail.com","threadId":"39291","inReplyTo":"1431288041-21077-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v6 5/7] p7300: add performance tests for clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-05-10T20:00:39Z","receivedAt":"2015-05-10T20:00:39Z","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 \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.0.60.gf7143f7\n"},{"id":"260941","messageId":"1431288041-21077-7-git-send-email-erik.elfstrom@gmail.com","threadId":"39291","inReplyTo":"1431288041-21077-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v6 6/7] clean: improve performance when removing lots of directories","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-05-10T20:00:40Z","receivedAt":"2015-05-10T20:00:40Z","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 \nSigned-off-by: Erik Elfström \n---\n builtin/clean.c | 31 +++++++++++++++++++++++++++----\n t/t7300-clean.sh | 10 ++++------\n 2 files changed, 31 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 98c103f..d739dcf 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,32 @@ 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 gitfile_error == READ_GITFILE_ERR_CANT_VERIFY_PATH)\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 +180,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, "ed);\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 23962e4..fbab888 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 possible_sub2 &&\n \tmkdir to_clean possible_sub1 &&\n \ttest_when_finished \"rm -rf possible_sub*\" &&\n@@ -519,7 +517,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@@ -541,7 +539,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.0.60.gf7143f7\n"},{"id":"260942","messageId":"1431288041-21077-8-git-send-email-erik.elfstrom@gmail.com","threadId":"39291","inReplyTo":"1431288041-21077-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v6 7/7] RFC: Change error handling scheme in read_gitfile_gently","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-05-10T20:00:41Z","receivedAt":"2015-05-10T20:00:41Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Signed-off-by: Erik Elfström \n---\n\nSince there was a lot of discussion on error reporting strategy on\nthe previous patch I have done a quick prototype of the theme\nproposed by Jonathan Nieder.\n\nI believe the conclusion was to NOT go this route but this way people\nget to see an example of what it could look like to make the\ndiscussion and decision a bit easier.\n\nI will either drop this patch or split it up and squash it into the\nappropriate commits (along with change requests if any) depending on\nthe outcome of the review discussion.\n\n builtin/clean.c | 3 +-\n cache.h | 5 +--\n setup.c | 106 +++++++++++++++++++++++++++++++-------------------------\n 3 files changed, 63 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex d739dcf..7047d6e 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -162,7 +162,8 @@ static int is_git_repository(struct strbuf *path)\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+\tif (read_gitfile_gently(path->buf, &gitfile_error, NULL) ||\n+\t 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 ||\ndiff --git a/cache.h b/cache.h\nindex 7c8abcb..76d311a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -453,8 +453,9 @@ extern const char *get_git_work_tree(void);\n #define READ_GITFILE_ERR_CANT_VERIFY_PATH 7\n #define READ_GITFILE_ERR_NOT_A_REPO 8\n #define READ_GITFILE_ERR_TOO_LARGE 9\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 *read_gitfile_gently(const char *path, int *return_err, struct strbuf *err_msg);\n+extern const char *read_gitfile(const char *path);\n+\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 bfaf4a6..49274b3 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -374,15 +374,16 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\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+ * In the event of an error, return_err will be set to an error code\n+ * and err_msg will be set to an error message describing the error\n+ * and NULL will be returned. If no error reporting is required, pass\n+ * NULL for return_err and/or err_msg.\n */\n-const char *read_gitfile_gently(const char *path, int *return_error_code)\n+const char *read_gitfile_gently(const char *path, int *return_err,\n+\t\t\t\tstruct strbuf *err_msg)\n {\n \tstatic const int one_MB = 1 << 20;\n-\tint error_code = 0;\n+\tconst char *ret = NULL;\n \tchar *buf = NULL;\n \tchar *dir = NULL;\n \tconst char *slash;\n@@ -390,42 +391,59 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \tint fd;\n \tssize_t len;\n \tint is_git_dir;\n-\tstruct strbuf err_msg = STRBUF_INIT;\n+\tint is_git_dir_err;\n+\n+\tif (return_err)\n+\t\t*return_err = 0;\n \n \tif (stat(path, &st)) {\n-\t\terror_code = READ_GITFILE_ERR_STAT_FAILED;\n+\t\tset_error(return_err, err_msg,\n+\t\t\t READ_GITFILE_ERR_STAT_FAILED,\n+\t\t\t \"Could not stat: '%s'\", path);\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\tset_error(return_err, err_msg,\n+\t\t\t READ_GITFILE_ERR_NOT_A_FILE,\n+\t\t\t \"Not a file: '%s'\", path);\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+\t\tset_error(return_err, err_msg,\n+\t\t\t READ_GITFILE_ERR_OPEN_FAILED,\n+\t\t\t \"Error opening '%s'\", path);\n \t\tgoto cleanup_return;\n \t}\n \tif (st.st_size > one_MB) {\n \t\tclose(fd);\n-\t\terror_code = READ_GITFILE_ERR_TOO_LARGE;\n+\t\tset_error(return_err, err_msg,\n+\t\t\t READ_GITFILE_ERR_TOO_LARGE,\n+\t\t\t \"Too large to be a .git file: '%s'\", path);\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\terror_code = READ_GITFILE_ERR_READ_FAILED;\n+\t\tset_error(return_err, err_msg,\n+\t\t\t READ_GITFILE_ERR_READ_FAILED,\n+\t\t\t \"Error reading %s\", path);\n \t\tgoto cleanup_return;\n \t}\n \tbuf[len] = '\\0';\n \tif (!starts_with(buf, \"gitdir: \")) {\n-\t\terror_code = READ_GITFILE_ERR_INVALID_FORMAT;\n+\t\tset_error(return_err, err_msg,\n+\t\t\t READ_GITFILE_ERR_INVALID_FORMAT,\n+\t\t\t \"Invalid gitfile format: %s\", path);\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\terror_code = READ_GITFILE_ERR_NO_PATH;\n+\t\tset_error(return_err, err_msg,\n+\t\t\t READ_GITFILE_ERR_NO_PATH,\n+\t\t\t \"No path in gitfile: %s\", path);\n \t\tgoto cleanup_return;\n \t}\n \tbuf[len] = '\\0';\n@@ -442,52 +460,44 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \t\tbuf = dir;\n \t}\n \n-\tis_git_dir = is_git_directory_gently(dir, &error_code, &err_msg);\n-\tif (error_code) {\n-\t\terror_code = READ_GITFILE_ERR_CANT_VERIFY_PATH;\n+\tis_git_dir = is_git_directory_gently(dir, &is_git_dir_err, err_msg);\n+\tif (is_git_dir_err) {\n+\t\tif (return_err)\n+\t\t\t*return_err = READ_GITFILE_ERR_CANT_VERIFY_PATH;\n \t\tgoto cleanup_return;\n \t}\n \tif (!is_git_dir) {\n-\t\terror_code = READ_GITFILE_ERR_NOT_A_REPO;\n+\t\tset_error(return_err, err_msg,\n+\t\t\t READ_GITFILE_ERR_NOT_A_REPO,\n+\t\t\t \"Not a git repository: %s\", dir);\n \t\tgoto cleanup_return;\n \t}\n-\tpath = real_path(dir);\n+\tret = real_path(dir);\n \n cleanup_return:\n \tfree(buf);\n+\treturn ret;\n+}\n \n-\tif (return_error_code)\n-\t\t*return_error_code = error_code;\n+const char *read_gitfile(const char *path)\n+{\n+\tint err;\n+\tconst char *ret;\n+\tstruct strbuf err_msg = STRBUF_INIT;\n \n-\tif (error_code) {\n-\t\tif (return_error_code)\n-\t\t\treturn NULL;\n+\tret = read_gitfile_gently(path, &err, &err_msg);\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_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-\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_CANT_VERIFY_PATH:\n-\t\t\tdie(\"%s\", err_msg.buf);\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+\tswitch (err) {\n+\tcase 0: /* No need to free err_msg, will only be\n+\t\t * touched in case of error */\n+\t\treturn ret;\n+\tcase READ_GITFILE_ERR_STAT_FAILED:\n+\tcase READ_GITFILE_ERR_NOT_A_FILE:\n+\t\tstrbuf_release(&err_msg);\n+\t\treturn NULL;\n+\tdefault:\n+\t\tdie(\"%s\", err_msg.buf);\n \t}\n-\n-\tstrbuf_release(&err_msg);\n-\treturn path;\n }\n \n static const char *setup_explicit_git_dir(const char *gitdirenv,\n-- \n2.4.0.60.gf7143f7\n"},{"id":"261040","messageId":"CAMpP7NYEqTeVm3ybPmV7TvRkj5MjRYvD0J76ezXi9_jQsFterg@mail.gmail.com","threadId":"39291","inReplyTo":"1431288041-21077-4-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v6 3/7] setup: sanity check file size in read_gitfile_gently","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-05-12T06:46:08Z","receivedAt":"2015-05-12T06:46:08Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Sun, May 10, 2015 at 10:00 PM, Erik Elfström wrote:\n> @@ -404,6 +405,11 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> error_code = READ_GITFILE_ERR_OPEN_FAILED;\n> goto cleanup_return;\n> }\n> + if (st.st_size > one_MB) {\n> + close(fd);\n> + error_code = READ_GITFILE_ERR_TOO_LARGE;\n> + goto cleanup_return;\n> + }\n\nHmm... The order should probably be changed here. It would make more\nsense to check the size before opening the file. That way the error\nhandling in clean would be more consistent if we can't open a large\n.git file.\n\nRight now we would treat any file that we can't open as a potential\nrepository and avoid cleaning but if we can open it and it turns out\nthat it is larger than 1MB we will ignore it and clean. By switching\nthe order here we would always ignore files larger than 1MB regardless\nof if we can open them or not and I think that would make more sense.\nIt would also remove the need to close the file when erroring out due\nto size so it makes more sense from a pure structural point of view as\nwell.\n\nSorry for not thinking of this earlier.\n\n/Erik\n"}]}