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.GC27945@xxxxxxxx>, 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..0000000 --- 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html