threads / patch / 42357

v2Ignore dirty submodule states during stash

Subject: [PATCH v2] Ignore dirty submodule states during stash

## tl;dr

6 messages between May 17, 2016 and May 17, 2016. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Vasily Titskiy· May 17, 2016, 13:16 UTC · lore

As stash does not know how to deal with submodules, it should not try to save/restore their states as it leads to redundant merge conflicts.

Added test checks if 'stash pop' does not trigger merge conflics in submodules.

Signed-off-by: Vasily Titskiy <qehgt0@gmail.com>
---
 git-stash.sh     |  2 +-
 t/t3903-stash.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

Show changes to 2 files +35 −1

git-stash.sh, t/t3903-stash.sh

diff --git a/git-stash.sh b/git-stash.sh
index c7c65e2..b500c44 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -116,7 +116,7 @@ create_stash () {
 			git read-tree --index-output="$TMPindex" -m $i_tree &&
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
-			git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
+			git diff --name-only --ignore-submodules -z HEAD -- >"$TMP-stagenames" &&
 			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2142c1f..1be62f3 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -731,4 +731,38 @@ test_expect_success 'stash list --cc shows combined diff' '
 	test_cmp expect actual
 '
 
+test_expect_success 'stash ignores changes in submodules' '
+	git submodule init &&
+	git init sub1 &&
+	(
+		cd sub1 &&
+		echo "x" >file1 &&
+		git add file1 &&
+		git commit -a -m "initial sub1"
+	) &&
+	git submodule add ./. sub1 &&
+	echo "main" >file1 &&
+	git add file1 &&
+	git commit -a -m "initial main" &&
+	# make changes in submodule
+	(
+		cd sub1 &&
+		echo "y" >>file1 &&
+		git commit -a -m "change y"
+	) &&
+	git commit sub1 -m "update reference" &&
+	# switch submodule to another revision
+	(
+		cd sub1 &&
+		echo "z" >>file1 &&
+		git commit -a -m "change z"
+	) &&
+	# everything is prepared, check if changes in submodules are ignored
+	echo "local change" >>file1 &&
+	git stash save &&
+	git checkout HEAD~1 &&
+	git submodule update &&
+	git stash pop
+'
+
 test_done

-- 
2.1.4

Junio C Hamano· May 17, 2016, 16:15 UTC · re: Vasily Titskiy · lore

Re: [PATCH v2] Ignore dirty submodule states during stash

Vasily Titskiy <qehgt0@gmail.com> writes:

Show 10 quoted lines

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 2142c1f..1be62f3 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -731,4 +731,38 @@ test_expect_success 'stash list --cc shows combined diff' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'stash ignores changes in submodules' '
> +	git submodule init &&

Hmmmm... what is this "submodule init" needed for at this point in the test sequence?

Show 33 quoted lines

> +	git init sub1 &&
> +	(
> +		cd sub1 &&
> +		echo "x" >file1 &&
> +		git add file1 &&
> +		git commit -a -m "initial sub1"
> +	) &&
> +	git submodule add ./. sub1 &&
> +	echo "main" >file1 &&
> +	git add file1 &&
> +	git commit -a -m "initial main" &&
> +	# make changes in submodule
> +	(
> +		cd sub1 &&
> +		echo "y" >>file1 &&
> +		git commit -a -m "change y"
> +	) &&
> +	git commit sub1 -m "update reference" &&
> +	# switch submodule to another revision
> +	(
> +		cd sub1 &&
> +		echo "z" >>file1 &&
> +		git commit -a -m "change z"
> +	) &&
> +	# everything is prepared, check if changes in submodules are ignored
> +	echo "local change" >>file1 &&
> +	git stash save &&
> +	git checkout HEAD~1 &&
> +	git submodule update &&
> +	git stash pop
> +'
> +
>  test_done

Vasily Titskiy· May 17, 2016, 16:38 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Ignore dirty submodule states during stash

Hi Junio,

You're right, it's redundant here. Should I resubmit the path without this line?

--
  Regards,
  Vasily Titskiy

On Tue, May 17, 2016 at 12:15 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 50 quoted lines

>
> Vasily Titskiy <qehgt0@gmail.com> writes:
>
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index 2142c1f..1be62f3 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -731,4 +731,38 @@ test_expect_success 'stash list --cc shows combined diff' '
> >       test_cmp expect actual
> >  '
> >
> > +test_expect_success 'stash ignores changes in submodules' '
> > +     git submodule init &&
>
> Hmmmm... what is this "submodule init" needed for at this point in
> the test sequence?
>
> > +     git init sub1 &&
> > +     (
> > +             cd sub1 &&
> > +             echo "x" >file1 &&
> > +             git add file1 &&
> > +             git commit -a -m "initial sub1"
> > +     ) &&
> > +     git submodule add ./. sub1 &&
> > +     echo "main" >file1 &&
> > +     git add file1 &&
> > +     git commit -a -m "initial main" &&
> > +     # make changes in submodule
> > +     (
> > +             cd sub1 &&
> > +             echo "y" >>file1 &&
> > +             git commit -a -m "change y"
> > +     ) &&
> > +     git commit sub1 -m "update reference" &&
> > +     # switch submodule to another revision
> > +     (
> > +             cd sub1 &&
> > +             echo "z" >>file1 &&
> > +             git commit -a -m "change z"
> > +     ) &&
> > +     # everything is prepared, check if changes in submodules are ignored
> > +     echo "local change" >>file1 &&
> > +     git stash save &&
> > +     git checkout HEAD~1 &&
> > +     git submodule update &&
> > +     git stash pop
> > +'
> > +
> >  test_done

Junio C Hamano· May 17, 2016, 17:04 UTC · re: Vasily Titskiy · lore

Re: [PATCH v2] Ignore dirty submodule states during stash

Vasily Titskiy <qehgt0@gmail.com> writes:

> You're right, it's redundant here. Should I resubmit the path without this line?

I wasn't pointing out that it was not needed. I was only asking what it was meant to do.

If you now think it shouldn't have been there, that merely means your code was wrong. It does not mean I'm right ;-)

With that line removed, would the patch now becomes right? Are there other bugs?

Vasily Titskiy· May 17, 2016, 17:12 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Ignore dirty submodule states during stash

The command does nothing, so it's not needed. There is also a typo in
my patch description, so I'll resend it again with needed changes.
--
  Regards,
  Vasily Titskiy

On Tue, May 17, 2016 at 1:04 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Vasily Titskiy <qehgt0@gmail.com> writes:
>
>> You're right, it's redundant here. Should I resubmit the path without this line?
>
> I wasn't pointing out that it was not needed.  I was only asking
> what it was meant to do.
>
> If you now think it shouldn't have been there, that merely means
> your code was wrong.  It does not mean I'm right ;-)
>
> With that line removed, would the patch now becomes right?  Are
> there other bugs?

Eric Sunshine· May 17, 2016, 16:58 UTC · re: Vasily Titskiy · lore

Re: [PATCH v2] Ignore dirty submodule states during stash

On Tue, May 17, 2016 at 9:16 AM, Vasily Titskiy <qehgt0@gmail.com> wrote:

Show 5 quoted lines

> As stash does not know how to deal with submodules,
> it should not try to save/restore their states
> as it leads to redundant merge conflicts.
>
> Added test checks if 'stash pop' does not trigger merge conflics

/conflics/conflicts/

> in submodules.
>
> Signed-off-by: Vasily Titskiy <qehgt0@gmail.com>