Re: [PATCH] Ignore dirty submodule states during stash

[Stefan Beller <sbeller@google.com>]

On Mon, May 16, 2016 at 8:46 AM, Vasily Titskiy <qehgt0@gmail.com> wrote:
> Hi Stefan,
>
> On Sun, May 15, 2016 at 11:37:20PM -0700, Stefan Beller wrote:
>> On Sun, May 15, 2016 at 7:07 PM, Vasily Titskiy <qehgt0@gmail.com> wrote:
>> > Do not save states of submodules as stash should ignore it.
>>
>> Can you explain why this is a good idea?
>> (It is not obvious to me either way.)
> Actually, submodules are already ignored by stash, but not fully (it was introduced in commit 6848d58).
> Current behavior is counter-intuitive, for example (if one has a project with a submodule):
>  $ cd sub1
>  $ edit .. commit .. edit .. commit. Alternatively, just checkout some other commit
>  $ cd .. # back to main project
>  $ git stash save
>    No local changes to save
>  $ # so, stash declares there are no changes
>  $ edit main.cpp
>  $ # For example, I need to update my working tree to latest master
>  $ git stash save # save local changes of 'main.cpp'...
>  $ git pull --recurse-submodules && git submodule update --recursive # update to latest
>  $ git stash pop # I expect to get stashed changes for 'main.cpp', but...
>    warning: Failed to merge submodule sub1 (commits don't follow merge-base)
>    Auto-merging sub1
>    CONFLICT (submodule): Merge conflict in sub1
>
> So, this is the issue. Instead of getting my local changes, I got a conflict (and stash is not
> poped out). The root cause is the 'stash' command does not know how to deal with submodules,
> but currently it tries to save the state of submodules, and even tries to re-apply the state
> (and it fails of course). The proposed solution fixes this behaviour.
>
> All internal tests work fine with the change.

I think you could take the example as above and make it into a test?
Showing that this change actually fixes a bug.

Looking for a good place, I would have expected t/t3906-stash-submodule.sh
would be a good place to put your code, but I am not sure how to
properly integrate that test there.

Maybe we can put the test in t3903 instead?

>
>
>>
>> Do we need a test/documentation updates for this?
> I don't think so, 'stash' have never claimed submodule support.

But it also never explicitly claimed it doesn't support it.

Maybe we want to squash in something like
(with better wording):

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 92df596..b2649eb 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -41,6 +41,8 @@ the usual reflog syntax (e.g. `stash@{0}` is the most recently
 created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`
 is also possible).

+Stashing ignores submodule operations completely.
+
 OPTIONS
 -------


Thanks,
Stefan



>
>>
>> >
>> > Signed-off-by: Vasily Titskiy <qehgt0@gmail.com>
>> > ---
>> >  git-stash.sh | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > 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"
>> > --
>> > 2.1.4
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe git" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html