Skip to contentSkip to conversation

Fix git stash with skip-worktree entries

PATCH Superseded Johannes Schindelin via GitGitGadget 3 participants 23 messages Sep 26, 2019 – Nov 2, 2019 Documentation

lore
Share & tools
lore

Johannes Schindelin via GitGitGadget

author
#0 lore raw

cover Fix git stash with skip-worktree entries

My colleague Dan Thompson reported a bug in a sparse checkout, where git stash (after resolving merge conflicts and then making up their mind to stash the changes instead of committing them) would "lose" files (and files that were not even in the sparse checkout's cone!).

I first considered changing the behavior of git diff-index to simply ignore skip-worktree entries. But after re-reading the documentation of the skip-worktree bit, I became convinced that this would be incorrect a fix because the command really does what it advertises to do.

Then, I briefly considered introducing a flag that would change the behavior thusly, but ended up deciding against it.

The actual problem, after all, is the git update-index call and that it heeds the --remove (but not the --add) option for skip-worktree entries. "Heeds", I should say, because the idea of the skip-worktree bit really is documented to imply that the worktree files should be considered identical to their staged versions.

So arguably, it could be considered a bug that git update-index --remove even bothers to touch skip-worktree entries. But this behavior has been in place for over 10 years, so I opted to introduce a new mode that does what git stash needs in order to fix the bug.

Johannes Schindelin (2):
update-index: optionally leave skip-worktree entries alone
stash: handle staged changes in skip-worktree files correctly

 Documentation/git-update-index.txt |  6 ++++++
 builtin/stash.c                    |  5 +++--
 builtin/update-index.c             |  6 +++++-
 git-legacy-stash.sh                |  3 ++-
 t/t3903-stash.sh                   | 11 +++++++++++
 5 files changed, 27 insertions(+), 4 deletions(-)

base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-355%2Fdscho%2Ffix-stash-with-skip-worktree-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-355/dscho/fix-stash-with-skip-worktree-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/355

Signature · 1 line
gitgitgadget
author
#1 lore raw

PATCH 1/2 update-index: optionally leave skip-worktree entries alone

From: Johannes Schindelin <[email protected]>

While git update-index mostly ignores paths referring to index entries whose skip-worktree bit is set, in b4d1690df11 (Teach Git to respect skip-worktree bit (reading part), 2009-08-20), for reasons that are not entirely obvious, the --remove option was made special: it _does_ remove index entries even if their skip-worktree bit is set.

Seeing as this behavior has been in place for a decade now, it does not make sense to change it.

However, in preparation for fixing a bug in git stash where it pretends that skip-worktree entries have actually been removed, we need a mode where git update-index leaves all skip-worktree entries alone, even if the --remove option was passed.

Signed-off-by Johannes Schindelin <[email protected]>
---
 Documentation/git-update-index.txt | 6 ++++++
 builtin/update-index.c             | 6 +++++-
 2 files changed, 11 insertions(+), 1 deletion(-)
Documentation/git-update-index.txt +6 -0
index 1c4d146a41..08393445e7 100644
@@ -16,6 +16,7 @@ SYNOPSIS
 	     [--chmod=(+|-)x]
 	     [--[no-]assume-unchanged]
 	     [--[no-]skip-worktree]
+	     [--[no-]ignore-skip-worktree-entries]
 	     [--[no-]fsmonitor-valid]
 	     [--ignore-submodules]
 	     [--[no-]split-index]

@@ -113,6 +114,11 @@ you will need to handle the situation manually.
 	set and unset the "skip-worktree" bit for the paths. See
 	section "Skip-worktree bit" below for more information.
 
+
+--[no-]ignore-skip-worktree-entries::
+	Do not remove skip-worktree (AKA "index-only") entries even when
+	the `--remove` option was specified.
+
 --[no-]fsmonitor-valid::
 	When one of these flags is specified, the object name recorded
 	for the paths are not updated. Instead, these options
builtin/update-index.c +5 -1
index dff2f4b837..074d563df0 100644
@@ -35,6 +35,7 @@ static int verbose;
 static int mark_valid_only;
 static int mark_skip_worktree_only;
 static int mark_fsmonitor_only;
+static int ignore_skip_worktree_entries;
 #define MARK_FLAG 1
 #define UNMARK_FLAG 2
 static struct strbuf mtime_dir = STRBUF_INIT;

@@ -381,7 +382,8 @@ static int process_path(const char *path, struct stat *st, int stat_errno)
 		 * so updating it does not make sense.
 		 * On the other hand, removing it from index should work
 		 */
-		if (allow_remove && remove_file_from_cache(path))
+		if (!ignore_skip_worktree_entries && allow_remove &&
+		    remove_file_from_cache(path))
 			return error("%s: cannot remove from the index", path);
 		return 0;
 	}

@@ -1013,6 +1015,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		{OPTION_SET_INT, 0, "no-skip-worktree", &mark_skip_worktree_only, NULL,
 			N_("clear skip-worktree bit"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
+		OPT_BOOL(0, "ignore-skip-worktree-entries", &ignore_skip_worktree_entries,
+			 N_("do not touch index-only entries")),
 		OPT_SET_INT(0, "info-only", &info_only,
 			N_("add to index only; do not add content to object database"), 1),
 		OPT_SET_INT(0, "force-remove", &force_remove,
Signature · 1 line
gitgitgadget
author
#5 lore raw

PATCH 2/2 stash: handle staged changes in skip-worktree files correctly

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

When calling git stash while changes were staged for files that are marked with the skip-worktree bit (e.g. files that are excluded in a sparse checkout), the files are recorded as _deleted_ instead.

The reason is that git stash tries to construct the tree reflecting the worktree essentially by copying the index to a temporary one and then updating the files from the worktree. Crucially, it calls git diff-index to update also those files that are in the HEAD but have been unstaged in the index.

However, when the temporary index is updated via git update-index --add --remove, skip-worktree entries mark the files as deleted by mistake.

Let's use the newly-introduced --ignore-skip-worktree-entries option of git update-index to prevent exactly this from happening.

Note that the regression test case deliberately avoids replicating the scenario described above and instead tries to recreate just the symptom.

Reported by Dan Thompson.

Signed-off-by Johannes Schindelin <[email protected]>
---
 builtin/stash.c     |  5 +++--
 git-legacy-stash.sh |  3 ++-
 t/t3903-stash.sh    | 11 +++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)
builtin/stash.c +3 -2
index b5a301f24d..56f3b551e4 100644
@@ -1082,8 +1082,9 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	}
 
 	cp_upd_index.git_cmd = 1;
-	argv_array_pushl(&cp_upd_index.args, "update-index", "-z", "--add",
-			 "--remove", "--stdin", NULL);
+	argv_array_pushl(&cp_upd_index.args, "update-index",
+			 "--ignore-skip-worktree-entries",
+			 "-z", "--add", "--remove", "--stdin", NULL);
 	argv_array_pushf(&cp_upd_index.env_array, "GIT_INDEX_FILE=%s",
 			 stash_index_path.buf);
git-legacy-stash.sh +2 -1
index f60e9b3e87..5398a5161d 100755
@@ -193,7 +193,8 @@ create_stash () {
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
 			git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
-			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
+			git update-index --ignore-skip-worktree-entries \
+				-z --add --remove --stdin <"$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
 		) ) ||
