Re: [PATCH 3/3] stash: require a clean index to apply
Jeff King p...@peff.net writes: It seems like applying a stash made with -k is fundamentally misdesigned in the current code. We would want to apply to the working tree the difference between the index and the working tree, but instead we try to apply the difference between the HEAD and the working tree. Which is nonsensical for this use case (i.e., to apply the diff between $stash and $stash^2, not $stash^1). I don't think there is any way to tell that -k was used, though. But even if the user knew that, I do not think there is any option to tell stash apply to do it this way. I dunno. With respect to the original patch, I am OK if we just want to revert it. This area of stash seems a bit under-designed IMHO, but if people were happy enough with it before, I do not think the safety benefit from ed178ef is that great (it is not saving you from destroying working tree content, only the index state; the individual content blobs are still available from git-fsck). Yeah, I agree. Somebody care to do the log message? This is a tangent, but I'd actually not just agree with 'stash -k' is a bit under-designed but also would say something stronger than that, IMHO ;-) The very original idea of git stash to ssave away working tree changes with respect to HEAD without bothering the index was at least internally consistent. You save away and then may later pop the change on top of a different commit after dealing with an emergency, at which time there may be conflicts that you would need to resolve, and because it is only between HEAD and working tree, you can safely use the index to resolve the conflicts just like in any other situation when you help Git to resolve them. In that context, the flaw ed178ef1 (stash: require a clean index to apply, 2015-04-22) tried to correct made a lot of sense. What broke the entire system was the throwing the save the index, too into the mix, which was done without careful thinking. While it made the command more useful (when it works), it made the command internally inconsistent---the command has to punt restoring index state when it can't, for example, losing the state it tried to save. I think that is where the under-designed started, I would think; of course, -k needed to build on top of save also the index, but that is not the primary culprit. -- 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: [PATCH 3/3] stash: require a clean index to apply
Jeff King p...@peff.net writes: So I am trying to figure out what the use case here is. Clearly the above is a toy case, but why is stash -k followed by a quick pop useful in general? Certainly I use stash (without -k) and a quick pop all the time, and I think that is what stash was designed for. The best use case I can think of is Jonathan's original: to see only the staged content in the working tree, and then restore the original state. But stash does not currently work very well for that, as shown above. The canonical use case for stash -k is to see only the content to be committed (for testing), commit it after testing and then pop on top of the committed result, which is the same as what you saw in the working tree and the index when you did stash -k. I do not think stash -k stash pop was in the design parameter when -k was added (as you demonstrated, it would not fundamentally work reliably depending on the differences between HEAD-Index-Worktree). -- 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: [PATCH 3/3] stash: require a clean index to apply
On Wed, Jun 10, 2015 at 12:16:25PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: So I am trying to figure out what the use case here is. Clearly the above is a toy case, but why is stash -k followed by a quick pop useful in general? Certainly I use stash (without -k) and a quick pop all the time, and I think that is what stash was designed for. The best use case I can think of is Jonathan's original: to see only the staged content in the working tree, and then restore the original state. But stash does not currently work very well for that, as shown above. The canonical use case for stash -k is to see only the content to be committed (for testing), commit it after testing and then pop on top of the committed result, which is the same as what you saw in the working tree and the index when you did stash -k. I do not think stash -k stash pop was in the design parameter when -k was added (as you demonstrated, it would not fundamentally work reliably depending on the differences between HEAD-Index-Worktree). It seems like applying a stash made with -k is fundamentally misdesigned in the current code. We would want to apply to the working tree the difference between the index and the working tree, but instead we try to apply the difference between the HEAD and the working tree. Which is nonsensical for this use case (i.e., to apply the diff between $stash and $stash^2, not $stash^1). I don't think there is any way to tell that -k was used, though. But even if the user knew that, I do not think there is any option to tell stash apply to do it this way. I dunno. With respect to the original patch, I am OK if we just want to revert it. This area of stash seems a bit under-designed IMHO, but if people were happy enough with it before, I do not think the safety benefit from ed178ef is that great (it is not saving you from destroying working tree content, only the index state; the individual content blobs are still available from git-fsck). -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: [PATCH 3/3] stash: require a clean index to apply
On Wed, Jun 10, 2015 at 03:19:41PM -0300, bär wrote: On Sun, Jun 7, 2015 at 9:40 AM, Jeff King p...@peff.net wrote: Hrm. The new protection in v2.4.2 is meant to prevent you from losing your index state during step 4 when we run into a conflict. But here you know something that git doesn't: that we just created the stash based on this same state, so it should apply cleanly. It is strange that `git stash --keep-index git stash pop` while having something in the index, fails with a Cannot apply stash: Your index contains uncommitted changes. error, even if we have a `--force` param it find it awkward that one needs to force applying/pop'ing even though the stash was created from the same snapshot where the stash is being merged with. I understand the original issue, but at least it was expected, when you stash save/pop/apply, you should know what you are doing anyways. Yeah, I would expect stash pop to work in general. But I find it puzzling that it does not always with -k (this is with v2.3.x): $ git init -q $ echo head file git add file git commit -qm head $ echo index file git add file $ echo tree file $ git stash --keep-index git stash pop Saved working directory and index state WIP on master: 74f6d33 head HEAD is now at 74f6d33 head Auto-merging file CONFLICT (content): Merge conflict in file So I am trying to figure out what the use case here is. Clearly the above is a toy case, but why is stash -k followed by a quick pop useful in general? Certainly I use stash (without -k) and a quick pop all the time, and I think that is what stash was designed for. The best use case I can think of is Jonathan's original: to see only the staged content in the working tree, and then restore the original state. But stash does not currently work very well for that, as shown above. -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: [PATCH 3/3] stash: require a clean index to apply
On Sun, Jun 7, 2015 at 9:40 AM, Jeff King p...@peff.net wrote: Hrm. The new protection in v2.4.2 is meant to prevent you from losing your index state during step 4 when we run into a conflict. But here you know something that git doesn't: that we just created the stash based on this same state, so it should apply cleanly. It is strange that `git stash --keep-index git stash pop` while having something in the index, fails with a Cannot apply stash: Your index contains uncommitted changes. error, even if we have a `--force` param it find it awkward that one needs to force applying/pop'ing even though the stash was created from the same snapshot where the stash is being merged with. I understand the original issue, but at least it was expected, when you stash save/pop/apply, you should know what you are doing anyways. -- Ber Clausen -- 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: [PATCH 3/3] stash: require a clean index to apply
On Wed, Jun 10, 2015 at 4:27 PM, Jeff King p...@peff.net wrote: I dunno. With respect to the original patch, I am OK if we just want to revert it. This area of stash seems a bit under-designed IMHO, but if people were happy enough with it before, I do not think the safety benefit from ed178ef is that great (it is not saving you from destroying working tree content, only the index state; the individual content blobs are still available from git-fsck). I feel the same way, in fact I'm +1 to revert it until we figure out a better way to deal with this properly. -- Ber Clausen -- 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: [PATCH 3/3] stash: require a clean index to apply
On Thu, Jun 04, 2015 at 08:43:00PM -0400, Jonathan Kamens wrote: I'm writing about the patch that Jeff King submitted on April 22, in 20150422193101.gc27...@peff.net, in particular, https://github.com/git/git/commit/ed178ef13a26136d86ff4e33bb7b1afb5033f908 . It appears that this patch was included in git 2.4.2, and it breaks my workflow. In particular, I have a pre-commit hook whith does the following: 1. Stash unstaged changes (git stash -k). 2. Run flake8 over all staged changes. 3. If flake8 complains, then error out of the commit. 4. Otherwise, apply the stash and exit. Hrm. The new protection in v2.4.2 is meant to prevent you from losing your index state during step 4 when we run into a conflict. But here you know something that git doesn't: that we just created the stash based on this same state, so it should apply cleanly. So besides the obvious fix of reverting the patch, we could perhaps do something along the lines of: 1. Add a --force option to tell git to do it anyway. 2. Only have the protection kick in when we would in fact have a conflict. This is probably hard to implement, though, as we don't know until we do the merge (so it would probably involve teaching merge a mode where it bails immediately on conflicts rather than writing out the conflicted entries to the index). However, I am puzzled by something in your workflow: does it work when you have staged and working tree changes to the same hunk? For example: [differing content for HEAD, index, and working tree] $ git init $ echo base file git add file git commit -m base $ echo index file git add file $ echo working file [make our stash] $ git stash -k Saved working directory and index state WIP on master: dc7f0dd base HEAD is now at dc7f0dd base [new version] $ git.v2.4.2 stash apply Cannot apply stash: Your index contains uncommitted changes. [old version] $ git.v2.4.1 stash apply Auto-merging file CONFLICT (content): Merge conflict in file $ git diff diff --cc file index 9015a7a,d26b33d..000 --- a/file +++ b/file @@@ -1,1 -1,1 +1,5 @@@ ++ Updated upstream +index ++=== + working ++ Stashed changes So v2.4.1 shows a conflict, and loses the index state you had. Is there something more to your hook that I'm missing (stash options, or something else) that covers this case? The reason I have to do things this way is as follows. Suppose I did the following: 1. Stage changes that have a flake8 violation. 2. Fix the flake8 violation in the unstaged version of the staged file. 3. Commit the previously staged changes. If my commit hook runs over the unstaged version of the file, then it won't detect the flake8 violation, and as a result the violation will be committed. Yeah, the fundamental pattern makes sense. You want to test what is going into the commit, not what happens to be in the working tree. One way to do that would be to checkout the proposed index to a temporary directory and operate on that. But of course that's inefficient if most of the files are unchanged. Are you running flake8 across the whole tree, or just on the files with proposed changes? Does it need to see the whole tree? If you can get away with feeding it single files, then it should be very efficient to loop over the output of git diff-index HEAD and feed the proposed updated blobs to it. If you can get away with feeding single lines, then feeding the actual diffs to it would be even better, but I assume that is not enough (I do not use flake8 myself). -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: [PATCH 3/3] stash: require a clean index to apply
On Sun, Jun 07, 2015 at 08:40:01AM -0400, Jeff King wrote: Are you running flake8 across the whole tree, or just on the files with proposed changes? Does it need to see the whole tree? If you can get away with feeding it single files, then it should be very efficient to loop over the output of git diff-index HEAD and feed the proposed updated blobs to it. If you can get away with feeding single lines, then feeding the actual diffs to it would be even better, but I assume that is not enough (I do not use flake8 myself). Like I said, I do not use it, but peeking at the flake8 source code, it has an --install-hook option to set up a pre-commit hook. It looks like the hook runs git diff-index --cached --name-only HEAD to get the list of modified files, filters that only for *.py, and then copies the results into a temporary directory to operate on. -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: [PATCH 3/3] stash: require a clean index to apply
I'm writing about the patch that Jeff King submitted on April 22, in 20150422193101.gc27...@peff.net, in particular, https://github.com/git/git/commit/ed178ef13a26136d86ff4e33bb7b1afb5033f908 . It appears that this patch was included in git 2.4.2, and it breaks my workflow. In particular, I have a pre-commit hook whith does the following: 1. Stash unstaged changes (git stash -k). 2. Run flake8 over all staged changes. 3. If flake8 complains, then error out of the commit. 4. Otherwise, apply the stash and exit. This way I am prevented from committing staged changes that don't pass flake8. I can't imagine that this is a terribly uncommon workflow. This worked fine until the aforementioned comment, after which my hook complains, Cannot apply stash: Your index contains uncommitted changes. The reason I have to do things this way is as follows. Suppose I did the following: 1. Stage changes that have a flake8 violation. 2. Fix the flake8 violation in the unstaged version of the staged file. 3. Commit the previously staged changes. If my commit hook runs over the unstaged version of the file, then it won't detect the flake8 violation, and as a result the violation will be committed. If anyone has a suggestion for how I can achieve the desired goal within the constraints of the 2.4.2 version of git-stash.sh, I'd love to hear it. Otherwise, I'd like to ask for this patch to be reconsidered. Thank you, Jonathan Kamens -- 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
