On Tue, Nov 23 2021, Elijah Newren wrote: > On Tue, Nov 23, 2021 at 10:19 AM Glen Choo <chooglen@xxxxxxxxxx> wrote: >> >> Elijah Newren <newren@xxxxxxxxx> writes: >> [...] >> I'm not going to say that we'll *definitely* need remove_path() >> in its current form, but mixing concerns like this is an invitation to >> unexpected behavior. An (imperfect) example that demonstrates this >> principle is https://lore.kernel.org/git/24bffdab139435173712101aaf72f7277298c99d.1632497954.git.gitgitgadget@xxxxxxxxx/, >> where we made a change to a generic path matching function in order to >> speed up unpack_trees(), but accidentally ended up breaking gitignore. > > There's no mixture of concerns; my patch is correcting this library > function to more fully match its documented intent; from dir.h: > > /* tries to remove the path with empty directories along it, > ignores ENOENT */ > int remove_path(const char *path); > > Since the parent process's current working directory is still likely > parked in that directory, there is a good reason to treat it as > non-empty. Thus the cwd should not be one of those directories > removed along with the specified path. No need to die, just stop > removing the leading directories once it hits the cwd (much like it'd > stop once it hit a directory that had files left in it). I can buy that we'd pick this new behavior as a worthwhile trade-off, but not that anyone intended for this to be the behavior all along. I don't think "a process is sitting in it" has ever been anyone's idea of a "non-empty directory". Rather it's what rmdir() returning EEXIST or ENOTEMPTY maps to. Doesn't this series also change the behavior of e.g.: cd contrib/subtree git rm -r ../subtree If so then re the "spidey sense" comment I had earlier: There's no rm codepaths or tests changed by this series, so the implementation of doing it at this lower level might be casting too wide a net. Wouldn't e.g. changing callers that use "remove_dir_recursively()" to use a "remove_dir_recursively_not_cwd()" (or whatever) be a gentler way of introducing this, and make sure that each step of the way we grok what's being changed, that there's test coverage etc.
