Skip to contentSkip to conversation

stash: do not return before restoring untracked files

PATCH In review Elijah Newren via GitGitGadget 2 participants 2 messages Jan 4, 2022

lore
Share & tools
lore

Elijah Newren via GitGitGadget

author
#0 lore raw

PATCH 1/1 stash: do not return before restoring untracked files

From: Elijah Newren <[email protected]>

In commit bee8691f19 ("stash: restore untracked files AFTER restoring tracked files", 2021-09-10), we correctly identified that we should restore changes to tracked files before attempting to restore untracked files, and accordingly moved the code for restoring untracked files a few lines down in do_apply_stash(). Unfortunately, the intervening lines had some early return statements meaning that we suddenly stopped restoring untracked files in some cases.

Even before the previous commit, there was another possible issue with the current code -- a post-stash-apply 'git status' that was intended to be run after restoring the stash was skipped when we hit a conflict (or other error condition), which seems slightly inconsistent.

Fix both issues by saving the return status, and letting other functionality run before returning.

Reported-by AJ Henderson
Test-case-by Randall S. Becker <[email protected]>
Signed-off-by Elijah Newren <[email protected]>

---
stash: do not return before restoring untracked files

This fixes a regression in v2.34.1 relative to v2.33.0.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1180%2Fnewren%2Ffix-stash-restore-untracked-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1180/newren/fix-stash-restore-untracked-v1
Pull-Request: https://github.com/git/git/pull/1180

 builtin/stash.c  |  9 +++++----
 t/t3903-stash.sh | 24 ++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 4 deletions(-)
builtin/stash.c +5 -4
index 18c812bbe03..397210168da 100644
@@ -561,18 +561,19 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 		if (index)
 			fprintf_ln(stderr, _("Index was not unstashed."));
 
-		return ret;
+		goto restore_untracked;
 	}
 
 	if (has_index) {
 		if (reset_tree(&index_tree, 0, 0))
-			return -1;
+			ret = -1;
 	} else {
 		unstage_changes_unless_new(&c_tree);
 	}
 
+restore_untracked:
 	if (info->has_u && restore_untracked(&info->u_tree))
-		return error(_("could not restore untracked files from stash"));
+		ret = error(_("could not restore untracked files from stash"));
 
 	if (!quiet) {
 		struct child_process cp = CHILD_PROCESS_INIT;

@@ -592,7 +593,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 		run_command(&cp);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int apply_stash(int argc, const char **argv, const char *prefix)
t/t3903-stash.sh +24 -0
index 2c66cfbc3b7..2a7d8e511db 100755
@@ -1376,4 +1376,28 @@ test_expect_success 'git stash can pop directory -> file saved changes' '
 	)
 '
 
+test_expect_success 'restore untracked files even when we hit conflicts' '
+	git init restore_untracked_after_conflict &&
+	(
+		cd restore_untracked_after_conflict &&
+
+		echo hi >a &&
+		echo there >b &&
+		git add . &&
+		git commit -m first &&
+		echo hello >a &&
+		echo something >c &&
+
+		git stash push --include-untracked &&
+
+		echo conflict >a &&
+		git add a &&
+		git commit -m second &&
+
+		test_must_fail git stash pop &&
+
+		test_path_is_file c
+	)
+'
+
 test_done

base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 14 lines · Elijah Newren via GitGitGadget
builtin/stash.c +1 -1
index 18c812bbe03..397210168da 100644
@@ -561,18 +561,19 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 		if (index)
 			fprintf_ln(stderr, _("Index was not unstashed."));
 
-		return ret;
+		goto restore_untracked;
 	}
 
 	if (has_index) {
 		if (reset_tree(&index_tree, 0, 0))

builtin/stash.c static int do_apply_stash(const char *prefix, struct stash_info *info, · line 576

Quoted 10 lines
builtin/stash.c +3 -2
-			return -1;
+			ret = -1;
 	} else {
 		unstage_changes_unless_new(&c_tree);
 	}
 
+restore_untracked:
 	if (info->has_u && restore_untracked(&info->u_tree))
-		return error(_("could not restore untracked files from stash"));
+		ret = error(_("could not restore untracked files from stash"));

In other words, instead of doing an early return, we try to resurrect untracked ones and always show the "git status" before returning.

I think that takes us in the right direction. I looked at other early returns in this function, and while there may be a room for improvements when the --index stash does not apply well, I think it can be left outside the topic of this patch.

Will queue.

Thanks.

Quoted 46 lines
builtin/stash.c +1 -1
 	if (!quiet) {
 		struct child_process cp = CHILD_PROCESS_INIT;

@@ -592,7 +593,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 		run_command(&cp);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int apply_stash(int argc, const char **argv, const char *prefix)
t/t3903-stash.sh +24 -0
index 2c66cfbc3b7..2a7d8e511db 100755
@@ -1376,4 +1376,28 @@ test_expect_success 'git stash can pop directory -> file saved changes' '
 	)
 '
 
+test_expect_success 'restore untracked files even when we hit conflicts' '
+	git init restore_untracked_after_conflict &&
+	(
+		cd restore_untracked_after_conflict &&
+
+		echo hi >a &&
+		echo there >b &&
+		git add . &&
+		git commit -m first &&
+		echo hello >a &&
+		echo something >c &&
+
+		git stash push --include-untracked &&
+
+		echo conflict >a &&
+		git add a &&
+		git commit -m second &&
+
+		test_must_fail git stash pop &&
+
+		test_path_is_file c
+	)
+'
+
 test_done

base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907