Bug: stash staged file move loses original file deletion

Bug: stash staged file move loses original file deletion

[Matthew Patey]

Git version 2.8.0 (sorry can't update to more recent on my machine) on Ubuntu.

After moving a file, if the new file is in the index but the deletion
of the old file is not staged, git stash loses the deletion operation.
Repro:

1. git mv a b
This will have both the "deletion" and the "file added" in the index

2. git stash; git stash pop
Now file a is still in the index, but file b deletion is not staged.

3. git stash; git stash show -v
This will show that the deletion operation is not in the stash

4. git stash pop
Again confirms the issue, file a is in the index, but file b is
present and unmodified in the working directory.

Re: Bug: stash staged file move loses original file deletion

[Jeff King]

On Mon, Dec 05, 2016 at 09:37:51AM -0500, Matthew Patey wrote:

> Git version 2.8.0 (sorry can't update to more recent on my machine) on Ubuntu.

The behavior is the same on more recent versions of git, too. The short
answer is "use --index". The longer one is below.

> After moving a file, if the new file is in the index but the deletion
> of the old file is not staged, git stash loses the deletion operation.
> Repro:
> 
> 1. git mv a b
> This will have both the "deletion" and the "file added" in the index
> 
> 2. git stash; git stash pop
> Now file a is still in the index, but file b deletion is not staged.
> 
> 3. git stash; git stash show -v
> This will show that the deletion operation is not in the stash
> 
> 4. git stash pop
> Again confirms the issue, file a is in the index, but file b is
> present and unmodified in the working directory.

Thanks for a clear reproduction case. I think the oddball, though, is
not that "b" is not staged for deletion, but that the addition of "a"
_is_ staged.

Applying a stash usually does not re-stage index contents, unless you
specify --index. For example, try:

  # Make a staged change
  echo change >>a
  git add a

  # This puts the change back into the working tree, but does _not_
  # put it into the index.
  git stash apply

  # Now reset to try again.
  git reset --hard

  # This does restore the index.
  git stash apply --index

So in your case, the deletion of "b" is following that same rule. What's
unusual is that "a" is staged. There's code specifically in git-stash
specifically to make sure this is the case, but I don't remember offhand
why this is so. The code comes from the original f2c66ed19 (Add
git-stash script, 2007-06-30), which in turn seems to come from Junio's
comments in:

  http://public-inbox.org/git/7vmyyq2zrz.fsf@assigned-by-dhcp.pobox.com/

I don't see any specific reasoning, but I think it is simply that it is
confusing for the files to become untracked totally. These days we have
intent-to-add via "git add -N", so that might actually be a better
choice for this case.

Anyway, that history digression is neither here nor there for your case.
If you want to restore the index contents, use "--index". That does what
you were expecting:

  $ git mv a b
  $ git stash && git stash apply --index
  Saved working directory and index state WIP on master: 5355755 foo
  HEAD is now at 5355755 foo
  On branch master
  Changes to be committed:
          renamed:    a -> b

-Peff

Re: Bug: stash staged file move loses original file deletion

[Jeff King]

On Tue, Dec 06, 2016 at 02:45:05PM +0000, Matthew Patey wrote:

> Thanks for the reply! I agree that it is weird that applying a stash with a
> move only puts the addition back in the index, and thanks for the tip on
> "index" to properly apply that. For this case I was focusing on the
> behavior of the second stash/apply, with the first stash/apply as an
> example of how to get into a weird state that triggers the odd behavior of
> the second.

Oh, heh, I totally missed that.

I agree that the second stash not saving the deletion seems like a bug.
Oddly, just stashing a deletion, like:

  rm a
  git stash
  git stash apply

does store it. So it's not like we can't represent the deletion. Somehow
the presence of "b" in the index matters.

It looks like the culprit may be this part of create_stash():

  git diff --name-only -z HEAD -- >"$TMP-stagenames"

That is using the "git diff" porcelain, which will respect renames, and
the --name-only bit mentions only "b".

If you run:

  git -c diff.renames=false stash

then it works.

-Peff

[PATCH] stash: disable renames when calling git-diff

[Jeff King]

On Tue, Dec 06, 2016 at 10:25:30AM -0500, Jeff King wrote:

> I agree that the second stash not saving the deletion seems like a bug.
> Oddly, just stashing a deletion, like:
> 
>   rm a
>   git stash
>   git stash apply
> 
> does store it. So it's not like we can't represent the deletion. Somehow
> the presence of "b" in the index matters.
> 
> It looks like the culprit may be this part of create_stash():
> 
>   git diff --name-only -z HEAD -- >"$TMP-stagenames"
> 
> That is using the "git diff" porcelain, which will respect renames, and
> the --name-only bit mentions only "b".
> 
> If you run:
> 
>   git -c diff.renames=false stash
> 
> then it works.

And here's a patch to fix it.

-- >8 --
Subject: [PATCH] stash: disable renames when calling git-diff

When creating a stash, we need to look at the diff between
the working tree and HEAD. There's no plumbing command that
covers this case, so we do so by calling the git-diff
porcelain. This means we also inherit the usual list of
porcelain options, which can impact the output in confusing
ways.

There's at least one known problem: --name-only will not
mention the source side of a rename, meaning that we will
fail to stash a deletion in such a case.

The correct solution is to move to an appropriate plumbing
command. But since there isn't one available, in the short
term we can plug this particular problem by manually asking
git-diff not to respect renames.

Reported-by: Matthew Patey <matthew.patey2167@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
There may be other similar problems lurking depending on the various
config options you have set, but I don't think so. Most of the options
only affect --patch output.

I do find it interesting that --name-only does not mention the source
side of a rename. The longer forms like --name-status mention both
sides. Certainly --name-status does not have any way of saying "this is
the source side of a rename", but I'd think it would simply mention both
sides. Anyway, even if that were changed (which would fix this bug), I
think adding --no-renames is still a good thing to be doing here.

 git-stash.sh     | 2 +-
 t/t3903-stash.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index 4546abaae..40ab2710e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -115,7 +115,7 @@ create_stash () {
 			git read-tree --index-output="$TMPindex" -m $i_tree &&
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
-			git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
+			git diff --name-only -z --no-renames HEAD -- >"$TMP-stagenames" &&
 			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index e1a6ccc00..2de3e18ce 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -766,4 +766,13 @@ test_expect_success 'stash list --cc shows combined diff' '
 	test_cmp expect actual
 '
 
+test_expect_success 'stash is not confused by partial renames' '
+	mv file renamed &&
+	git add renamed &&
+	git stash &&
+	git stash apply &&
+	test_path_is_file renamed &&
+	test_path_is_missing file
+'
+
 test_done
-- 
2.11.0.191.gdb26c57

Re: [PATCH] stash: disable renames when calling git-diff

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> If you run:
>> 
>>   git -c diff.renames=false stash
>> 
>> then it works.
>
> And here's a patch to fix it.

Yuck.  This obviously has easier to bite more people since we
enabled the renames by default.  Thanks for a quick fix.

I wonder why we are using "git diff" here, not the plumbing,
though.

>
> -- >8 --
> Subject: [PATCH] stash: disable renames when calling git-diff
>
> When creating a stash, we need to look at the diff between
> the working tree and HEAD. There's no plumbing command that
> covers this case, so we do so by calling the git-diff
> porcelain. This means we also inherit the usual list of
> porcelain options, which can impact the output in confusing
> ways.
>
> There's at least one known problem: --name-only will not
> mention the source side of a rename, meaning that we will
> fail to stash a deletion in such a case.
>
> The correct solution is to move to an appropriate plumbing
> command. But since there isn't one available, in the short
> term we can plug this particular problem by manually asking
> git-diff not to respect renames.
>
> Reported-by: Matthew Patey <matthew.patey2167@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> There may be other similar problems lurking depending on the various
> config options you have set, but I don't think so. Most of the options
> only affect --patch output.
>
> I do find it interesting that --name-only does not mention the source
> side of a rename. The longer forms like --name-status mention both
> sides. Certainly --name-status does not have any way of saying "this is
> the source side of a rename", but I'd think it would simply mention both
> sides. Anyway, even if that were changed (which would fix this bug), I
> think adding --no-renames is still a good thing to be doing here.
>
>  git-stash.sh     | 2 +-
>  t/t3903-stash.sh | 9 +++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 4546abaae..40ab2710e 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -115,7 +115,7 @@ create_stash () {
>  			git read-tree --index-output="$TMPindex" -m $i_tree &&
>  			GIT_INDEX_FILE="$TMPindex" &&
>  			export GIT_INDEX_FILE &&
> -			git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
> +			git diff --name-only -z --no-renames HEAD -- >"$TMP-stagenames" &&
>  			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
>  			git write-tree &&
>  			rm -f "$TMPindex"
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index e1a6ccc00..2de3e18ce 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -766,4 +766,13 @@ test_expect_success 'stash list --cc shows combined diff' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'stash is not confused by partial renames' '
> +	mv file renamed &&
> +	git add renamed &&
> +	git stash &&
> +	git stash apply &&
> +	test_path_is_file renamed &&
> +	test_path_is_missing file
> +'
> +
>  test_done

Re: [PATCH] stash: disable renames when calling git-diff

[Jeff King]

On Tue, Dec 06, 2016 at 11:20:18AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> If you run:
> >> 
> >>   git -c diff.renames=false stash
> >> 
> >> then it works.
> >
> > And here's a patch to fix it.
> 
> Yuck.  This obviously has easier to bite more people since we
> enabled the renames by default.  Thanks for a quick fix.
> 
> I wonder why we are using "git diff" here, not the plumbing,
> though.

I don't think there's a plumbing command which works for diffing the
working tree directly to a git tree. In the long run, it might be a good
idea to remedy that.

Though I'm not sure that "git add -u" would not accomplish the same
thing as these several commands.

-Peff

Re: [PATCH] stash: disable renames when calling git-diff

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Dec 06, 2016 at 11:20:18AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >> If you run:
>> >> 
>> >>   git -c diff.renames=false stash
>> >> 
>> >> then it works.
>> >
>> > And here's a patch to fix it.
>> 
>> Yuck.  This obviously has easier to bite more people since we
>> enabled the renames by default.  Thanks for a quick fix.
>> 
>> I wonder why we are using "git diff" here, not the plumbing,
>> though.
>
> I don't think there's a plumbing command which works for diffing the
> working tree directly to a git tree. In the long run, it might be a good
> idea to remedy that.

I do not think that one is doing anything different from "git
diff-index --name-only -z HEAD".

> Though I'm not sure that "git add -u" would not accomplish the same
> thing as these several commands.

Yeah, it looks like "add -u" to me, too.  Perhaps the script was old
enough that it didn't exist back then?  I dunno.

Re: [PATCH] stash: disable renames when calling git-diff

[Jeff King]

On Tue, Dec 06, 2016 at 11:51:16AM -0800, Junio C Hamano wrote:

> > I don't think there's a plumbing command which works for diffing the
> > working tree directly to a git tree. In the long run, it might be a good
> > idea to remedy that.
> 
> I do not think that one is doing anything different from "git
> diff-index --name-only -z HEAD".
> [...]
> Yeah, it looks like "add -u" to me, too.  Perhaps the script was old
> enough that it didn't exist back then?  I dunno.

Hmm, nope. It _was_ "git add -u" at one point and switched. See
7aa5d43cc (stash: Don't overwrite files that have gone from the index,
2010-04-18).

I think you are right that diff-index could work, though. I always
forget that without "--cached", diff-index looks at the working tree
files.

-Peff

Re: [PATCH] stash: disable renames when calling git-diff

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think you are right that diff-index could work, though. I always
> forget that without "--cached", diff-index looks at the working tree
> files.

I'll reword the log message while queuing.  It was super surprising
to me to hear that there was something "git diff" did that three
"git diff-*" plumbing brothers couldn't do at the basic "what to
compare with what" level, as I wrote all four ;-)

Re: [PATCH] stash: disable renames when calling git-diff

[Jeff King]

On Tue, Dec 06, 2016 at 03:11:30PM -0500, Jeff King wrote:

> > Yeah, it looks like "add -u" to me, too.  Perhaps the script was old
> > enough that it didn't exist back then?  I dunno.
> 
> Hmm, nope. It _was_ "git add -u" at one point and switched. See
> 7aa5d43cc (stash: Don't overwrite files that have gone from the index,
> 2010-04-18).
> 
> I think you are right that diff-index could work, though. I always
> forget that without "--cached", diff-index looks at the working tree
> files.

Here it is in patch form. Perhaps I am missing some subtle case that
diff-index would not handle, but it does pass the test suite. And
looking over the original thread, I don't see any particular reason to
prefer git-diff.

+cc Charles just in case he remembers anything.

-- >8 --
Subject: [PATCH] stash: prefer plumbing over git-diff

When creating a stash, we need to look at the diff between
the working tree and HEAD, and do so using the git-diff
porcelain.  Because git-diff enables porcelain config like
renames by default, this causes at least one problem. The
--name-only format will not mention the source side of a
rename, meaning we will fail to stash a deletion that is
part of a rename.

We could fix that case by passing --no-renames, but this is
a symptom of a larger problem. We should be using the
diff-index plumbing here, which does not have renames
enabled by default, and also does not respect any
potentially confusing config options.

Reported-by: Matthew Patey <matthew.patey2167@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 git-stash.sh     | 2 +-
 t/t3903-stash.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index 4546abaae..10c284d1a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -115,7 +115,7 @@ create_stash () {
 			git read-tree --index-output="$TMPindex" -m $i_tree &&
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
-			git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
+			git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
 			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index e1a6ccc00..2de3e18ce 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -766,4 +766,13 @@ test_expect_success 'stash list --cc shows combined diff' '
 	test_cmp expect actual
 '
 
+test_expect_success 'stash is not confused by partial renames' '
+	mv file renamed &&
+	git add renamed &&
+	git stash &&
+	git stash apply &&
+	test_path_is_file renamed &&
+	test_path_is_missing file
+'
+
 test_done
-- 
2.11.0.191.gdb26c57

Re: [PATCH] stash: disable renames when calling git-diff

[Jeff King]

On Tue, Dec 06, 2016 at 12:22:25PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think you are right that diff-index could work, though. I always
> > forget that without "--cached", diff-index looks at the working tree
> > files.
> 
> I'll reword the log message while queuing.  It was super surprising
> to me to hear that there was something "git diff" did that three
> "git diff-*" plumbing brothers couldn't do at the basic "what to
> compare with what" level, as I wrote all four ;-)

Oops, our emails just crossed; I just sent a revised patch.

I know there are a few cases that git-diff can handle that the others
can't, but I think they are all direct blob-to-blob comparisons.

-Peff

Re: [PATCH] stash: disable renames when calling git-diff

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> I think you are right that diff-index could work, though. I always
>> forget that without "--cached", diff-index looks at the working tree
>> files.
>
> I'll reword the log message while queuing.

The last paragraph became like so:

    The correct solution is to move to an appropriate plumbing
    command. But in the short term lets ask git-diff not to respect
    renames.

Re: [PATCH] stash: disable renames when calling git-diff

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here it is in patch form. Perhaps I am missing some subtle case that
> diff-index would not handle, but it does pass the test suite. And
> looking over the original thread, I don't see any particular reason to
> prefer git-diff.

Ah, our mails crossed ;-)  I am reasonably sure that "diff HEAD" and
"diff-index HEAD" would behave identically.

> @@ -115,7 +115,7 @@ create_stash () {
>  			git read-tree --index-output="$TMPindex" -m $i_tree &&
>  			GIT_INDEX_FILE="$TMPindex" &&
>  			export GIT_INDEX_FILE &&
> -			git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
> +			git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
>  			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
>  			git write-tree &&
>  			rm -f "$TMPindex"

Will queue this one instead.  Thanks.