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

[Glen Choo <chooglen@google.com>]

Elijah Newren <newren@gmail.com> writes:

>> I agree that most, possibly all, of our commands should prefer to die
>> than to remove the cwd, but that doesn't justify adding
>> application-level concerns to a general-purpose utility function. Even
>> if it sounds overly defensive, having an obviously correct utility
>> function makes it easier for future authors to know exactly what their
>> code is doing and why. And surely if we're imaginative enough, we can
>> definitely dream up some possible use cases for remove_path() that don't
>> want this dying behavior e.g. other applications that link to our
>> libraries, or some new merge strategy that may need to remove + restore
>> the cwd.
>
> Sounds like your objections here are based on a misunderstanding.  I
> totally agree with you that adding dying behavior to these functions
> would be wrong.
>
> My patch doesn't do that.

Ah my mistake, that should be s/die/'stop gently'. Even so, that is not
at the core of my objection, mixing of concerns is.

>> 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);

I don't think that there is a mismatch; reading the implementation +
documented intent seems to make it clear that 'emptiness' is defined by
directory contents, not the presence of any processes using it as its
current working directory.

> 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).

This doesn't sound like a typical definition of 'emptiness' to me, but I
can accept it if others also find it compelling. IOW if your definition
of 'emptiness' is compelling enough, then I'll be convinced that there
is no mixing of concerns and there would be no objection.