Re: [PATCH 3/3] stash: require a clean index to apply

[Jeff King <peff@peff.net>]

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@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..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