{"thread":{"id":"39159","subject":"[PATCH v4 1/5] setup: add gentle version of read_gitfile","startedAt":"2015-04-25T09:06:36Z","lastAt":"2015-04-30T01:35:09Z","messageCount":38,"participants":["Erik Elfström","Junio C Hamano","Jeff King","Johannes Schindelin","Doug Kelly","erik elfström","Jonathan Nieder","Stefan Beller"],"isPatch":true,"patchVersion":4,"patchTotal":5},"messages":[{"id":"259989","messageId":"1429952801-2646-1-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":null,"subject":"[PATCH v4 0/5] Improving performance of git clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-25T09:06:36Z","receivedAt":"2015-04-25T09:06:36Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"v3 of the patch can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/267422\n\nChanges in v4:\n* changed some tests to use more meaningful dir names.\n* fixed performance test by doing \"git clean -n\" to avoid\n timing setup code. Increased test size to 100000 directories\n (~0.5s runtime).\n* changed interface of read_gitfile_gently to be able to\n return error code.\n* fixed a compiler warning in read_gitfile_gently (\"warning:\n ‘dir’ may be used uninitialized in this function\").\n* added sanity check of git file size in read_gitfile_gently\n* updated commit message in [5/5] to more clearly motivate\n remaining behavioral changes of git clean.\n\nThanks to Junio C Hamano and Jeff King for comments and help on v3.\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 | 26 +++++++++--\n cache.h | 3 +-\n setup.c | 88 ++++++++++++++++++++++++++++-------\n t/perf/p7300-clean.sh | 31 +++++++++++++\n t/t7300-clean.sh | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 252 insertions(+), 22 deletions(-)\n create mode 100755 t/perf/p7300-clean.sh\n\n-- \n2.4.0.rc3.8.g4ebd28d\n"},{"id":"259987","messageId":"1429952801-2646-2-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"1429952801-2646-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v4 1/5] setup: add gentle version of read_gitfile","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-25T09:06:37Z","receivedAt":"2015-04-25T09:06:37Z","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 | 3 ++-\n setup.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++--------------\n 2 files changed, 67 insertions(+), 18 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3d3244b..6e29068 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -431,7 +431,8 @@ 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+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 979b13f..e1897cc 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -335,35 +335,53 @@ 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 \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 = 1;\n+\t\tgoto cleanup_return;\n+\t}\n+\tif (!S_ISREG(st.st_mode)) {\n+\t\terror_code = 2;\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 = 3;\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 = 4;\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 = 5;\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 = 6;\n+\t\tgoto cleanup_return;\n+\t}\n \tbuf[len] = '\\0';\n \tdir = buf + 8;\n \n@@ -378,11 +396,41 @@ 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+\tif (!is_git_directory(dir)) {\n+\t\terror_code = 7;\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 1: // failed to stat\n+\t\tcase 2: // not regular file\n+\t\t\treturn NULL;\n+\t\tcase 3:\n+\t\t\tdie_errno(\"Error opening '%s'\", path);\n+\t\tcase 4:\n+\t\t\tdie(\"Error reading %s\", path);\n+\t\tcase 5:\n+\t\t\tdie(\"Invalid gitfile format: %s\", path);\n+\t\tcase 6:\n+\t\t\tdie(\"No path in gitfile: %s\", path);\n+\t\tcase 7:\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.0.rc3.8.g4ebd28d\n"},{"id":"259988","messageId":"1429952801-2646-3-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"1429952801-2646-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v4 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-25T09:06:38Z","receivedAt":"2015-04-25T09:06:38Z","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\nI'm not sure about this one but it felt like the safe thing to do.\nThis patch can be dropped if it is not desired.\n\nI considered testing it using\n \"mkdir foo && truncate -s 200G foo/.git && git clean -f -d\"\nbut that feels like a pretty evil test that is likely to cause lots\nof problems and not fail in any good way.\n\n setup.c | 20 +++++++++++++-------\n 1 file changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex e1897cc..ed87334 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -364,22 +364,26 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \t\terror_code = 3;\n \t\tgoto cleanup_return;\n \t}\n+\tif (st.st_size > PATH_MAX * 4) {\n+\t\terror_code = 4;\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 = 4;\n+\t\terror_code = 5;\n \t\tgoto cleanup_return;\n \t}\n \tbuf[len] = '\\0';\n \tif (!starts_with(buf, \"gitdir: \")) {\n-\t\terror_code = 5;\n+\t\terror_code = 6;\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 = 6;\n+\t\terror_code = 7;\n \t\tgoto cleanup_return;\n \t}\n \tbuf[len] = '\\0';\n@@ -397,7 +401,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \t}\n \n \tif (!is_git_directory(dir)) {\n-\t\terror_code = 7;\n+\t\terror_code = 8;\n \t\tgoto cleanup_return;\n \t}\n \tpath = real_path(dir);\n@@ -419,12 +423,14 @@ cleanup_return:\n \t\tcase 3:\n \t\t\tdie_errno(\"Error opening '%s'\", path);\n \t\tcase 4:\n-\t\t\tdie(\"Error reading %s\", path);\n+\t\t\tdie(\"Too large to be a .git file: '%s'\", path);\n \t\tcase 5:\n-\t\t\tdie(\"Invalid gitfile format: %s\", path);\n+\t\t\tdie(\"Error reading %s\", path);\n \t\tcase 6:\n-\t\t\tdie(\"No path in gitfile: %s\", path);\n+\t\t\tdie(\"Invalid gitfile format: %s\", path);\n \t\tcase 7:\n+\t\t\tdie(\"No path in gitfile: %s\", path);\n+\t\tcase 8:\n \t\t\tdie(\"Not a git repository: %s\", dir);\n \t\tdefault:\n \t\t\tassert(0);\n-- \n2.4.0.rc3.8.g4ebd28d\n"},{"id":"259990","messageId":"1429952801-2646-4-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"1429952801-2646-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v4 3/5] t7300: add tests to document behavior of clean and nested git","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-25T09:06:39Z","receivedAt":"2015-04-25T09:06:39Z","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 | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 128 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 99be5d9..11f3a6d 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -455,6 +455,134 @@ 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 '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.rc3.8.g4ebd28d\n"},{"id":"259991","messageId":"1429952801-2646-5-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"1429952801-2646-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v4 4/5] p7300: add performance tests for clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-25T09:06:40Z","receivedAt":"2015-04-25T09:06:40Z","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..6ae55ec\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_large_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.rc3.8.g4ebd28d\n"},{"id":"259992","messageId":"1429952801-2646-6-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"1429952801-2646-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v4 5/5] clean: improve performance when removing lots of directories","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-25T09:06:41Z","receivedAt":"2015-04-25T09:06:41Z","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\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 | 26 ++++++++++++++++++++++----\n t/t7300-clean.sh | 8 +++-----\n 2 files changed, 25 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 98c103f..17a1c13 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,27 @@ 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 unused_error_code;\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, &unused_error_code) || is_git_directory(path->buf))\n+\t\tret = 1;\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 +175,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 11f3a6d..4d07a4a 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 '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@@ -523,7 +521,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.rc3.8.g4ebd28d\n"},{"id":"259998","messageId":"xmqqa8xw17ew.fsf@gitster.dls.corp.google.com","threadId":"39159","inReplyTo":"1429952801-2646-3-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v4 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-25T16:47:51Z","receivedAt":"2015-04-25T16:47:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Elfström writes:\n\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 \n> ---\n>\n> I'm not sure about this one but it felt like the safe thing to do.\n> This patch can be dropped if it is not desired.\n\nI do not think it is wrong per-se, but the changes in this patch\nshows why hardcoded values assigned to error_code without #define is\nnot a good idea, as these values are now exposed to the callers of\nthe new function. After we gain a new caller that does care about\nthe exact error code (e.g. to react differently to the reason why we\nfailed to read by giving different error messages) if we decide to\nrevert this step, or if we decide to add a new error type, for\nwhatever reason, this organization forces the caller to be updated.\n\n> I considered testing it using\n> \"mkdir foo && truncate -s 200G foo/.git && git clean -f -d\"\n> but that feels like a pretty evil test that is likely to cause lots\n> of problems and not fail in any good way.\n\nAmen to that.\n\n>\n> setup.c | 20 +++++++++++++-------\n> 1 file changed, 13 insertions(+), 7 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index e1897cc..ed87334 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -364,22 +364,26 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> \t\terror_code = 3;\n> \t\tgoto cleanup_return;\n> \t}\n> +\tif (st.st_size > PATH_MAX * 4) {\n> +\t\terror_code = 4;\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 = 4;\n> +\t\terror_code = 5;\n> \t\tgoto cleanup_return;\n> \t}\n> \tbuf[len] = '\\0';\n> \tif (!starts_with(buf, \"gitdir: \")) {\n> -\t\terror_code = 5;\n> +\t\terror_code = 6;\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 = 6;\n> +\t\terror_code = 7;\n> \t\tgoto cleanup_return;\n> \t}\n> \tbuf[len] = '\\0';\n> @@ -397,7 +401,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n> \t}\n> \n> \tif (!is_git_directory(dir)) {\n> -\t\terror_code = 7;\n> +\t\terror_code = 8;\n> \t\tgoto cleanup_return;\n> \t}\n> \tpath = real_path(dir);\n> @@ -419,12 +423,14 @@ cleanup_return:\n> \t\tcase 3:\n> \t\t\tdie_errno(\"Error opening '%s'\", path);\n> \t\tcase 4:\n> -\t\t\tdie(\"Error reading %s\", path);\n> +\t\t\tdie(\"Too large to be a .git file: '%s'\", path);\n> \t\tcase 5:\n> -\t\t\tdie(\"Invalid gitfile format: %s\", path);\n> +\t\t\tdie(\"Error reading %s\", path);\n> \t\tcase 6:\n> -\t\t\tdie(\"No path in gitfile: %s\", path);\n> +\t\t\tdie(\"Invalid gitfile format: %s\", path);\n> \t\tcase 7:\n> +\t\t\tdie(\"No path in gitfile: %s\", path);\n> +\t\tcase 8:\n> \t\t\tdie(\"Not a git repository: %s\", dir);\n> \t\tdefault:\n> \t\t\tassert(0);\n"},{"id":"259999","messageId":"xmqq1tj8178e.fsf@gitster.dls.corp.google.com","threadId":"39159","inReplyTo":"1429952801-2646-2-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v4 1/5] setup: add gentle version of read_gitfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-25T16:51:45Z","receivedAt":"2015-04-25T16:51:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Elfström writes:\n\n> read_gitfile will die on most error cases. This makes it unsuitable\n> for speculative calls. Extract the core logic and provide a gentle\n> version that returns NULL on failure.\n>\n> The first usecase of the new gentle version will be to probe for\n> submodules during git clean.\n>\n> Helped-by: Junio C Hamano \n> Helped-by: Jeff King \n> Signed-off-by: Erik Elfström \n> ---\n> cache.h | 3 ++-\n> setup.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++--------------\n> 2 files changed, 67 insertions(+), 18 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 3d3244b..6e29068 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -431,7 +431,8 @@ 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> +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> \n> diff --git a/setup.c b/setup.c\n> index 979b13f..e1897cc 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -335,35 +335,53 @@ 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> \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 = 1;\n> +\t\tgoto cleanup_return;\n> +\t}\n> +\tif (!S_ISREG(st.st_mode)) {\n> +\t\terror_code = 2;\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 = 3;\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 = 4;\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 = 5;\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 = 6;\n> +\t\tgoto cleanup_return;\n> +\t}\n> \tbuf[len] = '\\0';\n> \tdir = buf + 8;\n> \n> @@ -378,11 +396,41 @@ 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> +\tif (!is_git_directory(dir)) {\n> +\t\terror_code = 7;\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 1: // failed to stat\n> +\t\tcase 2: // not regular file\n\nPlease do not use C++ // comments.\n\n> +\t\t\treturn NULL;\n> +\t\tcase 3:\n> +\t\t\tdie_errno(\"Error opening '%s'\", path);\n> +\t\tcase 4:\n> +\t\t\tdie(\"Error reading %s\", path);\n> +\t\tcase 5:\n> +\t\t\tdie(\"Invalid gitfile format: %s\", path);\n> +\t\tcase 6:\n> +\t\t\tdie(\"No path in gitfile: %s\", path);\n> +\t\tcase 7:\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"},{"id":"260000","messageId":"xmqqwq10ywpw.fsf@gitster.dls.corp.google.com","threadId":"39159","inReplyTo":"xmqq1tj8178e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 1/5] setup: add gentle version of read_gitfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-25T16:54:51Z","receivedAt":"2015-04-25T16:54:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\t\tswitch (error_code) {\n>> +\t\tcase 1: // failed to stat\n>> +\t\tcase 2: // not regular file\n>\n> Please do not use C++ // comments.\n\nNo need to resend for this; I'll locally amend.\n"},{"id":"260002","messageId":"1429984780-22868-1-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"xmqqa8xw17ew.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-25T17:59:40Z","receivedAt":"2015-04-25T17:59:40Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Sat, Apr 25, 2015 at 6:47 PM, Junio C Hamano wrote:\n> I do not think it is wrong per-se, but the changes in this patch\n> shows why hardcoded values assigned to error_code without #define is\n> not a good idea, as these values are now exposed to the callers of\n> the new function. After we gain a new caller that does care about\n> the exact error code (e.g. to react differently to the reason why we\n> failed to read by giving different error messages) if we decide to\n> revert this step, or if we decide to add a new error type, for\n> whatever reason, this organization forces the caller to be updated.\n\nYes, it was a bit silly of me not to add that. Would something like\nthis be ok?\n\n---\n cache.h | 9 +++++++++\n setup.c | 32 ++++++++++++++++----------------\n 2 files changed, 25 insertions(+), 16 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6e29068..177337a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -431,6 +431,15 @@ 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+\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_TOO_LARGE 4\n+#define READ_GITFILE_ERR_READ_FAILED 5\n+#define READ_GITFILE_ERR_INVALID_FORMAT 6\n+#define READ_GITFILE_ERR_NO_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);\ndiff --git a/setup.c b/setup.c\nindex ed87334..792c37b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -352,38 +352,38 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \tssize_t len;\n \n \tif (stat(path, &st)) {\n-\t\terror_code = 1;\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 = 2;\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\terror_code = 3;\n+\t\terror_code = READ_GITFILE_ERR_OPEN_FAILED;\n \t\tgoto cleanup_return;\n \t}\n \tif (st.st_size > PATH_MAX * 4) {\n-\t\terror_code = 4;\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 \tif (len != st.st_size) {\n-\t\terror_code = 5;\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\terror_code = 6;\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\terror_code = 7;\n+\t\terror_code = READ_GITFILE_ERR_NO_PATH;\n \t\tgoto cleanup_return;\n \t}\n \tbuf[len] = '\\0';\n@@ -401,7 +401,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \t}\n \n \tif (!is_git_directory(dir)) {\n-\t\terror_code = 8;\n+\t\terror_code = READ_GITFILE_ERR_NOT_A_REPO;\n \t\tgoto cleanup_return;\n \t}\n \tpath = real_path(dir);\n@@ -417,20 +417,20 @@ cleanup_return:\n \t\t\treturn NULL;\n \n \t\tswitch (error_code) {\n-\t\tcase 1: // failed to stat\n-\t\tcase 2: // not regular file\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 3:\n+\t\tcase READ_GITFILE_ERR_OPEN_FAILED:\n \t\t\tdie_errno(\"Error opening '%s'\", path);\n-\t\tcase 4:\n+\t\tcase READ_GITFILE_ERR_TOO_LARGE:\n \t\t\tdie(\"Too large to be a .git file: '%s'\", path);\n-\t\tcase 5:\n+\t\tcase READ_GITFILE_ERR_READ_FAILED:\n \t\t\tdie(\"Error reading %s\", path);\n-\t\tcase 6:\n+\t\tcase READ_GITFILE_ERR_INVALID_FORMAT:\n \t\t\tdie(\"Invalid gitfile format: %s\", path);\n-\t\tcase 7:\n+\t\tcase READ_GITFILE_ERR_NO_PATH:\n \t\t\tdie(\"No path in gitfile: %s\", path);\n-\t\tcase 8:\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-- \n2.4.0.rc3.8.g86acfbd.dirty\n"},{"id":"260010","messageId":"xmqqfv7nzf56.fsf@gitster.dls.corp.google.com","threadId":"39159","inReplyTo":"1429984780-22868-1-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v4 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-26T04:29:09Z","receivedAt":"2015-04-26T04:29:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Elfström writes:\n\n> On Sat, Apr 25, 2015 at 6:47 PM, Junio C Hamano wrote:\n>> I do not think it is wrong per-se, but the changes in this patch\n>> shows why hardcoded values assigned to error_code without #define is\n>> not a good idea, as these values are now exposed to the callers of\n>> the new function. After we gain a new caller that does care about\n>> the exact error code (e.g. to react differently to the reason why we\n>> failed to read by giving different error messages) if we decide to\n>> revert this step, or if we decide to add a new error type, for\n>> whatever reason, this organization forces the caller to be updated.\n>\n> Yes, it was a bit silly of me not to add that. Would something like\n> this be ok?\n\nYeah, if you used symbolic constants from the get-go, then the\nsecond patch to add the \"too-large? it cannot be gitfile\" safety\nwould not have to renumber everything; instead it would be adding a\nsingle constant to the header file and adding a new case to setup.c\nthat uses the error code.\n"},{"id":"260028","messageId":"1430030985-14499-1-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"xmqqfv7nzf56.fsf@gitster.dls.corp.google.com","subject":"[PATCH v5 0/5] Improving performance of git clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-26T06:49:40Z","receivedAt":"2015-04-26T06:49:40Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Changes in v5:\n* Added defines for read_gitfile_gently error codes. \n This was a silly mistake, sorry about that.\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 | 26 +++++++++--\n cache.h | 12 ++++-\n setup.c | 88 ++++++++++++++++++++++++++++-------\n t/perf/p7300-clean.sh | 31 +++++++++++++\n t/t7300-clean.sh | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 261 insertions(+), 22 deletions(-)\n create mode 100755 t/perf/p7300-clean.sh\n\n-- \n2.4.0.rc3.8.gbb31afb\n"},{"id":"260027","messageId":"1430030985-14499-2-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"1430030985-14499-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-26T06:49:41Z","receivedAt":"2015-04-26T06:49:41Z","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 | 11 ++++++++-\n setup.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++--------------\n 2 files changed, 75 insertions(+), 18 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3d3244b..868e4d3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -431,7 +431,16 @@ 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_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 979b13f..c4538ca 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -335,35 +335,53 @@ 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 \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@@ -378,11 +396,41 @@ 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+\tif (!is_git_directory(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_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.0.rc3.8.gbb31afb\n"},{"id":"260029","messageId":"1430030985-14499-3-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"1430030985-14499-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v5 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-26T06:49:42Z","receivedAt":"2015-04-26T06:49:42Z","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 | 6 ++++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 868e4d3..c9f1f8e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -439,6 +439,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 c4538ca..792c37b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -364,6 +364,10 @@ 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 > PATH_MAX * 4) {\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@@ -418,6 +422,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.rc3.8.gbb31afb\n"},{"id":"260030","messageId":"1430030985-14499-4-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"1430030985-14499-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v5 3/5] t7300: add tests to document behavior of clean and nested git","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-26T06:49:43Z","receivedAt":"2015-04-26T06:49:43Z","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 | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 128 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 99be5d9..11f3a6d 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -455,6 +455,134 @@ 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 '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.rc3.8.gbb31afb\n"},{"id":"260031","messageId":"1430030985-14499-5-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"1430030985-14499-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v5 4/5] p7300: add performance tests for clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-26T06:49:44Z","receivedAt":"2015-04-26T06:49:44Z","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..6ae55ec\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_large_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.rc3.8.gbb31afb\n"},{"id":"260032","messageId":"1430030985-14499-6-git-send-email-erik.elfstrom@gmail.com","threadId":"39159","inReplyTo":"1430030985-14499-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH v5 5/5] clean: improve performance when removing lots of directories","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-26T06:49:45Z","receivedAt":"2015-04-26T06:49:45Z","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\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 | 26 ++++++++++++++++++++++----\n t/t7300-clean.sh | 8 +++-----\n 2 files changed, 25 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 98c103f..17a1c13 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,27 @@ 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 unused_error_code;\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, &unused_error_code) || is_git_directory(path->buf))\n+\t\tret = 1;\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 +175,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 11f3a6d..4d07a4a 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 '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@@ -523,7 +521,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.rc3.8.gbb31afb\n"},{"id":"260102","messageId":"20150428060222.GK24580@peff.net","threadId":"39159","inReplyTo":"1430030985-14499-3-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v5 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-28T06:02:22Z","receivedAt":"2015-04-28T06:02:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 26, 2015 at 08:49:42AM +0200, Erik Elfström wrote:\n\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> [...]\n> +\tif (st.st_size > PATH_MAX * 4) {\n> +\t\terror_code = READ_GITFILE_ERR_TOO_LARGE;\n> +\t\tgoto cleanup_return;\n> +\t}\n\nMy understanding is that PATH_MAX is set absurdly low on Windows\nsystems (and doesn't actually represent the real limit of a path!).\nSince the value is picked arbitrarily anyway, could use something more\nindependent (like 100K or something, which is large enough to be beyond\nabsurd and small enough that a malloc isn't a big deal)?\n\n-Peff\n"},{"id":"260103","messageId":"20150428061741.GL24580@peff.net","threadId":"39159","inReplyTo":"1430030985-14499-2-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-28T06:17:42Z","receivedAt":"2015-04-28T06:17:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 26, 2015 at 08:49:41AM +0200, Erik Elfström wrote:\n\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\nThere was a discussion not too long ago on strategies for returning\nerrors, and one of the suggestions was to return an \"error strbuf\"\nrather than a code[1]. That's less flexible, as the caller can't react\ndifferently based on the type of error. But for cases like this, where\nthe only fate for the code is to get converted back into a message,\nit can reduce the boilerplate.\n\nWhat you have here is OK to me, and I don't want to hold up your patch\nseries in a flamewar about error-reporting techniques. But I think it's\nan interesting case study.\n\n-Peff\n\n[1] The original thread was here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/259695/focus=260722\n\n I'm still a little wary of the allocation boilerplate introduced by\n the strbuf approach. But in this case it would not be too bad, I\n think.\n"},{"id":"260104","messageId":"20150428062422.GM24580@peff.net","threadId":"39159","inReplyTo":"1430030985-14499-6-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v5 5/5] clean: improve performance when removing lots of directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-28T06:24:23Z","receivedAt":"2015-04-28T06:24:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 26, 2015 at 08:49:45AM +0200, Erik Elfström wrote:\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 unused_error_code;\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, &unused_error_code) || is_git_directory(path->buf))\n> +\t\tret = 1;\n> +\tstrbuf_setlen(path, orig_path_len);\n> +\treturn ret;\n> +}\n\nThis iteration looks reasonable overall to me.\n\nShould this is_git_repository() helper be available to other files? I\nthink there are other calls to resolve_gitlink_ref() that would want the\nsame treatment (e.g., I think \"git status\" may have a similar issue).\n\n-Peff\n"},{"id":"260107","messageId":"20150428063311.GN24580@peff.net","threadId":"39159","inReplyTo":"1430030985-14499-5-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v5 4/5] p7300: add performance tests for clean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-28T06:33:11Z","receivedAt":"2015-04-28T06:33:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 26, 2015 at 08:49:44AM +0200, Erik Elfström wrote:\n\n> diff --git a/t/perf/p7300-clean.sh b/t/perf/p7300-clean.sh\n> new file mode 100755\n> index 0000000..6ae55ec\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_large_repo\n\nDo we actually need a large repo here? The real cost is coming from the\ndirectories we create. We could actually start with a totally empty\nrepository if we wanted (though I don't think the t/perf system handles\nthat right now). But if there's not a reason to use the large repo, I\nthink using test_perf_default_repo is better, as it works out of the box\nwithout specifying extra environment variables (well, it works either\nway, but you get a nasty warning from perf-lib.sh).\n\n-Peff\n"},{"id":"260110","messageId":"e7c614c97eb4b1819cec25567a7adc97@www.dscho.org","threadId":"39159","inReplyTo":"20150428060222.GK24580@peff.net","subject":"Windows path limites, was Re: [PATCH v5 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-28T07:21:30Z","receivedAt":"2015-04-28T07:21:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn 2015-04-28 08:02, Jeff King wrote:\n\n> My understanding is that PATH_MAX is set absurdly low on Windows\n> systems (and doesn't actually represent the real limit of a path!).\n\nWell, yes and no. Yes, it is absurdly low on Windows, and yes, it is not the real limit of a path *if you know how to work around it*. Most Win32 API calls actually do have that absurdly low limit, but internally longer paths can be represented (and there is a hack where you prefix the path -- which must be an absolute one for this hack -- by `\\\\?\\`). Keep in mind, though, that even the Windows Explorer is (at least sometimes) limited by the absurdly low path limit.\n\nNote that we have support for `core.longpaths` in Git for Windows (to be submitted just after I get 2.x out), but it is disabled by default because of the Windows Explorer issue.\n\nCiao,\nDscho\n"},{"id":"260130","messageId":"CAEtYS8QMSSbfd2xSCMh2YTW2NSz7uBnsmKUi9SjSCrJ1TBEH+w@mail.gmail.com","threadId":"39159","inReplyTo":"e7c614c97eb4b1819cec25567a7adc97@www.dscho.org","subject":"Re: Windows path limites, was Re: [PATCH v5 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-04-28T15:33:42Z","receivedAt":"2015-04-28T15:33:42Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Tue, Apr 28, 2015 at 2:23 AM Johannes Schindelin\n wrote:\n>\n> Hi Peff,\n>\n> On 2015-04-28 08:02, Jeff King wrote:\n>\n> > My understanding is that PATH_MAX is set absurdly low on Windows\n> > systems (and doesn't actually represent the real limit of a path!).\n>\n> Well, yes and no. Yes, it is absurdly low on Windows, and yes, it is not the real limit of a path *if you know how to work around it*. Most Win32 API calls actually do have that absurdly low limit, but internally longer paths can be represented (and there is a hack where you prefix the path -- which must be an absolute one for this hack -- by `\\\\?\\`). Keep in mind, though, that even the Windows Explorer is (at least sometimes) limited by the absurdly low path limit.\n>\n\nOne more thing worth noting is that path lengths are actually restricted further\nwithin git_path_submodule(), I would presume to reserve some amount of space\nwhen cloning a submodule (it seems to set aside 100 characters)? For other\ncases, yes, Dscho is absolutely right: the MAX_PATH constant is 260 (which gives\nsomething like 256 usable characters). If you're able to do everything through\nthe Unicode Win32 APIs, you can reach 65535 characters, assuming the filesystem\nsupports it (NTFS does, FAT32 would not, for example). I recall there being one\nfunction (possibly thinking of mktemp) that couldn't properly handle the long\npaths, but I believe that was it.\n\nOther apps may or may not handle the longer paths well; the core.longpaths\nswitch may prevent mishaps in more than just Explorer, but the downside isn't\ndisaster always either -- for example, Explorer just refuses to browse\nto that path.\n(Note that other apps may include both the msys bits as well as the Windows\ncommand line built-in commands, which may make things that farm out to sh\nbehave in unusual ways. I vaguely remember testing this myself, but I\ndon't recall\nwhat I did exactly or what happened.)\n\n--Doug\n"},{"id":"260131","messageId":"c33a7d869ecfb49d0b548f895d0f5da3@www.dscho.org","threadId":"39159","inReplyTo":"CAEtYS8QMSSbfd2xSCMh2YTW2NSz7uBnsmKUi9SjSCrJ1TBEH+w@mail.gmail.com","subject":"Re: Windows path limits, was Re: [PATCH v5 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-28T16:20:13Z","receivedAt":"2015-04-28T16:20:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn 2015-04-28 17:33, Doug Kelly wrote:\n\n> If you're able to do everything through the Unicode Win32 APIs, you can reach 65535 characters, assuming the filesystem supports it (NTFS does, FAT32 would not, for example). I recall there being one function (possibly thinking of mktemp) that couldn't properly handle the long paths, but I believe that was it.\n\nIIRC the MSDN documentation of at least one of the directory-related functions was not quite guaranteeing that it would support long paths with all Windows versions, either.\n\n> Other apps may or may not handle the longer paths well; the core.longpaths\n> switch may prevent mishaps in more than just Explorer, but the downside isn't\n> disaster always either -- for example, Explorer just refuses to browse\n> to that path.\n\nThe problem comes from a different angle, though: I had *a lot* of fun when working on the tests for the `core.longpaths` feature because our very own `rm.exe` would not handle the long paths. As a consequence, quite a few tests failed to remove the trash directories of previous, failed test runs.\n\nIn such a case you will have to resort to use different tools than you are used to, and *that* is the reason why we do not enable that feature by default.\n\nCiao,\nDscho\n"},{"id":"260136","messageId":"CAMpP7NY76Tj_=_6Evxvox20OmQ5LnNiK1j4VHLd0jKhRmci51g@mail.gmail.com","threadId":"39159","inReplyTo":"20150428060222.GK24580@peff.net","subject":"Re: [PATCH v5 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-28T19:28:12Z","receivedAt":"2015-04-28T19:28:12Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Tue, Apr 28, 2015 at 8:02 AM, Jeff King wrote:\n>\n> My understanding is that PATH_MAX is set absurdly low on Windows\n> systems (and doesn't actually represent the real limit of a path!).\n> Since the value is picked arbitrarily anyway, could use something more\n> independent (like 100K or something, which is large enough to be beyond\n> absurd and small enough that a malloc isn't a big deal)?\n>\n> -Peff\n\nI'm happy to set the limit to anything that makes everybody feel safe. I'll set\nit to 1MB to be on the safe side.\n\nI'm not sure though how the code (in general) is supposed to keep working if\na path can exceed PATH_MAX? A cursory search for PATH_MAX comes\nup with char array sizes and check-and-die kind of things. If a path is longer\nthen surely we will be unable to handle it and abort in all sorts of places?\n\nAre you only worried we might have a submodule with a too long path (that will\ncreate various other problems in different codepaths) that we may mistakenly\nclean (if it doesn't trigger any other abort earlier in the clean call\nchain) or do\nyou want clean to keep working and do the right thing even in this case?\n\nWhile digging around looking at this I also noticed that there is\nanother problem\nI have overlooked previously.\n\nread_gitfile_gently will call is_git_directory at the very end and it\ncontains the\nfollowing check at the very beginning:\n\n if (PATH_MAX <= len + strlen(\"/objects\"))\n die(\"Too long path: %.*s\", 60, suspect);\n\nNow, this is good in the way that we will avoid mistakenly cleaning\nstuff because\nthe path is too long but also bad because it makes read_gitfile_gently\nbehave very\nungently in this case.\n\nI suspect I should make a gentle version of this also. The question is\nwhat to do\nin clean if the path is reported as too long? Abort? Avoid cleaning it\nto be safe?\nIgnore and clean it?\n\nis_git_directory is also called from the new is_git_repository\ndirectly but here I think\ndying is ok since this path is a path in the working tree and if we\ncan't handle the paths\nin the tree then there seem to be little point in trying to go on (as\nopposed to when\nsome string in a file is too large for a path)\n\nThoughts?\n\n/Erik\n"},{"id":"260137","messageId":"CAMpP7NZM2CUbL_UEJozbgzb9yDnSp29B4hj7XY4pVKA_OZDgRg@mail.gmail.com","threadId":"39159","inReplyTo":"20150428063311.GN24580@peff.net","subject":"Re: [PATCH v5 4/5] p7300: add performance tests for clean","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-28T19:36:36Z","receivedAt":"2015-04-28T19:36:36Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Tue, Apr 28, 2015 at 8:33 AM, Jeff King wrote:\n>\n> Do we actually need a large repo here? The real cost is coming from the\n> directories we create. We could actually start with a totally empty\n> repository if we wanted (though I don't think the t/perf system handles\n> that right now). But if there's not a reason to use the large repo, I\n> think using test_perf_default_repo is better, as it works out of the box\n> without specifying extra environment variables (well, it works either\n> way, but you get a nasty warning from perf-lib.sh).\n>\n> -Peff\n\nSure, I'll change this to the default. I wasn't sure about the\nperformance characteristics of clean (does working tree size matter?)\nwhen I started so it felt safest to go with a large repo.\n"},{"id":"260139","messageId":"CAMpP7Nb3aiMoTjtPJNJPv38G54ZawS8B+NDX0x2iNe6FA-L+Lw@mail.gmail.com","threadId":"39159","inReplyTo":"20150428061741.GL24580@peff.net","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-28T20:07:43Z","receivedAt":"2015-04-28T20:07:43Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Tue, Apr 28, 2015 at 8:17 AM, Jeff King wrote:\n>\n> There was a discussion not too long ago on strategies for returning\n> errors, and one of the suggestions was to return an \"error strbuf\"\n> rather than a code[1]. That's less flexible, as the caller can't react\n> differently based on the type of error. But for cases like this, where\n> the only fate for the code is to get converted back into a message,\n> it can reduce the boilerplate.\n>\n> What you have here is OK to me, and I don't want to hold up your patch\n> series in a flamewar about error-reporting techniques. But I think it's\n> an interesting case study.\n>\n> -Peff\n\nThanks. I haven't had time to look through that thread yet, I'll try\nto get to that later.\n\nMy initial reaction is a bit skeptical though. For this case we\ncurrently don't want any error reporting, the NULL return is\nsufficient and even allocating and sending in the int* is pure noise.\nAllocating and releasing a strbuf seems like a lot more overhead for\nthis type of caller? The one other potential candidate caller for\nread_gitfile_gently that I have seen (clone.c:get_repo_path) don't\nwant any error code or message either as far as i can tell.\n\nAlso if it turns out that we actually need to treat the \"file too\nlarge\" error differently in clean (as discussed in thread on the file\nsize check) then we can no longer communicate that back using the\nstrbuf interface.\n\nOverall it seems like a less attractive solution to me but I can be\npersuaded if there is more support expressed for the strbuf direction.\n\n/Erik\n"},{"id":"260140","messageId":"20150428201918.GA10902@peff.net","threadId":"39159","inReplyTo":"CAMpP7Nb3aiMoTjtPJNJPv38G54ZawS8B+NDX0x2iNe6FA-L+Lw@mail.gmail.com","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-28T20:19:19Z","receivedAt":"2015-04-28T20:19:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2015 at 10:07:43PM +0200, erik elfström wrote:\n\n> On Tue, Apr 28, 2015 at 8:17 AM, Jeff King wrote:\n> >\n> > There was a discussion not too long ago on strategies for returning\n> > errors, and one of the suggestions was to return an \"error strbuf\"\n> > rather than a code[1]. That's less flexible, as the caller can't react\n> > differently based on the type of error. But for cases like this, where\n> > the only fate for the code is to get converted back into a message,\n> > it can reduce the boilerplate.\n> >\n> > What you have here is OK to me, and I don't want to hold up your patch\n> > series in a flamewar about error-reporting techniques. But I think it's\n> > an interesting case study.\n> >\n> > -Peff\n> \n> Thanks. I haven't had time to look through that thread yet, I'll try\n> to get to that later.\n> \n> My initial reaction is a bit skeptical though. For this case we\n> currently don't want any error reporting, the NULL return is\n> sufficient and even allocating and sending in the int* is pure noise.\n> Allocating and releasing a strbuf seems like a lot more overhead for\n> this type of caller? The one other potential candidate caller for\n> read_gitfile_gently that I have seen (clone.c:get_repo_path) don't\n> want any error code or message either as far as i can tell.\n\nI had envisioned that the strbuf would be optional. I.e., you would\nhave:\n\n /* like error(), but dump the message in a strbuf instead of stderr */\n int error_buf(struct strbuf *buf, const char *fmt, ...)\n {\n\tif (buf) {\n\t\tva_list ap;\n\t\tva_start(ap, fmt);\n\t\tstrbuf_vaddf(buf, fmt, ap);\n\t\tva_end(ap);\n\t}\n\treturn -1;\n }\n\nand then in the error-reporting function:\n\n const char *read_gitfile_gently(const char *path, struct strbuf *err)\n {\n\t...\n\tfd = open(path, O_RDONLY);\n\tif (fd < 0) {\n\t\terror_buf(err, \"unable to open %s: %s\", path, strerror(errno));\n\t\treturn NULL; /* or goto cleanup if necessary */\n\t}\n }\n\nand then one caller can do:\n\n if (!read_gitfile_gently(path, NULL)) {\n\t/* we know there was an error, but we did not ask for details */\n\t...\n }\n\nand the non-gentle read_gitfile() becomes:\n\n const char *read_gitfile(const char *path)\n {\n\tstruct strbuf err = STRBUF_INIT;\n\tconst char *ret = read_gitfile_gently(path, &err);\n\tif (!ret)\n\t\tdie(\"%s\", err.buf);\n\t/* no need to free err; if there was no error, nothing was written */\n\treturn path;\n }\n\nNote that the \"return -1\" from error_buf() is not useful here, but it\nmight be used as a shortcut in other situations (e.g., the same places\nwe call \"return error()\" now).\n\n> Also if it turns out that we actually need to treat the \"file too\n> large\" error differently in clean (as discussed in thread on the file\n> size check) then we can no longer communicate that back using the\n> strbuf interface.\n\nYeah, agreed. This system breaks down as soon as you need to\nprogramatically know which error happened.\n\n-Peff\n"},{"id":"260142","messageId":"CAMpP7Nbm9Gy6AgULBCCwgeEUBWU4M5r8+SC22iuu8VmaUtst_w@mail.gmail.com","threadId":"39159","inReplyTo":"20150428062422.GM24580@peff.net","subject":"Re: [PATCH v5 5/5] clean: improve performance when removing lots of directories","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-28T20:31:47Z","receivedAt":"2015-04-28T20:31:47Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Tue, Apr 28, 2015 at 8:24 AM, Jeff King wrote:\n>\n> This iteration looks reasonable overall to me.\n>\n> Should this is_git_repository() helper be available to other files? I\n> think there are other calls to resolve_gitlink_ref() that would want the\n> same treatment (e.g., I think \"git status\" may have a similar issue).\n>\n> -Peff\n\nYes that is probably the direction to go in but I think the current\nversion is too tailored to the clean case to be generally useful\n(although I haven't check the status case). Right now this is more of\na is_this_a_repo_clean_should_care_about_ignoring_some_cornercase_quirks\nthan a true is_git_repository check.\n\nI would think a general version would look more like this:\n\nstatic int is_git_repository(struct strbuf *path)\n{\n if (is_bare_repository(path))\n return BARE_REPO;\n if (is_submodule(path))\n return SUBMODULE;\n if (is_git_director(path))\n return REPO;\n return 0;\n}\n\nprobably also communicating any errors back to the caller.\n\nTo sum it up I think you are correct in the direction but I suspect\nthat the current version is not sufficient for the general case and\nthat it would be best to leave the generalization and making it public\nfor future work.\n\n/Erik\n"},{"id":"260143","messageId":"20150428203400.GN5467@google.com","threadId":"39159","inReplyTo":"20150428201918.GA10902@peff.net","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-04-28T20:34:00Z","receivedAt":"2015-04-28T20:34:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Apr 28, 2015 at 10:07:43PM +0200, erik elfström wrote:\n\n>> Also if it turns out that we actually need to treat the \"file too\n>> large\" error differently in clean (as discussed in thread on the file\n>> size check) then we can no longer communicate that back using the\n>> strbuf interface.\n>\n> Yeah, agreed. This system breaks down as soon as you need to\n> programatically know which error happened.\n\nOn the contrary: it separates the information that is used\nprogramatically and the information intended for the user.\n\nThe return value (or an int * parameter) distinguishes errors that\naffect control flow. A string can provide information for the user.\n\nThis way it is easy to tweak the information that the user sees in\none place, without changing control flow.\n"},{"id":"260144","messageId":"20150428203645.GA11473@peff.net","threadId":"39159","inReplyTo":"20150428203400.GN5467@google.com","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-28T20:36:45Z","receivedAt":"2015-04-28T20:36:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2015 at 01:34:00PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> > On Tue, Apr 28, 2015 at 10:07:43PM +0200, erik elfström wrote:\n> \n> >> Also if it turns out that we actually need to treat the \"file too\n> >> large\" error differently in clean (as discussed in thread on the file\n> >> size check) then we can no longer communicate that back using the\n> >> strbuf interface.\n> >\n> > Yeah, agreed. This system breaks down as soon as you need to\n> > programatically know which error happened.\n> \n> On the contrary: it separates the information that is used\n> programatically and the information intended for the user.\n> \n> The return value (or an int * parameter) distinguishes errors that\n> affect control flow. A string can provide information for the user.\n> \n> This way it is easy to tweak the information that the user sees in\n> one place, without changing control flow.\n\nBut the NULL does not carry the information about _which_ error, and\nErik is suggesting that the caller may need to change behavior based on\nthat information. IOW, his current patch (return NULL and set the\nspecific integer code in a variable) allows this, but switching the\ninteger code out for a human-readable strbuf does not.\n\n-Peff\n"},{"id":"260145","messageId":"20150428204213.GO5467@google.com","threadId":"39159","inReplyTo":"20150428203645.GA11473@peff.net","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-04-28T20:42:13Z","receivedAt":"2015-04-28T20:42:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> But the NULL does not carry the information about _which_ error, and\n> Erik is suggesting that the caller may need to change behavior based on\n> that information. IOW, his current patch (return NULL and set the\n> specific integer code in a variable) allows this, but switching the\n> integer code out for a human-readable strbuf does not.\n\nRight. Two ways to handle that are:\n\n - two out parameters: an integer code and a human-readable string, or\n\n - an integer code returned using the ERR_PTR method and a\n human-readable string out parameter\n\nThe point I was trying to make clear is that the human-readable error\nmessage and the integer to affect control flow have different purposes\nand are not tightly related except in that they are produced at the\nsame time.\n\nUsing an integer to convey the human-readable error message (like errno\ndoes) unnecessarily forces the message to be more vague (by not being\nparameterized, and by always using the same error message for the same\ninteger error).\n"},{"id":"260146","messageId":"20150428204853.GA12153@peff.net","threadId":"39159","inReplyTo":"20150428204213.GO5467@google.com","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-28T20:48:53Z","receivedAt":"2015-04-28T20:48:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2015 at 01:42:13PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > But the NULL does not carry the information about _which_ error, and\n> > Erik is suggesting that the caller may need to change behavior based on\n> > that information. IOW, his current patch (return NULL and set the\n> > specific integer code in a variable) allows this, but switching the\n> > integer code out for a human-readable strbuf does not.\n> \n> Right. Two ways to handle that are:\n\nSure, but \"this system\" that I was referring to one was not one of\nthose ways. :)\n\n> - two out parameters: an integer code and a human-readable string, or\n> \n> - an integer code returned using the ERR_PTR method and a\n> human-readable string out parameter\n> \n> The point I was trying to make clear is that the human-readable error\n> message and the integer to affect control flow have different purposes\n> and are not tightly related except in that they are produced at the\n> same time.\n> \n> Using an integer to convey the human-readable error message (like errno\n> does) unnecessarily forces the message to be more vague (by not being\n> parameterized, and by always using the same error message for the same\n> integer error).\n\nYes, I agree converting the integer back into a string later does not\nalways carry all of the data. OTOH, the caller can often supply the\ncontext (i.e., this is basically how \"errno\" works). This gets back to\nthe idea we discussed a while ago of having a \"struct error\" that can\ncarry the code and the parameters (or if you like, the code and a\nfinished error message). It just feels in some ways like that ends up\nwith the worst of both worlds (a lot of boilerplate for integer codes,\n_and_ the allocation and cleanup issues associated with a string error\nmessage).\n\nI dunno. I'd be interested to see a rough draft of an idea applied to a\nspecific callsite.\n\n-Peff\n"},{"id":"260147","messageId":"20150428210647.GP5467@google.com","threadId":"39159","inReplyTo":"20150428204853.GA12153@peff.net","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-04-28T21:06:47Z","receivedAt":"2015-04-28T21:06:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Apr 28, 2015 at 01:42:13PM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> But the NULL does not carry the information about _which_ error, and\n>>> Erik is suggesting that the caller may need to change behavior based on\n>>> that information. IOW, his current patch (return NULL and set the\n>>> specific integer code in a variable) allows this, but switching the\n>>> integer code out for a human-readable strbuf does not.\n>>\n>> Right. Two ways to handle that are:\n>\n> Sure, but \"this system\" that I was referring to one was not one of\n> those ways. :)\n\nThat means I wasn't communicating well.\n\nI never advocated *not* providing an integer return code when you want\nto affect control flow.\n\nAn example of a function that provides information both to the caller\nand user and doesn't conflate the two is ref_transaction_commit. It\nreturns TRANSACTION_NAME_CONFLICT for a certain kind of recoverable\nerror and TRANSACTION_GENERIC_ERROR for all other errors. In either\ncase, the 'err' parameter is populated with information intended for\nthe user.\n\nSometimes you might want to affect control flow without providing\ninformation for the user. Then there's no need for a string output\nparameter and you can just provide the int.\n\nAt other times you might want to provide information for the user\nwithout affecting control flow. Then you can provide a string without\nthe int.\n\nProviding error codes when you also want to return a string or struct\npointer is an unrelated problem. One runs into that problem even if\nonly intending to provide a return code. Possible approaches, like\nmentioned before, are\n\n - an int * parameter to pass back the return code\n - using ERR_PTR to embed error codes in the pointer space\n\nThere is another approach which is sometimes nicer:\n\n - return int and pass the string or struct pointer through an output\n paramter\n\nThanks and sorry for the lack of clarity,\nJonathan\n"},{"id":"260152","messageId":"xmqq383jvndc.fsf@gitster.dls.corp.google.com","threadId":"39159","inReplyTo":"20150428204853.GA12153@peff.net","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-28T23:34:07Z","receivedAt":"2015-04-28T23:34:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yes, I agree converting the integer back into a string later does not\n> always carry all of the data. OTOH, the caller can often supply the\n> context (i.e., this is basically how \"errno\" works). This gets back to\n> the idea we discussed a while ago of having a \"struct error\" that can\n> carry the code and the parameters (or if you like, the code and a\n> finished error message). It just feels in some ways like that ends up\n> with the worst of both worlds (a lot of boilerplate for integer codes,\n> _and_ the allocation and cleanup issues associated with a string error\n> message).\n>\n> I dunno. I'd be interested to see a rough draft of an idea applied to a\n> specific callsite.\n\nFor this specific callsite, I think the error code alone is the\nright way forward. It conveys all information necessary out of the\ncallee back to the caller, so that the caller, when it decides to do\nso later, can turn error code to human readable form.\n\nFor some callsites, there may be cases where carrying a centrally\nproduced string may be a handy way to grab detailed error message\nout of a deep call chain, but even in such a case, it is likely that\nwe would need a separate error code, and a funtion that returns a\npointer as its primary return value would need \"two\" extra out\nparameters, one for message and one for code.\n"},{"id":"260176","messageId":"xmqqy4lb54bl.fsf@gitster.dls.corp.google.com","threadId":"39159","inReplyTo":"1430030985-14499-3-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v5 2/5] setup: sanity check file size in read_gitfile_gently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-29T15:42:22Z","receivedAt":"2015-04-29T15:42:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Elfström writes:\n\n> diff --git a/setup.c b/setup.c\n> index c4538ca..792c37b 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -364,6 +364,10 @@ 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 > PATH_MAX * 4) {\n> +\t\terror_code = READ_GITFILE_ERR_TOO_LARGE;\n\nYou have fd already open for the path at this point. When you\nreroll, please do not forget to close(fd) here.\n\n> +\t\tgoto cleanup_return;\n> +\t}\n"},{"id":"260238","messageId":"CAGZ79kY95mxKWwTPSB0S5c0eux1ButhuvGz91GQUz8WXt1w18w@mail.gmail.com","threadId":"39159","inReplyTo":"1430030985-14499-2-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-29T23:47:33Z","receivedAt":"2015-04-29T23:47:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Apr 25, 2015 at 11:49 PM, Erik Elfström wrote:\n> read_gitfile will die on most error cases. This makes it unsuitable\n> for speculative calls. Extract the core logic and provide a gentle\n> version that returns NULL on failure.\n>\n> The first usecase of the new gentle version will be to probe for\n> submodules during git clean.\n>\n> Helped-by: Junio C Hamano \n> Helped-by: Jeff King \n> Signed-off-by: Erik Elfström \n> ---\n> cache.h | 11 ++++++++-\n> setup.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++--------------\n> 2 files changed, 75 insertions(+), 18 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 3d3244b..868e4d3 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -431,7 +431,16 @@ 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_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>\n> diff --git a/setup.c b/setup.c\n> index 979b13f..c4538ca 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -335,35 +335,53 @@ 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> - char *buf;\n> - char *dir;\n> + int error_code = 0;\n> + char *buf = NULL;\n> + char *dir = NULL;\n> const char *slash;\n> struct stat st;\n> int fd;\n> ssize_t len;\n>\n> - if (stat(path, &st))\n> - return NULL;\n> - if (!S_ISREG(st.st_mode))\n> - return NULL;\n> + if (stat(path, &st)) {\n> + error_code = READ_GITFILE_ERR_STAT_FAILED;\n> + goto cleanup_return;\n> + }\n> + if (!S_ISREG(st.st_mode)) {\n> + error_code = READ_GITFILE_ERR_NOT_A_FILE;\n> + goto cleanup_return;\n> + }\n> fd = open(path, O_RDONLY);\n> - if (fd < 0)\n> - die_errno(\"Error opening '%s'\", path);\n> + if (fd < 0) {\n> + error_code = READ_GITFILE_ERR_OPEN_FAILED;\n> + goto cleanup_return;\n> + }\n> buf = xmalloc(st.st_size + 1);\n> len = read_in_full(fd, buf, st.st_size);\n> close(fd);\n> - if (len != st.st_size)\n> - die(\"Error reading %s\", path);\n> + if (len != st.st_size) {\n> + error_code = READ_GITFILE_ERR_READ_FAILED;\n> + goto cleanup_return;\n\nSorry for the late review.\n\nSo when you jump from here to the cleanup, there is no close(fd) involved?\nI think there are code paths now, which leak fd.\n\n> + }\n> buf[len] = '\\0';\n> - if (!starts_with(buf, \"gitdir: \"))\n> - die(\"Invalid gitfile format: %s\", path);\n> + if (!starts_with(buf, \"gitdir: \")) {\n> + error_code = READ_GITFILE_ERR_INVALID_FORMAT;\n> + goto cleanup_return;\n> + }\n> while (buf[len - 1] == '\\n' || buf[len - 1] == '\\r')\n> len--;\n> - if (len < 9)\n> - die(\"No path in gitfile: %s\", path);\n> + if (len < 9) {\n> + error_code = READ_GITFILE_ERR_NO_PATH;\n> + goto cleanup_return;\n> + }\n> buf[len] = '\\0';\n> dir = buf + 8;\n>\n> @@ -378,11 +396,41 @@ const char *read_gitfile(const char *path)\n> buf = dir;\n> }\n>\n> - if (!is_git_directory(dir))\n> - die(\"Not a git repository: %s\", dir);\n> + if (!is_git_directory(dir)) {\n> + error_code = READ_GITFILE_ERR_NOT_A_REPO;\n> + goto cleanup_return;\n> + }\n> path = real_path(dir);\n>\n> +cleanup_return:\n> free(buf);\n> +\n> + if (return_error_code)\n> + *return_error_code = error_code;\n> +\n> + if (error_code) {\n> + if (return_error_code)\n> + return NULL;\n> +\n> + switch (error_code) {\n> + case READ_GITFILE_ERR_STAT_FAILED:\n> + case READ_GITFILE_ERR_NOT_A_FILE:\n> + return NULL;\n> + case READ_GITFILE_ERR_OPEN_FAILED:\n> + die_errno(\"Error opening '%s'\", path);\n> + case READ_GITFILE_ERR_READ_FAILED:\n> + die(\"Error reading %s\", path);\n> + case READ_GITFILE_ERR_INVALID_FORMAT:\n> + die(\"Invalid gitfile format: %s\", path);\n> + case READ_GITFILE_ERR_NO_PATH:\n> + die(\"No path in gitfile: %s\", path);\n> + case READ_GITFILE_ERR_NOT_A_REPO:\n> + die(\"Not a git repository: %s\", dir);\n> + default:\n> + assert(0);\n> + }\n> + }\n> +\n> return path;\n> }\n>\n> --\n> 2.4.0.rc3.8.gbb31afb\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":"260246","messageId":"xmqq383i4cvm.fsf@gitster.dls.corp.google.com","threadId":"39159","inReplyTo":"CAGZ79kY95mxKWwTPSB0S5c0eux1ButhuvGz91GQUz8WXt1w18w@mail.gmail.com","subject":"Re: [PATCH v5 1/5] setup: add gentle version of read_gitfile","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-30T01:35:09Z","receivedAt":"2015-04-30T01:35:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n[administrivia: cull the parts of the original from your quote if\nyou are not addressing them]\n\n>> fd = open(path, O_RDONLY);\n>> - if (fd < 0)\n>> - die_errno(\"Error opening '%s'\", path);\n>> + if (fd < 0) {\n>> + error_code = READ_GITFILE_ERR_OPEN_FAILED;\n>> + goto cleanup_return;\n>> + }\n>> buf = xmalloc(st.st_size + 1);\n>> len = read_in_full(fd, buf, st.st_size);\n>> close(fd);\n>> - if (len != st.st_size)\n>> - die(\"Error reading %s\", path);\n>> + if (len != st.st_size) {\n>> + error_code = READ_GITFILE_ERR_READ_FAILED;\n>> + goto cleanup_return;\n>\n> Sorry for the late review.\n>\n> So when you jump from here to the cleanup, there is no close(fd) involved?\n> I think there are code paths now, which leak fd.\n\nThis one comes _after_ close(fd), so there is no issue.\n\nA later step does introduce an issue; see $gmane/267975.\n\nThanks.\n"}]}