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 
[off-list ref], 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

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
[off-list ref], 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

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

On Sun, Jun 7, 2015 at 9:40 AM, Jeff King [off-list ref] 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

On Wed, Jun 10, 2015 at 03:19:41PM -0300, bär wrote:

On Sun, Jun 7, 2015 at 9:40 AM, Jeff King [off-list ref] wrote:

quoted

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