Re: [PATCH 5/8] symlinks: do not include current working directory in dir removal

From: Elijah Newren <newren@gmail.com>
Date: Nov 23, 2021, 00:35 UTC
Message-ID: <CABPp-BF4WtoRig=TUQa76G1sfG3uOiEZw5vk7cGbDtJNahhgnw@mail.gmail.com>
In-Reply-To: <150f9c88-f175-963d-2947-7ae9f3a2ef12@web.de>

On Sun, Nov 21, 2021 at 12:56 AM René Scharfe <l.s.r@web.de> wrote:

Show 31 quoted lines

>
> Am 21.11.21 um 01:46 schrieb Elijah Newren via GitGitGadget:
> > From: Elijah Newren <newren@gmail.com>
> >
> > symlinks has a pair of schedule_dir_for_removal() and
> > remove_scheduled_dirs() functions that ensure that directories made
> > empty by removing other files also themselves get removed.  However, we
> > want to exclude the current working directory and leave it around so
> > that subsequent git commands (and non-git commands) that the user runs
> > afterwards don't cause the user to get confused.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  symlinks.c           | 12 +++++++++++-
> >  t/t2501-cwd-empty.sh | 12 ++++++------
> >  2 files changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/symlinks.c b/symlinks.c
> > index 5232d02020c..84622bedcde 100644
> > --- a/symlinks.c
> > +++ b/symlinks.c
> > @@ -275,11 +275,18 @@ static int threaded_has_dirs_only_path(struct cache_def *cache, const char *name
> >
> >  static struct strbuf removal = STRBUF_INIT;
> >
> > +static int cant_remove(char *dirname)
> > +{
> > +     if (the_cwd && !strcmp(dirname, the_cwd))
>
> Initializing the_cwd to an empty string would allow removing the NULL check
> everywhere.

I actually went and made this change, because although it'd add protection to the toplevel directory (the_cwd is relative to the toplevel), we usually want it protected. However, dir.c's remove_dir_recursively() can be used to remove the toplevel directory as well and has an explicit flag for that, so I need to be able to distinguish between uninitialized and explicitly set to the toplevel directory.

> Is strcmp() sufficient or do we need fspathcmp() in these kinds of checks?
> Do we need to worry about normalizing directory separators?

Good catch, I should normalize the_cwd when I create it; I was essentially creating it to match "prefix", but it appears that isn't pre-emptively normalized, and instead later callers normalize any combination of prefix plus another path.

Show 7 quoted lines

> > +             return 1;
> > +     return rmdir(dirname);
> > +}
>
> I wouldn't expect a function of that name to actually try to remove
> the directory.  Or with that body to require a non-const dirname.
> It's used only once, perhaps inline it?

Sure.

Show 20 quoted lines

> > +
> >  static void do_remove_scheduled_dirs(int new_len)
> >  {
> >       while (removal.len > new_len) {
> >               removal.buf[removal.len] = '\0';
> > -             if (rmdir(removal.buf))
> > +             if (cant_remove(removal.buf))
> >                       break;
> >               do {
> >                       removal.len--;
> > @@ -293,6 +300,9 @@ void schedule_dir_for_removal(const char *name, int len)
> >  {
> >       int match_len, last_slash, i, previous_slash;
> >
> > +     if (the_cwd && !strcmp(name, the_cwd))
> > +             return; /* Do not remove the current working directory */
> > +
> >       match_len = last_slash = i =
> >               longest_path_match(name, len, removal.buf, removal.len,
> >                                  &previous_slash);