* Apparent bug in 'git stash push <subdir>' loses untracked files
@ 2017-12-13 17:32 Reid Price
2017-12-13 21:20 ` Igor Djordjevic
2017-12-13 23:05 ` Thomas Gummerer
0 siblings, 2 replies; 10+ messages in thread
From: Reid Price @ 2017-12-13 17:32 UTC (permalink / raw)
To: git
When running 'git stash push <subdir>' if there are both tracked and
untracked files in this subdirectory, the tracked files are stashed
but the untracked files are discarded.
I can reproduce this on my system (OSX, git 2.14.1) by running the
below script as
bash -x ./stashbug.sh &> output.txt
I could not find this indicated anywhere as an existing issue by
performing generic searches, apologies if this is known.
-Reid
Contents of stashbug.sh
------------------------
#!/bin/sh
uname -a
git --version
mkdir -p stashbug
cd stashbug
git init
mkdir dir
touch dir/tracked
git add dir/tracked
git commit -m 'initial'
tree; git status
mkdir dir/untracked_dir
touch dir/untracked_dir/casualty1
touch dir/casualty2
echo 'contents' > dir/tracked
tree; git status
git stash push dir/
git stash show -v
tree; git status
git stash pop
tree; git status
------------------------
Resulting output.txt
---------------------
+ uname -a
Darwin Reids-MacBook-Pro.local 15.6.0 Darwin Kernel Version
15.6.0: Tue Apr 11 16:00:51 PDT 2017;
root:xnu-3248.60.11.5.3~1/RELEASE_X86_64 x86_64
+ git --version
git version 2.14.1
+ mkdir -p stashbug
+ cd stashbug
+ git init
Initialized empty Git repository in /Users/reid/git/stashbug/.git/
+ mkdir dir
+ touch dir/tracked
+ git add dir/tracked
+ git commit -m initial
[master (root-commit) 895197e] initial
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 dir/tracked
+ tree
.
└── dir
└── tracked
1 directory, 1 file
+ git status
On branch master
nothing to commit, working tree clean
+ mkdir dir/untracked_dir
+ touch dir/untracked_dir/casualty1
+ touch dir/casualty2
+ echo contents
+ tree
.
└── dir
├── casualty2
├── tracked
└── untracked_dir
└── casualty1
2 directories, 3 files
+ git status
On branch master
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git checkout -- <file>..." to discard changes in working directory)
modified: dir/tracked
Untracked files:
(use "git add <file>..." to include in what will be committed)
dir/casualty2
dir/untracked_dir/
no changes added to commit (use "git add" and/or "git commit -a")
+ git stash push dir/
Saved working directory and index state WIP on master: 895197e initial
+ git stash show -v
diff --git a/dir/tracked b/dir/tracked
index e69de29..12f00e9 100644
--- a/dir/tracked
+++ b/dir/tracked
@@ -0,0 +1 @@
+contents
+ tree
.
└── dir
└── tracked
1 directory, 1 file
+ git status
On branch master
nothing to commit, working tree clean
+ git stash pop
On branch master
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git checkout -- <file>..." to discard changes in working directory)
modified: dir/tracked
no changes added to commit (use "git add" and/or "git commit -a")
Dropped refs/stash@{0} (93ceee344b947ecd8a27a672e3aedd2b2e1acc99)
+ tree
.
└── dir
└── tracked
1 directory, 1 file
+ git status
On branch master
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git checkout -- <file>..." to discard changes in working directory)
modified: dir/tracked
no changes added to commit (use "git add" and/or "git commit -a")
---------------------
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Apparent bug in 'git stash push <subdir>' loses untracked files
2017-12-13 17:32 Apparent bug in 'git stash push <subdir>' loses untracked files Reid Price
@ 2017-12-13 21:20 ` Igor Djordjevic
2017-12-13 23:14 ` Thomas Gummerer
2017-12-13 23:05 ` Thomas Gummerer
1 sibling, 1 reply; 10+ messages in thread
From: Igor Djordjevic @ 2017-12-13 21:20 UTC (permalink / raw)
To: Reid Price; +Cc: git
Hi Reid,
On 13/12/2017 18:32, Reid Price wrote:
>
> When running 'git stash push <subdir>' if there are both tracked and
> untracked files in this subdirectory, the tracked files are stashed
> but the untracked files are discarded.
I can reproduce this as well (git version 2.15.1.windows.2).
For what it`s worth, using `git stash save <subdir>` instead seems to
(still) work as expected... but on the other hand, `git-stash`[1]
manpage seems not to mention this usage ("save" with "pathspec")?
Regards, Buga
[1] https://git-scm.com/docs/git-stash
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Apparent bug in 'git stash push <subdir>' loses untracked files
2017-12-13 17:32 Apparent bug in 'git stash push <subdir>' loses untracked files Reid Price
2017-12-13 21:20 ` Igor Djordjevic
@ 2017-12-13 23:05 ` Thomas Gummerer
2017-12-16 18:33 ` Junio C Hamano
1 sibling, 1 reply; 10+ messages in thread
From: Thomas Gummerer @ 2017-12-13 23:05 UTC (permalink / raw)
To: Reid Price; +Cc: git
Hi,
On 12/13, Reid Price wrote:
> When running 'git stash push <subdir>' if there are both tracked and
> untracked files in this subdirectory, the tracked files are stashed
> but the untracked files are discarded.
>
> I can reproduce this on my system (OSX, git 2.14.1) by running the
> below script as
>
> bash -x ./stashbug.sh &> output.txt
>
> I could not find this indicated anywhere as an existing issue by
> performing generic searches, apologies if this is known.
Thanks for the detailed bug report below. This is indeed a bug, and
indeed has been broken ever since I introduced the pathspec feature.
Sorry about that.
While I did implement this feature in the first place, I must admit
that I'm a bit out of my depth in terms of my shell foo to fix this
cleanly here.
Just to demonstrate what is wrong, the below diff fixes it (but it
does so in an ugly way that needs a temporary file, and needs a
specific version of the 'read' utility, that supports the '-d' flag,
which isn't in POSIX).
For the pathspec case what we're doing after creating the stash is to
essentially emulate what 'git reset --hard <pathspec>' would do. As
'git reset --hard <pathspec>' doesn't exist, we do so using the
sequence below of reset -> checkout-index -> clean for the given
pathspec.
This works when the pathspec doesn't match any untracked files, but if
it matches untracked files such as in your case it removes them as
well as the files it should have removed.
One solution to that would be to limit the pathspec given to 'git
clean'. And that is where my shell foo doesn't go far enough for me
being able to do that, as lists of filenames, which can include spaces
tend to get quite hairy.
Maybe the best solution would be to introduce 'git reset --hard --
<pathspec>', or maybe someone who knows shell programming a little
better than me has an idea?
diff --git a/git-stash.sh b/git-stash.sh
index 1114005ce2..01bf74015e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -322,10 +322,15 @@ push_stash () {
if test $# != 0
then
+ git ls-files -z >"$(git rev-parse --git-dir)"/stash-to-remove
git reset -q -- "$@"
git ls-files -z --modified -- "$@" |
git checkout-index -z --force --stdin
- git clean --force -q -d -- "$@"
+ while read -r -d '' to_delete
+ do
+ git clean --force -q -d -- "$to_delete"
+ done <"$(git rev-parse --git-dir)"/stash-to-remove
+ rm "$(git rev-parse --git-dir)"/stash-to-remove
else
git reset --hard -q
fi
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 39c7f2ebd7..6952a031b2 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1064,4 +1064,20 @@ test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '
test foo,bar = $(cat foo),$(cat bar)
'
+test_expect_success 'stash -- <subdir> leaves untracked files in subdir intact' '
+ git reset &&
+ >subdir/untracked &&
+ >subdir/tracked1 &&
+ >subdir/tracked2 &&
+ git add subdir/tracked* &&
+ git stash -- subdir/ &&
+ test_path_is_missing subdir/tracked1 &&
+ test_path_is_missing subdir/tracked2 &&
+ test_path_is_file subdir/untracked &&
+ git stash pop &&
+ test_path_is_file subdir/tracked1 &&
+ test_path_is_file subdir/tracked2 &&
+ test_path_is_file subdir/untracked
+'
+
test_done
> -Reid
>
> Contents of stashbug.sh
> ------------------------
> #!/bin/sh
>
> uname -a
> git --version
> mkdir -p stashbug
> cd stashbug
> git init
> mkdir dir
> touch dir/tracked
> git add dir/tracked
> git commit -m 'initial'
> tree; git status
> mkdir dir/untracked_dir
> touch dir/untracked_dir/casualty1
> touch dir/casualty2
> echo 'contents' > dir/tracked
> tree; git status
> git stash push dir/
> git stash show -v
> tree; git status
> git stash pop
> tree; git status
> ------------------------
>
> Resulting output.txt
> ---------------------
> + uname -a
> Darwin Reids-MacBook-Pro.local 15.6.0 Darwin Kernel Version
> 15.6.0: Tue Apr 11 16:00:51 PDT 2017;
> root:xnu-3248.60.11.5.3~1/RELEASE_X86_64 x86_64
> + git --version
> git version 2.14.1
> + mkdir -p stashbug
> + cd stashbug
> + git init
> Initialized empty Git repository in /Users/reid/git/stashbug/.git/
> + mkdir dir
> + touch dir/tracked
> + git add dir/tracked
> + git commit -m initial
> [master (root-commit) 895197e] initial
> 1 file changed, 0 insertions(+), 0 deletions(-)
> create mode 100644 dir/tracked
> + tree
> .
> └── dir
> └── tracked
>
> 1 directory, 1 file
> + git status
> On branch master
> nothing to commit, working tree clean
> + mkdir dir/untracked_dir
> + touch dir/untracked_dir/casualty1
> + touch dir/casualty2
> + echo contents
> + tree
> .
> └── dir
> ├── casualty2
> ├── tracked
> └── untracked_dir
> └── casualty1
>
> 2 directories, 3 files
> + git status
> On branch master
> Changes not staged for commit:
> (use "git add <file>..." to update what will be committed)
> (use "git checkout -- <file>..." to discard changes in working directory)
>
> modified: dir/tracked
>
> Untracked files:
> (use "git add <file>..." to include in what will be committed)
>
> dir/casualty2
> dir/untracked_dir/
>
> no changes added to commit (use "git add" and/or "git commit -a")
> + git stash push dir/
> Saved working directory and index state WIP on master: 895197e initial
> + git stash show -v
> diff --git a/dir/tracked b/dir/tracked
> index e69de29..12f00e9 100644
> --- a/dir/tracked
> +++ b/dir/tracked
> @@ -0,0 +1 @@
> +contents
> + tree
> .
> └── dir
> └── tracked
>
> 1 directory, 1 file
> + git status
> On branch master
> nothing to commit, working tree clean
> + git stash pop
> On branch master
> Changes not staged for commit:
> (use "git add <file>..." to update what will be committed)
> (use "git checkout -- <file>..." to discard changes in working directory)
>
> modified: dir/tracked
>
> no changes added to commit (use "git add" and/or "git commit -a")
> Dropped refs/stash@{0} (93ceee344b947ecd8a27a672e3aedd2b2e1acc99)
> + tree
> .
> └── dir
> └── tracked
>
> 1 directory, 1 file
> + git status
> On branch master
> Changes not staged for commit:
> (use "git add <file>..." to update what will be committed)
> (use "git checkout -- <file>..." to discard changes in working directory)
>
> modified: dir/tracked
>
> no changes added to commit (use "git add" and/or "git commit -a")
> ---------------------
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Apparent bug in 'git stash push <subdir>' loses untracked files
2017-12-13 21:20 ` Igor Djordjevic
@ 2017-12-13 23:14 ` Thomas Gummerer
2017-12-13 23:46 ` Igor Djordjevic
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Gummerer @ 2017-12-13 23:14 UTC (permalink / raw)
To: Igor Djordjevic; +Cc: Reid Price, git
On 12/13, Igor Djordjevic wrote:
> Hi Reid,
>
> On 13/12/2017 18:32, Reid Price wrote:
> >
> > When running 'git stash push <subdir>' if there are both tracked and
> > untracked files in this subdirectory, the tracked files are stashed
> > but the untracked files are discarded.
>
> I can reproduce this as well (git version 2.15.1.windows.2).
>
> For what it`s worth, using `git stash save <subdir>` instead seems to
> (still) work as expected...
I think that depends on what you expect ;) 'git stash save <subdir>'
will create a stash of the whole working directory with the message
"<subdir>". So while it would indeed work for the presumably
simplified example Reid provided, it would not do what you'd expect if
there are any tracked and modified files outside of the <subdir>.
In that case 'git stash save <subdir>' would include the tracked files
outside of <subdir>, while what I assume Reid wanted is to keep them
in place, and only stash the files in <subdir>.
> but on the other hand, `git-stash`[1]
> manpage seems not to mention this usage ("save" with "pathspec")?
"stash save" with "pathspec" doesn't exist, and it will probably never
exist. We decided to introduce a new "push" verb for 'git stash'
because the command line for 'git stash save' takes a message as its
last argument, instead of taking the message with a -m flag like other
commands do. Introducing a pathspec argument for "git stash save"
would have either broken backward compatibility, or it would have had
some syntax that's very inconsistent with other git commands.
> Regards, Buga
>
> [1] https://git-scm.com/docs/git-stash
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Apparent bug in 'git stash push <subdir>' loses untracked files
2017-12-13 23:14 ` Thomas Gummerer
@ 2017-12-13 23:46 ` Igor Djordjevic
0 siblings, 0 replies; 10+ messages in thread
From: Igor Djordjevic @ 2017-12-13 23:46 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: Reid Price, git
Hi Thomas,
On 14/12/2017 00:14, Thomas Gummerer wrote:
>
> > For what it`s worth, using `git stash save <subdir>` instead seems
> > to (still) work as expected...
>
> I think that depends on what you expect ;) 'git stash save <subdir>'
> will create a stash of the whole working directory with the message
> "<subdir>". So while it would indeed work for the presumably
> simplified example Reid provided, it would not do what you'd expect
> if there are any tracked and modified files outside of the <subdir>.
>
> In that case 'git stash save <subdir>' would include the tracked
> files outside of <subdir>, while what I assume Reid wanted is to keep
> them in place, and only stash the files in <subdir>.
Indeed, I didn`t pay enough attention to the fact that even `git
stash save\push` produced different output messages, the difference
being exactly automatic (push) versus provided (save) stash message.
And I did use `git stash save <message>` in the past... :$ Not too
often, I guess.
> > but on the other hand, `git-stash`[1] manpage seems not to mention
> > this usage ("save" with "pathspec")?
>
> "stash save" with "pathspec" doesn't exist, and it will probably
> never exist. We decided to introduce a new "push" verb for 'git
> stash' because the command line for 'git stash save' takes a message
> as its last argument, instead of taking the message with a -m flag
> like other commands do. Introducing a pathspec argument for "git
> stash save" would have either broken backward compatibility, or it
> would have had some syntax that's very inconsistent with other git
> commands.
Yeah, I`m aware of the "transition", thus teaching myself to use `git
stash push` lately. That`s also what made me curious to try out the
"old" `git stash save` behavior, but obviously in a bit hasty manner.
Sorry for the confusion, and thanks, for both clarification
and your work.
Regards, Buga
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Apparent bug in 'git stash push <subdir>' loses untracked files
2017-12-13 23:05 ` Thomas Gummerer
@ 2017-12-16 18:33 ` Junio C Hamano
2017-12-17 18:05 ` Thomas Gummerer
0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2017-12-16 18:33 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: Reid Price, git
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Maybe the best solution would be to introduce 'git reset --hard --
> <pathspec>', or maybe someone who knows shell programming a little
> better than me has an idea?
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 1114005ce2..01bf74015e 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -322,10 +322,15 @@ push_stash () {
>
> if test $# != 0
> then
> + git ls-files -z >"$(git rev-parse --git-dir)"/stash-to-remove
> git reset -q -- "$@"
> git ls-files -z --modified -- "$@" |
> git checkout-index -z --force --stdin
> - git clean --force -q -d -- "$@"
> + while read -r -d '' to_delete
> + do
> + git clean --force -q -d -- "$to_delete"
> + done <"$(git rev-parse --git-dir)"/stash-to-remove
> + rm "$(git rev-parse --git-dir)"/stash-to-remove
> else
> git reset --hard -q
> fi
Hmph. I personally did not care (nor use) pathspec limited stash
creation, so I admit that I do not offhand know what this code
(before or after the above fix) is trying to do, even though it has
been in our tree for some time X-<. But judging from the fact that
the other side is a mere "git reset --hard" (for the whole tree), I
guess for each and every path in the index and/or in the HEAD that
match pathspec "$@", you are trying to
- remove it from the index if it is not in HEAD;
- add it to the index if it is not in the index but is in HEAD; or
- reset the index to the version in HEAD if it is in the index.
and then match the working tree version to that of the index?
I am not convinced that "git reset --hard -- <pathspec>" is a good
UI [*1*] but I agree that a low level facility that does these would
be quite helpful to implement the then-clause of the above.
But I suspect that you do not have to do shell "logic" nor low-level
plumbing update in this case. Would the attached work?
[Footnote]
*1* Primarily because "git reset [<tree-ish>] -- <pathspec>" that
already exists is not a good UI. The remainder of "git reset"
is about repointing the HEAD, and the various operation modes
are there to adjust the index and the working tree to the
updated HEAD depending on why the user wants to repoint the HEAD
in the first place. On the other hand, the pathspec limited
"reset" is all about affecting the index without moving the HEAD
and operation modes, if --hard is also taught not to reject
pathspec, will become "do we do index-only or do we do both?"
But updating the index and the working tree is what "git
checkout [<tree-ish>] -- <pathspec>" is for, and the "git reset
[<tree-ish>] -- <pathspec>" should have been "checkout --cached
[<tree-ish>] -- <pathspec>" (and the "reset --hard" equivalent
should be the "default" for checkout).
There is a small glitch in the current behaviour of "checkout --
<pathspec>" to complicate the above vision, but assuming that it
is surmountable, and if we were going to update the UI for
consistency along the above line, then I do not mind making "git
checkout" solely for "checking out paths" and adding a different
subcommannd to "checking out a branch to work on".
"reset.<commit>" is about resetting the HEAD and "reset
[<tree-ish>] -- <pathspec>" is about resetting the index, which
is the same duality some people complain about "git checkout"
that is both about checking out the paths and checking out a
branch to work on. Adding more mode(s) to "git reset" that
makes the command not about repointing the HEAD is piling more
on top.
The "small glitch" is that "checkout <tree-ish> -- <pathspec>"
is defined as an overlay operation---take the current index (and
the working tree), then for each (path, contents) in <tree-ish>
that match the <pathspec>, add it in if path does not exist in
the current index, or replace if path does. If we have a path
in the index that match <pathspec>, and that path does not exist
in <tree-ish>, "checkout <tree-ish> -- <pathspec>" does not
remove that path from the index.
So even the index+working tree mode (which is how "reset --hard"
would want to work) may need to take a new option. Between two
modes of "checkout <tree-ish> -- <pathspec>" that allows
deletion, "--index" would be the one that works on both the
index and the working tree, and "--cached" would be the one that
works only on the index (this follows gitcli convention that is
used by existing commands), and the current "overlay" mode will
stay to be the default, at least until we know that the "--index"
mode should become the default and decide to transition.
---
git-stash.sh | 5 ++---
t/t3903-stash.sh | 16 ++++++++++++++++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 1114005ce2..a979bfb665 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -322,10 +322,9 @@ push_stash () {
if test $# != 0
then
- git reset -q -- "$@"
- git ls-files -z --modified -- "$@" |
+ git ls-files -z -- "$@" |
git checkout-index -z --force --stdin
- git clean --force -q -d -- "$@"
+ git diff-index -p HEAD -- "$@" | git apply --index -R
else
git reset --hard -q
fi
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 39c7f2ebd7..6952a031b2 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1064,4 +1064,20 @@ test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '
test foo,bar = $(cat foo),$(cat bar)
'
+test_expect_success 'stash -- <subdir> leaves untracked files in subdir intact' '
+ git reset &&
+ >subdir/untracked &&
+ >subdir/tracked1 &&
+ >subdir/tracked2 &&
+ git add subdir/tracked* &&
+ git stash -- subdir/ &&
+ test_path_is_missing subdir/tracked1 &&
+ test_path_is_missing subdir/tracked2 &&
+ test_path_is_file subdir/untracked &&
+ git stash pop &&
+ test_path_is_file subdir/tracked1 &&
+ test_path_is_file subdir/tracked2 &&
+ test_path_is_file subdir/untracked
+'
+
test_done
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Apparent bug in 'git stash push <subdir>' loses untracked files
2017-12-16 18:33 ` Junio C Hamano
@ 2017-12-17 18:05 ` Thomas Gummerer
2017-12-18 18:24 ` Junio C Hamano
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Gummerer @ 2017-12-17 18:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Reid Price, git
On 12/16, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > Maybe the best solution would be to introduce 'git reset --hard --
> > <pathspec>', or maybe someone who knows shell programming a little
> > better than me has an idea?
> >
> > diff --git a/git-stash.sh b/git-stash.sh
> > index 1114005ce2..01bf74015e 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -322,10 +322,15 @@ push_stash () {
> >
> > if test $# != 0
> > then
> > + git ls-files -z >"$(git rev-parse --git-dir)"/stash-to-remove
> > git reset -q -- "$@"
> > git ls-files -z --modified -- "$@" |
> > git checkout-index -z --force --stdin
> > - git clean --force -q -d -- "$@"
> > + while read -r -d '' to_delete
> > + do
> > + git clean --force -q -d -- "$to_delete"
> > + done <"$(git rev-parse --git-dir)"/stash-to-remove
> > + rm "$(git rev-parse --git-dir)"/stash-to-remove
> > else
> > git reset --hard -q
> > fi
>
> Hmph. I personally did not care (nor use) pathspec limited stash
> creation, so I admit that I do not offhand know what this code
> (before or after the above fix) is trying to do, even though it has
> been in our tree for some time X-<.
Yeah unfortunately this has been broken for pathspecs that match
untracked files since I introduced it :(
> But judging from the fact that
> the other side is a mere "git reset --hard" (for the whole tree), I
> guess for each and every path in the index and/or in the HEAD that
> match pathspec "$@", you are trying to
>
> - remove it from the index if it is not in HEAD;
>
> - add it to the index if it is not in the index but is in HEAD; or
>
> - reset the index to the version in HEAD if it is in the index.
>
> and then match the working tree version to that of the index?
That's correct. That's how I would have imagined 'git reset --hard --
<pathspec>'.
> I am not convinced that "git reset --hard -- <pathspec>" is a good
> UI [*1*] but I agree that a low level facility that does these would
> be quite helpful to implement the then-clause of the above.
Thanks a lot for spelling this out below. After reading that I
definitely agree that 'git reset --hard -- <pathspec>' wouldn't be a
good UI, and something like that would belong in checkout.
> But I suspect that you do not have to do shell "logic" nor low-level
> plumbing update in this case. Would the attached work?
Ah interesting, what you have below looks good to me indeed, it
matches what I'd expect it to do and fixes the bug that was reported.
Thanks!
I've taken the liberty to take what you have below and turned into a
proper patch, giving you authorship of it, as you wrote the code for
the fix. Hope that's the appropriate credit for you for this patch.
[...]
--- >8 ---
From: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH] stash: don't delete untracked files that match pathspec
Currently when 'git stash push -- <pathspec>' is used, untracked files
that match the pathspec will be deleted, even though they do not end up
in a stash anywhere.
Untracked files should be left along by 'git stash push -- <pathspec>'
unless the --untracked or --all flags are given. Fix that.
Reported-by: Reid Price <reid.price@gmail.com>
Test-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
git-stash.sh | 5 ++---
t/t3903-stash.sh | 16 ++++++++++++++++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 1114005ce2..a979bfb665 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -322,10 +322,9 @@ push_stash () {
if test $# != 0
then
- git reset -q -- "$@"
- git ls-files -z --modified -- "$@" |
+ git ls-files -z -- "$@" |
git checkout-index -z --force --stdin
- git clean --force -q -d -- "$@"
+ git diff-index -p HEAD -- "$@" | git apply --index -R
else
git reset --hard -q
fi
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 39c7f2ebd7..6952a031b2 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1064,4 +1064,20 @@ test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '
test foo,bar = $(cat foo),$(cat bar)
'
+test_expect_success 'stash -- <subdir> leaves untracked files in subdir intact' '
+ git reset &&
+ >subdir/untracked &&
+ >subdir/tracked1 &&
+ >subdir/tracked2 &&
+ git add subdir/tracked* &&
+ git stash -- subdir/ &&
+ test_path_is_missing subdir/tracked1 &&
+ test_path_is_missing subdir/tracked2 &&
+ test_path_is_file subdir/untracked &&
+ git stash pop &&
+ test_path_is_file subdir/tracked1 &&
+ test_path_is_file subdir/tracked2 &&
+ test_path_is_file subdir/untracked
+'
+
test_done
--
2.15.1.390.ga48b24a585.dirty
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Apparent bug in 'git stash push <subdir>' loses untracked files
2017-12-17 18:05 ` Thomas Gummerer
@ 2017-12-18 18:24 ` Junio C Hamano
2018-01-05 20:03 ` Thomas Gummerer
0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2017-12-18 18:24 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: Reid Price, git
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Ah interesting, what you have below looks good to me indeed, it
> matches what I'd expect it to do and fixes the bug that was reported.
> Thanks!
>
> I've taken the liberty to take what you have below and turned into a
> proper patch, giving you authorship of it, as you wrote the code for
> the fix. Hope that's the appropriate credit for you for this patch.
Not so fast.
I know why the updated code works like "--hard -- <pathspec>", but I
do not quite get what the original was trying to do and how it is
different. Even with your proposed log message, which describes a
symptom (i.e. "untracked files ... be deleted"), it is unclear why
this deletion was happening in the first place. Specifically, it is
unclear why that "clean --force -q -d" was in there, and are we
breaking other cases by rewriting this codepath without it?
In any case, instead of the
ls-files -z -- "$@" | checkout-index -z --force --stdin
diff-index -p HEAD -- "$@" | apply --index -R
sequence, a shorter variant that should work is
add -u -- "$@"
diff-index -p --cached HEAD -- "$@" | apply --index -R
Both of these share the same idea.
- The first step is to match what is in the index and what is in
the working tree (i.e. make "diff-files" silent). The version
you have does so by checking out what is in the index to the
working tree. The shorter one goes the other way and updates the
index with what is in the working tree.
- Once that is done, we ask diff-index what got changed since the
HEAD in the working tree (or in the index in the updated
one---after the first step that makes the two match, comparing
with the working tree and comparing with the index should result
in the same diff; I have a suspicion that "--cached" is faster,
but we need to benchmark to pick), and ask apply to get rid of
all these changes, which includes removal of added files, and
resurrection of removed files.
I think the original that did 'git reset -- "$@"' upfront lost new
paths from the index (without removing it from the working tree), I
am guessing that it is why "clean" was there to get rid of them, and
if that is the reason, I can understand why the original code was
behaving incorrectly---it would get rid of new files that did not
exist in HEAD correctly, but it also would remove untracked ones,
because after that first 'reset', the code couldn't tell between
them.
And I think that is what we want, i.e. why the original was wrong
and how the new one fixes it, to describe in the log message to
justify this change.
One thing that I didn't think through and you need to verify is if
we need to do anything extra to deal with binary files (in the old
days, we needed --full-index and --binary options to produce and
apply a binary patch; I do not offhand know if that is still the
case) in the final "diff-index piped to apply -R --index" dance.
So I am asking you to fill in quite a lot of gaps that I didn't do
with only the above two-liner ;-) You should take the authorship
and, if you like, credit me with helped-by: or something.
Thanks.
>
> [...]
>
> --- >8 ---
> From: Junio C Hamano <gitster@pobox.com>
> Subject: [PATCH] stash: don't delete untracked files that match pathspec
>
> Currently when 'git stash push -- <pathspec>' is used, untracked files
> that match the pathspec will be deleted, even though they do not end up
> in a stash anywhere.
>
> Untracked files should be left along by 'git stash push -- <pathspec>'
> unless the --untracked or --all flags are given. Fix that.
>
> Reported-by: Reid Price <reid.price@gmail.com>
> Test-by: Thomas Gummerer <t.gummerer@gmail.com>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> git-stash.sh | 5 ++---
> t/t3903-stash.sh | 16 ++++++++++++++++
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 1114005ce2..a979bfb665 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -322,10 +322,9 @@ push_stash () {
>
> if test $# != 0
> then
> - git reset -q -- "$@"
> - git ls-files -z --modified -- "$@" |
> + git ls-files -z -- "$@" |
> git checkout-index -z --force --stdin
> - git clean --force -q -d -- "$@"
> + git diff-index -p HEAD -- "$@" | git apply --index -R
> else
> git reset --hard -q
> fi
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 39c7f2ebd7..6952a031b2 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1064,4 +1064,20 @@ test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '
> test foo,bar = $(cat foo),$(cat bar)
> '
>
> +test_expect_success 'stash -- <subdir> leaves untracked files in subdir intact' '
> + git reset &&
> + >subdir/untracked &&
> + >subdir/tracked1 &&
> + >subdir/tracked2 &&
> + git add subdir/tracked* &&
> + git stash -- subdir/ &&
> + test_path_is_missing subdir/tracked1 &&
> + test_path_is_missing subdir/tracked2 &&
> + test_path_is_file subdir/untracked &&
> + git stash pop &&
> + test_path_is_file subdir/tracked1 &&
> + test_path_is_file subdir/tracked2 &&
> + test_path_is_file subdir/untracked
> +'
> +
> test_done
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Apparent bug in 'git stash push <subdir>' loses untracked files
2017-12-18 18:24 ` Junio C Hamano
@ 2018-01-05 20:03 ` Thomas Gummerer
2018-01-06 0:24 ` [PATCH v2] stash: don't delete untracked files that match pathspec Thomas Gummerer
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Gummerer @ 2018-01-05 20:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Reid Price, git
On 12/18, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > Ah interesting, what you have below looks good to me indeed, it
> > matches what I'd expect it to do and fixes the bug that was reported.
> > Thanks!
> >
> > I've taken the liberty to take what you have below and turned into a
> > proper patch, giving you authorship of it, as you wrote the code for
> > the fix. Hope that's the appropriate credit for you for this patch.
>
> Not so fast.
>
> I know why the updated code works like "--hard -- <pathspec>", but I
> do not quite get what the original was trying to do and how it is
> different. Even with your proposed log message, which describes a
> symptom (i.e. "untracked files ... be deleted"), it is unclear why
> this deletion was happening in the first place. Specifically, it is
> unclear why that "clean --force -q -d" was in there, and are we
> breaking other cases by rewriting this codepath without it?
As you hint at below, the intention was to get rid of the files that
were unstaged by 'git reset', but didn't realize that it would also
delete files that were already untracked before the 'git reset' and
thus not in the stash.
It behaves fine if the pathspec matches a filename exactly, which is
why it worked in all my tests, but is obviously broken in the case
where the pathspec matches untracked files that are around.
I completely failed to realize this, and we wouldn't break any
intended usecases with the change. I'll update the commit message to
include this information.
> In any case, instead of the
>
> ls-files -z -- "$@" | checkout-index -z --force --stdin
> diff-index -p HEAD -- "$@" | apply --index -R
>
> sequence, a shorter variant that should work is
>
> add -u -- "$@"
> diff-index -p --cached HEAD -- "$@" | apply --index -R
>
> Both of these share the same idea.
>
> - The first step is to match what is in the index and what is in
> the working tree (i.e. make "diff-files" silent). The version
> you have does so by checking out what is in the index to the
> working tree. The shorter one goes the other way and updates the
> index with what is in the working tree.
>
> - Once that is done, we ask diff-index what got changed since the
> HEAD in the working tree (or in the index in the updated
> one---after the first step that makes the two match, comparing
> with the working tree and comparing with the index should result
> in the same diff; I have a suspicion that "--cached" is faster,
> but we need to benchmark to pick), and ask apply to get rid of
> all these changes, which includes removal of added files, and
> resurrection of removed files.
Thanks for the explanation.
> I think the original that did 'git reset -- "$@"' upfront lost new
> paths from the index (without removing it from the working tree), I
> am guessing that it is why "clean" was there to get rid of them, and
> if that is the reason, I can understand why the original code was
> behaving incorrectly---it would get rid of new files that did not
> exist in HEAD correctly, but it also would remove untracked ones,
> because after that first 'reset', the code couldn't tell between
> them.
>
> And I think that is what we want, i.e. why the original was wrong
> and how the new one fixes it, to describe in the log message to
> justify this change.
>
> One thing that I didn't think through and you need to verify is if
> we need to do anything extra to deal with binary files (in the old
> days, we needed --full-index and --binary options to produce and
> apply a binary patch; I do not offhand know if that is still the
> case) in the final "diff-index piped to apply -R --index" dance.
>
> So I am asking you to fill in quite a lot of gaps that I didn't do
> with only the above two-liner ;-) You should take the authorship
> and, if you like, credit me with helped-by: or something.
Thanks, I'll fill in the gaps, and send a new patch, hopefully over
the weekend.
> Thanks.
>
>
> >
> > [...]
> >
> > --- >8 ---
> > From: Junio C Hamano <gitster@pobox.com>
> > Subject: [PATCH] stash: don't delete untracked files that match pathspec
> >
> > Currently when 'git stash push -- <pathspec>' is used, untracked files
> > that match the pathspec will be deleted, even though they do not end up
> > in a stash anywhere.
> >
> > Untracked files should be left along by 'git stash push -- <pathspec>'
> > unless the --untracked or --all flags are given. Fix that.
> >
> > Reported-by: Reid Price <reid.price@gmail.com>
> > Test-by: Thomas Gummerer <t.gummerer@gmail.com>
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > git-stash.sh | 5 ++---
> > t/t3903-stash.sh | 16 ++++++++++++++++
> > 2 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/git-stash.sh b/git-stash.sh
> > index 1114005ce2..a979bfb665 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -322,10 +322,9 @@ push_stash () {
> >
> > if test $# != 0
> > then
> > - git reset -q -- "$@"
> > - git ls-files -z --modified -- "$@" |
> > + git ls-files -z -- "$@" |
> > git checkout-index -z --force --stdin
> > - git clean --force -q -d -- "$@"
> > + git diff-index -p HEAD -- "$@" | git apply --index -R
> > else
> > git reset --hard -q
> > fi
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index 39c7f2ebd7..6952a031b2 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -1064,4 +1064,20 @@ test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '
> > test foo,bar = $(cat foo),$(cat bar)
> > '
> >
> > +test_expect_success 'stash -- <subdir> leaves untracked files in subdir intact' '
> > + git reset &&
> > + >subdir/untracked &&
> > + >subdir/tracked1 &&
> > + >subdir/tracked2 &&
> > + git add subdir/tracked* &&
> > + git stash -- subdir/ &&
> > + test_path_is_missing subdir/tracked1 &&
> > + test_path_is_missing subdir/tracked2 &&
> > + test_path_is_file subdir/untracked &&
> > + git stash pop &&
> > + test_path_is_file subdir/tracked1 &&
> > + test_path_is_file subdir/tracked2 &&
> > + test_path_is_file subdir/untracked
> > +'
> > +
> > test_done
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] stash: don't delete untracked files that match pathspec
2018-01-05 20:03 ` Thomas Gummerer
@ 2018-01-06 0:24 ` Thomas Gummerer
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Gummerer @ 2018-01-06 0:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Reid Price, Thomas Gummerer
Currently when 'git stash push -- <pathspec>' is used, untracked files
that match the pathspec will be deleted, even though they do not end up
in a stash anywhere.
This is because the original commit introducing the pathspec feature in
git stash push (df6bba0937 ("stash: teach 'push' (and 'create_stash') to
honor pathspec", 2017-02-28)) used the sequence of 'git reset <pathspec>
&& git ls-files --modified <pathspec> | git checkout-index && git clean
<pathspec>'.
The intention was to emulate what 'git reset --hard -- <pathspec>' would
do. The call to 'git clean' was supposed to clean up the files that
were unstaged by 'git reset'. This would work fine if the pathspec
doesn't match any files that were untracked before 'git stash push --
<pathspec>'. However if <pathspec> matches a file that was untracked
before invoking the 'stash' command, all untracked files matching the
pathspec would inadvertently be deleted as well, even though they
wouldn't end up in the stash, and are therefore lost.
This behaviour was never what was intended, only blobs that also end up
in the stash should be reset to their state in HEAD, previously
untracked files should be left alone.
To achieve this, first match what's in the index and what's in the
working tree by adding all changes to the index, ask diff-index what
changed between HEAD and the current index, and then apply that patch in
reverse to get rid of the changes, which includes removal of added
files and resurrection of removed files.
Reported-by: Reid Price <reid.price@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
> Thanks, I'll fill in the gaps, and send a new patch, hopefully over
> the weekend.
Here it is :)
Changes since the previous version:
- handle binary files correctly and add a test for that
- updated commit message
- took back authorship, and added Helped-by: Junio as he mentioned
would be the best way earlier in the thread.
git-stash.sh | 5 ++---
t/t3903-stash.sh | 32 ++++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 1114005ce2..fc8f8ae640 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -322,10 +322,9 @@ push_stash () {
if test $# != 0
then
- git reset -q -- "$@"
- git ls-files -z --modified -- "$@" |
+ git add -u -- "$@" |
git checkout-index -z --force --stdin
- git clean --force -q -d -- "$@"
+ git diff-index -p --cached --binary HEAD -- "$@" | git apply --index -R
else
git reset --hard -q
fi
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 39c7f2ebd7..aefde7b172 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1064,4 +1064,36 @@ test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '
test foo,bar = $(cat foo),$(cat bar)
'
+test_expect_success 'stash -- <subdir> leaves untracked files in subdir intact' '
+ git reset &&
+ >subdir/untracked &&
+ >subdir/tracked1 &&
+ >subdir/tracked2 &&
+ git add subdir/tracked* &&
+ git stash -- subdir/ &&
+ test_path_is_missing subdir/tracked1 &&
+ test_path_is_missing subdir/tracked2 &&
+ test_path_is_file subdir/untracked &&
+ git stash pop &&
+ test_path_is_file subdir/tracked1 &&
+ test_path_is_file subdir/tracked2 &&
+ test_path_is_file subdir/untracked
+'
+
+test_expect_success 'stash -- <subdir> works with binary files' '
+ git reset &&
+ >subdir/untracked &&
+ >subdir/tracked &&
+ cp "$TEST_DIRECTORY"/test-binary-1.png subdir/tracked-binary &&
+ git add subdir/tracked* &&
+ git stash -- subdir/ &&
+ test_path_is_missing subdir/tracked &&
+ test_path_is_missing subdir/tracked-binary &&
+ test_path_is_file subdir/untracked &&
+ git stash pop &&
+ test_path_is_file subdir/tracked &&
+ test_path_is_file subdir/tracked-binary &&
+ test_path_is_file subdir/untracked
+'
+
test_done
--
2.15.1.620.gb9897f4670
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-01-06 0:22 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-13 17:32 Apparent bug in 'git stash push <subdir>' loses untracked files Reid Price
2017-12-13 21:20 ` Igor Djordjevic
2017-12-13 23:14 ` Thomas Gummerer
2017-12-13 23:46 ` Igor Djordjevic
2017-12-13 23:05 ` Thomas Gummerer
2017-12-16 18:33 ` Junio C Hamano
2017-12-17 18:05 ` Thomas Gummerer
2017-12-18 18:24 ` Junio C Hamano
2018-01-05 20:03 ` Thomas Gummerer
2018-01-06 0:24 ` [PATCH v2] stash: don't delete untracked files that match pathspec Thomas Gummerer
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).