[PATCH] stash: fix handling removed files with --keep-index

[Thomas Gummerer <t.gummerer@gmail.com>]

On 07/11, Martin Nicolay wrote:
> Hi!
> 
> I don't know if this is a software or documentation bug.
> 
> man git-stash says about --keep-index:
>     If the --keep-index option is used, all changes already added to
>     the index are left intact.
> 
> If a file is deleted and this deletion is in the index a following
>     $ git stash push --keep-index
> keeps this deletion in the index but not in the working-tree.
> 
> If a file is changed and this change is in the index a following
>     $ git stash push --keep-index
> keeps this change in the index and also in the working-tree.
> 
> This is inconsistent.

Thanks for your report.  This has come up before in
https://public-inbox.org/git/1555437849815.60450@rasenplanscher.info/,
which I first thought was expected behaviour, but that was just me
misunderstanding the --keep-index option.  So I belive this is indeed
a bug.

Luckily I had some more time to actually look at this this time
around, so below is a potential fix.

This comes with a small caveat of overwriting untracked files if they
have been removed from the index, and replaced with a file that has
not been added yet.  I think that's okay as that happens in other
places as well in stash, but wanted to point it out anyway.

--- >8 ---
Subject: [PATCH] stash: fix handling removed files with --keep-index

git stash push --keep-index is supposed to keep all changes that have
been added to the index, both in the index and on disk.

Currently this doesn't behave correctly when a file is removed from
the index.  Instead of keeping it deleted on disk, --keep-index
currently restores the file.

Fix that behaviour by using 'git restore' which can faithfully restore
the index and working tree.  This also simplifies the code.

Note that this will overwrite untracked files if the untracked file
has the same name as a file that has been deleted in the index.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/stash.c  | 34 ++++++++++------------------------
 t/t3903-stash.sh |  7 +++++++
 2 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index fde6397caa..2a58c007e1 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1391,30 +1391,16 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 		}
 
 		if (keep_index == 1 && !is_null_oid(&info.i_tree)) {
-			struct child_process cp_ls = CHILD_PROCESS_INIT;
-			struct child_process cp_checkout = CHILD_PROCESS_INIT;
-			struct strbuf out = STRBUF_INIT;
-
-			if (reset_tree(&info.i_tree, 0, 1)) {
-				ret = -1;
-				goto done;
-			}
-
-			cp_ls.git_cmd = 1;
-			argv_array_pushl(&cp_ls.args, "ls-files", "-z",
-					 "--modified", "--", NULL);
-
-			add_pathspecs(&cp_ls.args, ps);
-			if (pipe_command(&cp_ls, NULL, 0, &out, 0, NULL, 0)) {
-				ret = -1;
-				goto done;
-			}
-
-			cp_checkout.git_cmd = 1;
-			argv_array_pushl(&cp_checkout.args, "checkout-index",
-					 "-z", "--force", "--stdin", NULL);
-			if (pipe_command(&cp_checkout, out.buf, out.len, NULL,
-					 0, NULL, 0)) {
+			struct child_process cp_restore = CHILD_PROCESS_INIT;
+
+			cp_restore.git_cmd = 1;
+			argv_array_pushl(&cp_restore.args, "restore", "--source", oid_to_hex(&info.i_tree),
+					 "--staged", "--worktree", NULL);
+			if (!ps->nr)
+				argv_array_push(&cp_restore.args, ".");
+			else
+				add_pathspecs(&cp_restore.args, ps);
+			if (run_command(&cp_restore)) {
 				ret = -1;
 				goto done;
 			}
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b22e671608..b8e337893f 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1234,4 +1234,11 @@ test_expect_success 'stash works when user.name and user.email are not set' '
 	)
 '
 
+test_expect_success 'stash --keep-index with file deleted in index does not resurrect it on disk' '
+	test_commit to-remove to-remove &&
+	git rm to-remove &&
+	git stash --keep-index &&
+	test_path_is_missing to-remove
+'
+
 test_done
-- 
2.22.0.599.gf5cf68d754