How do I resolve conflict after popping stash without adding the file to index?

How do I resolve conflict after popping stash without adding the file to index?

[Dmitry Gutov]

Hi all,

After the user does 'git stash pop', it may result in conflicts.

However, in many cases they may not intend to commit the stashed changes 
right away, so staging the applied changes is often not what they intend 
to do. However, the conflict is there until you mark it as resolved.

What's the proper thing to do there? 'git add file.ext' followed by 'git 
reset file.ext'? Or simply 'git reset file.ext'?

Either will reset already-staged changes from the said file, which is an 
irreversible operation.

Best regards,
Dmitry.

Re: How do I resolve conflict after popping stash without adding the file to index?

[Junio C Hamano]

Dmitry Gutov <dgutov@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".

Re: How do I resolve conflict after popping stash without adding the file to index?

[Dmitry Gutov]

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 "aaaaa" > test
git add test
git commit -m "first"
echo "aaa" > test
git stash save
echo "bbbbb" > 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

Re: How do I resolve conflict after popping stash without adding the file to index?

[Jeff King]

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 "aaaaa" > test
> git add test
> git commit -m "first"
> echo "aaa" > test
> git stash save
> echo "bbbbb" > 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 "bbbbb" 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 "bbbbb" 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

Re: How do I resolve conflict after popping stash without adding the file to index?

[Dmitry Gutov]

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 "bbbbb" 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 "bbbbb" 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.

Re: How do I resolve conflict after popping stash without adding the file to index?

[Jeff King]

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 "bbbbb" 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

Re: How do I resolve conflict after popping stash without adding the file to index?

[Junio C Hamano]

Jeff King <peff@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.

Re: How do I resolve conflict after popping stash without adding the file to index?

[Jeff King]

On Wed, Apr 22, 2015 at 10:41:04AM -0700, Junio C Hamano wrote:

> Jeff King <peff@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

Re: How do I resolve conflict after popping stash without adding the file to index?

[Jeff King]

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

[PATCH 1/3] t3903: stop hard-coding commit sha1s

[Jeff King]

When testing the diff output of "git stash list", we look
for the stash's subject of "WIP on master: $sha1", even
though it's not relevant to the diff output. This makes the
test brittle to refactoring, as any changes to earlier tests
may impact the commit sha1.

Since we don't care about the commit subject here, we can
simply ask stash not to print it.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3903-stash.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 1e29962..6da4856 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -695,8 +695,8 @@ test_expect_success 'setup stash with index and worktree changes' '
 '
 
 test_expect_success 'stash list implies --first-parent -m' '
-	cat >expect <<-\EOF &&
-	stash@{0}: WIP on master: b27a2bc subdir
+	cat >expect <<-EOF &&
+	stash@{0}
 
 	diff --git a/file b/file
 	index 257cc56..d26b33d 100644
@@ -706,13 +706,13 @@ test_expect_success 'stash list implies --first-parent -m' '
 	-foo
 	+working
 	EOF
-	git stash list -p >actual &&
+	git stash list --format=%gd -p >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'stash list --cc shows combined diff' '
 	cat >expect <<-\EOF &&
-	stash@{0}: WIP on master: b27a2bc subdir
+	stash@{0}
 
 	diff --cc file
 	index 257cc56,9015a7a..d26b33d
@@ -723,7 +723,7 @@ test_expect_success 'stash list --cc shows combined diff' '
 	 -index
 	++working
 	EOF
-	git stash list -p --cc >actual &&
+	git stash list --format=%gd -p --cc >actual &&
 	test_cmp expect actual
 '
 
-- 
2.4.0.rc2.498.g02440db

[PATCH 2/3] t3903: avoid applying onto dirty index

[Jeff King]

One of the tests in t3903 wants to make sure that applying a
stash that touches only "file" can still happen even if there
are working tree changes to "other-file". To do so, it adds
"other-file" to the index (since otherwise it is an
untracked file, voiding the purpose of the test).

