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

Commit Message

Johannes Schindelin via GitGitGadget Oct. 28, 2019, 11:20 a.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
 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(-)

Comments

Junio C Hamano Oct. 30, 2019, 1:13 a.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> 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

Johannes Schindelin Oct. 30, 2019, 8:34 a.m. UTC | #2

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > From: Johannes Schindelin <[e-mail]>
> >
> > 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
>
>

Junio C Hamano Nov. 2, 2019, 3:04 a.m. UTC | #3

Johannes Schindelin <[e-mail]> 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.

Johannes Schindelin Nov. 2, 2019, 11:03 p.m. UTC | #4

Hi Junio,

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

> Johannes Schindelin <[e-mail]> 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

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