Re* [PATCH 3/2] notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 15, 2012, 08:04 UTC
- Message-ID
- <7vzkbijnnn.fsf_-_@alter.siamese.dyndns.org>
- In-Reply-To
- <CALKQrgfpM-y=9O=h33jxirVoOO8dHHO8tWCR9RatxTottpRXFA@mail.gmail.com>
Johan Herland <johan@herland.net> writes:
Show 12 quoted lines
> On Thu, Mar 15, 2012 at 08:16, Junio C Hamano <gitster@pobox.com> wrote: > ... >> I actually suspect that the passing of "only_empty" in the original may be >> a bug in a0f4afb (clean: require double -f options to nuke nested git >> repository and work tree, 2009-06-30), and this patch might be a fix to >> the bug, but I didn't think things through, and it is getting late, so... > > I noticed the same while looking at this function, and I think your > analysis is correct. As it stands, REMOVE_DIR_KEEP_NESTED_GIT only > applies to .git folders located directly in the toplevel dir, and not > inside a subdirectory. That strikes me as odd given the name of the > flag.
This ended up to be totally unrelated to what you wanted to achieve, but here is a potential fix. I only tested this by running the usual tests and additional tests in this patch; we know the coverage of "git clean" is spotty and its implementation is lower quality than others, so please take it with a large grain of salt.
The basic idea is to report from the lower level of recursion if it decided to leave something in the directory back to the upper level, and have the upper level refrain from removing itself (and when this happens, that upper level in turn reports that it kept the directory it is in charge of to its own caller), to avoid returning -1 from the remove_dir_recursively() to the caller when we deliberately not run rmdir() on a directory to prevent "git clean" from issuing a warning() or exiting with an error.
-- >8 -- Subject: clean: preserve nested git worktree in subdirectories
remove_dir_recursively() had a check to avoid removing the directory it was asked to remove without recursing into it and report success when the directory is the top level of a working tree of a nested git repository, to protext such a repository from "clean -f" (without double -f). If a working tree of a nested git repository is in a subdirectory of a toplevel project, however, this protection did not apply.
Pass REMOVE_DIR_KEEP_NESTED_GIT flag down to the recursive removal codepath, and also teach the higher level not to remove the directory it is asked to remove, when the recursed invocation did not remove the directory it was asked to remove due to a nested git repository, as it is not an error to leave the parent directories of such a nested repository.
Signed-off-by: Junio C Hamano <gitster@pobox.com> --- dir.c | 21 ++++++++++++++++----- t/t7300-clean.sh | 27 ++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/dir.c b/dir.c index 6432728..0e09556 100644 --- a/dir.c +++ b/dir.c @@ -1172,23 +1172,27 @@ int is_empty_dir(const char *path) return ret; } -int remove_dir_recursively(struct strbuf *path, int flag) +static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up) { DIR *dir; struct dirent *e; - int ret = 0, original_len = path->len, len; + int ret = 0, original_len = path->len, len, kept_down = 0; int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY); int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL); unsigned char submodule_head[20]; if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) && - !resolve_gitlink_ref(path->buf, "HEAD", submodule_head)) + !resolve_gitlink_ref(path->buf, "HEAD", submodule_head)) { /* Do not descend and nuke a nested git work tree. */ + if (kept_up) + *kept_up = 1; return 0; + } flag &= ~REMOVE_DIR_KEEP_TOPLEVEL; dir = opendir(path->buf); if (!dir) { + /* an empty dir could be removed even if it is unreadble */ if (!keep_toplevel) return rmdir(path->buf); else @@ -1208,7 +1212,7 @@ int remove_dir_recursively(struct strbuf *path, int flag) if (lstat(path->buf, &st)) ; /* fall thru */ else if (S_ISDIR(st.st_mode)) { - if (!remove_dir_recursively(path, flag)) + if (!remove_dir_recurse(path, flag, &kept_down)) continue; /* happy */ } else if (!only_empty && !unlink(path->buf)) continue; /* happy, too */ @@ -1220,11 +1224,18 @@ int remove_dir_recursively(struct strbuf *path, int flag) closedir(dir); strbuf_setlen(path, original_len); - if (!ret && !keep_toplevel) + if (!ret && !keep_toplevel && !kept_down) ret = rmdir(path->buf); + else if (kept_up) + *kept_up = 1; return ret; } +int remove_dir_recursively(struct strbuf *path, int flag) +{ + return remove_dir_recurse(path, flag, NULL); +} + void setup_standard_excludes(struct dir_struct *dir) { const char *path; diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh index 800b536..ccfb54d 100755 --- a/t/t7300-clean.sh +++ b/t/t7300-clean.sh @@ -399,8 +399,8 @@ test_expect_success SANITY 'removal failure' ' ' test_expect_success 'nested git work tree' ' - rm -fr foo bar && - mkdir foo bar && + rm -fr foo bar baz && + mkdir -p foo bar baz/boo && ( cd foo && git init && @@ -412,15 +412,24 @@ test_expect_success 'nested git work tree' ' cd bar && >goodbye.people ) && + ( + cd baz/boo && + git init && + >deeper.world + git add . && + git commit -a -m deeply.nested + ) && git clean -f -d && test -f foo/.git/index && test -f foo/hello.world && + test -f baz/boo/.git/index && + test -f baz/boo/deeper.world && ! test -d bar ' test_expect_success 'force removal of nested git work tree' ' - rm -fr foo bar && - mkdir foo bar && + rm -fr foo bar baz && + mkdir -p foo bar baz/boo && ( cd foo && git init && @@ -432,9 +441,17 @@ test_expect_success 'force removal of nested git work tree' ' cd bar && >goodbye.people ) && + ( + cd baz/boo && + git init && + >deeper.world + git add . && + git commit -a -m deeply.nested + ) && git clean -f -f -d && ! test -d foo && - ! test -d bar + ! test -d bar && + ! test -d baz ' test_expect_success 'git clean -e' '