On Mon, May 16, 2016 at 8:46 AM, Vasily Titskiy <qeh...@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 <qeh...@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 <qeh...@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 majord...@vger.kernel.org
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
