On Sun, Nov 21, 2021 at 12:56 AM René Scharfe <l.s.r@xxxxxx> wrote:
>
> Am 21.11.21 um 01:46 schrieb Elijah Newren via GitGitGadget:
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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.
> > + 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.
> > +
> > 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);
