[PATCH 0/2] Fix git stash with skip-worktree entries

[PATCH 0/2] Fix git stash with skip-worktree entries

[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.

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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 Documentation/git-update-index.txt | 6 ++++++
 builtin/update-index.c             | 6 +++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index 1c4d146a41..08393445e7 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -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
diff --git a/builtin/update-index.c b/builtin/update-index.c
index dff2f4b837..074d563df0 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -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,
-- 
gitgitgadget

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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?

>
> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
> index 1c4d146a41..08393445e7 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -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
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index dff2f4b837..074d563df0 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -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,

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

[Johannes Schindelin]

Hi Junio,

On Mon, 28 Oct 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
>
> > 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.
>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  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

>
> >
> > diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
> > index 1c4d146a41..08393445e7 100644
> > --- a/Documentation/git-update-index.txt
> > +++ b/Documentation/git-update-index.txt
> > @@ -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
> > diff --git a/builtin/update-index.c b/builtin/update-index.c
> > index dff2f4b837..074d563df0 100644
> > --- a/builtin/update-index.c
> > +++ b/builtin/update-index.c
> > @@ -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,
>

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 builtin/stash.c     |  5 +++--
 git-legacy-stash.sh |  3 ++-
 t/t3903-stash.sh    | 11 +++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index b5a301f24d..56f3b551e4 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -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);
 
diff --git a/git-legacy-stash.sh b/git-legacy-stash.sh
index f60e9b3e87..5398a5161d 100755
--- a/git-legacy-stash.sh
+++ b/git-legacy-stash.sh
@@ -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"
 		) ) ||
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b8e337893f..1e977145b8 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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
-- 
gitgitgadget

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
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.

Re: [PATCH 0/2] Fix git stash with skip-worktree entries

[Johannes Schindelin]

Hi Junio,

On Thu, 26 Sep 2019, Johannes Schindelin via GitGitGadget wrote:

> 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

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

Re: [PATCH 0/2] Fix git stash with skip-worktree entries

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Thu, 26 Sep 2019, Johannes Schindelin via GitGitGadget wrote:
>
>> 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?

Re: [PATCH 0/2] Fix git stash with skip-worktree entries

[Johannes Schindelin]

Hi Junio,

On Mon, 28 Oct 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Thu, 26 Sep 2019, Johannes Schindelin via GitGitGadget wrote:
> >
> >> 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-<.

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.

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

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

Ciao,
Dscho

Re: [PATCH 0/2] Fix git stash with skip-worktree entries

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Re: [PATCH 0/2] Fix git stash with skip-worktree entries

[Johannes Schindelin]

Hi Junio,

On Tue, 29 Oct 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> 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

[PATCH v2 0/2] Fix git stash with skip-worktree entries

[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' '

-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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(-)

diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index 1c4d146a41..08393445e7 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -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
diff --git a/builtin/update-index.c b/builtin/update-index.c
index dff2f4b837..074d563df0 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -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,
diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index 9d1abe50ef..28b1b0b2b9 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -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
-- 
gitgitgadget

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

> +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.

> +
> +	: 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.

> +	: 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.

> +'
> +
>  #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

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

[Johannes Schindelin]

Hi Junio,

On Wed, 30 Oct 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
>
> > +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.

> > +
> > +	: 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.
>
> > +	: 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

>
> > +'
> > +
> >  #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
>
>

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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

[Johannes Schindelin]

Hi Junio,

On Sat, 2 Nov 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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.

I hope you are feeling better?

> 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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 builtin/stash.c     |  5 +++--
 git-legacy-stash.sh |  3 ++-
 t/t3903-stash.sh    | 11 +++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index ab30d1e920..e3962bf73e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -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);
 
diff --git a/git-legacy-stash.sh b/git-legacy-stash.sh
index f60e9b3e87..5398a5161d 100755
--- a/git-legacy-stash.sh
+++ b/git-legacy-stash.sh
@@ -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"
 		) ) ||
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 392954d6dd..57258d5668 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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
-- 
gitgitgadget

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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

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

[PATCH v3 0/2] Fix git stash with skip-worktree entries

[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

-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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(-)

diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index 1c4d146a41..08393445e7 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -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
diff --git a/builtin/update-index.c b/builtin/update-index.c
index dff2f4b837..074d563df0 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -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,
diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index 9d1abe50ef..7476781979 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -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
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 builtin/stash.c     |  5 +++--
 git-legacy-stash.sh |  3 ++-
 t/t3903-stash.sh    | 11 +++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index ab30d1e920..e3962bf73e 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -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);
 
diff --git a/git-legacy-stash.sh b/git-legacy-stash.sh
index f60e9b3e87..5398a5161d 100755
--- a/git-legacy-stash.sh
+++ b/git-legacy-stash.sh
@@ -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"
 		) ) ||
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 392954d6dd..57258d5668 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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
-- 
gitgitgadget