diff mbox series

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

Message ID 8a69d2878c99e1b9321e57073d266cf797dc5630.1637455620.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Avoid removing the current working directory, even if it becomes empty | expand

Commit Message

Elijah Newren Nov. 21, 2021, 12:46 a.m. UTC 
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(-)

Comments

René Scharfe Nov. 21, 2021, 8:56 a.m. UTC | #1 
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.

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

> +		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?

> +
>  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);
Elijah Newren Nov. 23, 2021, 12:35 a.m. UTC | #2 
On Sun, Nov 21, 2021 at 12:56 AM René Scharfe <l.s.r@web.de> wrote:
>
> 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.

> > +             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);
diff mbox series

Patch

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))
+		return 1;
+	return rmdir(dirname);
+}
+
 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);
diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index 526d8ec2ee3..b92e1a9bb16 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh
@@ -24,7 +24,7 @@  test_expect_success setup '
 	git commit -m dirORfile
 '
 
-test_expect_failure 'checkout does not clean cwd incidentally' '
+test_expect_success 'checkout does not clean cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_path_is_dir foo/bar &&
 
@@ -53,7 +53,7 @@  test_expect_success 'checkout fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'reset --hard does not clean cwd incidentally' '
+test_expect_success 'reset --hard does not clean cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_path_is_dir foo/bar &&
 
@@ -82,7 +82,7 @@  test_expect_success 'reset --hard fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'merge does not remove cwd incidentally' '
+test_expect_success 'merge does not remove cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -109,7 +109,7 @@  test_expect_success 'merge fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'cherry-pick does not remove cwd incidentally' '
+test_expect_success 'cherry-pick does not remove cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -136,7 +136,7 @@  test_expect_success 'cherry-pick fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'rebase does not remove cwd incidentally' '
+test_expect_success 'rebase does not remove cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -163,7 +163,7 @@  test_expect_success 'rebase fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'revert does not remove cwd incidentally' '
+test_expect_success 'revert does not remove cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&