From: Elijah Newren via GitGitGadget <hidden> Date: 2021-09-08 01:43:54
This series fixes a few D/F issues in the stash command. These were some
issues I found while working on unintentional removal of untracked
files/directories and the current working directory, and I'm just submitting
them separately.
Elijah Newren (3):
t3903: document a pair of directory/file bugs
stash: avoid feeding directories to update-index
stash: restore untracked files AFTER restoring tracked files
builtin/stash.c | 15 ++++++++++++---
t/t3903-stash.sh | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+), 3 deletions(-)
base-commit: e0a2f5cbc585657e757385ad918f167f519cfb96
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1086%2Fnewren%2Fstash-df-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1086/newren/stash-df-fixes-v1
Pull-Request: https://github.com/git/git/pull/1086
--
gitgitgadget
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-09-08 01:43:55
From: Elijah Newren <redacted>
When a file is removed from the cache, but there is a file of the same
name present in the working directory, we would normally treat that file
in the working directory as untracked. However, in the case of stash,
doing that would prevent a simple 'git stash push', because the untracked
file would be in the way of restoring the deleted file.
git stash, however, blindly assumes that whatever is in the working
directory for a deleted file is wanted and passes that path along to
update-index. That causes problems when the working directory contains
a directory with the same name as the deleted file. Add some code for
this special case that will avoid passing directory names to
update-index.
Signed-off-by: Elijah Newren <redacted>
---
builtin/stash.c | 9 +++++++++
t/t3903-stash.sh | 2 +-
2 files changed, 10 insertions(+), 1 deletion(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-09-08 01:43:56
From: Elijah Newren <redacted>
If a user deletes a file and places a directory of untracked files
there, then stashes all these changes, the untracked directory of files
cannot be restored until after the corresponding file in the way is
removed. So, restore changes to tracked files before restoring
untracked files.
There is no similar problem to worry about in the opposite directory,
because untracked files are always additive. Said another way, there's
no way to "stash a removal of an untracked file" because if an untracked
file is removed, git simply doesn't know about it.
Signed-off-by: Elijah Newren <redacted>
---
builtin/stash.c | 6 +++---
t/t3903-stash.sh | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
From: Johannes Schindelin <hidden> Date: 2021-09-08 08:02:43
Hi Elijah,
On Wed, 8 Sep 2021, Elijah Newren via GitGitGadget wrote:
quoted hunk
From: Elijah Newren <redacted>
When a file is removed from the cache, but there is a file of the same
name present in the working directory, we would normally treat that file
in the working directory as untracked. However, in the case of stash,
doing that would prevent a simple 'git stash push', because the untracked
file would be in the way of restoring the deleted file.
git stash, however, blindly assumes that whatever is in the working
directory for a deleted file is wanted and passes that path along to
update-index. That causes problems when the working directory contains
a directory with the same name as the deleted file. Add some code for
this special case that will avoid passing directory names to
update-index.
Signed-off-by: Elijah Newren <redacted>
---
builtin/stash.c | 9 +++++++++
t/t3903-stash.sh | 2 +-
2 files changed, 10 insertions(+), 1 deletion(-)
@@ -313,6 +313,12 @@ static int reset_head(void)returnrun_command(&cp);}+staticintis_path_a_directory(constchar*path)+{+structstatst;+return(!lstat(path,&st)&&S_ISDIR(st.st_mode));+}
Git's API is unnecessarily confusing (because grown organically), so it is
easy to get confused by that `is_directory()` function that is declared in
`cache.h` and defined in `abspath.c`:
/*
* Do not use this for inspecting *tracked* content. When path is a
* symlink to a directory, we do not want to say it is a directory when
* dealing with tracked content in the working tree.
*/
int is_directory(const char *path)
{
struct stat st;
return (!stat(path, &st) && S_ISDIR(st.st_mode));
}
The difference I see is that you use an `lstat()`, which is kind of
important here.
Maybe you could add a paragraph pointing out that we cannot use
`is_directory()` here because it would follow symbolic links, which we
need to avoid here?
@@ -320,6 +326,9 @@ static void add_diff_to_buf(struct diff_queue_struct *q, int i; for (i = 0; i < q->nr; i++) {+ if (is_path_a_directory(q->queue[i]->one->path))+ continue;+ strbuf_addstr(data, q->queue[i]->one->path); /* NUL-terminate: will be fed to update-index -z */
From: Johannes Schindelin <hidden> Date: 2021-09-08 08:04:36
Hi Elijah,
On Wed, 8 Sep 2021, Elijah Newren via GitGitGadget wrote:
This series fixes a few D/F issues in the stash command. These were some
issues I found while working on unintentional removal of untracked
files/directories and the current working directory, and I'm just submitting
them separately.
Awesome work! Apart from asking for an additional clarification in the
commit message of the second patch, I have nothing else to offer but my
sincere thanks for working on the `stash` code.
Thank you,
Dscho
On 9/7/2021 9:43 PM, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <redacted>
If a user deletes a file and places a directory of untracked files
there, then stashes all these changes, the untracked directory of files
cannot be restored until after the corresponding file in the way is
removed. So, restore changes to tracked files before restoring
untracked files.
There is no similar problem to worry about in the opposite directory,
s/directory/direction/ ?
because untracked files are always additive. Said another way, there's
no way to "stash a removal of an untracked file" because if an untracked
file is removed, git simply doesn't know about it.
Hi Elijah,
On Wed, 8 Sep 2021, Elijah Newren via GitGitGadget wrote:
quoted
This series fixes a few D/F issues in the stash command. These were some
issues I found while working on unintentional removal of untracked
files/directories and the current working directory, and I'm just submitting
them separately.
Awesome work! Apart from asking for an additional clarification in the
commit message of the second patch, I have nothing else to offer but my
sincere thanks for working on the `stash` code.
I found what is probably a typo in a commit message, but otherwise this
looks great.
Thanks,
-Stolee
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-09-10 10:30:01
This series fixes a few D/F issues in the stash command. These were some
issues I found while working on unintentional removal of untracked
files/directories and the current working directory, and I'm just submitting
them separately.
Changes since v1:
* Fix accidental creation of file named 'expect' (copy-paste problem...)
* Documented the reason for adding is_path_a_directory() and not using
is_directory()
* Removed typo, fixed up confusing wording, and added a companion test to
show that F->D and D->F have the same fix.
Elijah Newren (3):
t3903: document a pair of directory/file bugs
stash: avoid feeding directories to update-index
stash: restore untracked files AFTER restoring tracked files
builtin/stash.c | 20 ++++++++++++++---
t/t3903-stash.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+), 3 deletions(-)
base-commit: e0a2f5cbc585657e757385ad918f167f519cfb96
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1086%2Fnewren%2Fstash-df-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1086/newren/stash-df-fixes-v2
Pull-Request: https://github.com/git/git/pull/1086
Range-diff vs v1:
1: bc66a6ae75d ! 1: 5ddb70d332b t3903: document a pair of directory/file bugs
@@ Metadata
## Commit message ##
t3903: document a pair of directory/file bugs
+ There are three tests here, because the second bug is documented with
+ two tests: a file -> directory change and a directory -> file change.
+ The reason for the two tests is just to verify that both are indeed
+ broken but that both will be fixed by the same simple change (which will
+ be provided in a subsequent patch).
+
Signed-off-by: Elijah Newren [off-list ref]
## t/t3903-stash.sh ##
@@ t/t3903-stash.sh: test_expect_success 'stash -c stash.useBuiltin=false warning '
+ git rm filler &&
+ mkdir filler &&
+ echo contents >filler/file &&
-+ cp filler/file expect &&
+ git stash push
+ )
+'
+
-+test_expect_failure 'git stash can pop directory/file saved changes' '
++test_expect_failure 'git stash can pop file -> directory saved changes' '
+ test_create_repo directory_file_switch_v2 &&
+ (
+ cd directory_file_switch_v2 &&
@@ t/t3903-stash.sh: test_expect_success 'stash -c stash.useBuiltin=false warning '
+ test_cmp expect filler/file
+ )
+'
++
++test_expect_failure 'git stash can pop directory -> file saved changes' '
++ test_create_repo directory_file_switch_v3 &&
++ (
++ cd directory_file_switch_v3 &&
++ test_commit init &&
++
++ mkdir filler &&
++ test_write_lines some words >filler/file1 &&
++ test_write_lines and stuff >filler/file2 &&
++ git add filler &&
++ git commit -m filler &&
++
++ git rm -rf filler &&
++ echo contents >filler &&
++ cp filler expect &&
++ git stash push --include-untracked &&
++ git stash apply --index &&
++ test_cmp expect filler
++ )
++'
+
test_done
2: c7f5ae66a92 ! 2: 31e38c6c33c stash: avoid feeding directories to update-index
@@ builtin/stash.c: static int reset_head(void)
+static int is_path_a_directory(const char *path)
+{
++ /*
++ * This function differs from abspath.c:is_directory() in that
++ * here we use lstat() instead of stat(); we do not want to
++ * follow symbolic links here.
++ */
+ struct stat st;
+ return (!lstat(path, &st) && S_ISDIR(st.st_mode));
+}
3: ac8ca07481d ! 3: 6254938948c stash: restore untracked files AFTER restoring tracked files
@@ Commit message
removed. So, restore changes to tracked files before restoring
untracked files.
- There is no similar problem to worry about in the opposite directory,
- because untracked files are always additive. Said another way, there's
- no way to "stash a removal of an untracked file" because if an untracked
- file is removed, git simply doesn't know about it.
+ There is no counterpart problem to worry about with the user deleting an
+ untracked file and then add a tracked one in its place. Git does not
+ track untracked files, and so will not know the untracked file was
+ deleted, and thus won't be able to stash the removal of that file.
Signed-off-by: Elijah Newren [off-list ref]
@@ t/t3903-stash.sh: test_expect_success 'git stash succeeds despite directory/file
)
'
--test_expect_failure 'git stash can pop directory/file saved changes' '
-+test_expect_success 'git stash can pop directory/file saved changes' '
+-test_expect_failure 'git stash can pop file -> directory saved changes' '
++test_expect_success 'git stash can pop file -> directory saved changes' '
test_create_repo directory_file_switch_v2 &&
(
cd directory_file_switch_v2 &&
+@@ t/t3903-stash.sh: test_expect_failure 'git stash can pop file -> directory saved changes' '
+ )
+ '
+
+-test_expect_failure 'git stash can pop directory -> file saved changes' '
++test_expect_success 'git stash can pop directory -> file saved changes' '
+ test_create_repo directory_file_switch_v3 &&
+ (
+ cd directory_file_switch_v3 &&
--
gitgitgadget
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-09-10 10:30:03
From: Elijah Newren <redacted>
There are three tests here, because the second bug is documented with
two tests: a file -> directory change and a directory -> file change.
The reason for the two tests is just to verify that both are indeed
broken but that both will be fixed by the same simple change (which will
be provided in a subsequent patch).
Signed-off-by: Elijah Newren <redacted>
---
t/t3903-stash.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-09-10 10:30:04
From: Elijah Newren <redacted>
When a file is removed from the cache, but there is a file of the same
name present in the working directory, we would normally treat that file
in the working directory as untracked. However, in the case of stash,
doing that would prevent a simple 'git stash push', because the untracked
file would be in the way of restoring the deleted file.
git stash, however, blindly assumes that whatever is in the working
directory for a deleted file is wanted and passes that path along to
update-index. That causes problems when the working directory contains
a directory with the same name as the deleted file. Add some code for
this special case that will avoid passing directory names to
update-index.
Signed-off-by: Elijah Newren <redacted>
---
builtin/stash.c | 14 ++++++++++++++
t/t3903-stash.sh | 2 +-
2 files changed, 15 insertions(+), 1 deletion(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-09-10 10:30:04
From: Elijah Newren <redacted>
If a user deletes a file and places a directory of untracked files
there, then stashes all these changes, the untracked directory of files
cannot be restored until after the corresponding file in the way is
removed. So, restore changes to tracked files before restoring
untracked files.
There is no counterpart problem to worry about with the user deleting an
untracked file and then add a tracked one in its place. Git does not
track untracked files, and so will not know the untracked file was
deleted, and thus won't be able to stash the removal of that file.
Signed-off-by: Elijah Newren <redacted>
---
builtin/stash.c | 6 +++---
t/t3903-stash.sh | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)