[2/3] stash: avoid feeding directories to update-index

Commit Message

Elijah Newren Sept. 8, 2021, 1:43 a.m. UTC

From: Elijah Newren <[e-mail]>

When a file is removed from the cache, but there is a file of the same
name present in the working directory, we would normally treat that file
in the working directory as untracked.  However, in the case of stash,
doing that would prevent a simple 'git stash push', because the untracked
file would be in the way of restoring the deleted file.

git stash, however, blindly assumes that whatever is in the working
directory for a deleted file is wanted and passes that path along to
update-index.  That causes problems when the working directory contains
a directory with the same name as the deleted file.  Add some code for
this special case that will avoid passing directory names to
update-index.

Signed-off-by: Elijah Newren <[e-mail]>
---
 builtin/stash.c  | 9 +++++++++
 t/t3903-stash.sh | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Johannes Schindelin Sept. 8, 2021, 8:02 a.m. UTC | #1

Hi Elijah,

On Wed, 8 Sep 2021, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <[e-mail]>
>
> When a file is removed from the cache, but there is a file of the same
> name present in the working directory, we would normally treat that file
> in the working directory as untracked.  However, in the case of stash,
> doing that would prevent a simple 'git stash push', because the untracked
> file would be in the way of restoring the deleted file.
>
> git stash, however, blindly assumes that whatever is in the working
> directory for a deleted file is wanted and passes that path along to
> update-index.  That causes problems when the working directory contains
> a directory with the same name as the deleted file.  Add some code for
> this special case that will avoid passing directory names to
> update-index.
>
> Signed-off-by: Elijah Newren <[e-mail]>
> ---
>  builtin/stash.c  | 9 +++++++++
>  t/t3903-stash.sh | 2 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 8f42360ca91..b85cf9d267e 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -313,6 +313,12 @@ static int reset_head(void)
>  	return run_command(&cp);
>  }
>
> +static int is_path_a_directory(const char *path)
> +{
> +	struct stat st;
> +	return (!lstat(path, &st) && S_ISDIR(st.st_mode));
> +}

Git's API is unnecessarily confusing (because grown organically), so it is
easy to get confused by that `is_directory()` function that is declared in
`cache.h` and defined in `abspath.c`:

	/*
	 * Do not use this for inspecting *tracked* content.  When path is a
	 * symlink to a directory, we do not want to say it is a directory when
	 * dealing with tracked content in the working tree.
	 */
	int is_directory(const char *path)
	{
		struct stat st;
		return (!stat(path, &st) && S_ISDIR(st.st_mode));
	}

The difference I see is that you use an `lstat()`, which is kind of
important here.

Maybe you could add a paragraph pointing out that we cannot use
`is_directory()` here because it would follow symbolic links, which we
need to avoid here?

> +
>  static void add_diff_to_buf(struct diff_queue_struct *q,
>  			    struct diff_options *options,
>  			    void *data)
> @@ -320,6 +326,9 @@ static void add_diff_to_buf(struct diff_queue_struct *q,
>  	int i;
>
>  	for (i = 0; i < q->nr; i++) {
> +		if (is_path_a_directory(q->queue[i]->one->path))
> +			continue;
> +
>  		strbuf_addstr(data, q->queue[i]->one->path);
>
>  		/* NUL-terminate: will be fed to update-index -z */
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 0727a494aa4..fc74918ccc0 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1307,7 +1307,7 @@ test_expect_success 'stash -c stash.useBuiltin=false warning ' '
>  	test_must_be_empty err
>  '
>
> -test_expect_failure 'git stash succeeds despite directory/file change' '
> +test_expect_success 'git stash succeeds despite directory/file change' '

Excellent!

Thanks,
Dscho

>  	test_create_repo directory_file_switch_v1 &&
>  	(
>  		cd directory_file_switch_v1 &&
> --
> gitgitgadget
>
>

Patch

diff --git a/builtin/stash.c b/builtin/stash.c
index 8f42360ca91..b85cf9d267e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -313,6 +313,12 @@  static int reset_head(void)
 	return run_command(&cp);
 }
 
+static int is_path_a_directory(const char *path)
+{
+	struct stat st;
+	return (!lstat(path, &st) && S_ISDIR(st.st_mode));
+}
+
 static void add_diff_to_buf(struct diff_queue_struct *q,
 			    struct diff_options *options,
 			    void *data)
@@ -320,6 +326,9 @@  static void add_diff_to_buf(struct diff_queue_struct *q,
 	int i;
 
 	for (i = 0; i < q->nr; i++) {
+		if (is_path_a_directory(q->queue[i]->one->path))
+			continue;
+
 		strbuf_addstr(data, q->queue[i]->one->path);
 
 		/* NUL-terminate: will be fed to update-index -z */
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 0727a494aa4..fc74918ccc0 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1307,7 +1307,7 @@  test_expect_success 'stash -c stash.useBuiltin=false warning ' '
 	test_must_be_empty err
 '
 
-test_expect_failure 'git stash succeeds despite directory/file change' '
+test_expect_success 'git stash succeeds despite directory/file change' '
 	test_create_repo directory_file_switch_v1 &&
 	(
 		cd directory_file_switch_v1 &&