diff mbox series

[v2,3/9] unpack-trees: refuse to remove startup_info->original_cwd

Message ID e74975e83cc7a11b8f0378d59a8c2c4a97d3aa50.1637829556.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. 25, 2021, 8:39 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

In the past, when a directory needs to be removed to make room for a
file, we have always errored out when that directory contains any
untracked (but not ignored) files.  Add an extra condition on that: also
error out if the directory is the current working directory we inherited
from our parent process.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t2501-cwd-empty.sh | 10 +++++-----
 unpack-trees.c       | 17 +++++++++++++----
 unpack-trees.h       |  1 +
 3 files changed, 19 insertions(+), 9 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Nov. 25, 2021, 10:56 a.m. UTC | #1 
On Thu, Nov 25 2021, Elijah Newren via GitGitGadget wrote:

> +	/* ERROR_CWD_IN_THE_WAY */
> +	"Refusing to remove '%s' since it is the current working directory.",
> +
>  	/* ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN */
>  	"Untracked working tree file '%s' would be overwritten by merge.",
>  
> @@ -131,6 +134,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
>  	msgs[ERROR_NOT_UPTODATE_DIR] =
>  		_("Updating the following directories would lose untracked files in them:\n%s");
>  
> +	msgs[ERROR_CWD_IN_THE_WAY] =
> +		_("Refusing to remove the current working directory:\n%s");
> +

We end up capitalizing the first letter here, which isn't our usual
style, but I see we do it for all of unpack_plumbing_errors already, and
some related things like the error setup.c would emit.

Still, perhaps it's better to not follow that convention for new
messages?
Elijah Newren Nov. 26, 2021, 6:06 p.m. UTC | #2 
On Thu, Nov 25, 2021 at 2:57 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Thu, Nov 25 2021, Elijah Newren via GitGitGadget wrote:
>
> > +     /* ERROR_CWD_IN_THE_WAY */
> > +     "Refusing to remove '%s' since it is the current working directory.",
> > +
> >       /* ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN */
> >       "Untracked working tree file '%s' would be overwritten by merge.",
> >
> > @@ -131,6 +134,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
> >       msgs[ERROR_NOT_UPTODATE_DIR] =
> >               _("Updating the following directories would lose untracked files in them:\n%s");
> >
> > +     msgs[ERROR_CWD_IN_THE_WAY] =
> > +             _("Refusing to remove the current working directory:\n%s");
> > +
>
> We end up capitalizing the first letter here, which isn't our usual
> style, but I see we do it for all of unpack_plumbing_errors already, and
> some related things like the error setup.c would emit.
>
> Still, perhaps it's better to not follow that convention for new
> messages?

If someone else wants to submit a patch afterwards to clean up all
these error messages, that'd be fine, but I think being internally
inconsistent is more jarring than not following the global rule.  I'll
leave it as is, and let someone else do the wider cleanup.
Derrick Stolee Nov. 29, 2021, 2:10 p.m. UTC | #3 
On 11/25/2021 3:39 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>

> @@ -36,6 +36,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
>  	/* ERROR_NOT_UPTODATE_DIR */
>  	"Updating '%s' would lose untracked files in it",
>  
> +	/* ERROR_CWD_IN_THE_WAY */
> +	"Refusing to remove '%s' since it is the current working directory.",
> +
>  	/* ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN */
>  	"Untracked working tree file '%s' would be overwritten by merge.",

Your new message includes a hard stop (".") which is non-standard. I
see that the message after yours has one, but the preceding one does
not. Since the file you are in is not consistent, I would choose to
drop the hard stop here.

Thanks,
-Stolee
Elijah Newren Nov. 29, 2021, 5:26 p.m. UTC | #4 
On Mon, Nov 29, 2021 at 6:10 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 11/25/2021 3:39 AM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
>
> > @@ -36,6 +36,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
> >       /* ERROR_NOT_UPTODATE_DIR */
> >       "Updating '%s' would lose untracked files in it",
> >
> > +     /* ERROR_CWD_IN_THE_WAY */
> > +     "Refusing to remove '%s' since it is the current working directory.",
> > +
> >       /* ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN */
> >       "Untracked working tree file '%s' would be overwritten by merge.",
>
> Your new message includes a hard stop (".") which is non-standard. I
> see that the message after yours has one, but the preceding one does
> not. Since the file you are in is not consistent, I would choose to
> drop the hard stop here.

