Re: [PATCH 8/8] dir: avoid removing the current working directory

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Nov 23 2021, Elijah Newren wrote:

> On Tue, Nov 23, 2021 at 10:19 AM Glen Choo <chooglen@google.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> 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@gmail.com/,
>> 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.