{"thread":{"id":"39012","subject":"[PATCH 0/3] Improving performance of git clean","startedAt":"2015-04-06T11:48:21Z","lastAt":"2015-04-08T21:29:36Z","messageCount":14,"participants":["Erik Elfström","Torsten Bögershausen","Eric Sunshine","erik elfström"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"259054","messageId":"1428320904-12366-1-git-send-email-erik.elfstrom@gmail.com","threadId":"39012","inReplyTo":null,"subject":"[PATCH 0/3] Improving performance of git clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-06T11:48:21Z","receivedAt":"2015-04-06T11:48:21Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"This series addresses a performance issue of git clean previously\ndiscussed here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/265560/focus=265560\nand here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/266777/focus=266777\n\nThe issue manifests when trying to clean a large number of untracked\ndirectories. In my case this scenario triggered by a test suite\nrunning in the repository creating a directory for each test resulting\nin a build directory with ~100000 sub directories that needs to be\ncleaned. For some extreme cases, clean times of more than 1h have been\nobserved.\n\nWith this series, the time to clean an untracked directory containing\n100000 sub directories goes from 61s to 1.7s.\n\nThe main change is to switch the repository check in\nclean.c:remove_dirs from using refs.c:resolve_gitlink_ref to\nsetup.c:is_git_directory.\n\nOne potential issue that is_git_directory contains the following check:\n\n\tif (getenv(DB_ENVIRONMENT)) {\n\t\tif (access(getenv(DB_ENVIRONMENT), X_OK))\n\t\t\treturn 0;\n\t}\n\nI'm not sure how this will affect this usecase (checking for some\nother nested git repo). Please give some thought to this when\nreviewing.\n\nJeff King also expressed concerns that we may have similar performance\nissues in other commands and that it could be good to unify these \"is\nthis a repo?\"-checks. This series only attempts to solve the git-clean\ncase.\n\nErik Elfström (3):\n t7300: add tests to document behavior of clean and nested git\n p7300: added performance tests for clean\n clean: improve performance when removing lots of directories\n\n builtin/clean.c | 23 ++++++++++++---\n t/perf/p7300-clean.sh | 37 +++++++++++++++++++++++\n t/t7300-clean.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 138 insertions(+), 4 deletions(-)\n create mode 100755 t/perf/p7300-clean.sh\n\n-- \n2.4.0.rc0.37.ga3b75b3\n"},{"id":"259057","messageId":"1428320904-12366-2-git-send-email-erik.elfstrom@gmail.com","threadId":"39012","inReplyTo":"1428320904-12366-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH 1/3] t7300: add tests to document behavior of clean and nested git","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-06T11:48:22Z","receivedAt":"2015-04-06T11:48:22Z","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\nThese tests were added so that I could understand the corner case\nbehaviors of clean and nested repositories and document the changes in\nbehavior that this series will cause. The ones marked as expect\nfailure will be changed to expect success later in the series.\n\n t/t7300-clean.sh | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 82 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 99be5d9..cfdf6d4 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -455,6 +455,88 @@ test_expect_success 'nested git work tree' '\n \t! test -d bar\n '\n \n+test_expect_failure 'nested git (only init) should be kept' '\n+\trm -fr foo bar &&\n+\tmkdir foo bar &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init\n+\t) &&\n+\t(\n+\t\tcd bar &&\n+\t\t>goodbye.people\n+\t) &&\n+\tgit clean -f -d &&\n+\ttest -f foo/.git/HEAD &&\n+\t! test -d bar\n+'\n+\n+test_expect_failure 'nested git (bare) should be kept' '\n+\trm -fr foo bar &&\n+\tmkdir foo bar &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init --bare\n+\t) &&\n+\t(\n+\t\tcd bar &&\n+\t\t>goodbye.people\n+\t) &&\n+\tgit clean -f -d &&\n+\ttest -f foo/HEAD &&\n+\t! test -d bar\n+'\n+\n+test_expect_success 'giving path in nested git work tree will remove it' '\n+\trm -fr foo &&\n+\tmkdir foo &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init &&\n+\t\tmkdir -p bar/baz &&\n+\t\tcd bar/baz &&\n+\t\t>hello.world\n+\t\tgit add . &&\n+\t\tgit commit -a -m nested\n+\t) &&\n+\tgit clean -f -d foo/bar/baz &&\n+\ttest -f foo/.git/HEAD &&\n+\ttest -d foo/bar/ &&\n+\t! test -d foo/bar/baz\n+'\n+\n+test_expect_success 'giving path to nested .git will not remove it' '\n+\trm -fr foo &&\n+\tmkdir foo bar &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init &&\n+\t\t>hello.world\n+\t\tgit add . &&\n+\t\tgit commit -a -m nested\n+\t) &&\n+\tgit clean -f -d foo/.git &&\n+\ttest -f foo/.git/HEAD &&\n+\ttest -d foo/.git/refs &&\n+\ttest -d foo/.git/objects &&\n+\ttest -d bar/\n+'\n+\n+test_expect_success 'giving path to nested .git/ will remove contents' '\n+\trm -fr foo bar &&\n+\tmkdir foo bar &&\n+\t(\n+\t\tcd foo &&\n+\t\tgit init &&\n+\t\t>hello.world\n+\t\tgit add . &&\n+\t\tgit commit -a -m nested\n+\t) &&\n+\tgit clean -f -d foo/.git/ &&\n+\ttest 0 = $(ls -A foo/.git | wc -l) &&\n+\ttest -d foo/.git\n+'\n+\n test_expect_success 'force removal of nested git work tree' '\n \trm -fr foo bar baz &&\n \tmkdir -p foo bar baz/boo &&\n-- \n2.4.0.rc0.37.ga3b75b3\n"},{"id":"259055","messageId":"1428320904-12366-3-git-send-email-erik.elfstrom@gmail.com","threadId":"39012","inReplyTo":"1428320904-12366-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH 2/3] p7300: added performance tests for clean","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-06T11:48:23Z","receivedAt":"2015-04-06T11:48:23Z","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..3f56fb2\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 $(seq 1 500)\n+\tdo\n+\t\tmkdir 500_sub_dirs/dir$i\n+\tdone &&\n+\tfor i in $(seq 1 100)\n+\tdo\n+\t\tcp -r 500_sub_dirs 50000_sub_dirs/dir$i\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 0 = $(ls -A clean_test_dir/ | wc -l)\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 0 = $(ls -A clean_test_dir/ | wc -l)\n+'\n+\n+test_done\n-- \n2.4.0.rc0.37.ga3b75b3\n"},{"id":"259056","messageId":"1428320904-12366-4-git-send-email-erik.elfstrom@gmail.com","threadId":"39012","inReplyTo":"1428320904-12366-1-git-send-email-erik.elfstrom@gmail.com","subject":"[PATCH 3/3] clean: improve performance when removing lots of directories","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-06T11:48:24Z","receivedAt":"2015-04-06T11:48:24Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Before this change, clean used resolve_gitlink_ref to check for the\npresence of nested git repositories. This had the drawback of creating\na ref_cache entry for every directory that should potentially be\ncleaned. The linear search through the ref_cache list caused a massive\nperformance hit for large number of directories.\n\nTeach clean.c:remove_dirs to use setup.c:is_git_directory\ninstead. is_git_directory will actually open HEAD and parse the HEAD\nref but this implies a nested git repository and should be rare when\ncleaning.\n\nUsing is_git_directory should give a more standardized check for what\nis and what isn't a git repository but also gives a slight behavioral\nchange. We will now detect and respect bare and empty nested git\nrepositories (only init run). Update t7300 to reflect this.\n\nThe time to clean an untracked directory containing 100000 sub\ndirectories went from 61s to 1.7s after this change.\n\nSigned-off-by: Erik Elfström \nHelped-by: Jeff King \n---\n builtin/clean.c | 23 +++++++++++++++++++----\n t/t7300-clean.sh | 4 ++--\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 98c103f..e951bd9 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,24 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int is_git_repository(struct strbuf *path)\n+{\n+\tint ret = 0;\n+\tif (is_git_directory(path->buf))\n+\t\tret = 1;\n+\telse {\n+\t\tint orig_path_len = path->len;\n+\t\tif (path->buf[orig_path_len - 1] != '/')\n+\t\t\tstrbuf_addch(path, '/');\n+\t\tstrbuf_addstr(path, \".git\");\n+\t\tif (is_git_directory(path->buf))\n+\t\t\tret = 1;\n+\t\tstrbuf_setlen(path, orig_path_len);\n+\t}\n+\n+\treturn ret;\n+}\n+\n static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \t\tint dry_run, int quiet, int *dir_gone)\n {\n@@ -155,13 +172,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 cfdf6d4..ada569d 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -455,7 +455,7 @@ test_expect_success 'nested git work tree' '\n \t! test -d bar\n '\n \n-test_expect_failure 'nested git (only init) should be kept' '\n+test_expect_success 'nested git (only init) should be kept' '\n \trm -fr foo bar &&\n \tmkdir foo bar &&\n \t(\n@@ -471,7 +471,7 @@ test_expect_failure 'nested git (only init) should be kept' '\n \t! test -d bar\n '\n \n-test_expect_failure 'nested git (bare) should be kept' '\n+test_expect_success 'nested git (bare) should be kept' '\n \trm -fr foo bar &&\n \tmkdir foo bar &&\n \t(\n-- \n2.4.0.rc0.37.ga3b75b3\n"},{"id":"259095","messageId":"5522EF29.9000008@web.de","threadId":"39012","inReplyTo":"1428320904-12366-3-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH 2/3] p7300: added performance tests for clean","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-04-06T20:40:09Z","receivedAt":"2015-04-06T20:40:09Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-04-06 13.48, Erik Elfström wrote:\n> 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> \n> diff --git a/t/perf/p7300-clean.sh b/t/perf/p7300-clean.sh\n> new file mode 100755\n> index 0000000..3f56fb2\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 $(seq 1 500)\nI think \"seq\" is bash-only, and can be easily replaced by \"test_seq\" \n\n> +\tdo\n> +\t\tmkdir 500_sub_dirs/dir$i\n> +\tdone &&\n> +\tfor i in $(seq 1 100)\n> +\tdo\n> +\t\tcp -r 500_sub_dirs 50000_sub_dirs/dir$i\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 0 = $(ls -A clean_test_dir/ | wc -l)\n\nIs the \"ls -A\" portable on all systems:\nhttp://pubs.opengroup.org/onlinepubs/009695399/utilities/ls.html\n\nMy feeling is that the \"emptiness\" of a directory can by tested by simply removing it:\n> +\tgit clean -q -f -d clean_test_dir/ &&\n> +\trmdir clean_test_dir\n(And similar below)\n"},{"id":"259105","messageId":"CAPig+cTHp3rc_o+cpgguXkPj_aUU6-Hw-Kv6RXyGv8Yt06conQ@mail.gmail.com","threadId":"39012","inReplyTo":"1428320904-12366-2-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH 1/3] t7300: add tests to document behavior of clean and nested git","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-06T22:06:50Z","receivedAt":"2015-04-06T22:06:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 6, 2015 at 7:48 AM, Erik Elfström wrote:\n> Signed-off-by: Erik Elfström \n> ---\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index 99be5d9..cfdf6d4 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -455,6 +455,88 @@ test_expect_success 'nested git work tree' '\n> ! test -d bar\n> '\n>\n> +test_expect_failure 'nested git (only init) should be kept' '\n> + rm -fr foo bar &&\n> + mkdir foo bar &&\n> + (\n> + cd foo &&\n> + git init\n> + ) &&\n> + (\n> + cd bar &&\n> + >goodbye.people\n> + ) &&\n\n(minor; ignore if desired) The above could be simplified to:\n\n rm -fr foo bar &&\n git init foo &&\n mkdir bar &&\n >bar/goodbye.people &&\n\n> + git clean -f -d &&\n> + test -f foo/.git/HEAD &&\n> + ! test -d bar\n\nHere and elsewhere, you could instead use test_path_is_file() and\ntest_path_is_missing(), respectively.\n\n> +'\n> +\n> +test_expect_failure 'nested git (bare) should be kept' '\n> + rm -fr foo bar &&\n> + mkdir foo bar &&\n> + (\n> + cd foo &&\n> + git init --bare\n> + ) &&\n> + (\n> + cd bar &&\n> + >goodbye.people\n> + ) &&\n\nSimplified:\n\n rm -fr foo bar &&\n git init --bare foo &&\n mkdir bar &&\n >bar/goodbye.people &&\n\n> + git clean -f -d &&\n> + test -f foo/HEAD &&\n> + ! test -d bar\n> +'\n> +\n> +test_expect_success 'giving path in nested git work tree will remove it' '\n> + rm -fr foo &&\n> + mkdir foo &&\n> + (\n> + cd foo &&\n> + git init &&\n> + mkdir -p bar/baz &&\n> + cd bar/baz &&\n> + >hello.world\n> + git add . &&\n> + git commit -a -m nested\n> + ) &&\n> + git clean -f -d foo/bar/baz &&\n> + test -f foo/.git/HEAD &&\n> + test -d foo/bar/ &&\n\nAlternative, here and elsewhere: test_path_is_dir()\n\n> + ! test -d foo/bar/baz\n> +'\n> +\n> +test_expect_success 'giving path to nested .git will not remove it' '\n> + rm -fr foo &&\n> + mkdir foo bar &&\n> + (\n> + cd foo &&\n> + git init &&\n> + >hello.world\n> + git add . &&\n> + git commit -a -m nested\n> + ) &&\n> + git clean -f -d foo/.git &&\n> + test -f foo/.git/HEAD &&\n> + test -d foo/.git/refs &&\n> + test -d foo/.git/objects &&\n> + test -d bar/\n> +'\n> +\n> +test_expect_success 'giving path to nested .git/ will remove contents' '\n> + rm -fr foo bar &&\n> + mkdir foo bar &&\n> + (\n> + cd foo &&\n> + git init &&\n> + >hello.world\n> + git add . &&\n> + git commit -a -m nested\n> + ) &&\n> + git clean -f -d foo/.git/ &&\n> + test 0 = $(ls -A foo/.git | wc -l) &&\n\nAlthough in the latest POSIX, -A may not be portable.\n\nAlternative: test_dir_is_empty()\n\n> + test -d foo/.git\n> +'\n> +\n> test_expect_success 'force removal of nested git work tree' '\n> rm -fr foo bar baz &&\n> mkdir -p foo bar baz/boo &&\n> --\n> 2.4.0.rc0.37.ga3b75b3\n"},{"id":"259106","messageId":"CAPig+cRSv0qsctJ_2-ZH2y_E7kU_f787ate_5CnnKHE+vBddNw@mail.gmail.com","threadId":"39012","inReplyTo":"5522EF29.9000008@web.de","subject":"Re: [PATCH 2/3] p7300: added performance tests for clean","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-06T22:09:46Z","receivedAt":"2015-04-06T22:09:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 6, 2015 at 4:40 PM, Torsten Bögershausen wrote:\n> On 2015-04-06 13.48, Erik Elfström wrote:\n>> Signed-off-by: Erik Elfström \n>> ---\n>> diff --git a/t/perf/p7300-clean.sh b/t/perf/p7300-clean.sh\n>> new file mode 100755\n>> index 0000000..3f56fb2\n>> --- /dev/null\n>> +++ b/t/perf/p7300-clean.sh\n>> @@ -0,0 +1,37 @@\n>> +#!/bin/sh\n>> +\n>> +test_description=\"Test git-clean performance\"\n>> +\n>> +. ./perf-lib.sh\n>> +\n>> +test_perf_large_repo\n>> +test_checkout_worktree\n>> +\n>> +test_expect_success 'setup untracked directory with many sub dirs' '\n>> + rm -rf 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n>> + mkdir 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n>> + for i in $(seq 1 500)\n> I think \"seq\" is bash-only, and can be easily replaced by \"test_seq\"\n\ntest_seq is definitely desirable. 'seq' is not present on some systems I use.\n\n>\n>> + do\n>> + mkdir 500_sub_dirs/dir$i\n\nYou may want:\n\n mkdir 500_sub_dirs/dir$i || return $?\n\nto catch failure of 'mkdir'.\n\n>> + done &&\n>> + for i in $(seq 1 100)\n>> + do\n>> + cp -r 500_sub_dirs 50000_sub_dirs/dir$i\n\nDitto:\n\n cp -r 500_sub_dirs 50000_sub_dirs/dir$i || return $?\n\n>> + done\n>> +'\n>> +\n>> +test_perf 'clean many untracked sub dirs, check for nested git' '\n>> + rm -rf clean_test_dir/50000_sub_dirs_cpy &&\n>> + cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n>> + git clean -q -f -d clean_test_dir/ &&\n>> + test 0 = $(ls -A clean_test_dir/ | wc -l)\n>\n> Is the \"ls -A\" portable on all systems:\n> http://pubs.opengroup.org/onlinepubs/009695399/utilities/ls.html\n>\n> My feeling is that the \"emptiness\" of a directory can by tested by simply removing it:\n>> + git clean -q -f -d clean_test_dir/ &&\n>> + rmdir clean_test_dir\n> (And similar below)\n\nThere's also the test_dir_is_empty() function which makes the intent\neven clearer than 'rmdir'.\n"},{"id":"259107","messageId":"CAPig+cQOLJcy-QuACrvd+XrCpP74k0SXxj0rBkNneG5Ovnf47Q@mail.gmail.com","threadId":"39012","inReplyTo":"1428320904-12366-4-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH 3/3] clean: improve performance when removing lots of directories","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-06T22:10:17Z","receivedAt":"2015-04-06T22:10:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 6, 2015 at 7:48 AM, Erik Elfström wrote:\n> Before this change, clean used resolve_gitlink_ref to check for the\n> presence of nested git repositories. This had the drawback of creating\n> a ref_cache entry for every directory that should potentially be\n> cleaned. The linear search through the ref_cache list caused a massive\n> performance hit for large number of directories.\n>\n> Teach clean.c:remove_dirs to use setup.c:is_git_directory\n> instead. is_git_directory will actually open HEAD and parse the HEAD\n> ref but this implies a nested git repository and should be rare when\n> cleaning.\n>\n> Using is_git_directory should give a more standardized check for what\n> is and what isn't a git repository but also gives a slight behavioral\n> change. We will now detect and respect bare and empty nested git\n> repositories (only init run). Update t7300 to reflect this.\n>\n> The time to clean an untracked directory containing 100000 sub\n> directories went from 61s to 1.7s after this change.\n\nImpressive.\n\n> Signed-off-by: Erik Elfström \n> Helped-by: Jeff King \n\nIt is customary for your sign-off to be last.\n\nMore below...\n\n> ---\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 98c103f..e951bd9 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -148,6 +147,24 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)\n> return 0;\n> }\n>\n> +static int is_git_repository(struct strbuf *path)\n> +{\n> + int ret = 0;\n> + if (is_git_directory(path->buf))\n> + ret = 1;\n> + else {\n> + int orig_path_len = path->len;\n> + if (path->buf[orig_path_len - 1] != '/')\n\nMinor: I don't know how others feel about it, but I always find it a\nbit disturbing to see a potential negative array access without a\nsafety check that orig_path_len is not 0, either directly in the\nconditional or as a documenting assert().\n\n> + strbuf_addch(path, '/');\n> + strbuf_addstr(path, \".git\");\n> + if (is_git_directory(path->buf))\n> + ret = 1;\n> + strbuf_setlen(path, orig_path_len);\n> + }\n> +\n> + return ret;\n> +}\n> +\n> static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n> int dry_run, int quiet, int *dir_gone)\n> {\n"},{"id":"259147","messageId":"CAMpP7NZS5QB7YJF72OgRwSNM=ui=hm0KGaq_s35TS_5Nd40xOw@mail.gmail.com","threadId":"39012","inReplyTo":"CAPig+cTHp3rc_o+cpgguXkPj_aUU6-Hw-Kv6RXyGv8Yt06conQ@mail.gmail.com","subject":"Re: [PATCH 1/3] t7300: add tests to document behavior of clean and nested git","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-07T19:27:19Z","receivedAt":"2015-04-07T19:27:19Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"will fix!\n\nOn Tue, Apr 7, 2015 at 12:06 AM, Eric Sunshine wrote:\n> On Mon, Apr 6, 2015 at 7:48 AM, Erik Elfström wrote:\n>> Signed-off-by: Erik Elfström \n>> ---\n>> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n>> index 99be5d9..cfdf6d4 100755\n>> --- a/t/t7300-clean.sh\n>> +++ b/t/t7300-clean.sh\n>> @@ -455,6 +455,88 @@ test_expect_success 'nested git work tree' '\n>> ! test -d bar\n>> '\n>>\n>> +test_expect_failure 'nested git (only init) should be kept' '\n>> + rm -fr foo bar &&\n>> + mkdir foo bar &&\n>> + (\n>> + cd foo &&\n>> + git init\n>> + ) &&\n>> + (\n>> + cd bar &&\n>> + >goodbye.people\n>> + ) &&\n>\n> (minor; ignore if desired) The above could be simplified to:\n>\n> rm -fr foo bar &&\n> git init foo &&\n> mkdir bar &&\n> >bar/goodbye.people &&\n>\n>> + git clean -f -d &&\n>> + test -f foo/.git/HEAD &&\n>> + ! test -d bar\n>\n> Here and elsewhere, you could instead use test_path_is_file() and\n> test_path_is_missing(), respectively.\n>\n>> +'\n>> +\n>> +test_expect_failure 'nested git (bare) should be kept' '\n>> + rm -fr foo bar &&\n>> + mkdir foo bar &&\n>> + (\n>> + cd foo &&\n>> + git init --bare\n>> + ) &&\n>> + (\n>> + cd bar &&\n>> + >goodbye.people\n>> + ) &&\n>\n> Simplified:\n>\n> rm -fr foo bar &&\n> git init --bare foo &&\n> mkdir bar &&\n> >bar/goodbye.people &&\n>\n>> + git clean -f -d &&\n>> + test -f foo/HEAD &&\n>> + ! test -d bar\n>> +'\n>> +\n>> +test_expect_success 'giving path in nested git work tree will remove it' '\n>> + rm -fr foo &&\n>> + mkdir foo &&\n>> + (\n>> + cd foo &&\n>> + git init &&\n>> + mkdir -p bar/baz &&\n>> + cd bar/baz &&\n>> + >hello.world\n>> + git add . &&\n>> + git commit -a -m nested\n>> + ) &&\n>> + git clean -f -d foo/bar/baz &&\n>> + test -f foo/.git/HEAD &&\n>> + test -d foo/bar/ &&\n>\n> Alternative, here and elsewhere: test_path_is_dir()\n>\n>> + ! test -d foo/bar/baz\n>> +'\n>> +\n>> +test_expect_success 'giving path to nested .git will not remove it' '\n>> + rm -fr foo &&\n>> + mkdir foo bar &&\n>> + (\n>> + cd foo &&\n>> + git init &&\n>> + >hello.world\n>> + git add . &&\n>> + git commit -a -m nested\n>> + ) &&\n>> + git clean -f -d foo/.git &&\n>> + test -f foo/.git/HEAD &&\n>> + test -d foo/.git/refs &&\n>> + test -d foo/.git/objects &&\n>> + test -d bar/\n>> +'\n>> +\n>> +test_expect_success 'giving path to nested .git/ will remove contents' '\n>> + rm -fr foo bar &&\n>> + mkdir foo bar &&\n>> + (\n>> + cd foo &&\n>> + git init &&\n>> + >hello.world\n>> + git add . &&\n>> + git commit -a -m nested\n>> + ) &&\n>> + git clean -f -d foo/.git/ &&\n>> + test 0 = $(ls -A foo/.git | wc -l) &&\n>\n> Although in the latest POSIX, -A may not be portable.\n>\n> Alternative: test_dir_is_empty()\n>\n>> + test -d foo/.git\n>> +'\n>> +\n>> test_expect_success 'force removal of nested git work tree' '\n>> rm -fr foo bar baz &&\n>> mkdir -p foo bar baz/boo &&\n>> --\n>> 2.4.0.rc0.37.ga3b75b3\n"},{"id":"259148","messageId":"CAMpP7NaKdArh_AA=9fSdWvEzk6MVyptX-y25_j665UA07xTiBQ@mail.gmail.com","threadId":"39012","inReplyTo":"CAPig+cRSv0qsctJ_2-ZH2y_E7kU_f787ate_5CnnKHE+vBddNw@mail.gmail.com","subject":"Re: [PATCH 2/3] p7300: added performance tests for clean","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-07T19:35:07Z","receivedAt":"2015-04-07T19:35:07Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"Will fix!\n\nAlso I forgot to ask, does anyone have a good way of moving the copy\nout of the performance timing?\n\nAfter the fix this test spends more time copying than cleaning and\nthat is not so good. I'm not very good at shell scripting and the only\nway I could think of was to make multiple copies at the start and then\nin the test check and clean the first non empty directory. That feels\nkind of ugly and will fail if a different number of iterations per\ntest is used. Any suggestions?\n\nI looked a bit at the framework code but I couldn't figure out if it\nwas easy to add a \"setup\" option to be called be before each\niteration.\n\nOn Tue, Apr 7, 2015 at 12:09 AM, Eric Sunshine wrote:\n> On Mon, Apr 6, 2015 at 4:40 PM, Torsten Bögershausen wrote:\n>> On 2015-04-06 13.48, Erik Elfström wrote:\n>>> Signed-off-by: Erik Elfström \n>>> ---\n>>> diff --git a/t/perf/p7300-clean.sh b/t/perf/p7300-clean.sh\n>>> new file mode 100755\n>>> index 0000000..3f56fb2\n>>> --- /dev/null\n>>> +++ b/t/perf/p7300-clean.sh\n>>> @@ -0,0 +1,37 @@\n>>> +#!/bin/sh\n>>> +\n>>> +test_description=\"Test git-clean performance\"\n>>> +\n>>> +. ./perf-lib.sh\n>>> +\n>>> +test_perf_large_repo\n>>> +test_checkout_worktree\n>>> +\n>>> +test_expect_success 'setup untracked directory with many sub dirs' '\n>>> + rm -rf 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n>>> + mkdir 500_sub_dirs 50000_sub_dirs clean_test_dir &&\n>>> + for i in $(seq 1 500)\n>> I think \"seq\" is bash-only, and can be easily replaced by \"test_seq\"\n>\n> test_seq is definitely desirable. 'seq' is not present on some systems I use.\n>\n>>\n>>> + do\n>>> + mkdir 500_sub_dirs/dir$i\n>\n> You may want:\n>\n> mkdir 500_sub_dirs/dir$i || return $?\n>\n> to catch failure of 'mkdir'.\n>\n>>> + done &&\n>>> + for i in $(seq 1 100)\n>>> + do\n>>> + cp -r 500_sub_dirs 50000_sub_dirs/dir$i\n>\n> Ditto:\n>\n> cp -r 500_sub_dirs 50000_sub_dirs/dir$i || return $?\n>\n>>> + done\n>>> +'\n>>> +\n>>> +test_perf 'clean many untracked sub dirs, check for nested git' '\n>>> + rm -rf clean_test_dir/50000_sub_dirs_cpy &&\n>>> + cp -r 50000_sub_dirs clean_test_dir/50000_sub_dirs_cpy &&\n>>> + git clean -q -f -d clean_test_dir/ &&\n>>> + test 0 = $(ls -A clean_test_dir/ | wc -l)\n>>\n>> Is the \"ls -A\" portable on all systems:\n>> http://pubs.opengroup.org/onlinepubs/009695399/utilities/ls.html\n>>\n>> My feeling is that the \"emptiness\" of a directory can by tested by simply removing it:\n>>> + git clean -q -f -d clean_test_dir/ &&\n>>> + rmdir clean_test_dir\n>> (And similar below)\n>\n> There's also the test_dir_is_empty() function which makes the intent\n> even clearer than 'rmdir'.\n"},{"id":"259149","messageId":"CAPig+cQ9DD8BwG6-KDRqLzJSYh6O7c--GD0TR7-VAf_7PZx+Zw@mail.gmail.com","threadId":"39012","inReplyTo":"1428320904-12366-2-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH 1/3] t7300: add tests to document behavior of clean and nested git","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-07T19:40:12Z","receivedAt":"2015-04-07T19:40:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 6, 2015 at 7:48 AM, Erik Elfström wrote:\n> Signed-off-by: Erik Elfström \n> ---\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index 99be5d9..cfdf6d4 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -455,6 +455,88 @@ test_expect_success 'nested git work tree' '\n> +test_expect_success 'giving path in nested git work tree will remove it' '\n> + rm -fr foo &&\n> + mkdir foo &&\n> + (\n> + cd foo &&\n> + git init &&\n> + mkdir -p bar/baz &&\n> + cd bar/baz &&\n> + >hello.world\n\nIn my earlier review, I utterly forgot to mention the broken &&-chain\nhere and throughout the patch.\n\n> + git add . &&\n> + git commit -a -m nested\n> + ) &&\n> + git clean -f -d foo/bar/baz &&\n> + test -f foo/.git/HEAD &&\n> + test -d foo/bar/ &&\n> + ! test -d foo/bar/baz\n> +'\n"},{"id":"259150","messageId":"552435D2.4040900@web.de","threadId":"39012","inReplyTo":"CAPig+cQ9DD8BwG6-KDRqLzJSYh6O7c--GD0TR7-VAf_7PZx+Zw@mail.gmail.com","subject":"Re: [PATCH 1/3] t7300: add tests to document behavior of clean and nested git","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-04-07T19:53:54Z","receivedAt":"2015-04-07T19:53:54Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-04-07 21.40, Eric Sunshine wrote:\n> On Mon, Apr 6, 2015 at 7:48 AM, Erik Elfström wrote:\n>> Signed-off-by: Erik Elfström \n>> ---\n>> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n>> index 99be5d9..cfdf6d4 100755\n>> --- a/t/t7300-clean.sh\n>> +++ b/t/t7300-clean.sh\n>> @@ -455,6 +455,88 @@ test_expect_success 'nested git work tree' '\n>> +test_expect_success 'giving path in nested git work tree will remove it' '\n>> + rm -fr foo &&\n>> + mkdir foo &&\n>> + (\n>> + cd foo &&\n>> + git init &&\n>> + mkdir -p bar/baz &&\n>> + cd bar/baz &&\n>> + >hello.world\n> \n> In my earlier review, I utterly forgot to mention the broken &&-chain\n> here and throughout the patch.\n> \n>> + git add . &&\n>> + git commit -a -m nested\n>> + ) &&\nBeside that, all \"cd\" commands should be done within an own sub-shell.\nIn other words, something like this:\n\t\tmkdir -p bar/baz &&\n\t\t(\n \t\tcd bar/baz &&\n \t\t>hello.world &&\n \t\tgit add . &&\n \t\tgit commit -a -m nested\n\t\t)\nSide note:\nNeeded to drop Eric:\nAn error occurred while sending mail. The mail server responded: Requested action not taken: mailbox unavailable\ninvalid DNS MX or A/AAAA resource record. Please check the message recipient sunshine@sunshineco.com and try again.\n"},{"id":"259151","messageId":"CAMpP7NYixn4491EdPTDX+RQFr3VZfuAoUWZ4JXuYg2rqp9XTeg@mail.gmail.com","threadId":"39012","inReplyTo":"CAPig+cQOLJcy-QuACrvd+XrCpP74k0SXxj0rBkNneG5Ovnf47Q@mail.gmail.com","subject":"Re: [PATCH 3/3] clean: improve performance when removing lots of directories","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-04-07T19:55:24Z","receivedAt":"2015-04-07T19:55:24Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Tue, Apr 7, 2015 at 12:10 AM, Eric Sunshine wrote:\n> On Mon, Apr 6, 2015 at 7:48 AM, Erik Elfström wrote:\n>> Before this change, clean used resolve_gitlink_ref to check for the\n>> presence of nested git repositories. This had the drawback of creating\n>> a ref_cache entry for every directory that should potentially be\n>> cleaned. The linear search through the ref_cache list caused a massive\n>> performance hit for large number of directories.\n>>\n>> Teach clean.c:remove_dirs to use setup.c:is_git_directory\n>> instead. is_git_directory will actually open HEAD and parse the HEAD\n>> ref but this implies a nested git repository and should be rare when\n>> cleaning.\n>>\n>> Using is_git_directory should give a more standardized check for what\n>> is and what isn't a git repository but also gives a slight behavioral\n>> change. We will now detect and respect bare and empty nested git\n>> repositories (only init run). Update t7300 to reflect this.\n>>\n>> The time to clean an untracked directory containing 100000 sub\n>> directories went from 61s to 1.7s after this change.\n>\n> Impressive.\n>\n>> Signed-off-by: Erik Elfström \n>> Helped-by: Jeff King \n>\n> It is customary for your sign-off to be last.\n>\n> More below...\n>\n>> ---\n>> diff --git a/builtin/clean.c b/builtin/clean.c\n>> index 98c103f..e951bd9 100644\n>> --- a/builtin/clean.c\n>> +++ b/builtin/clean.c\n>> @@ -148,6 +147,24 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)\n>> return 0;\n>> }\n>>\n>> +static int is_git_repository(struct strbuf *path)\n>> +{\n>> + int ret = 0;\n>> + if (is_git_directory(path->buf))\n>> + ret = 1;\n>> + else {\n>> + int orig_path_len = path->len;\n>> + if (path->buf[orig_path_len - 1] != '/')\n>\n> Minor: I don't know how others feel about it, but I always find it a\n> bit disturbing to see a potential negative array access without a\n> safety check that orig_path_len is not 0, either directly in the\n> conditional or as a documenting assert().\n>\n\n\nI think I would prefer to accept empty input and return false rather\nthan assert. What to you think about:\n\nstatic int is_git_repository(struct strbuf *path)\n{\n int ret = 0;\n size_t orig_path_len = path->len;\n if (orig_path_len == 0)\n ret = 0;\n else if (is_git_directory(path->buf))\n ret = 1;\n else {\n if (path->buf[orig_path_len - 1] != '/')\n strbuf_addch(path, '/');\n strbuf_addstr(path, \".git\");\n if (is_git_directory(path->buf))\n ret = 1;\n strbuf_setlen(path, orig_path_len);\n }\n\n return ret;\n}\n\n\nAlso I borrowed this pattern from remove_dirs and it has the same\nproblem. Should I add something like this as a separate commit?\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex ccffd8a..88850e3 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -173,7 +173,8 @@ static int remove_dirs(struct strbuf *path, const\nchar *prefix, int force_flag,\n DIR *dir;\n struct strbuf quoted = STRBUF_INIT;\n struct dirent *e;\n- int res = 0, ret = 0, gone = 1, original_len = path->len, len;\n+ int res = 0, ret = 0, gone = 1;\n+ size_t original_len = path->len, len;\n struct string_list dels = STRING_LIST_INIT_DUP;\n\n *dir_gone = 1;\n@@ -201,6 +202,7 @@ static int remove_dirs(struct strbuf *path, const\nchar *prefix, int force_flag,\n return res;\n }\n\n+ assert(original_len > 0 && \"expects non-empty path\");\n if (path->buf[original_len - 1] != '/')\n strbuf_addch(path, '/');\n\n\n>> + strbuf_addch(path, '/');\n>> + strbuf_addstr(path, \".git\");\n>> + if (is_git_directory(path->buf))\n>> + ret = 1;\n>> + strbuf_setlen(path, orig_path_len);\n>> + }\n>> +\n>> + return ret;\n>> +}\n>> +\n>> static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n>> int dry_run, int quiet, int *dir_gone)\n>> {\n"},{"id":"259188","messageId":"CAPig+cSAMT1sECNueDckLMvBQ9q47L_t-wsnUeJ3WjCHvtOEAQ@mail.gmail.com","threadId":"39012","inReplyTo":"CAMpP7NYixn4491EdPTDX+RQFr3VZfuAoUWZ4JXuYg2rqp9XTeg@mail.gmail.com","subject":"Re: [PATCH 3/3] clean: improve performance when removing lots of directories","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-08T21:29:36Z","receivedAt":"2015-04-08T21:29:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 7, 2015 at 3:55 PM, erik elfström wrote:\n> On Tue, Apr 7, 2015 at 12:10 AM, Eric Sunshine wrote:\n>> On Mon, Apr 6, 2015 at 7:48 AM, Erik Elfström wrote:\n>>> diff --git a/builtin/clean.c b/builtin/clean.c\n>>> index 98c103f..e951bd9 100644\n>>> --- a/builtin/clean.c\n>>> +++ b/builtin/clean.c\n>>> +static int is_git_repository(struct strbuf *path)\n>>> +{\n>>> + int ret = 0;\n>>> + if (is_git_directory(path->buf))\n>>> + ret = 1;\n>>> + else {\n>>> + int orig_path_len = path->len;\n>>> + if (path->buf[orig_path_len - 1] != '/')\n>>\n>> Minor: I don't know how others feel about it, but I always find it a\n>> bit disturbing to see a potential negative array access without a\n>> safety check that orig_path_len is not 0, either directly in the\n>> conditional or as a documenting assert().\n>\n> I think I would prefer to accept empty input and return false rather\n> than assert. What to you think about:\n>\n> static int is_git_repository(struct strbuf *path)\n> {\n> int ret = 0;\n> size_t orig_path_len = path->len;\n> if (orig_path_len == 0)\n> ret = 0;\n\nMy concern in raising the issue is that someone reviewing the patch or\nreading the code later won't necessarily know whether you took the\npotential negative array access into account and dismissed it as\n\"can't happen\", or if you overlooked the possibility altogether. Had\nthere been an explicit check in the code (either assert() or other\nspecial handling such as returning 'false'), a comment in the code, or\nmention in the commit message, then it would have been clear that you\ntook the case into consideration, and I wouldn't have worried about\nit.\n\nAs for the this proposed version of is_git_repository(), I don't have\nstrong feelings, and can formulate arguments either way. If it doesn't\nmake sense for is_git_repository() ever to be called with empty input,\nthen assert() may be the better choice for documenting that fact.\nHowever, if you foresee some need for allowing empty input, or if you\naudited the functionality and found that it can already be called with\nempty input, then returning 'false' makes sense. Use your best\njudgment.\n\n> else if (is_git_directory(path->buf))\n> ret = 1;\n> else {\n> if (path->buf[orig_path_len - 1] != '/')\n> strbuf_addch(path, '/');\n> strbuf_addstr(path, \".git\");\n> if (is_git_directory(path->buf))\n> ret = 1;\n> strbuf_setlen(path, orig_path_len);\n> }\n>\n> return ret;\n> }\n>\n>\n> Also I borrowed this pattern from remove_dirs and it has the same\n> problem. Should I add something like this as a separate commit?\n>\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index ccffd8a..88850e3 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -201,6 +202,7 @@ static int remove_dirs(struct strbuf *path, const\n> char *prefix, int force_flag,\n> return res;\n> }\n>\n> + assert(original_len > 0 && \"expects non-empty path\");\n> if (path->buf[original_len - 1] != '/')\n> strbuf_addch(path, '/');\n\nI personally wouldn't mind such a patch. (I'm not sure that the string\nwithin the assert() adds much value, and it's a not-much-used idiom\nwithin the Git source.)\n"}]}