[v3] stash: avoid recursive hard reset on submodules
diff mbox series

Message ID 20191011222448.GA650182@OJAN-PAAVO.localdomain
State New
Headers show
Series
  • [v3] stash: avoid recursive hard reset on submodules
Related show

Commit Message

Jakob Jarmar Oct. 11, 2019, 10:24 p.m. UTC
git stash push does not recursively stash submodules, but if
submodule.recurse is set, it may recursively reset --hard them. Having
only the destructive action recurse is likely to be surprising
behaviour, and unlikely to be desirable, so the easiest fix should be to
ensure that the call to git reset --hard never recurses into submodules.

This matches the behavior of check_changes_tracked_files, which ignores
submodules.

Signed-off-by: Jakob Jarmar <jakob@jarmar.se>
---

Notes:
    I went for two separate tests with the same basic setup, one that does
    push + pop (which should be valid regardless of future changes), and one
    that just does push before checking that the submodule still is dirty
    (which would become invalid if git stash were to learn --recurse-submodules
    in the future, but matches this approach).

 builtin/stash.c            |  2 +-
 git-legacy-stash.sh        |  2 +-
 t/t3906-stash-submodule.sh | 42 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 43 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Oct. 12, 2019, 1:46 a.m. UTC | #1
Jakob Jarmar <jakob.j@gmail.com> writes:

> diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
> index d7219d6f8f..83106fa958 100755
> --- a/t/t3906-stash-submodule.sh
> +++ b/t/t3906-stash-submodule.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  
> -test_description='stash apply can handle submodules'
> +test_description='stash can handle submodules'

Good attention to the detail ;-)

> +setup_basic() {

Style.  SP on both sides of () in our shell scripts (as seen in the
existing shell function in the same file).

> +	git init sub &&
> +	(
> +		cd sub &&
> +		test_commit sub_file
> +	) &&
> +	git init main &&
> +	(
> +		cd main &&
> +		git submodule add ../sub &&
> +		test_commit main_file
> +	) &&
> +	test_when_finished "rm -rf main sub"

Have test_when_finished that removes main and sub _before_ you start
creating sub and main.  

When the &&-cascade breaks anywhere, the control may not even reach
your test_when_finished that registers the clean-up procedure.
Imagine "git init sub" succeeds but "git init main" somehow
fails---you still want to clean up "sub".

Other than that, looks reasonably well done.  

Thanks for working on this.


> +}
> +
> +test_expect_success 'stash push with submodule.recurse=true preserves dirty submodule worktree' '
> +	setup_basic &&
> +	(
> +		cd main &&
> +		git config submodule.recurse true &&
> +		echo "x" >main_file.t &&
> +		echo "y" >sub/sub_file.t &&
> +		git stash push &&
> +		test_must_fail git -C sub diff --quiet
> +	)
> +'
> +
> +test_expect_success 'stash push and pop with submodule.recurse=true preserves dirty submodule worktree' '
> +	setup_basic &&
> +	(
> +		cd main &&
> +		git config submodule.recurse true &&
> +		echo "x" >main_file.t &&
> +		echo "y" >sub/sub_file.t &&
> +		git stash push &&
> +		git stash pop &&
> +		test_must_fail git -C sub diff --quiet
> +	)
> +'
> +
>  test_done
Jakob Jarmar Oct. 12, 2019, 3:24 p.m. UTC | #2
On Sat, Oct 12, 2019 at 10:46:36AM +0900, Junio C Hamano wrote:
> > +	git init sub &&
> > +	(
> > +		cd sub &&
> > +		test_commit sub_file
> > +	) &&
> > +	git init main &&
> > +	(
> > +		cd main &&
> > +		git submodule add ../sub &&
> > +		test_commit main_file
> > +	) &&
> > +	test_when_finished "rm -rf main sub"
> 
> Have test_when_finished that removes main and sub _before_ you start
> creating sub and main.  
> 
> When the &&-cascade breaks anywhere, the control may not even reach
> your test_when_finished that registers the clean-up procedure.
> Imagine "git init sub" succeeds but "git init main" somehow
> fails---you still want to clean up "sub".

Ah, of course, will fix this (and the other point as well).

> Other than that, looks reasonably well done.  

Thanks. This was fun to figure out and fix.

Patch
diff mbox series

diff --git a/builtin/stash.c b/builtin/stash.c
index b5a301f24d..c986c258f2 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1383,7 +1383,7 @@  static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 			struct child_process cp = CHILD_PROCESS_INIT;
 			cp.git_cmd = 1;
 			argv_array_pushl(&cp.args, "reset", "--hard", "-q",
-					 NULL);
+					 "--no-recurse-submodules", NULL);
 			if (run_command(&cp)) {
 				ret = -1;
 				goto done;
diff --git a/git-legacy-stash.sh b/git-legacy-stash.sh
index f60e9b3e87..07ad4a5459 100755
--- a/git-legacy-stash.sh
+++ b/git-legacy-stash.sh
@@ -370,7 +370,7 @@  push_stash () {
 			git diff-index -p --cached --binary HEAD -- "$@" |
 			git apply --index -R
 		else
-			git reset --hard -q
+			git reset --hard -q --no-recurse-submodules
 		fi
 
 		if test "$keep_index" = "t" && test -n "$i_tree"
diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
index d7219d6f8f..83106fa958 100755
--- a/t/t3906-stash-submodule.sh
+++ b/t/t3906-stash-submodule.sh
@@ -1,6 +1,6 @@ 
 #!/bin/sh
 
-test_description='stash apply can handle submodules'
+test_description='stash can handle submodules'
 
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
@@ -21,4 +21,44 @@  KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 test_submodule_switch "git_stash"
 
+setup_basic() {
+	git init sub &&
+	(
+		cd sub &&
+		test_commit sub_file
+	) &&
+	git init main &&
+	(
+		cd main &&
+		git submodule add ../sub &&
+		test_commit main_file
+	) &&
+	test_when_finished "rm -rf main sub"
+}
+
+test_expect_success 'stash push with submodule.recurse=true preserves dirty submodule worktree' '
+	setup_basic &&
+	(
+		cd main &&
+		git config submodule.recurse true &&
+		echo "x" >main_file.t &&
+		echo "y" >sub/sub_file.t &&
+		git stash push &&
+		test_must_fail git -C sub diff --quiet
+	)
+'
+
+test_expect_success 'stash push and pop with submodule.recurse=true preserves dirty submodule worktree' '
+	setup_basic &&
+	(
+		cd main &&
+		git config submodule.recurse true &&
+		echo "x" >main_file.t &&
+		echo "y" >sub/sub_file.t &&
+		git stash push &&
+		git stash pop &&
+		test_must_fail git -C sub diff --quiet
+	)
+'
+
 test_done