{"thread":{"id":"39110","subject":"[PATCH/RFC v3 0/4] Improving performance of git clean","startedAt":"2015-04-18T20:41:08Z","lastAt":"2015-04-22T19:53:08Z","messageCount":14,"participants":["Erik Elfström","Junio C Hamano","Thomas Gummerer","erik elfström","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"259629","messageId":"1429389672-30209-1-git-send-email-erik.elfstrom@gmail.com","threadId":"39110","inReplyTo":null,"subject":"[PATCH/RFC v3 0/4] Improving performance of git clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-18T20:41:08Z","receivedAt":"2015-04-18T20:41:08Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"I've marked this RFC since there are known problems here.\n\nv2 of the patch can be found here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/267023/focus=267023\n\nChanges in v3:\n* Created setup.c:read_gitfile_gently to use for submodule\n probing\n* Cleanup of some tests by use of test_commit helper\n* Added more tests of cleaning in the presence of submodules\n* Reversed expectation of test for cleaning nested bare repos.\n They are now expected to be cleaned. Added one more case.\n* Fixed bug where submodules could be cleaned by using new\n read_gitfile_gently for additional submodule check in\n clean.c:is_git_repository\n* Attempt to change behavior of patch implementation to clean\n bare repositories (only partially successful)\n* Reworded commit message of the performance fix commit\n\nKnown Problems:\n* Unsure about the setup.c:read_gitfile refactor, feels a bit\n messy?\n* Potentially a missing sanity check of git file size in\n setup.c:read_gitfile_gently_or_non_gently\n* We still get a behavioral change for empty bare repositories\n placed in a \".git\" directory. Currently we clean empty bare\n repos in a .git folder but not non-empty one. After this\n patch we won't clean either. How serious is this? Is there\n an easy fix (preferebly to clean all bare repositories)?\n* Still have issues in the performance tests, see comments\n from Thomas Gummerer on v2\n\nThanks to Junio C Hamano and Jeff King for spotting fundamental\nproblems in v2 and suggesting a solution.\n\nErik Elfström (4):\n setup: add gentle version of read_gitfile\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 | 25 ++++++++--\n cache.h | 1 +\n setup.c | 94 ++++++++++++++++++++++++++++---------\n t/perf/p7300-clean.sh | 37 +++++++++++++++\n t/t7300-clean.sh | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 257 insertions(+), 25 deletions(-)\n create mode 100755 t/perf/p7300-clean.sh\n\n-- \n2.4.0.rc2.5.g2871d5e\n"},{"id":"259630","messageId":"1429389672-30209-2-git-send-email-erik.elfstrom@gmail.com","threadId":"39110","inReplyTo":"1429389672-30209-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH/RFC v3 1/4] setup: add gentle version of read_gitfile","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-18T20:41:09Z","receivedAt":"2015-04-18T20:41:09Z","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---\nIf this is going to be used for speculative probing should there\nbe a sanity check before:\n\tbuf = xmalloc(st.st_size + 1);\n\tlen = read_in_full(fd, buf, st.st_size);\nSomething like:\n\tif (st.st_size > PATH_MAX*2) {\n\t\terror = N;\n\t\tgoto cleanup_return;\n\t{\nWhat do you think?\n\n cache.h | 1 +\n setup.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------\n 2 files changed, 74 insertions(+), 21 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3d3244b..9d9199d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -432,6 +432,7 @@ 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);\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..a33b293 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -332,38 +332,46 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\n \treturn 0;\n }\n \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-const char *read_gitfile(const char *path)\n+static const char *read_gitfile_gently_or_non_gently(const char *path, int gently)\n {\n-\tchar *buf;\n+\tint error = 0;\n+\tchar *buf = NULL;\n \tchar *dir;\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 = 1;\n+\t\tgoto cleanup_return;\n+\t}\n+\tif (!S_ISREG(st.st_mode)) {\n+\t\terror = 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 = 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 = 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 = 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 = 6;\n+\t\tgoto cleanup_return;\n+\t}\n \tbuf[len] = '\\0';\n \tdir = buf + 8;\n \n@@ -378,14 +386,58 @@ 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 = 7;\n+\t\tgoto cleanup_return;\n+\t}\n \tpath = real_path(dir);\n \n+cleanup_return:\n \tfree(buf);\n+\n+\tif (error) {\n+\t\tif (gently)\n+\t\t\treturn NULL;\n+\n+\t\tswitch (error) {\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+/*\n+ * Try to read the location of the git directory from the .git file,\n+ * return path to git directory if found, die on (most) failures.\n+ */\n+const char *read_gitfile(const char *path)\n+{\n+\treturn read_gitfile_gently_or_non_gently(path, 0);\n+}\n+\n+/*\n+ * Same as read_gitfile but return NULL on failure.\n+ */\n+const char *read_gitfile_gently(const char *path)\n+{\n+\treturn read_gitfile_gently_or_non_gently(path, 1);\n+}\n+\n static const char *setup_explicit_git_dir(const char *gitdirenv,\n \t\t\t\t\t struct strbuf *cwd,\n \t\t\t\t\t int *nongit_ok)\n-- \n2.4.0.rc2.5.g2871d5e\n"},{"id":"259632","messageId":"1429389672-30209-3-git-send-email-erik.elfstrom@gmail.com","threadId":"39110","inReplyTo":"1429389672-30209-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH/RFC v3 2/4] t7300: add tests to document behavior of clean and nested git","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-18T20:41:10Z","receivedAt":"2015-04-18T20:41:10Z","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 | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 127 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 99be5d9..4b9a72a 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -455,6 +455,133 @@ 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 foo bar &&\n+\tgit init foo &&\n+\tmkdir bar &&\n+\t>bar/goodbye.people &&\n+\tgit clean -f -d &&\n+\ttest_path_is_file foo/.git/HEAD &&\n+\ttest_path_is_missing bar\n+'\n+\n+test_expect_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 foo &&\n+\tmkdir foo &&\n+\t(\n+\t\tcd foo &&\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 foo/bar/baz &&\n+\ttest_path_is_file foo/.git/HEAD &&\n+\ttest_path_is_dir foo/bar/ &&\n+\ttest_path_is_missing foo/bar/baz\n+'\n+\n+test_expect_success 'giving path to nested .git will not remove it' '\n+\trm -fr foo &&\n+\tmkdir foo bar &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init &&\n+\t\ttest_commit msg hello.world\n+\t) &&\n+\tgit clean -f -d foo/.git &&\n+\ttest_path_is_file foo/.git/HEAD &&\n+\ttest_path_is_dir foo/.git/refs &&\n+\ttest_path_is_dir foo/.git/objects &&\n+\ttest_path_is_dir bar/\n+'\n+\n+test_expect_success 'giving path to nested .git/ will remove contents' '\n+\trm -fr foo bar &&\n+\tmkdir foo bar &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init &&\n+\t\ttest_commit msg hello.world\n+\t) &&\n+\tgit clean -f -d foo/.git/ &&\n+\ttest_path_is_dir foo/.git &&\n+\ttest_dir_is_empty foo/.git\n+'\n+\n test_expect_success 'force removal of nested git work tree' '\n \trm -fr foo bar baz &&\n \tmkdir -p foo bar baz/boo &&\n-- \n2.4.0.rc2.5.g2871d5e\n"},{"id":"259633","messageId":"1429389672-30209-4-git-send-email-erik.elfstrom@gmail.com","threadId":"39110","inReplyTo":"1429389672-30209-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH/RFC v3 3/4] p7300: add performance tests for clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-18T20:41:11Z","receivedAt":"2015-04-18T20:41:11Z","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/perf/p7300-clean.sh | 37 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n create mode 100755 t/perf/p7300-clean.sh\n\ndiff --git a/t/perf/p7300-clean.sh b/t/perf/p7300-clean.sh\nnew file mode 100755\nindex 0000000..af50d5d\n--- /dev/null\n+++ b/t/perf/p7300-clean.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description=\"Test git-clean performance\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+test_expect_success 'setup untracked directory with many sub dirs' '\n+\trm -rf 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n+\tmkdir 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n+\tfor i in $(test_seq 1 500)\n+\tdo\n+\t\tmkdir 500_sub_dirs/dir$i || return $?\n+\tdone &&\n+\tfor i in $(test_seq 1 100)\n+\tdo\n+\t\tcp -r 500_sub_dirs 50000_sub_dirs/dir$i || return $?\n+\tdone\n+'\n+\n+test_perf 'clean many untracked sub dirs, check for nested git' '\n+\trm -rf clean_test_dir/50000_sub_dirs_cpy &&\n+\tcp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n+\tgit clean -q -f -d clean_test_dir/ &&\n+\ttest_dir_is_empty clean_test_dir\n+'\n+\n+test_perf 'clean many untracked sub dirs, ignore nested git' '\n+\trm -rf clean_test_dir/50000_sub_dirs_cpy &&\n+\tcp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n+\tgit clean -q -f -f -d clean_test_dir/ &&\n+\ttest_dir_is_empty clean_test_dir\n+'\n+\n+test_done\n-- \n2.4.0.rc2.5.g2871d5e\n"},{"id":"259631","messageId":"1429389672-30209-5-git-send-email-erik.elfstrom@gmail.com","threadId":"39110","inReplyTo":"1429389672-30209-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH/RFC v3 4/4] clean: improve performance when removing lots of directories","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-18T20:41:12Z","receivedAt":"2015-04-18T20:41:12Z","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 a slight behavioral change. We will now detect and respect\nempty nested git repositories (only init run) and empty bare\nrepositories that have been placed in a \".git\" directory. We will also\nno longer die when cleaning a file named \".git\" with garbage content\n(it will be cleaned instead). Update t7300 to reflect this.\n\nThe time to clean an untracked directory containing 100000 sub\ndirectories went from 61s to 1.7s after this change.\n\nHelped-by: Jeff King \nSigned-off-by: Erik Elfström \n---\n builtin/clean.c | 25 +++++++++++++++++++++----\n t/t7300-clean.sh | 8 +++-----\n 2 files changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 98c103f..5cda3c5 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,26 @@ 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+\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) || 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 +174,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 4b9a72a..1bbb8ef 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 foo bar &&\n \tgit init foo &&\n \tmkdir bar &&\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.rc2.5.g2871d5e\n"},{"id":"259638","messageId":"xmqqfv7wor6e.fsf@gitster.dls.corp.google.com","threadId":"39110","inReplyTo":"1429389672-30209-1-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH/RFC v3 0/4] Improving performance of git clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-19T01:14:17Z","receivedAt":"2015-04-19T01:14:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Elfström writes:\n\n> Known Problems:\n> * Unsure about the setup.c:read_gitfile refactor, feels a bit\n> messy?\n\nThe interface indeed feels somewhat messy. I suspect that a better\ninterface might be more like setup_git_directory_gently() that is a\ngentler version of setup_git_directory(). The function takes an\noptional pointer to an int, and it behaves not-gently-at-all when\nthe optional argument is NULL. The location the optional pointer\npoints at, when it is not NULL, can be used to return the error\nstate to the caller. That function pair only uses the optional\nargument to convey one bit of information (i.e. \"are we in any git\nrepository or not?\") back to the caller, but the interface could be\nused to tell the caller a lot more if we wanted to.\n\nIf you model read_gitfile_gently() after that pattern, I would\nexpect that\n\n - The extra pattern would be \"int *error\";\n - The implementation of read_gitfile() would be\n\n #define read_gitfile(path) read_gitfile_gently((path), NULL)\n\n and the _gently() version will die when \"error\" parameter is set\n to NULL and finds any error.\n\n - The caller of the gentle variant can use the error code to\n determine what went wrong, not just the fact that it failed. I\n do not think your caller does not have an immediate need to tell\n between \"invalid gitfile format\" and \"No path in gitfile\", but\n such an interface leaves that possibility open.\n\nThanks.\n"},{"id":"259712","messageId":"20150420221414.GA13813@hank","threadId":"39110","inReplyTo":"1429389672-30209-1-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH/RFC v3 0/4] Improving performance of git clean","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-04-20T22:14:14Z","receivedAt":"2015-04-20T22:14:14Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/18, Erik Elfström wrote:\n> * Still have issues in the performance tests, see comments\n> from Thomas Gummerer on v2\n\nI've looked at the \"modern\" style tests again, and I don't the code\nchurn is worth it just for using them for the performance tests. If\nanyone wants to take a look at the code, it's at\ngithub.com/tgummerer/git tg/perf-lib.\n\nI think adding the test_perf_setup_cleanup command would make more\nsense in this case. If you want I can send a patch for that.\n"},{"id":"259751","messageId":"CAMpP7NaFYmpjbVVJSXX9OTiOm=ycL+nABgLTHBDV3h75ZqbVTw@mail.gmail.com","threadId":"39110","inReplyTo":"xmqqfv7wor6e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC v3 0/4] Improving performance of git clean","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-21T18:17:04Z","receivedAt":"2015-04-21T18:17:04Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Sun, Apr 19, 2015 at 3:14 AM, Junio C Hamano wrote:\n> Erik Elfström writes:\n>\n>> Known Problems:\n>> * Unsure about the setup.c:read_gitfile refactor, feels a bit\n>> messy?\n>\n> The interface indeed feels somewhat messy. I suspect that a better\n> interface might be more like setup_git_directory_gently() that is a\n> gentler version of setup_git_directory(). The function takes an\n> optional pointer to an int, and it behaves not-gently-at-all when\n> the optional argument is NULL. The location the optional pointer\n> points at, when it is not NULL, can be used to return the error\n> state to the caller. That function pair only uses the optional\n> argument to convey one bit of information (i.e. \"are we in any git\n> repository or not?\") back to the caller, but the interface could be\n> used to tell the caller a lot more if we wanted to.\n>\n> If you model read_gitfile_gently() after that pattern, I would\n> expect that\n>\n> - The extra pattern would be \"int *error\";\n> - The implementation of read_gitfile() would be\n>\n> #define read_gitfile(path) read_gitfile_gently((path), NULL)\n>\n> and the _gently() version will die when \"error\" parameter is set\n> to NULL and finds any error.\n>\n> - The caller of the gentle variant can use the error code to\n> determine what went wrong, not just the fact that it failed. I\n> do not think your caller does not have an immediate need to tell\n> between \"invalid gitfile format\" and \"No path in gitfile\", but\n> such an interface leaves that possibility open.\n>\n> Thanks.\n>\n\nOk, I'll try that approach, thanks. (and apologies for the late reply)\n\n/Erik\n"},{"id":"259752","messageId":"CAMpP7NaUv10Ox0gNsE8cg4hUnNNiFi8NZSLw6F6SW+SLrt0VwQ@mail.gmail.com","threadId":"39110","inReplyTo":"20150420221414.GA13813@hank","subject":"Re: [PATCH/RFC v3 0/4] Improving performance of git clean","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-21T18:21:37Z","receivedAt":"2015-04-21T18:21:37Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Ok, thanks for looking into this.\n\nI have no well founded opinions on the implementation but I do\nthink the performance tests would be more meaningful if the\nsetup/cleanup code could be removed from the timed section.\nIf the community agrees on an implementation I would be happy\nto convert the new tests, either directly in this series or as a follow\nup if that is preferred.\n\n/Erik\n\nOn Tue, Apr 21, 2015 at 12:14 AM, Thomas Gummerer wrote:\n> On 04/18, Erik Elfström wrote:\n>> * Still have issues in the performance tests, see comments\n>> from Thomas Gummerer on v2\n>\n> I've looked at the \"modern\" style tests again, and I don't the code\n> churn is worth it just for using them for the performance tests. If\n> anyone wants to take a look at the code, it's at\n> github.com/tgummerer/git tg/perf-lib.\n>\n> I think adding the test_perf_setup_cleanup command would make more\n> sense in this case. If you want I can send a patch for that.\n"},{"id":"259754","messageId":"xmqqlhhlgv8y.fsf@gitster.dls.corp.google.com","threadId":"39110","inReplyTo":"CAMpP7NaUv10Ox0gNsE8cg4hUnNNiFi8NZSLw6F6SW+SLrt0VwQ@mail.gmail.com","subject":"Re: [PATCH/RFC v3 0/4] Improving performance of git clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-21T19:02:37Z","receivedAt":"2015-04-21T19:02:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"erik elfström writes:\n\n> Ok, thanks for looking into this.\n>\n> I have no well founded opinions on the implementation but I do\n> think the performance tests would be more meaningful if the\n> setup/cleanup code could be removed from the timed section.\n> If the community agrees on an implementation I would be happy\n> to convert the new tests, either directly in this series or as a follow\n> up if that is preferred.\n\nLet's not delay the fix and do the perf thing as a follow-up series,\npossibly an even independent one.\n\nIn other words, let's keep the topics small.\n\n>\n> /Erik\n>\n> On Tue, Apr 21, 2015 at 12:14 AM, Thomas Gummerer wrote:\n>> On 04/18, Erik Elfström wrote:\n>>> * Still have issues in the performance tests, see comments\n>>> from Thomas Gummerer on v2\n>>\n>> I've looked at the \"modern\" style tests again, and I don't the code\n>> churn is worth it just for using them for the performance tests. If\n>> anyone wants to take a look at the code, it's at\n>> github.com/tgummerer/git tg/perf-lib.\n>>\n>> I think adding the test_perf_setup_cleanup command would make more\n>> sense in this case. If you want I can send a patch for that.\n"},{"id":"259762","messageId":"20150421212427.GB13230@peff.net","threadId":"39110","inReplyTo":"CAMpP7NaUv10Ox0gNsE8cg4hUnNNiFi8NZSLw6F6SW+SLrt0VwQ@mail.gmail.com","subject":"Re: [PATCH/RFC v3 0/4] Improving performance of git clean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-21T21:24:27Z","receivedAt":"2015-04-21T21:24:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 21, 2015 at 08:21:37PM +0200, erik elfström wrote:\n\n> Ok, thanks for looking into this.\n> \n> I have no well founded opinions on the implementation but I do\n> think the performance tests would be more meaningful if the\n> setup/cleanup code could be removed from the timed section.\n> If the community agrees on an implementation I would be happy\n> to convert the new tests, either directly in this series or as a follow\n> up if that is preferred.\n\nIf I understand correctly, the reason that you need per-run setup is\nthat your \"git clean\" command actually cleans things, and you need to\nrestore the original state for each time-trial. Can you instead use \"git\nclean -n\" to do a dry-run? I think what you are timing is really the\n\"figure out what to clean\" step, and not the cleaning itself.\n\n-Peff\n"},{"id":"259825","messageId":"CAMpP7NbruKC97L1ROXV0Up9Fks8FJmgB_nxTTWpSHp-8VVE+Xw@mail.gmail.com","threadId":"39110","inReplyTo":"20150421212427.GB13230@peff.net","subject":"Re: [PATCH/RFC v3 0/4] Improving performance of git clean","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-22T19:30:20Z","receivedAt":"2015-04-22T19:30:20Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Tue, Apr 21, 2015 at 11:24 PM, Jeff King wrote:\n>\n> If I understand correctly, the reason that you need per-run setup is\n> that your \"git clean\" command actually cleans things, and you need to\n> restore the original state for each time-trial. Can you instead use \"git\n> clean -n\" to do a dry-run? I think what you are timing is really the\n> \"figure out what to clean\" step, and not the cleaning itself.\n>\n> -Peff\n\n\nYes, that is the problem. A dry run will spot this particular performance\nissue but maybe we lose some value as a general performance test if\nwe only do \"half\" the clean? Admittedly we clearly lose some value in\nthe current state as well due to the copying taking more time than the\ncleaning. I could go either way here.\n\n/Erik\n"},{"id":"259832","messageId":"20150422194601.GB27656@peff.net","threadId":"39110","inReplyTo":"CAMpP7NbruKC97L1ROXV0Up9Fks8FJmgB_nxTTWpSHp-8VVE+Xw@mail.gmail.com","subject":"Re: [PATCH/RFC v3 0/4] Improving performance of git clean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T19:46:01Z","receivedAt":"2015-04-22T19:46:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2015 at 09:30:20PM +0200, erik elfström wrote:\n\n> On Tue, Apr 21, 2015 at 11:24 PM, Jeff King wrote:\n> >\n> > If I understand correctly, the reason that you need per-run setup is\n> > that your \"git clean\" command actually cleans things, and you need to\n> > restore the original state for each time-trial. Can you instead use \"git\n> > clean -n\" to do a dry-run? I think what you are timing is really the\n> > \"figure out what to clean\" step, and not the cleaning itself.\n> \n> Yes, that is the problem. A dry run will spot this particular performance\n> issue but maybe we lose some value as a general performance test if\n> we only do \"half\" the clean? Admittedly we clearly lose some value in\n> the current state as well due to the copying taking more time than the\n> cleaning. I could go either way here.\n\nI guess it is a matter of opinion. I think testing only the \"find out\nwhat to clean\" half separately is actually beneficial, because it helps\nus isolate any slowdown. If we want to add a test for the other half, we\ncan, but I do not actually think it is currently that interesting (it is\njust calling unlink() in a loop).\n\nSo even leaving the practical matters aside, I do not think it is a bad\nthing to split it up. When you add in the fact that it is practically\nmuch easier to test the first half, it seems to me that testing just\nthat is a good first step.\n\n-Peff\n"},{"id":"259834","messageId":"CAMpP7Napqj1riCQ_iNWnGS8m3ncH6+fY+Rppn5cUfDT=n8aUWg@mail.gmail.com","threadId":"39110","inReplyTo":"20150422194601.GB27656@peff.net","subject":"Re: [PATCH/RFC v3 0/4] Improving performance of git clean","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-22T19:53:08Z","receivedAt":"2015-04-22T19:53:08Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Wed, Apr 22, 2015 at 9:46 PM, Jeff King wrote:\n> On Wed, Apr 22, 2015 at 09:30:20PM +0200, erik elfström wrote:\n>>\n>> Yes, that is the problem. A dry run will spot this particular performance\n>> issue but maybe we lose some value as a general performance test if\n>> we only do \"half\" the clean? Admittedly we clearly lose some value in\n>> the current state as well due to the copying taking more time than the\n>> cleaning. I could go either way here.\n>\n> I guess it is a matter of opinion. I think testing only the \"find out\n> what to clean\" half separately is actually beneficial, because it helps\n> us isolate any slowdown. If we want to add a test for the other half, we\n> can, but I do not actually think it is currently that interesting (it is\n> just calling unlink() in a loop).\n>\n> So even leaving the practical matters aside, I do not think it is a bad\n> thing to split it up. When you add in the fact that it is practically\n> much easier to test the first half, it seems to me that testing just\n> that is a good first step.\n>\n> -Peff\n\nSounds reasonable to me. I'll make this change in v4, thanks!\n\n(Sorry for the duplicate email Jeff, I'm bad at this mailing list thing...)\n\n/Erik\n"}]}