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

[Elijah Newren <newren@gmail.com>]

On Tue, Nov 23, 2021 at 10:19 AM Glen Choo <chooglen@google.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> I don't think it's appropriate to implement user-facing concern (don't remove
> >> cwd because this will confuse users) in library functions like remove_path().
> >> remove_path() has other callers e.g. midx.c, and possible future callers e.g.
> >> we're working on adding a command to delete corrupted commit-graphs and this
> >> library function would be extremely handy.
> >
> > I think we'd want this code change for those cases too.  Said another
> > way, why wouldn't these callers want to avoid deleting the original
> > current working directory of the git process (which is likely still
> > the current working directory of the parent process)?  Deleting that
> > directory causes problems regardless of whether it's a user-facing
> > command (rm, merge, stash, etc.) or something more internal (midx or
> > commit-graphs stuff being called by gc) that is doing the deleting.
>
> 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.

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