On Mon, Nov 22, 2021 at 4:40 PM Glen Choo <chooglen@xxxxxxxxxx> wrote:
>
> > @@ -3259,9 +3259,12 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
> > closedir(dir);
> >
> > strbuf_setlen(path, original_len);
> > - if (!ret && !keep_toplevel && !kept_down)
> > - ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
> > - else if (kept_up)
> > + if (!ret && !keep_toplevel && !kept_down) {
> > + if (the_cwd && !strcmp(the_cwd, path->buf))
> > + ret = -1; /* Do not remove current working directory */
> > + else
> > + ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
> > + } else if (kept_up)
> > /*
> > * report the uplevel that it is not an error that we
> > * did not rmdir() our directory.
> > @@ -3327,6 +3330,8 @@ int remove_path(const char *name)
> > slash = dirs + (slash - name);
> > do {
> > *slash = '\0';
> > + if (the_cwd && !strcmp(the_cwd, dirs))
> > + break;
> > } while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));
> > free(dirs);
> > }
>
> 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.
Putting it in this helper function means we protect all current and
future callers without developers having to remember which
"remove_path()" variant they need and why.
> It seems more appropriate to check the_cwd from builtin/add.c and builtin/rm.c
> instead.
Not sure how you determined that those two paths are affected or that
those are the only two.
