{"thread":{"id":"42339","subject":"[PATCH] Ignore dirty submodule states during stash","startedAt":"2016-05-16T02:07:35Z","lastAt":"2016-05-17T03:17:05Z","messageCount":6,"participants":["Vasily Titskiy","Stefan Beller","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"286637","messageId":"20160516020735.GA7884@gmail.com","threadId":"42339","inReplyTo":null,"subject":"[PATCH] Ignore dirty submodule states during stash","fromName":"Vasily Titskiy","fromEmail":"qehgt0@gmail.com","sentAt":"2016-05-16T02:07:35Z","receivedAt":"2016-05-16T02:07:35Z","isPatch":true,"sender":{"key":"qehgt0@gmail.com","avatar":"https://gravatar.com/avatar/a8e2406642cc8b86fc31e7c8858f81d66604cb18719377eaa6afc91e134f34a9?d=mp&s=160"},"body":"Do not save states of submodules as stash should ignore it.\n\nSigned-off-by: Vasily Titskiy <qehgt0@gmail.com>\n---\n git-stash.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex c7c65e2..b500c44 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -116,7 +116,7 @@ create_stash () {\n \t\t\tgit read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n \t\t\tGIT_INDEX_FILE=\"$TMPindex\" &&\n \t\t\texport GIT_INDEX_FILE &&\n-\t\t\tgit diff --name-only -z HEAD -- >\"$TMP-stagenames\" &&\n+\t\t\tgit diff --name-only --ignore-submodules -z HEAD -- >\"$TMP-stagenames\" &&\n \t\t\tgit update-index -z --add --remove --stdin <\"$TMP-stagenames\" &&\n \t\t\tgit write-tree &&\n \t\t\trm -f \"$TMPindex\"\n-- \n2.1.4\n"},{"id":"286647","messageId":"CAGZ79kaTss6ctZDCiRP2wjuxH+rJ79RKFLM79_FJN+37Bed+HQ@mail.gmail.com","threadId":"42339","inReplyTo":"20160516020735.GA7884@gmail.com","subject":"Re: [PATCH] Ignore dirty submodule states during stash","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T06:37:20Z","receivedAt":"2016-05-16T06:37:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, May 15, 2016 at 7:07 PM, Vasily Titskiy <qehgt0@gmail.com> wrote:\n> Do not save states of submodules as stash should ignore it.\n\nCan you explain why this is a good idea?\n(It is not obvious to me either way.)\n\nDo we need a test/documentation updates for this?\n\n>\n> Signed-off-by: Vasily Titskiy <qehgt0@gmail.com>\n> ---\n>  git-stash.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-stash.sh b/git-stash.sh\n> index c7c65e2..b500c44 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -116,7 +116,7 @@ create_stash () {\n>                         git read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n>                         GIT_INDEX_FILE=\"$TMPindex\" &&\n>                         export GIT_INDEX_FILE &&\n> -                       git diff --name-only -z HEAD -- >\"$TMP-stagenames\" &&\n> +                       git diff --name-only --ignore-submodules -z HEAD -- >\"$TMP-stagenames\" &&\n>                         git update-index -z --add --remove --stdin <\"$TMP-stagenames\" &&\n>                         git write-tree &&\n>                         rm -f \"$TMPindex\"\n> --\n> 2.1.4\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"286665","messageId":"20160516154606.GA8797@gmail.com","threadId":"42339","inReplyTo":"CAGZ79kaTss6ctZDCiRP2wjuxH+rJ79RKFLM79_FJN+37Bed+HQ@mail.gmail.com","subject":"Re: [PATCH] Ignore dirty submodule states during stash","fromName":"Vasily Titskiy","fromEmail":"qehgt0@gmail.com","sentAt":"2016-05-16T15:46:06Z","receivedAt":"2016-05-16T15:46:06Z","isPatch":true,"sender":{"key":"qehgt0@gmail.com","avatar":"https://gravatar.com/avatar/a8e2406642cc8b86fc31e7c8858f81d66604cb18719377eaa6afc91e134f34a9?d=mp&s=160"},"body":"Hi Stefan,\n\nOn Sun, May 15, 2016 at 11:37:20PM -0700, Stefan Beller wrote:\n> On Sun, May 15, 2016 at 7:07 PM, Vasily Titskiy <qehgt0@gmail.com> wrote:\n> > Do not save states of submodules as stash should ignore it.\n> \n> Can you explain why this is a good idea?\n> (It is not obvious to me either way.)\nActually, submodules are already ignored by stash, but not fully (it was introduced in commit 6848d58).\nCurrent behavior is counter-intuitive, for example (if one has a project with a submodule):\n $ cd sub1\n $ edit .. commit .. edit .. commit. Alternatively, just checkout some other commit\n $ cd .. # back to main project\n $ git stash save\n   No local changes to save\n $ # so, stash declares there are no changes\n $ edit main.cpp\n $ # For example, I need to update my working tree to latest master\n $ git stash save # save local changes of 'main.cpp'...\n $ git pull --recurse-submodules && git submodule update --recursive # update to latest\n $ git stash pop # I expect to get stashed changes for 'main.cpp', but...\n   warning: Failed to merge submodule sub1 (commits don't follow merge-base)\n   Auto-merging sub1\n   CONFLICT (submodule): Merge conflict in sub1\n\nSo, this is the issue. Instead of getting my local changes, I got a conflict (and stash is not \npoped out). The root cause is the 'stash' command does not know how to deal with submodules,\nbut currently it tries to save the state of submodules, and even tries to re-apply the state\n(and it fails of course). The proposed solution fixes this behaviour.\n\nAll internal tests work fine with the change.\n\n\n> \n> Do we need a test/documentation updates for this?\nI don't think so, 'stash' have never claimed submodule support.\n\n> \n> >\n> > Signed-off-by: Vasily Titskiy <qehgt0@gmail.com>\n> > ---\n> >  git-stash.sh | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/git-stash.sh b/git-stash.sh\n> > index c7c65e2..b500c44 100755\n> > --- a/git-stash.sh\n> > +++ b/git-stash.sh\n> > @@ -116,7 +116,7 @@ create_stash () {\n> >                         git read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n> >                         GIT_INDEX_FILE=\"$TMPindex\" &&\n> >                         export GIT_INDEX_FILE &&\n> > -                       git diff --name-only -z HEAD -- >\"$TMP-stagenames\" &&\n> > +                       git diff --name-only --ignore-submodules -z HEAD -- >\"$TMP-stagenames\" &&\n> >                         git update-index -z --add --remove --stdin <\"$TMP-stagenames\" &&\n> >                         git write-tree &&\n> >                         rm -f \"$TMPindex\"\n> > --\n> > 2.1.4\n> >\n> > --\n> > To unsubscribe from this list: send the line \"unsubscribe git\" in\n> > the body of a message to majordomo@vger.kernel.org\n> > More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"286671","messageId":"CAGZ79kZESuKiEt2RJJdWJPBySgbDA6abGkZMiTFgzaNCUP1_mA@mail.gmail.com","threadId":"42339","inReplyTo":"20160516154606.GA8797@gmail.com","subject":"Re: [PATCH] Ignore dirty submodule states during stash","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-16T16:09:40Z","receivedAt":"2016-05-16T16:09:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 16, 2016 at 8:46 AM, Vasily Titskiy <qehgt0@gmail.com> wrote:\n> Hi Stefan,\n>\n> On Sun, May 15, 2016 at 11:37:20PM -0700, Stefan Beller wrote:\n>> On Sun, May 15, 2016 at 7:07 PM, Vasily Titskiy <qehgt0@gmail.com> wrote:\n>> > Do not save states of submodules as stash should ignore it.\n>>\n>> Can you explain why this is a good idea?\n>> (It is not obvious to me either way.)\n> Actually, submodules are already ignored by stash, but not fully (it was introduced in commit 6848d58).\n> Current behavior is counter-intuitive, for example (if one has a project with a submodule):\n>  $ cd sub1\n>  $ edit .. commit .. edit .. commit. Alternatively, just checkout some other commit\n>  $ cd .. # back to main project\n>  $ git stash save\n>    No local changes to save\n>  $ # so, stash declares there are no changes\n>  $ edit main.cpp\n>  $ # For example, I need to update my working tree to latest master\n>  $ git stash save # save local changes of 'main.cpp'...\n>  $ git pull --recurse-submodules && git submodule update --recursive # update to latest\n>  $ git stash pop # I expect to get stashed changes for 'main.cpp', but...\n>    warning: Failed to merge submodule sub1 (commits don't follow merge-base)\n>    Auto-merging sub1\n>    CONFLICT (submodule): Merge conflict in sub1\n>\n> So, this is the issue. Instead of getting my local changes, I got a conflict (and stash is not\n> poped out). The root cause is the 'stash' command does not know how to deal with submodules,\n> but currently it tries to save the state of submodules, and even tries to re-apply the state\n> (and it fails of course). The proposed solution fixes this behaviour.\n>\n> All internal tests work fine with the change.\n\nI think you could take the example as above and make it into a test?\nShowing that this change actually fixes a bug.\n\nLooking for a good place, I would have expected t/t3906-stash-submodule.sh\nwould be a good place to put your code, but I am not sure how to\nproperly integrate that test there.\n\nMaybe we can put the test in t3903 instead?\n\n>\n>\n>>\n>> Do we need a test/documentation updates for this?\n> I don't think so, 'stash' have never claimed submodule support.\n\nBut it also never explicitly claimed it doesn't support it.\n\nMaybe we want to squash in something like\n(with better wording):\n\ndiff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\nindex 92df596..b2649eb 100644\n--- a/Documentation/git-stash.txt\n+++ b/Documentation/git-stash.txt\n@@ -41,6 +41,8 @@ the usual reflog syntax (e.g. `stash@{0}` is the most recently\n created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`\n is also possible).\n\n+Stashing ignores submodule operations completely.\n+\n OPTIONS\n -------\n\n\nThanks,\nStefan\n\n\n\n>\n>>\n>> >\n>> > Signed-off-by: Vasily Titskiy <qehgt0@gmail.com>\n>> > ---\n>> >  git-stash.sh | 2 +-\n>> >  1 file changed, 1 insertion(+), 1 deletion(-)\n>> >\n>> > diff --git a/git-stash.sh b/git-stash.sh\n>> > index c7c65e2..b500c44 100755\n>> > --- a/git-stash.sh\n>> > +++ b/git-stash.sh\n>> > @@ -116,7 +116,7 @@ create_stash () {\n>> >                         git read-tree --index-output=\"$TMPindex\" -m $i_tree &&\n>> >                         GIT_INDEX_FILE=\"$TMPindex\" &&\n>> >                         export GIT_INDEX_FILE &&\n>> > -                       git diff --name-only -z HEAD -- >\"$TMP-stagenames\" &&\n>> > +                       git diff --name-only --ignore-submodules -z HEAD -- >\"$TMP-stagenames\" &&\n>> >                         git update-index -z --add --remove --stdin <\"$TMP-stagenames\" &&\n>> >                         git write-tree &&\n>> >                         rm -f \"$TMPindex\"\n>> > --\n>> > 2.1.4\n>> >\n>> > --\n>> > To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> > the body of a message to majordomo@vger.kernel.org\n>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"286674","messageId":"alpine.DEB.2.20.1605161817590.3303@virtualbox","threadId":"42339","inReplyTo":"20160516154606.GA8797@gmail.com","subject":"Re: [PATCH] Ignore dirty submodule states during stash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-16T16:29:03Z","receivedAt":"2016-05-16T16:29:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Vasily,\n\nOn Mon, 16 May 2016, Vasily Titskiy wrote:\n\n> On Sun, May 15, 2016 at 11:37:20PM -0700, Stefan Beller wrote:\n> > On Sun, May 15, 2016 at 7:07 PM, Vasily Titskiy <qehgt0@gmail.com> wrote:\n> > > Do not save states of submodules as stash should ignore it.\n> > \n> > Can you explain why this is a good idea?\n> > (It is not obvious to me either way.)\n> Actually, submodules are already ignored by stash, but not fully (it was\n> introduced in commit 6848d58).\n> Current behavior is counter-intuitive, for example (if one has a project\n> with a submodule):\n>  [...]\n> \n> So, this is the issue. Instead of getting my local changes, I got a\n> conflict (and stash is not poped out). The root cause is the 'stash'\n> command does not know how to deal with submodules, but currently it\n> tries to save the state of submodules, and even tries to re-apply the\n> state (and it fails of course). The proposed solution fixes this\n> behaviour.\n\nPlease make it a habit to put such information into the commit message. I\nhave to admit that I was as puzzled as Stefan by the proposed patch. Such\nproblems, and tedious back-and-forth discussion, can be easily avoided by\nproviding additional background in the commit message. We even ask in our\nDocumentation/SubmittingPatches explicitly to do that:\n\n> The body should provide a meaningful commit message, which:\n>\n>   . explains the problem the change tries to solve, iow, what is wrong\n>     with the current code without the change.\n>\n>   . justifies the way the change solves the problem, iow, why the\n>     result with the change is better.\n>\n>   . alternate solutions considered but discarded, if any.\n\nBTW this is not something we ask to make contributors' lives harder. In\nfact it helps everybody, including the contributors themselves. Just\nimagine what kind of kick-ass commit message you would want to read in six\nmonths when you stumble over your very own commit and want to know what\nthe heck it was about.\n\nCiao,\nJohannes\n"},{"id":"286761","messageId":"20160517031705.GA2617@gmail.com","threadId":"42339","inReplyTo":"CAGZ79kZESuKiEt2RJJdWJPBySgbDA6abGkZMiTFgzaNCUP1_mA@mail.gmail.com","subject":"Re: [PATCH] Ignore dirty submodule states during stash","fromName":"Vasily Titskiy","fromEmail":"qehgt0@gmail.com","sentAt":"2016-05-17T03:17:05Z","receivedAt":"2016-05-17T03:17:05Z","isPatch":true,"sender":{"key":"qehgt0@gmail.com","avatar":"https://gravatar.com/avatar/a8e2406642cc8b86fc31e7c8858f81d66604cb18719377eaa6afc91e134f34a9?d=mp&s=160"},"body":"Hi Stefan,\n\n> > So, this is the issue. Instead of getting my local changes, I got a conflict (and stash is not\n> > poped out). The root cause is the 'stash' command does not know how to deal with submodules,\n> > but currently it tries to save the state of submodules, and even tries to re-apply the state\n> > (and it fails of course). The proposed solution fixes this behaviour.\n> >\n> > All internal tests work fine with the change.\n> \n> I think you could take the example as above and make it into a test?\n> Showing that this change actually fixes a bug.\n> \n> Looking for a good place, I would have expected t/t3906-stash-submodule.sh\n> would be a good place to put your code, but I am not sure how to\n> properly integrate that test there.\n> \n> Maybe we can put the test in t3903 instead?\nt3903 is better, as t3906 creates its submodule with forced 'ignore' option.\nIt's not a default parameter and it actually hides the issue. I'll send the\npatches soon.\n\n\n> >> Do we need a test/documentation updates for this?\n> > I don't think so, 'stash' have never claimed submodule support.\n> \n> But it also never explicitly claimed it doesn't support it.\n> \n> Maybe we want to squash in something like\n> (with better wording):\n> \n> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt\n> index 92df596..b2649eb 100644\n> --- a/Documentation/git-stash.txt\n> +++ b/Documentation/git-stash.txt\n> @@ -41,6 +41,8 @@ the usual reflog syntax (e.g. `stash@{0}` is the most recently\n>  created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`\n>  is also possible).\n> \n> +Stashing ignores submodule operations completely.\n> +\n>  OPTIONS\n>  -------\nLooks perfect to me.\n\n--\n  Vasily\n"}]}