But as we are about to refactor the dirty-index handling,
and as this test does not actually care about having a dirty
index (only a dirty working tree), let's bump the tracking
of "other-file" into the setup phase, so we can have _just_
a dirty working tree here.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3903-stash.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 6da4856..f179c93 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -10,6 +10,8 @@ test_description='Test git stash'
 test_expect_success 'stash some dirty working directory' '
 	echo 1 > file &&
 	git add file &&
+	echo unrelated >other-file &&
+	git add other-file &&
 	test_tick &&
 	git commit -m initial &&
 	echo 2 > file &&
@@ -45,8 +47,6 @@ test_expect_success 'applying bogus stash does nothing' '
 
 test_expect_success 'apply does not need clean working directory' '
 	echo 4 >other-file &&
-	git add other-file &&
-	echo 5 >other-file &&
 	git stash apply &&
 	echo 3 >expect &&
 	test_cmp expect file
-- 
2.4.0.rc2.498.g02440db

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

[Jeff King]

If you have staged contents in your index and run "stash
apply", we may hit a conflict and put new entries into the
index. Recovering to your original state is difficult at
that point, because tools like "git reset --keep" will blow
away anything staged.  We can make this safer by refusing to
apply when there are staged changes.

It's possible we could provide better tooling here, as "git
stash apply" should be writing only conflicts to the index
(so we know that any stage-0 entries are potentially
precious). But it is the odd duck; most "mergy" commands
will update the index for cleanly merged entries, and it is
not worth updating our tooling to support this use case
which is unlikely to be of interest (besides which, we would
still need to block a dirty index for "stash apply --index",
since that case _would_ be ambiguous).

Signed-off-by: Jeff King <peff@peff.net>
---
 git-stash.sh     | 2 ++
 t/t3903-stash.sh | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/git-stash.sh b/git-stash.sh
index d4cf818..cc28368 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -442,6 +442,8 @@ apply_stash () {
 	assert_stash_like "$@"
 
 	git update-index -q --refresh || die "$(gettext "unable to refresh index")"
+	git diff-index --cached --quiet --ignore-submodules HEAD -- ||
+		die "$(gettext "Cannot apply stash: Your index contains uncommitted changes.")"
 
 	# current index state
 	c_tree=$(git write-tree) ||
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index f179c93..0746eee 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -45,6 +45,13 @@ test_expect_success 'applying bogus stash does nothing' '
 	test_cmp expect file
 '
 
+test_expect_success 'apply requires a clean index' '
+	test_when_finished "git reset --hard" &&
+	echo changed >other-file &&
+	git add other-file &&
+	test_must_fail git stash apply
+'
+
 test_expect_success 'apply does not need clean working directory' '
 	echo 4 >other-file &&
 	git stash apply &&
-- 
2.4.0.rc2.498.g02440db

Re: How do I resolve conflict after popping stash without adding the file to index?

[Junio C Hamano]

Jeff King <peff@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?

Re: How do I resolve conflict after popping stash without adding the file to index?

[Jeff King]

On Wed, Apr 22, 2015 at 12:45:21PM -0700, Junio C Hamano wrote:

> Jeff King <peff@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

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

[Jonathan Kamens]

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.

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[bär]

On Sun, Jun 7, 2015 at 9:40 AM, Jeff King <peff@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

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

[Jeff King]

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

> On Sun, Jun 7, 2015 at 9:40 AM, Jeff King <peff@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

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

[Junio C Hamano]

Jeff King <peff@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).

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

[Jeff King]

On Wed, Jun 10, 2015 at 12:16:25PM -0700, Junio C Hamano wrote:

> Jeff King <peff@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

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

[bär]

On Wed, Jun 10, 2015 at 4:27 PM, Jeff King <peff@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

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

[Junio C Hamano]

Jeff King <peff@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.