t/t3903-stash.sh +11 -0
index b8e337893f..1e977145b8 100755
@@ -1241,4 +1241,15 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu
 	test_path_is_missing to-remove
 '
 
+test_expect_success 'stash handles skip-worktree entries nicely' '
+	test_commit A &&
+	echo changed >A.t &&
+	git add A.t &&
+	git update-index --skip-worktree A.t &&
+	rm A.t &&
+	git stash &&
+
+	git rev-parse --verify refs/stash:A.t
+'
+
 test_done
Signature · 1 line
gitgitgadget
#7 lore raw
In reply to Johannes Schindelin via GitGitGadget

Hi Junio,

Quoted 4 lines · Johannes Schindelin via GitGitGadget

My colleague Dan Thompson reported a bug in a sparse checkout, where git stash (after resolving merge conflicts and then making up their mind to stash the changes instead of committing them) would "lose" files (and files that were not even in the sparse checkout's cone!).

I only realized _now_ that this patch has not made it anywhere; I would like to have it at least in Git for Windows v2.24.0.

Could I ask for this to still be picked up into pu at least, so that it does not fall into oblivion?

Thanks,
Dscho

Quoted 40 lines

I first considered changing the behavior of git diff-index to simply ignore skip-worktree entries. But after re-reading the documentation of the skip-worktree bit, I became convinced that this would be incorrect a fix because the command really does what it advertises to do.

Then, I briefly considered introducing a flag that would change the behavior thusly, but ended up deciding against it.

The actual problem, after all, is the git update-index call and that it heeds the --remove (but not the --add) option for skip-worktree entries. "Heeds", I should say, because the idea of the skip-worktree bit really is documented to imply that the worktree files should be considered identical to their staged versions.

So arguably, it could be considered a bug that git update-index --remove even bothers to touch skip-worktree entries. But this behavior has been in place for over 10 years, so I opted to introduce a new mode that does what git stash needs in order to fix the bug.

Johannes Schindelin (2):
update-index: optionally leave skip-worktree entries alone
stash: handle staged changes in skip-worktree files correctly

 Documentation/git-update-index.txt |  6 ++++++
 builtin/stash.c                    |  5 +++--
 builtin/update-index.c             |  6 +++++-
 git-legacy-stash.sh                |  3 ++-
 t/t3903-stash.sh                   | 11 +++++++++++
 5 files changed, 27 insertions(+), 4 deletions(-)

base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-355%2Fdscho%2Ffix-stash-with-skip-worktree-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-355/dscho/fix-stash-with-skip-worktree-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/355
--
gitgitgadget

#8 lore raw
Quoted 10 lines · Johannes Schindelin

Hi Junio,

Quoted 4 lines · Johannes Schindelin via GitGitGadget

My colleague Dan Thompson reported a bug in a sparse checkout, where git stash (after resolving merge conflicts and then making up their mind to stash the changes instead of committing them) would "lose" files (and files that were not even in the sparse checkout's cone!).

I only realized _now_ that this patch has not made it anywhere.

Yeah, I do not recall seeing any of the patches in the topic (nor the cover letter). It is not clear to me what "lose" above means, which is an indication that I didn't read the topic a month ago X-<.

Did it even get any review by skip worktree bit experts back then?

#2 lore raw

Re: [PATCH 1/2] update-index: optionally leave skip-worktree entries alone

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 7 lines · writes:

From: Johannes Schindelin <[email protected]>

While git update-index mostly ignores paths referring to index entries whose skip-worktree bit is set, in b4d1690df11 (Teach Git to respect skip-worktree bit (reading part), 2009-08-20), for reasons that are not entirely obvious, the --remove option was made special: it _does_ remove index entries even if their skip-worktree bit is set.

I think that made sense to notice removal of the path, because skip-worktree bit was not "apply --cached semantics instead of looking at the working tree files". In other words, it was only about contents inside the files, and not existence of paths.

I am not offhand sure if it still makes sense; if I were being asked to review that commit today, I suspect that I may be tempted to say that we should ignore both contents change and presence change for entries that have skip-worktree bit set.

Quoted 4 lines

However, in preparation for fixing a bug in git stash where it pretends that skip-worktree entries have actually been removed, we need a mode where git update-index leaves all skip-worktree entries alone, even if the --remove option was passed.

We might want to make this the default eventually (is there a known use case where the current behaviour makes sense, I wonder?), but I agree that this is a safe thing to do at least for now.

Quoted 5 lines
Signed-off-by Johannes Schindelin <[email protected]>
---
 Documentation/git-update-index.txt | 6 ++++++
 builtin/update-index.c             | 6 +++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

Isn't this something reasonably easy to guard against regression with a test or two?

Quoted 56 lines
Documentation/git-update-index.txt +6 -0
index 1c4d146a41..08393445e7 100644
@@ -16,6 +16,7 @@ SYNOPSIS
 	     [--chmod=(+|-)x]
 	     [--[no-]assume-unchanged]
 	     [--[no-]skip-worktree]
+	     [--[no-]ignore-skip-worktree-entries]
 	     [--[no-]fsmonitor-valid]
 	     [--ignore-submodules]
 	     [--[no-]split-index]

@@ -113,6 +114,11 @@ you will need to handle the situation manually.
 	set and unset the "skip-worktree" bit for the paths. See
 	section "Skip-worktree bit" below for more information.
 
+
+--[no-]ignore-skip-worktree-entries::
+	Do not remove skip-worktree (AKA "index-only") entries even when
+	the `--remove` option was specified.
+
 --[no-]fsmonitor-valid::
 	When one of these flags is specified, the object name recorded
 	for the paths are not updated. Instead, these options
builtin/update-index.c +5 -1
index dff2f4b837..074d563df0 100644
@@ -35,6 +35,7 @@ static int verbose;
 static int mark_valid_only;
 static int mark_skip_worktree_only;
 static int mark_fsmonitor_only;
+static int ignore_skip_worktree_entries;
 #define MARK_FLAG 1
 #define UNMARK_FLAG 2
 static struct strbuf mtime_dir = STRBUF_INIT;

@@ -381,7 +382,8 @@ static int process_path(const char *path, struct stat *st, int stat_errno)
 		 * so updating it does not make sense.
 		 * On the other hand, removing it from index should work
 		 */
-		if (allow_remove && remove_file_from_cache(path))
+		if (!ignore_skip_worktree_entries && allow_remove &&
+		    remove_file_from_cache(path))
 			return error("%s: cannot remove from the index", path);
 		return 0;
 	}

@@ -1013,6 +1015,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		{OPTION_SET_INT, 0, "no-skip-worktree", &mark_skip_worktree_only, NULL,
 			N_("clear skip-worktree bit"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
+		OPT_BOOL(0, "ignore-skip-worktree-entries", &ignore_skip_worktree_entries,
+			 N_("do not touch index-only entries")),
 		OPT_SET_INT(0, "info-only", &info_only,
 			N_("add to index only; do not add content to object database"), 1),
 		OPT_SET_INT(0, "force-remove", &force_remove,
#6 lore raw

Re: [PATCH 2/2] stash: handle staged changes in skip-worktree files correctly

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 3 lines · writes:

When calling git stash while changes were staged for files that are marked with the skip-worktree bit (e.g. files that are excluded in a sparse checkout), the files are recorded as _deleted_ instead.

Assuming that 1/2 makes sense to all of us, I think the changes in 2/2 make 100% sense.

Will queue. Thanks.

author
#12 lore raw

v2 cover Fix git stash with skip-worktree entries

In reply to Johannes Schindelin via GitGitGadget

My colleague Dan Thompson reported a bug in a sparse checkout, where git stash (after resolving merge conflicts and then making up their mind to stash the changes instead of committing them) would "lose" files (and files that were not even in the sparse checkout's cone!).

I first considered changing the behavior of git diff-index to simply ignore skip-worktree entries. But after re-reading the documentation of the skip-worktree bit, I became convinced that this would be incorrect a fix because the command really does what it advertises to do.

Then, I briefly considered introducing a flag that would change the behavior thusly, but ended up deciding against it.

The actual problem, after all, is the git update-index call and that it heeds the --remove (but not the --add) option for skip-worktree entries. "Heeds", I should say, because the idea of the skip-worktree bit really is documented to imply that the worktree files should be considered identical to their staged versions.

So arguably, it could be considered a bug that git update-index --remove even bothers to touch skip-worktree entries. But this behavior has been in place for over 10 years, so I opted to introduce a new mode that does what git stash needs in order to fix the bug.

Changes since v1:

* Added a test even for the --ignore-skip-worktree-entries option alone (i.e. without going through git stash)
* Rebased onto tg/stash-refresh-index to avoid merge conflicts in t/t3903-stash.sh.

Johannes Schindelin (2):
update-index: optionally leave skip-worktree entries alone
stash: handle staged changes in skip-worktree files correctly

 Documentation/git-update-index.txt |  6 ++++++
 builtin/stash.c                    |  5 +++--
 builtin/update-index.c             |  6 +++++-
 git-legacy-stash.sh                |  3 ++-
 t/t3903-stash.sh                   | 11 +++++++++++
 t/t7012-skip-worktree-writing.sh   | 14 ++++++++++++++
 6 files changed, 41 insertions(+), 4 deletions(-)

base-commit: 34933d0eff5d4c91fae6ad6f71a6e6a69a496ced
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-355%2Fdscho%2Ffix-stash-with-skip-worktree-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-355/dscho/fix-stash-with-skip-worktree-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/355

Range-diff vs v1:

1: c263eb54b3 ! 1: 86dbb11f15 update-index: optionally leave skip-worktree entries alone

     @@ -72,3 +72,28 @@
       		OPT_SET_INT(0, "info-only", &info_only,
       			N_("add to index only; do not add content to object database"), 1),
       		OPT_SET_INT(0, "force-remove", &force_remove,
     +
     + diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
     + --- a/t/t7012-skip-worktree-writing.sh
     + +++ b/t/t7012-skip-worktree-writing.sh
     +@@
     + 	test_i18ncmp expected result
     + '
     + 
     ++test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '
     ++	test_commit geroff-me &&
     ++	git update-index --skip-worktree geroff-me.t &&
     ++	rm geroff-me.t &&
     ++
     ++	: ignoring the worktree &&
     ++	git update-index --remove --ignore-skip-worktree-entries geroff-me.t &&
     ++	git diff-index --cached --exit-code HEAD &&
     ++
     ++	: not ignoring the worktree, a deletion is staged &&
     ++	git update-index --remove geroff-me.t &&
     ++	test_must_fail git diff-index --cached --exit-code HEAD
     ++'
     ++
     + #TODO test_expect_failure 'git-apply adds file' false
     + #TODO test_expect_failure 'git-apply updates file' false
     + #TODO test_expect_failure 'git-apply removes file' false
 2:  4c684be179 ! 2:  9835e66399 stash: handle staged changes in skip-worktree files correctly
     @@ -59,7 +59,7 @@
       --- a/t/t3903-stash.sh
       +++ b/t/t3903-stash.sh
      @@
     - 	test_path_is_missing to-remove
     + 	git stash apply
       '
       
      +test_expect_success 'stash handles skip-worktree entries nicely' '
Signature · 1 line
gitgitgadget
author
#13 lore raw

PATCH v2 1/2 update-index: optionally leave skip-worktree entries alone

From: Johannes Schindelin <[email protected]>

While git update-index mostly ignores paths referring to index entries whose skip-worktree bit is set, in b4d1690df11 (Teach Git to respect skip-worktree bit (reading part), 2009-08-20), for reasons that are not entirely obvious, the --remove option was made special: it _does_ remove index entries even if their skip-worktree bit is set.

Seeing as this behavior has been in place for a decade now, it does not make sense to change it.

However, in preparation for fixing a bug in git stash where it pretends that skip-worktree entries have actually been removed, we need a mode where git update-index leaves all skip-worktree entries alone, even if the --remove option was passed.

Signed-off-by Johannes Schindelin <[email protected]>
---
 Documentation/git-update-index.txt |  6 ++++++
 builtin/update-index.c             |  6 +++++-
 t/t7012-skip-worktree-writing.sh   | 14 ++++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)
Documentation/git-update-index.txt +6 -0
index 1c4d146a41..08393445e7 100644
@@ -16,6 +16,7 @@ SYNOPSIS
 	     [--chmod=(+|-)x]
 	     [--[no-]assume-unchanged]
 	     [--[no-]skip-worktree]
+	     [--[no-]ignore-skip-worktree-entries]
 	     [--[no-]fsmonitor-valid]
 	     [--ignore-submodules]
 	     [--[no-]split-index]

@@ -113,6 +114,11 @@ you will need to handle the situation manually.
 	set and unset the "skip-worktree" bit for the paths. See
 	section "Skip-worktree bit" below for more information.
 
+
+--[no-]ignore-skip-worktree-entries::
+	Do not remove skip-worktree (AKA "index-only") entries even when
+	the `--remove` option was specified.
+
 --[no-]fsmonitor-valid::
 	When one of these flags is specified, the object name recorded
 	for the paths are not updated. Instead, these options
builtin/update-index.c +5 -1
index dff2f4b837..074d563df0 100644
@@ -35,6 +35,7 @@ static int verbose;
 static int mark_valid_only;
 static int mark_skip_worktree_only;
 static int mark_fsmonitor_only;
+static int ignore_skip_worktree_entries;
 #define MARK_FLAG 1
 #define UNMARK_FLAG 2
 static struct strbuf mtime_dir = STRBUF_INIT;

@@ -381,7 +382,8 @@ static int process_path(const char *path, struct stat *st, int stat_errno)
 		 * so updating it does not make sense.
 		 * On the other hand, removing it from index should work
 		 */
-		if (allow_remove && remove_file_from_cache(path))
+		if (!ignore_skip_worktree_entries && allow_remove &&
+		    remove_file_from_cache(path))
 			return error("%s: cannot remove from the index", path);
 		return 0;
 	}

@@ -1013,6 +1015,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		{OPTION_SET_INT, 0, "no-skip-worktree", &mark_skip_worktree_only, NULL,
 			N_("clear skip-worktree bit"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
+		OPT_BOOL(0, "ignore-skip-worktree-entries", &ignore_skip_worktree_entries,
+			 N_("do not touch index-only entries")),
 		OPT_SET_INT(0, "info-only", &info_only,
 			N_("add to index only; do not add content to object database"), 1),
 		OPT_SET_INT(0, "force-remove", &force_remove,
t/t7012-skip-worktree-writing.sh +14 -0
index 9d1abe50ef..28b1b0b2b9 100755
@@ -134,6 +134,20 @@ test_expect_success 'git-clean, dirty case' '
 	test_i18ncmp expected result
 '
 
+test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '
+	test_commit geroff-me &&
+	git update-index --skip-worktree geroff-me.t &&
+	rm geroff-me.t &&
+
+	: ignoring the worktree &&
+	git update-index --remove --ignore-skip-worktree-entries geroff-me.t &&
+	git diff-index --cached --exit-code HEAD &&
+
+	: not ignoring the worktree, a deletion is staged &&
+	git update-index --remove geroff-me.t &&
+	test_must_fail git diff-index --cached --exit-code HEAD
+'
+
 #TODO test_expect_failure 'git-apply adds file' false
 #TODO test_expect_failure 'git-apply updates file' false
 #TODO test_expect_failure 'git-apply removes file' false
Signature · 1 line
gitgitgadget
author
#18 lore raw

PATCH v2 2/2 stash: handle staged changes in skip-worktree files correctly

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

When calling git stash while changes were staged for files that are marked with the skip-worktree bit (e.g. files that are excluded in a sparse checkout), the files are recorded as _deleted_ instead.

The reason is that git stash tries to construct the tree reflecting the worktree essentially by copying the index to a temporary one and then updating the files from the worktree. Crucially, it calls git diff-index to update also those files that are in the HEAD but have been unstaged in the index.

However, when the temporary index is updated via git update-index --add --remove, skip-worktree entries mark the files as deleted by mistake.

Let's use the newly-introduced --ignore-skip-worktree-entries option of git update-index to prevent exactly this from happening.

Note that the regression test case deliberately avoids replicating the scenario described above and instead tries to recreate just the symptom.

Reported by Dan Thompson.

Signed-off-by Johannes Schindelin <[email protected]>
---
 builtin/stash.c     |  5 +++--
 git-legacy-stash.sh |  3 ++-
 t/t3903-stash.sh    | 11 +++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)
builtin/stash.c +3 -2
index ab30d1e920..e3962bf73e 100644
@@ -1082,8 +1082,9 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	}
 
 	cp_upd_index.git_cmd = 1;
-	argv_array_pushl(&cp_upd_index.args, "update-index", "-z", "--add",
-			 "--remove", "--stdin", NULL);
+	argv_array_pushl(&cp_upd_index.args, "update-index",
+			 "--ignore-skip-worktree-entries",
+			 "-z", "--add", "--remove", "--stdin", NULL);
 	argv_array_pushf(&cp_upd_index.env_array, "GIT_INDEX_FILE=%s",
 			 stash_index_path.buf);
git-legacy-stash.sh +2 -1
index f60e9b3e87..5398a5161d 100755
@@ -193,7 +193,8 @@ create_stash () {
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
 			git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
-			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
+			git update-index --ignore-skip-worktree-entries \
+				-z --add --remove --stdin <"$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
 		) ) ||
t/t3903-stash.sh +11 -0
index 392954d6dd..57258d5668 100755
@@ -1257,4 +1257,15 @@ test_expect_success 'stash apply should succeed with unmodified file' '
 	git stash apply
 '
 
+test_expect_success 'stash handles skip-worktree entries nicely' '
+	test_commit A &&
+	echo changed >A.t &&
+	git add A.t &&
+	git update-index --skip-worktree A.t &&
+	rm A.t &&
+	git stash &&
+
+	git rev-parse --verify refs/stash:A.t
+'
+
 test_done
Signature · 1 line
gitgitgadget
#9 lore raw
In reply to Junio C Hamano

Hi Junio,

Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Johannes Schindelin
Quoted 5 lines · Johannes Schindelin via GitGitGadget

My colleague Dan Thompson reported a bug in a sparse checkout, where git stash (after resolving merge conflicts and then making up their mind to stash the changes instead of committing them) would "lose" files (and files that were not even in the sparse checkout's cone!).

Quoted 5 lines
Quoted 1 line

I only realized _now_ that this patch has not made it anywhere.

Yeah, I do not recall seeing any of the patches in the topic (nor the cover letter). It is not clear to me what "lose" above means, which is an indication that I didn't read the topic a month ago X-<.

Sorry, in this context, "to lose a file" means that a git stash would record such a file as deleted. A subsequent git stash apply _would_ then delete it.

Quoted 1 line

Did it even get any review by skip worktree bit experts back then?

Sadly, it did not get any review back then...

Ciao,
Dscho

#3 lore raw

Re: [PATCH 1/2] update-index: optionally leave skip-worktree entries alone

In reply to Junio C Hamano

Hi Junio,

Quoted 30 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 7 lines · writes:

From: Johannes Schindelin <[email protected]>

While git update-index mostly ignores paths referring to index entries whose skip-worktree bit is set, in b4d1690df11 (Teach Git to respect skip-worktree bit (reading part), 2009-08-20), for reasons that are not entirely obvious, the --remove option was made special: it _does_ remove index entries even if their skip-worktree bit is set.

I think that made sense to notice removal of the path, because skip-worktree bit was not "apply --cached semantics instead of looking at the working tree files". In other words, it was only about contents inside the files, and not existence of paths.

I am not offhand sure if it still makes sense; if I were being asked to review that commit today, I suspect that I may be tempted to say that we should ignore both contents change and presence change for entries that have skip-worktree bit set.

Quoted 4 lines

However, in preparation for fixing a bug in git stash where it pretends that skip-worktree entries have actually been removed, we need a mode where git update-index leaves all skip-worktree entries alone, even if the --remove option was passed.

We might want to make this the default eventually (is there a known use case where the current behaviour makes sense, I wonder?), but I agree that this is a safe thing to do at least for now.

Quoted 8 lines
Quoted 5 lines
Signed-off-by Johannes Schindelin <[email protected]>
---
 Documentation/git-update-index.txt | 6 ++++++
 builtin/update-index.c             | 6 +++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

Isn't this something reasonably easy to guard against regression with a test or two?

I sent out a v2 with tests added to 1/2.

Thanks,
Dscho

Quoted 58 lines
Quoted 57 lines
Documentation/git-update-index.txt +6 -0
index 1c4d146a41..08393445e7 100644
@@ -16,6 +16,7 @@ SYNOPSIS
 	     [--chmod=(+|-)x]
 	     [--[no-]assume-unchanged]
 	     [--[no-]skip-worktree]
+	     [--[no-]ignore-skip-worktree-entries]
 	     [--[no-]fsmonitor-valid]
 	     [--ignore-submodules]
 	     [--[no-]split-index]

@@ -113,6 +114,11 @@ you will need to handle the situation manually.
 	set and unset the "skip-worktree" bit for the paths. See
 	section "Skip-worktree bit" below for more information.
 
+
+--[no-]ignore-skip-worktree-entries::
+	Do not remove skip-worktree (AKA "index-only") entries even when
+	the `--remove` option was specified.
+
 --[no-]fsmonitor-valid::
 	When one of these flags is specified, the object name recorded
 	for the paths are not updated. Instead, these options
builtin/update-index.c +5 -1
index dff2f4b837..074d563df0 100644
@@ -35,6 +35,7 @@ static int verbose;
 static int mark_valid_only;
 static int mark_skip_worktree_only;
 static int mark_fsmonitor_only;
+static int ignore_skip_worktree_entries;
 #define MARK_FLAG 1
 #define UNMARK_FLAG 2
 static struct strbuf mtime_dir = STRBUF_INIT;

@@ -381,7 +382,8 @@ static int process_path(const char *path, struct stat *st, int stat_errno)
 		 * so updating it does not make sense.
 		 * On the other hand, removing it from index should work
 		 */
-		if (allow_remove && remove_file_from_cache(path))
+		if (!ignore_skip_worktree_entries && allow_remove &&
+		    remove_file_from_cache(path))
 			return error("%s: cannot remove from the index", path);
 		return 0;
 	}

@@ -1013,6 +1015,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		{OPTION_SET_INT, 0, "no-skip-worktree", &mark_skip_worktree_only, NULL,
 			N_("clear skip-worktree bit"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
+		OPT_BOOL(0, "ignore-skip-worktree-entries", &ignore_skip_worktree_entries,
+			 N_("do not touch index-only entries")),
 		OPT_SET_INT(0, "info-only", &info_only,
 			N_("add to index only; do not add content to object database"), 1),
 		OPT_SET_INT(0, "force-remove", &force_remove,
#10 lore raw
In reply to Johannes Schindelin
Quoted 7 lines · Johannes Schindelin
Quoted 3 lines

Yeah, I do not recall seeing any of the patches in the topic (nor the cover letter). It is not clear to me what "lose" above means, which is an indication that I didn't read the topic a month ago X-<.

Sorry, in this context, "to lose a file" means that a git stash would record such a file as deleted. A subsequent git stash apply _would_ then delete it.

That would be a good thing to explain in the log message.

Thanks.

#4 lore raw

Re: [PATCH 1/2] update-index: optionally leave skip-worktree entries alone

In reply to Johannes Schindelin
Quoted 4 lines · Johannes Schindelin
Quoted 2 lines

Isn't this something reasonably easy to guard against regression with a test or two?

I sent out a v2 with tests added to 1/2.

Good; that way, not just "stash" but anything that relies on update-index would be protected from regressions.

Thanks.

#11 lore raw
In reply to Junio C Hamano

Hi Junio,

Quoted 11 lines · Junio C Hamano
Quoted 7 lines · Johannes Schindelin
Quoted 3 lines

Yeah, I do not recall seeing any of the patches in the topic (nor the cover letter). It is not clear to me what "lose" above means, which is an indication that I didn't read the topic a month ago X-<.

Sorry, in this context, "to lose a file" means that a git stash would record such a file as deleted. A subsequent git stash apply _would_ then delete it.

That would be a good thing to explain in the log message.

I thought that this paragraph in the commit message was describing this issue appropriately?

When calling git stash while changes were staged for files that are marked with the skip-worktree bit (e.g. files that are excluded in a sparse checkout), the files are recorded as _deleted_ instead.

Do you agree that this is good enough?

Ciao,
Dscho

#14 lore raw

Re: [PATCH v2 1/2] update-index: optionally leave skip-worktree entries alone

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 10 lines · writes:

From: Johannes Schindelin <[email protected]>

While git update-index mostly ignores paths referring to index entries whose skip-worktree bit is set, in b4d1690df11 (Teach Git to respect skip-worktree bit (reading part), 2009-08-20), for reasons that are not entirely obvious, the --remove option was made special: it _does_ remove index entries even if their skip-worktree bit is set.

Seeing as this behavior has been in place for a decade now, it does not make sense to change it.

If this were end-user facing Porcelain behaviour, even it is a decade old, the story would have been different, but given that it is in an obscure corner in a plumbing command, I agree that it does not make sense to even transition the default over releases.

t/t7012-skip-worktree-writing.sh test_expect_success 'git-clean, dirty case' ' · line 140

Quoted 4 lines
t/t7012-skip-worktree-writing.sh +4 -0
+test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '
+	test_commit geroff-me &&
+	git update-index --skip-worktree geroff-me.t &&
+	rm geroff-me.t &&

I do not see a need to swear with a sample file name. It may make sense to use words that relate to what the test is checking (e.g. skip-me or something like that), but otherwise meaningless filenames used in other tests (like 1, 2, etc) would be more in line with the existing tests.

t/t7012-skip-worktree-writing.sh test_expect_success 'git-clean, dirty case' ' · line 144

Quoted 4 lines
t/t7012-skip-worktree-writing.sh +4 -0
+
+	: ignoring the worktree &&
+	git update-index --remove --ignore-skip-worktree-entries geroff-me.t &&
+	git diff-index --cached --exit-code HEAD &&

HEAD has it, working tree does not, and the one in the index should have been kept thanks to the new option added by this patch. Makes sense.

t/t7012-skip-worktree-writing.sh test_expect_success 'git-clean, dirty case' ' · line 148

Quoted 3 lines
t/t7012-skip-worktree-writing.sh +3 -0
+	: not ignoring the worktree, a deletion is staged &&
+	git update-index --remove geroff-me.t &&
+	test_must_fail git diff-index --cached --exit-code HEAD

Testing the other side of the coin (i.e. adding the new feature did not accidentally stop the command from removing by default) is good; "should have no difference" was a good test for the other side, but in contrast, "should have some difference" is a very loose test when the difference we want to see is that the particular path gets removed and no other changes.

Quoted 5 lines

+'
+
#TODO test_expect_failure 'git-apply adds file' false #TODO test_expect_failure 'git-apply updates file' false #TODO test_expect_failure 'git-apply removes file' false

#19 lore raw

Re: [PATCH v2 2/2] stash: handle staged changes in skip-worktree files correctly

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 5 lines · writes:

From: Johannes Schindelin <[email protected]>

When calling git stash while changes were staged for files that are marked with the skip-worktree bit (e.g. files that are excluded in a sparse checkout), the files are recorded as _deleted_ instead.

Good. Much easier to see what is going on than the verb "lose" used in the cover letter of v1 that puzzled me.

Quoted 5 lines

However, when the temporary index is updated via git update-index --add --remove, skip-worktree entries mark the files as deleted by mistake.

Let's use the newly-introduced --ignore-skip-worktree-entries option of git update-index to prevent exactly this from happening.

Good.

Quoted 2 lines

Note that the regression test case deliberately avoids replicating the scenario described above and instead tries to recreate just the symptom.

That's good. Testing the end-user visible effect is just as important as the narrowly pointed root-cause test like the one in the previous patch.

Thanks.

#15 lore raw

Re: [PATCH v2 1/2] update-index: optionally leave skip-worktree entries alone

In reply to Junio C Hamano

Hi Junio,

Quoted 19 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 10 lines · writes:

From: Johannes Schindelin <[email protected]>

While git update-index mostly ignores paths referring to index entries whose skip-worktree bit is set, in b4d1690df11 (Teach Git to respect skip-worktree bit (reading part), 2009-08-20), for reasons that are not entirely obvious, the --remove option was made special: it _does_ remove index entries even if their skip-worktree bit is set.

Seeing as this behavior has been in place for a decade now, it does not make sense to change it.

If this were end-user facing Porcelain behaviour, even it is a decade old, the story would have been different, but given that it is in an obscure corner in a plumbing command, I agree that it does not make sense to even transition the default over releases.

Quoted 10 lines

t/t7012-skip-worktree-writing.sh test_expect_success 'git-clean, dirty case' ' · line 140

Quoted 4 lines
t/t7012-skip-worktree-writing.sh +4 -0
+test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '
+	test_commit geroff-me &&
+	git update-index --skip-worktree geroff-me.t &&
+	rm geroff-me.t &&

I do not see a need to swear with a sample file name. It may make sense to use words that relate to what the test is checking (e.g. skip-me or something like that), but otherwise meaningless filenames used in other tests (like 1, 2, etc) would be more in line with the existing tests.

I changed this locally to keep-me. But then I saw that you merged this patch pair to next already... Do you want an add-on patch, or revert it out of next, or leave as-is?

I'd like to know because I still want to merge this into Git for Windows v2.24.0-rc2, and I would love to deviate as little as possible from git.git there.

Quoted 9 lines

t/t7012-skip-worktree-writing.sh test_expect_success 'git-clean, dirty case' ' · line 144

Quoted 4 lines
t/t7012-skip-worktree-writing.sh +4 -0
+
+	: ignoring the worktree &&
+	git update-index --remove --ignore-skip-worktree-entries geroff-me.t &&
+	git diff-index --cached --exit-code HEAD &&

HEAD has it, working tree does not, and the one in the index should have been kept thanks to the new option added by this patch. Makes sense.

Quoted 10 lines

t/t7012-skip-worktree-writing.sh test_expect_success 'git-clean, dirty case' ' · line 148

Quoted 3 lines
t/t7012-skip-worktree-writing.sh +3 -0
+	: not ignoring the worktree, a deletion is staged &&
+	git update-index --remove geroff-me.t &&
+	test_must_fail git diff-index --cached --exit-code HEAD

Testing the other side of the coin (i.e. adding the new feature did not accidentally stop the command from removing by default) is good; "should have no difference" was a good test for the other side, but in contrast, "should have some difference" is a very loose test when the difference we want to see is that the particular path gets removed and no other changes.

True. I changed it to test_must_fail git rev-parse :keep-me locally (to test for the staged deletion, although it just occurred to me that I would rather want to add the --diff-filter=D option and filter by the file name to really verify that a deletion was staged), but again, I noticed that you already merged this to next...

So: revert out of next, add-on patch, or leave as-is?

Thanks,
Dscho

Quoted 8 lines
Quoted 6 lines

+'
+
#TODO test_expect_failure 'git-apply adds file' false #TODO test_expect_failure 'git-apply updates file' false #TODO test_expect_failure 'git-apply removes file' false

author
#20 lore raw

v3 cover Fix git stash with skip-worktree entries

In reply to Johannes Schindelin via GitGitGadget

[This patch pair has made it into next already, but since we're too close to v2.24.0, I think it would be good to revert it out of next and take v3 instead.]

My colleague Dan Thompson reported a bug in a sparse checkout, where git stash (after resolving merge conflicts and then making up their mind to stash the changes instead of committing them) would record files as deleted by mistake (and files that were not even in the sparse checkout's cone!).

I first considered changing the behavior of git diff-index to simply ignore skip-worktree entries. But after re-reading the documentation of the skip-worktree bit, I became convinced that this would be incorrect a fix because the command really does what it advertises to do.

Then, I briefly considered introducing a flag that would change the behavior thusly, but ended up deciding against it.

The actual problem, after all, is the git update-index call and that it heeds the --remove (but not the --add) option for skip-worktree entries. "Heeds", I should say, because the idea of the skip-worktree bit really is documented to imply that the worktree files should be considered identical to their staged versions.

So arguably, it could be considered a bug that git update-index --remove even bothers to touch skip-worktree entries. But this behavior has been in place for over 10 years, so I opted to introduce a new mode that does what git stash needs in order to fix the bug.

Changes since v2:

* Avoid a file name that some might consider rude in the test of 1/2.
* In the test of 1/2, verify explicitly that a deletion has been staged without --ignore-skip-worktree-entries.

Changes since v1:

* Added a test even for the --ignore-skip-worktree-entries option alone (i.e. without going through git stash)
* Rebased onto tg/stash-refresh-index to avoid merge conflicts in t/t3903-stash.sh.

Johannes Schindelin (2):
update-index: optionally leave skip-worktree entries alone
stash: handle staged changes in skip-worktree files correctly

 Documentation/git-update-index.txt |  6 ++++++
 builtin/stash.c                    |  5 +++--
 builtin/update-index.c             |  6 +++++-
 git-legacy-stash.sh                |  3 ++-
 t/t3903-stash.sh                   | 11 +++++++++++
 t/t7012-skip-worktree-writing.sh   | 15 +++++++++++++++
 6 files changed, 42 insertions(+), 4 deletions(-)

base-commit: 34933d0eff5d4c91fae6ad6f71a6e6a69a496ced
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-355%2Fdscho%2Ffix-stash-with-skip-worktree-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-355/dscho/fix-stash-with-skip-worktree-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/355

Range-diff vs v2:

1: 86dbb11f15 ! 1: 163b42dfa2 update-index: optionally leave skip-worktree entries alone

     @@ -81,17 +81,18 @@
       '
       
      +test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '
     -+	test_commit geroff-me &&
     -+	git update-index --skip-worktree geroff-me.t &&
     -+	rm geroff-me.t &&
     ++	test_commit keep-me &&
     ++	git update-index --skip-worktree keep-me.t &&
     ++	rm keep-me.t &&
      +
      +	: ignoring the worktree &&
     -+	git update-index --remove --ignore-skip-worktree-entries geroff-me.t &&
     ++	git update-index --remove --ignore-skip-worktree-entries keep-me.t &&
      +	git diff-index --cached --exit-code HEAD &&
      +
      +	: not ignoring the worktree, a deletion is staged &&
     -+	git update-index --remove geroff-me.t &&
     -+	test_must_fail git diff-index --cached --exit-code HEAD
     ++	git update-index --remove keep-me.t &&
     ++	test_must_fail git diff-index --cached --exit-code HEAD \
     ++		--diff-filter=D -- keep-me.t
      +'
      +
       #TODO test_expect_failure 'git-apply adds file' false

2: 9835e66399 = 2: 8f49a393e0 stash: handle staged changes in skip-worktree files correctly

Signature · 1 line
gitgitgadget
author
#21 lore raw

PATCH v3 1/2 update-index: optionally leave skip-worktree entries alone

From: Johannes Schindelin <[email protected]>

While git update-index mostly ignores paths referring to index entries whose skip-worktree bit is set, in b4d1690df11 (Teach Git to respect skip-worktree bit (reading part), 2009-08-20), for reasons that are not entirely obvious, the --remove option was made special: it _does_ remove index entries even if their skip-worktree bit is set.

Seeing as this behavior has been in place for a decade now, it does not make sense to change it.

However, in preparation for fixing a bug in git stash where it pretends that skip-worktree entries have actually been removed, we need a mode where git update-index leaves all skip-worktree entries alone, even if the --remove option was passed.

Signed-off-by Johannes Schindelin <[email protected]>
---
 Documentation/git-update-index.txt |  6 ++++++
 builtin/update-index.c             |  6 +++++-
 t/t7012-skip-worktree-writing.sh   | 15 +++++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)
Documentation/git-update-index.txt +6 -0
index 1c4d146a41..08393445e7 100644
@@ -16,6 +16,7 @@ SYNOPSIS
 	     [--chmod=(+|-)x]
 	     [--[no-]assume-unchanged]
 	     [--[no-]skip-worktree]
+	     [--[no-]ignore-skip-worktree-entries]
 	     [--[no-]fsmonitor-valid]
 	     [--ignore-submodules]
 	     [--[no-]split-index]

@@ -113,6 +114,11 @@ you will need to handle the situation manually.
 	set and unset the "skip-worktree" bit for the paths. See
 	section "Skip-worktree bit" below for more information.
 
+
+--[no-]ignore-skip-worktree-entries::
+	Do not remove skip-worktree (AKA "index-only") entries even when
+	the `--remove` option was specified.
+
 --[no-]fsmonitor-valid::
 	When one of these flags is specified, the object name recorded
 	for the paths are not updated. Instead, these options
builtin/update-index.c +5 -1
index dff2f4b837..074d563df0 100644
@@ -35,6 +35,7 @@ static int verbose;
 static int mark_valid_only;
 static int mark_skip_worktree_only;
 static int mark_fsmonitor_only;
+static int ignore_skip_worktree_entries;
 #define MARK_FLAG 1
 #define UNMARK_FLAG 2
 static struct strbuf mtime_dir = STRBUF_INIT;

@@ -381,7 +382,8 @@ static int process_path(const char *path, struct stat *st, int stat_errno)
 		 * so updating it does not make sense.
 		 * On the other hand, removing it from index should work
 		 */
-		if (allow_remove && remove_file_from_cache(path))
+		if (!ignore_skip_worktree_entries && allow_remove &&
+		    remove_file_from_cache(path))
 			return error("%s: cannot remove from the index", path);
 		return 0;
 	}

@@ -1013,6 +1015,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		{OPTION_SET_INT, 0, "no-skip-worktree", &mark_skip_worktree_only, NULL,
 			N_("clear skip-worktree bit"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
+		OPT_BOOL(0, "ignore-skip-worktree-entries", &ignore_skip_worktree_entries,
+			 N_("do not touch index-only entries")),
 		OPT_SET_INT(0, "info-only", &info_only,
 			N_("add to index only; do not add content to object database"), 1),
 		OPT_SET_INT(0, "force-remove", &force_remove,
t/t7012-skip-worktree-writing.sh +15 -0
index 9d1abe50ef..7476781979 100755
@@ -134,6 +134,21 @@ test_expect_success 'git-clean, dirty case' '
 	test_i18ncmp expected result
 '
 
+test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' '
+	test_commit keep-me &&
+	git update-index --skip-worktree keep-me.t &&
+	rm keep-me.t &&
+
+	: ignoring the worktree &&
+	git update-index --remove --ignore-skip-worktree-entries keep-me.t &&
+	git diff-index --cached --exit-code HEAD &&
+
+	: not ignoring the worktree, a deletion is staged &&
+	git update-index --remove keep-me.t &&
+	test_must_fail git diff-index --cached --exit-code HEAD \
+		--diff-filter=D -- keep-me.t
+'
+
 #TODO test_expect_failure 'git-apply adds file' false
 #TODO test_expect_failure 'git-apply updates file' false
 #TODO test_expect_failure 'git-apply removes file' false
Signature · 1 line
gitgitgadget
author
#22 lore raw

PATCH v3 2/2 stash: handle staged changes in skip-worktree files correctly

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

When calling git stash while changes were staged for files that are marked with the skip-worktree bit (e.g. files that are excluded in a sparse checkout), the files are recorded as _deleted_ instead.

The reason is that git stash tries to construct the tree reflecting the worktree essentially by copying the index to a temporary one and then updating the files from the worktree. Crucially, it calls git diff-index to update also those files that are in the HEAD but have been unstaged in the index.

However, when the temporary index is updated via git update-index --add --remove, skip-worktree entries mark the files as deleted by mistake.

Let's use the newly-introduced --ignore-skip-worktree-entries option of git update-index to prevent exactly this from happening.

Note that the regression test case deliberately avoids replicating the scenario described above and instead tries to recreate just the symptom.

Reported by Dan Thompson.

Signed-off-by Johannes Schindelin <[email protected]>
---
 builtin/stash.c     |  5 +++--
 git-legacy-stash.sh |  3 ++-
 t/t3903-stash.sh    | 11 +++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)
builtin/stash.c +3 -2
index ab30d1e920..e3962bf73e 100644
@@ -1082,8 +1082,9 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	}
 
 	cp_upd_index.git_cmd = 1;
-	argv_array_pushl(&cp_upd_index.args, "update-index", "-z", "--add",
-			 "--remove", "--stdin", NULL);
+	argv_array_pushl(&cp_upd_index.args, "update-index",
+			 "--ignore-skip-worktree-entries",
+			 "-z", "--add", "--remove", "--stdin", NULL);
 	argv_array_pushf(&cp_upd_index.env_array, "GIT_INDEX_FILE=%s",
 			 stash_index_path.buf);
git-legacy-stash.sh +2 -1
index f60e9b3e87..5398a5161d 100755
@@ -193,7 +193,8 @@ create_stash () {
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
 			git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
-			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
+			git update-index --ignore-skip-worktree-entries \
+				-z --add --remove --stdin <"$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
 		) ) ||
t/t3903-stash.sh +11 -0
index 392954d6dd..57258d5668 100755
@@ -1257,4 +1257,15 @@ test_expect_success 'stash apply should succeed with unmodified file' '
 	git stash apply
 '
 
+test_expect_success 'stash handles skip-worktree entries nicely' '
+	test_commit A &&
+	echo changed >A.t &&
+	git add A.t &&
+	git update-index --skip-worktree A.t &&
+	rm A.t &&
+	git stash &&
+
+	git rev-parse --verify refs/stash:A.t
+'
+
 test_done
Signature · 1 line
gitgitgadget
#16 lore raw

Re: [PATCH v2 1/2] update-index: optionally leave skip-worktree entries alone

In reply to Johannes Schindelin
Quoted 11 lines · Johannes Schindelin

I changed this locally to keep-me. But then I saw that you merged this patch pair to next already... Do you want an add-on patch, or revert it out of next, or leave as-is?

I'd like to know because I still want to merge this into Git for Windows v2.24.0-rc2, and I would love to deviate as little as possible from git.git there.

...
So: revert out of next, add-on patch, or leave as-is?

Sorry for a late response. I was under the weather and mostly offline for the past few days.

Whichever is easier for GGG is fine, but for the purpose of resulting history, I think revert-replace would be the best.

Thanks.

#17 lore raw

Re: [PATCH v2 1/2] update-index: optionally leave skip-worktree entries alone

Hi Junio,

Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Johannes Schindelin

I changed this locally to keep-me. But then I saw that you merged this patch pair to next already... Do you want an add-on patch, or revert it out of next, or leave as-is?

I'd like to know because I still want to merge this into Git for Windows v2.24.0-rc2, and I would love to deviate as little as possible from git.git there.

Quoted 5 lines
Quoted 2 lines

...
So: revert out of next, add-on patch, or leave as-is?

Sorry for a late response. I was under the weather and mostly offline for the past few days.

I hope you are feeling better?

Quoted 2 lines

Whichever is easier for GGG is fine, but for the purpose of resulting history, I think revert-replace would be the best.

Technically, GitGitGadget should be fine with reverting out and re-integrating. If not, that's a bug ;-)

Thanks for replacing,
Dscho