Skip to content

Fix git stash with skip-worktree entries #355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dscho wants to merge 2 commits into from
Closed

Fix git stash with skip-worktree entries #355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
163b42d
update-index: optionally leave skip-worktree entries alone
dscho Sep 25, 2019
8f49a39
stash: handle staged changes in skip-worktree files correctly
dscho Sep 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Documentation/git-update-index.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ SYNOPSIS
[--chmod=(+|-)x]
Copy link

@gitgitgadget gitgitgadget bot Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link

@gitgitgadget gitgitgadget bot Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@gitgitgadget gitgitgadget bot Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[--[no-]assume-unchanged]
[--[no-]skip-worktree]
[--[no-]ignore-skip-worktree-entries]
[--[no-]fsmonitor-valid]
[--ignore-submodules]
[--[no-]split-index]
Expand Down Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions builtin/stash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
6 changes: 5 additions & 1 deletion builtin/update-index.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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,
Expand Down
3 changes: 2 additions & 1 deletion git-legacy-stash.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
) ) ||
Expand Down
11 changes: 11 additions & 0 deletions t/t3903-stash.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
15 changes: 15 additions & 0 deletions t/t7012-skip-worktree-writing.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down