Fix `git stash` with skip-worktree entries #355

dscho · 2019-09-25T21:29:58Z

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

dscho · 2019-09-26T07:41:37Z

/submit

gitgitgadget · 2019-09-26T07:42:51Z

Submitted as [email protected]

WARNING: dscho has no public email address set on GitHub

gitgitgadget · 2019-10-27T21:41:57Z

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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 fi=
les
> 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 ign=
ore
> 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 beha=
vior
> 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 identic=
al
> 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 wh=
at
> 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%2F=
dscho%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
>
>

gitgitgadget · 2019-10-28T02:39:01Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> 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?

gitgitgadget · 2019-10-28T04:45:08Z

Documentation/git-update-index.txt

@@ -16,6 +16,7 @@ SYNOPSIS
 	     [--chmod=(+|-)x]


On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]> 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. > 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 <[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? > > 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,

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio, On Mon, 28 Oct 2019, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <[email protected]> > writes: > > > From: Johannes Schindelin <[email protected]> > > > > While `git update-index` mostly ignores paths referring to index entri= es > > whose skip-worktree bit is set, in b4d1690df11 (Teach Git to respect > > skip-worktree bit (reading part), 2009-08-20), for reasons that are no= t > > 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 nee= d > > 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 <[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 > > > > > diff --git a/Documentation/git-update-index.txt b/Documentation/git-up= date-index.txt > > index 1c4d146a41..08393445e7 100644 > > --- a/Documentation/git-update-index.txt > > +++ b/Documentation/git-update-index.txt > > @@ -16,6 +16,7 @@ SYNOPSIS > > [--chmod=3D(+|-)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 =3D STRBUF_INIT; > > @@ -381,7 +382,8 @@ static int process_path(const char *path, struct s= tat *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, N= ULL, > > N_("clear skip-worktree bit"), > > PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG}, > > + OPT_BOOL(0, "ignore-skip-worktree-entries", &ignore_skip_worktree_e= ntries, > > + 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, >

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> 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.

gitgitgadget · 2019-10-28T05:36:54Z

This branch is now known as js/update-index-ignore-removal-for-skip-worktree.

gitgitgadget · 2019-10-28T05:36:56Z

This patch series was integrated into pu via git@50f1da8.

builtin/stash.c

dscho · 2019-10-28T11:19:15Z

/submit

gitgitgadget · 2019-10-28T11:20:34Z

Submitted as [email protected]

gitgitgadget · 2019-10-29T04:33:56Z

This patch series was integrated into pu via git@46887b2.

Documentation/git-update-index.txt

builtin/stash.c

gitgitgadget · 2019-10-30T07:47:22Z

This patch series was integrated into pu via git@f781631.

gitgitgadget · 2019-10-30T07:47:23Z

This patch series was integrated into next via git@5c7c7a6.

While `git update-index` mostly ignores paths referring to index entries whose skip-worktree bit is set, in b4d1690 (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]>

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

dscho · 2019-10-30T10:48:48Z

/submit

gitgitgadget · 2019-10-30T10:49:54Z

Submitted as [email protected]

gitgitgadget · 2019-11-02T10:37:25Z

This patch series was integrated into pu via git@0b28041.

gitgitgadget · 2019-11-02T10:37:27Z

This patch series was integrated into next via git@57f7fb8.

gitgitgadget · 2019-11-04T05:33:28Z

This patch series was integrated into pu via git@9bba03a.

gitgitgadget · 2019-11-06T13:37:44Z

This patch series was integrated into pu via git@8585c16.

gitgitgadget · 2019-11-07T11:14:42Z

This patch series was integrated into pu via git@84d448f.

gitgitgadget · 2019-11-08T08:48:04Z

This patch series was integrated into pu via git@512a3ec.

gitgitgadget · 2019-11-10T08:58:51Z

This patch series was integrated into pu via git@31d847d.

gitgitgadget · 2019-11-11T04:50:33Z

This patch series was integrated into pu via git@57b5301.

gitgitgadget · 2019-11-11T04:50:35Z

This patch series was integrated into master via git@57b5301.

gitgitgadget · 2019-11-11T04:50:37Z

Closed via 57b5301.

dscho force-pushed the fix-stash-with-skip-worktree branch from fd62a5e to 4c684be Compare September 25, 2019 21:31

dscho changed the title ~~Fix stash with skip worktree~~ Fix git stash with skip-worktree entries Sep 25, 2019

dscho added the ready to submit Has commits that have not been submitted yet label Sep 25, 2019

dscho removed the ready to submit Has commits that have not been submitted yet label Sep 26, 2019

weekly-digest bot mentioned this pull request Sep 29, 2019

Weekly Digest (22 September, 2019 - 29 September, 2019) #366

Closed

gitgitgadget bot reviewed Oct 28, 2019

View reviewed changes

gitgitgadget bot added the pu label Oct 28, 2019

gitgitgadget bot reviewed Oct 28, 2019

View reviewed changes

builtin/stash.c Show resolved Hide resolved

dscho force-pushed the fix-stash-with-skip-worktree branch from 4c684be to 1654cba Compare October 28, 2019 10:32

dscho changed the base branch from master to tg/stash-refresh-index October 28, 2019 10:37

dscho force-pushed the fix-stash-with-skip-worktree branch from 1654cba to 9835e66 Compare October 28, 2019 10:41

dscho mentioned this pull request Oct 28, 2019

Fix git stash with skip-worktree entries git-for-windows/git#2378

Merged

This comment has been minimized.

Sign in to view

gitgitgadget bot reviewed Oct 30, 2019

View reviewed changes

Documentation/git-update-index.txt Show resolved Hide resolved

gitgitgadget bot reviewed Oct 30, 2019

View reviewed changes

builtin/stash.c Show resolved Hide resolved

gitgitgadget bot added the next label Oct 30, 2019

dscho force-pushed the fix-stash-with-skip-worktree branch from 9835e66 to c4b7b8c Compare October 30, 2019 10:04

dscho added 2 commits October 30, 2019 11:11

dscho force-pushed the fix-stash-with-skip-worktree branch from c4b7b8c to 8f49a39 Compare October 30, 2019 10:12

gitgitgadget bot added the master label Nov 11, 2019

gitgitgadget bot closed this Nov 11, 2019

dscho deleted the fix-stash-with-skip-worktree branch November 11, 2019 10:56

Fix git stash with skip-worktree entries #355

Fix git stash with skip-worktree entries #355

Uh oh!

Conversation

dscho commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Sep 26, 2019

Uh oh!

gitgitgadget bot commented Sep 26, 2019

Uh oh!

gitgitgadget bot commented Oct 27, 2019

Uh oh!

gitgitgadget bot commented Oct 28, 2019

Uh oh!

gitgitgadget bot Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot commented Oct 28, 2019

Uh oh!

gitgitgadget bot commented Oct 28, 2019

Uh oh!

Uh oh!

dscho commented Oct 28, 2019

Uh oh!

gitgitgadget bot commented Oct 28, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

gitgitgadget bot commented Oct 29, 2019

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

gitgitgadget bot commented Oct 30, 2019

Uh oh!

gitgitgadget bot commented Oct 30, 2019

Uh oh!

dscho commented Oct 30, 2019

Uh oh!

gitgitgadget bot commented Oct 30, 2019

Uh oh!

gitgitgadget bot commented Nov 2, 2019

Uh oh!

gitgitgadget bot commented Nov 2, 2019

Uh oh!

gitgitgadget bot commented Nov 4, 2019

Uh oh!

gitgitgadget bot commented Nov 6, 2019

Uh oh!

gitgitgadget bot commented Nov 7, 2019

Uh oh!

gitgitgadget bot commented Nov 8, 2019

Uh oh!

gitgitgadget bot commented Nov 10, 2019

Uh oh!

gitgitgadget bot commented Nov 11, 2019

Uh oh!

gitgitgadget bot commented Nov 11, 2019

Uh oh!

gitgitgadget bot commented Nov 11, 2019

Uh oh!

Uh oh!

Fix `git stash` with skip-worktree entries #355

Fix `git stash` with skip-worktree entries #355

dscho commented Sep 25, 2019 •

edited

Loading