Re: How do I resolve conflict after popping stash without adding the file to index?
Jeff King p...@peff.net writes: Right, I am suggesting that latter: that stash should abort if the index has modified entries. The abort for modified working tree files is done by git-merge, which can be selective about which entries will be changed (since it knows which ones need written). I haven't thought hard enough to say whether it should be doing the same for the index (i.e., whether this is a merge problem or a stash problem). This is a stash problem. I've always thought that it insisted on having a clean index and a clean working tree, but apparently it doesn't, as shown in Dmitry's example sequence. Generally speaking, any mergy operation should be done with a clean index (i.e. matches HEAD) so that any difference between the index and the HEAD after it stops in the middle is purely the result of that mergy operation, and the commands should stop when the index is not clean as a safety measure (e.g. git am -3, git merge) [*1*]. An especially tricky command may also insist on a clean working tree if it is not easy to guarantee that it will not clobber changes by the user when it stops in the middle (e.g. git rebase). But this is an escape hatch for lazy implementations ;-) It would be ideal if a command stops without doing anything when the set of paths it needs to touch would overlap the set of paths in which user has changes before the command is run (e.g. git merge works that way). I think that stash apply requires a clean working tree for the latter reason, and does not require a clean index because it just forgot that it must do so. -- 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
Re: How do I resolve conflict after popping stash without adding the file to index?
On Wed, Apr 22, 2015 at 10:41:04AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: Right, I am suggesting that latter: that stash should abort if the index has modified entries. The abort for modified working tree files is done by git-merge, which can be selective about which entries will be changed (since it knows which ones need written). I haven't thought hard enough to say whether it should be doing the same for the index (i.e., whether this is a merge problem or a stash problem). This is a stash problem. I've always thought that it insisted on having a clean index and a clean working tree, but apparently it doesn't, as shown in Dmitry's example sequence. It did check the working tree manually, until my e0e2a9c (stash: drop dirty worktree check on apply, 2011-04-05). But I don't think we ever checked that the index was clean with respect to HEAD. Generally speaking, any mergy operation should be done with a clean index (i.e. matches HEAD) so that any difference between the index and the HEAD after it stops in the middle is purely the result of that mergy operation, and the commands should stop when the index is not clean as a safety measure (e.g. git am -3, git merge) [*1*]. I guess this was the heart of my question. Should a mergy operation start with a clean index (which the caller can enforce), or does it only need the index entries that it is going to touch to be clean (which is known only to the merging code)? The latter is more permissive, as we know that we will not create conflict entries that overwrite what is staged in the index. But I don't think we have good tool support for operating on only those entries afterwards (e.g., git reset --keep would hose your staged changes along with undoing the parts modified by the merge). So probably asking for a completely clean index is the only sane thing. An especially tricky command may also insist on a clean working tree if it is not easy to guarantee that it will not clobber changes by the user when it stops in the middle (e.g. git rebase). But this is an escape hatch for lazy implementations ;-) It would be ideal if a command stops without doing anything when the set of paths it needs to touch would overlap the set of paths in which user has changes before the command is run (e.g. git merge works that way). Right, and I think we do that appropriately in stash since e0e2a9c. I think that stash apply requires a clean working tree for the latter reason, and does not require a clean index because it just forgot that it must do so. Ironically, the message before e0e2a9c actually recommends staging changes before applying the stash, which would lead to this exact situation! So I think the most trivial patch is: diff --git a/git-stash.sh b/git-stash.sh index d4cf818..f1865c9 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -442,6 +442,7 @@ apply_stash () { assert_stash_like $@ git update-index -q --refresh || die $(gettext unable to refresh index) + git diff-index --cached HEAD || die dirty index; cannot apply stash # current index state c_tree=$(git write-tree) || but it makes me wonder if somebody would find it annoying that they cannot apply a stash into their work-in-progress (i.e., it _might_ cause annoyance, but most of the time it will be convenient to do so). We also have require_clean_work_tree() in git-sh-setup.sh. We definitely don't want the first half of that, but we do want the diff-index check. So probably we'd want to refactor that into two separate functions, and only call the require_clean_index part. -Peff -- 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
Re: How do I resolve conflict after popping stash without adding the file to index?
On Wed, Apr 22, 2015 at 02:35:40PM -0400, Jeff King wrote: Ironically, the message before e0e2a9c actually recommends staging changes before applying the stash, which would lead to this exact situation! So I think the most trivial patch is: diff --git a/git-stash.sh b/git-stash.sh index d4cf818..f1865c9 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -442,6 +442,7 @@ apply_stash () { assert_stash_like $@ git update-index -q --refresh || die $(gettext unable to refresh index) + git diff-index --cached HEAD || die dirty index; cannot apply stash # current index state c_tree=$(git write-tree) || but it makes me wonder if somebody would find it annoying that they cannot apply a stash into their work-in-progress (i.e., it _might_ cause annoyance, but most of the time it will be convenient to do so). It does actually fail a test in t3903, but I think that test just incidentally had a dirty index, and didn't care about that particular feature. We also have require_clean_work_tree() in git-sh-setup.sh. We definitely don't want the first half of that, but we do want the diff-index check. So probably we'd want to refactor that into two separate functions, and only call the require_clean_index part. This turned out to be more work than it was worth. Most of the effort in that function is about adjusting the messages to handle the cases when either or both of the working tree and index are dirty. I did pick up the useful bits from there, though: - use --quiet to suppress output and so that the exit code actually matters - use -- to disambiguate the ref - I didn't pick up the `rev-parse HEAD` call. I don't think it's necessary (i.e., diff-index should barf for us if it can't read HEAD). Here are the patches. [1/3]: t3903: stop hard-coding commit sha1s [2/3]: t3903: avoid applying onto dirty index [3/3]: stash: require a clean index to apply -Peff -- 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
Re: How do I resolve conflict after popping stash without adding the file to index?
Jeff King p...@peff.net writes: Ironically, the message before e0e2a9c actually recommends staging changes before applying the stash, which would lead to this exact situation! The ancient history is hazy to me, but did we fall back to three-way merge in old days (or did anything to the index for that matter), I wonder? In a world git stash apply only applied the change to the working tree via git apply, that old recommendation would make perfect sense. But obviously we do not live in such a world right now. And because we are doing merge-recursive, we should insist on a clean index; otherwise there is no way to undo its effect without losing the changes by the end-user. So I think the most trivial patch is: diff --git a/git-stash.sh b/git-stash.sh index d4cf818..f1865c9 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -442,6 +442,7 @@ apply_stash () { assert_stash_like $@ git update-index -q --refresh || die $(gettext unable to refresh index) + git diff-index --cached HEAD || die dirty index; cannot apply stash Yes, that makes sense. The original report from Dmitry was triggering the safety from one line above and git stash pop doing the right thing by refusing to touch the index with unresolved mergy operation before doing anything, and with this additional safety, we would make it even safer from people who do git add and then git stash pop (which is somewhat strange thing to do, given that stash was designed for stash to save away; do other things; come back to the original commit state that is 'reset --hard' clean; unstash sequence in the first place). # current index state c_tree=$(git write-tree) || but it makes me wonder if somebody would find it annoying that they cannot apply a stash into their work-in-progress (i.e., it _might_ cause annoyance, but most of the time it will be convenient to do so). They can always do git stash show -p stash@{n} | git apply if they want to build changes incrementally X-, but it would be annoying. So probably we'd want to refactor that into two separate functions, and only call the require_clean_index part. Hmph, but what would that helper do, other than a single diff-index --quiet --cached HEAD call? -- 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
Re: How do I resolve conflict after popping stash without adding the file to index?
On Wed, Apr 22, 2015 at 12:45:21PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: Ironically, the message before e0e2a9c actually recommends staging changes before applying the stash, which would lead to this exact situation! The ancient history is hazy to me, but did we fall back to three-way merge in old days (or did anything to the index for that matter), I wonder? In a world git stash apply only applied the change to the working tree via git apply, that old recommendation would make perfect sense. Hmm, that advice came in 2a79d2f (Clarify how the user can satisfy stash's 'dirty state' check., 2008-09-29), at which point it looks like we were already running merge-recursive. So I think it was simply bad advice. ;) But obviously we do not live in such a world right now. And because we are doing merge-recursive, we should insist on a clean index; otherwise there is no way to undo its effect without losing the changes by the end-user. Yeah, agreed. but it makes me wonder if somebody would find it annoying that they cannot apply a stash into their work-in-progress (i.e., it _might_ cause annoyance, but most of the time it will be convenient to do so). They can always do git stash show -p stash@{n} | git apply if they want to build changes incrementally X-, but it would be annoying. I think the best thing to do is introduce this safety, let it cook for a while, and see what comes up. Perhaps we could add a --force or similar, but I'd rather see if anybody ever actually runs into the situation first. So probably we'd want to refactor that into two separate functions, and only call the require_clean_index part. Hmph, but what would that helper do, other than a single diff-index --quiet --cached HEAD call? I was wanting to keep the error message and the flags we feed to diff-index consistent. But yeah, there is little enough duplicate material and enough added boilerplate that I do not think it is worth it (see the series I just posted). -Peff -- 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
Re: How do I resolve conflict after popping stash without adding the file to index?
On 04/22/2015 12:29 AM, Jeff King wrote: Hmm, interestingly, if you do _not_ stage the changes (i.e., drop the final git add there), you get: $ git stash pop error: Your local changes to the following files would be overwritten by merge: test Please, commit your changes or stash them before you can merge. Aborting which makes sense. Writing conflict markers into the file would leave you in a situation where it is hard to recover the b content. Indeed. But we seem to skip that safety valve when the content has been staged, which seems questionable to me (technically we are slightly better off than the protected case because b was written to a git blob object, so you can recover it. But it may be difficult to find the correct blob in the object database). Any suggestions how to restore that content in the index programmatically? If it's non-trivial to do, maybe that is indeed a bug, and 'git stash pop' should abort before creating the conflict. -- 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
Re: How do I resolve conflict after popping stash without adding the file to index?
On Wed, Apr 22, 2015 at 01:35:05AM +0300, Dmitry Gutov wrote: But we seem to skip that safety valve when the content has been staged, which seems questionable to me (technically we are slightly better off than the protected case because b was written to a git blob object, so you can recover it. But it may be difficult to find the correct blob in the object database). Any suggestions how to restore that content in the index programmatically? If it's non-trivial to do, maybe that is indeed a bug, and 'git stash pop' should abort before creating the conflict. Right, I am suggesting that latter: that stash should abort if the index has modified entries. The abort for modified working tree files is done by git-merge, which can be selective about which entries will be changed (since it knows which ones need written). I haven't thought hard enough to say whether it should be doing the same for the index (i.e., whether this is a merge problem or a stash problem). -Peff -- 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
Re: How do I resolve conflict after popping stash without adding the file to index?
On Tue, Apr 21, 2015 at 01:54:56AM +0300, Dmitry Gutov wrote: I'm not really sure what higher stage entries are, but this scenario seems to be a counter-example: git init echo a test git add test git commit -m first echo aaa test git stash save echo b test git add test git stash pop Either that, or 'git stash pop' was a destructive operation, and ate the staged changes. Hmm, interestingly, if you do _not_ stage the changes (i.e., drop the final git add there), you get: $ git stash pop error: Your local changes to the following files would be overwritten by merge: test Please, commit your changes or stash them before you can merge. Aborting which makes sense. Writing conflict markers into the file would leave you in a situation where it is hard to recover the b content. But we seem to skip that safety valve when the content has been staged, which seems questionable to me (technically we are slightly better off than the protected case because b was written to a git blob object, so you can recover it. But it may be difficult to find the correct blob in the object database). -Peff -- 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
Re: How do I resolve conflict after popping stash without adding the file to index?
On 04/21/2015 12:11 AM, Junio C Hamano wrote: But the said file, if it had conflicted, would have had only the conflicted higher stage entries in the index, no? That is, the failed merge wouldn't have touched the index for the path if it already had changes there in the first place. I'm not really sure what higher stage entries are, but this scenario seems to be a counter-example: git init echo a test git add test git commit -m first echo aaa test git stash save echo b test git add test git stash pop Either that, or 'git stash pop' was a destructive operation, and ate the staged changes. If you want to keep them then you do not have to reset, but your question is about resolving conflict only in the working tree and leave the index clean, so I do not think git reset -- $path would not lose anything irreversibly. Rather, I'd prefer to leave the index as-is, if it makes sense. Basically, this is about tool automation, see the context here: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20292 -- 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
Re: How do I resolve conflict after popping stash without adding the file to index?
Dmitry Gutov dgu...@yandex.ru writes: Either will reset already-staged changes from the said file, which is an irreversible operation. But the said file, if it had conflicted, would have had only the conflicted higher stage entries in the index, no? That is, the failed merge wouldn't have touched the index for the path if it already had changes there in the first place. If you want to keep them then you do not have to reset, but your question is about resolving conflict only in the working tree and leave the index clean, so I do not think git reset -- $path would not lose anything irreversibly. -- 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