Seems pretty reasonable, but 9 out of 10 of the messages in that list
contain the hard stop.  True, that's still inconsistent, but adding
the hard stop feels more internally consistent.  I think I'd rather
have someone else post a subsequent cleanup to remove the hard stops
and lowercase the first letter and whatever else needs to be changed
with these messages.
diff mbox series

Patch

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index 5dfb456a691..212676d71c3 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh
@@ -38,7 +38,7 @@  test_expect_failure 'checkout does not clean cwd incidentally' '
 	test_path_is_dir foo
 '
 
-test_expect_failure 'checkout fails if cwd needs to be removed' '
+test_expect_success 'checkout fails if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -95,7 +95,7 @@  test_expect_failure 'merge does not remove cwd incidentally' '
 	test_path_is_dir subdir
 '
 
-test_expect_failure 'merge fails if cwd needs to be removed' '
+test_expect_success 'merge fails if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -122,7 +122,7 @@  test_expect_failure 'cherry-pick does not remove cwd incidentally' '
 	test_path_is_dir subdir
 '
 
-test_expect_failure 'cherry-pick fails if cwd needs to be removed' '
+test_expect_success 'cherry-pick fails if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -149,7 +149,7 @@  test_expect_failure 'rebase does not remove cwd incidentally' '
 	test_path_is_dir subdir
 '
 
-test_expect_failure 'rebase fails if cwd needs to be removed' '
+test_expect_success 'rebase fails if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -176,7 +176,7 @@  test_expect_failure 'revert does not remove cwd incidentally' '
 	test_path_is_dir subdir
 '
 
-test_expect_failure 'revert fails if cwd needs to be removed' '
+test_expect_success 'revert fails if cwd needs to be removed' '
 	git checkout fd_conflict &&
 	git revert HEAD &&
 	test_when_finished "git clean -fdx" &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 89ca95ce90b..6bc16f3a714 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -36,6 +36,9 @@  static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
 	/* ERROR_NOT_UPTODATE_DIR */
 	"Updating '%s' would lose untracked files in it",
 
+	/* ERROR_CWD_IN_THE_WAY */
+	"Refusing to remove '%s' since it is the current working directory.",
+
 	/* ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN */
 	"Untracked working tree file '%s' would be overwritten by merge.",
 
@@ -131,6 +134,9 @@  void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 	msgs[ERROR_NOT_UPTODATE_DIR] =
 		_("Updating the following directories would lose untracked files in them:\n%s");
 
+	msgs[ERROR_CWD_IN_THE_WAY] =
+		_("Refusing to remove the current working directory:\n%s");
+
 	if (!strcmp(cmd, "checkout"))
 		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by checkout:\n%%s"
@@ -2146,10 +2152,7 @@  static int verify_clean_subdirectory(const struct cache_entry *ce,
 		cnt++;
 	}
 
-	/*
-	 * Then we need to make sure that we do not lose a locally
-	 * present file that is not ignored.
-	 */
+	/* Do not lose a locally present file that is not ignored. */
 	pathbuf = xstrfmt("%.*s/", namelen, ce->name);
 
 	memset(&d, 0, sizeof(d));
@@ -2160,6 +2163,12 @@  static int verify_clean_subdirectory(const struct cache_entry *ce,
 	free(pathbuf);
 	if (i)
 		return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
+
+	/* Do not lose startup_info->original_cwd */
+	if (startup_info->original_cwd &&
+	    !strcmp(startup_info->original_cwd, ce->name))
+		return add_rejected_path(o, ERROR_CWD_IN_THE_WAY, ce->name);
+
 	return cnt;
 }
 
diff --git a/unpack-trees.h b/unpack-trees.h
index 71ffb7eeb0c..efb9edfbb27 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -19,6 +19,7 @@  enum unpack_trees_error_types {
 	ERROR_WOULD_OVERWRITE = 0,
 	ERROR_NOT_UPTODATE_FILE,
 	ERROR_NOT_UPTODATE_DIR,
+	ERROR_CWD_IN_THE_WAY,
 	ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN,
 	ERROR_WOULD_LOSE_UNTRACKED_REMOVED,
 	ERROR_BIND_OVERLAP,