[PATCH 0/8] introduce no-overlay and cached mode in git checkout

Here's the series I mentioned a couple of times on the list already,
introducing a no-overlay mode in 'git checkout'.  The inspiration for
this came from Junios message in [*1*].

Basically the idea is to also delete files when the match <pathspec>
in 'git checkout <tree-ish> -- <pathspec>' in the current tree, but
don't match <pathspec> in <tree-ish>.  The rest of the cases are
already properly taken care of by 'git checkout'.

The final step in the series is to actually make use of this in 'git
stash', which simplifies the code there a bit.  I am however happy to
hold off on this step until the stash-in-C series is merged, so we
don't delay that further.

In addition to the no-overlay mode, we also add a --cached mode, which
works only on the index, thus similar to 'git reset <tree-ish> -- <pathspec>'.

Actually deprecating 'git reset <tree-ish> -- <pathspec>' should come
later, probably not before Duy's restore-files command lands, as 'git
checkout --no-overlay <tree-ish> -- <pathspec>' is a bit cumbersome to
type compared to 'git reset <tree-ish> -- <pathspec>'.

My hope is also that the no-overlay mode could become the new default
in the restore-files command Duy is currently working on.

No documentation yet, as I wanted to get this out for review first.
I'm not familiar with most of the code I touched here, so there may
well be much better ways to implement some of this, that I wasn't able
to figure out.  I'd be very happy with some feedback around that.

Another thing I'm not sure about is how to deal with conflicts.  In
the cached mode this patch series is not dealing with it at all, as
'git checkout -- <pathspec>' when pathspec matches a file with
conflicts doesn't update the index.  For the no-overlay mode, the file
is removed if the corresponding stage is not found in the index.  I'm
however not sure this is the right thing to do in all cases?

*1*: [off-list ref]

Thomas Gummerer (8):
  move worktree tests to t24*
  entry: factor out unlink_entry function
  entry: support CE_WT_REMOVE flag in checkout_entry
  read-cache: add invalidate parameter to remove_marked_cache_entries
  checkout: introduce --{,no-}overlay option
  checkout: add --cached option
  checkout: add allow ignoring unmatched pathspec
  stash: use git checkout --index

 builtin/checkout.c                            |  66 +++++++++--
 cache.h                                       |   7 +-
 entry.c                                       |  22 ++++
 git-stash.sh                                  |  12 +-
 read-cache.c                                  |   8 +-
 split-index.c                                 |   2 +-
 t/t2016-checkout-patch.sh                     |   8 ++
 t/t2022-checkout-paths.sh                     |   9 ++
 t/t2025-checkout-no-overlay.sh                |  31 ++++++
 t/t2026-checkout-cached.sh                    | 103 ++++++++++++++++++
 ...-worktree-add.sh => t2400-worktree-add.sh} |   0
 ...ktree-prune.sh => t2401-worktree-prune.sh} |   0
 ...orktree-list.sh => t2402-worktree-list.sh} |   0
 t/t9902-completion.sh                         |   3 +
 unpack-trees.c                                |  21 +---
 15 files changed, 251 insertions(+), 41 deletions(-)
 create mode 100755 t/t2025-checkout-no-overlay.sh
 create mode 100755 t/t2026-checkout-cached.sh
 rename t/{t2025-worktree-add.sh => t2400-worktree-add.sh} (100%)
 rename t/{t2026-worktree-prune.sh => t2401-worktree-prune.sh} (100%)
 rename t/{t2027-worktree-list.sh => t2402-worktree-list.sh} (100%)

-- 
2.20.0.rc2.411.g8f28e744c2

The 'git worktree' command used to be just another mode in 'git
checkout', namely 'git checkout --to'.  When the tests for the latter
were retrofitted for the former, the test name was adjusted, but the
test number was kept, even though the test is testing a different
command now.  t/README states: "Second digit tells the particular
command we are testing.", so 'git worktree' should have a separate
number just for itself.

Move the worktree tests to t24* to adhere to that guideline. We're
going to make use of the free'd up numbers in a subsequent commit.

Signed-off-by: Thomas Gummerer <redacted>
---
 t/{t2025-worktree-add.sh => t2400-worktree-add.sh}     | 0
 t/{t2026-worktree-prune.sh => t2401-worktree-prune.sh} | 0
 t/{t2027-worktree-list.sh => t2402-worktree-list.sh}   | 0
 3 files changed, 0 insertions(+), 0 deletions(-)
 rename t/{t2025-worktree-add.sh => t2400-worktree-add.sh} (100%)
 rename t/{t2026-worktree-prune.sh => t2401-worktree-prune.sh} (100%)
 rename t/{t2027-worktree-list.sh => t2402-worktree-list.sh} (100%)

diff --git a/t/t2025-worktree-add.sh b/t/t2400-worktree-add.sh
similarity index 100%
rename from t/t2025-worktree-add.sh
rename to t/t2400-worktree-add.sh
diff --git a/t/t2026-worktree-prune.sh b/t/t2401-worktree-prune.sh
similarity index 100%
rename from t/t2026-worktree-prune.sh
rename to t/t2401-worktree-prune.sh
diff --git a/t/t2027-worktree-list.sh b/t/t2402-worktree-list.sh
similarity index 100%
rename from t/t2027-worktree-list.sh
rename to t/t2402-worktree-list.sh
-- 
2.20.0.405.gbc1bbc6f85

'checkout_entry()' currently only supports creating new entries in the
working tree, but not deleting them.  Add the ability to remove
entries at the same time if the entry is marked with the CE_WT_REMOVE
flag.

Currently this doesn't have any effect, as the CE_WT_REMOVE flag is
only used in unpack-tree, however we will make use of this in a
subsequent step in the series.

Signed-off-by: Thomas Gummerer <redacted>
---
 entry.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/entry.c b/entry.c
index 3ec148ceee..cd1c6601b6 100644
--- a/entry.c
+++ b/entry.c

@@ -441,6 +441,13 @@ int checkout_entry(struct cache_entry *ce,
 	static struct strbuf path = STRBUF_INIT;
 	struct stat st;
 
+	if (ce->ce_flags & CE_WT_REMOVE) {
+		if (topath)
+			BUG("Can't remove entry to a path");
+		unlink_entry(ce);
+		return 0;
+	}
+
 	if (topath)
 		return write_entry(ce, topath, state, 1);
 

-- 
2.20.0.405.gbc1bbc6f85

Factor out the 'unlink_entry()' function from unpack-trees.c to
entry.c.  It will be used in other places as well in subsequent
steps.

As it's no longer a static function, also move the documentation to
the header file to make it more discoverable.

Signed-off-by: Thomas Gummerer <redacted>
---
 cache.h        |  5 +++++
 entry.c        | 15 +++++++++++++++
 unpack-trees.c | 19 -------------------
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/cache.h b/cache.h
index ca36b44ee0..c1c953e810 100644
--- a/cache.h
+++ b/cache.h

@@ -1542,6 +1542,11 @@ struct checkout {
 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
 extern void enable_delayed_checkout(struct checkout *state);
 extern int finish_delayed_checkout(struct checkout *state);
+/*
+ * Unlink the last component and schedule the leading directories for
+ * removal, such that empty directories get removed.
+ */
+extern void unlink_entry(const struct cache_entry *ce);
 
 struct cache_def {
 	struct strbuf path;

diff --git a/entry.c b/entry.c
index 5d136c5d55..3ec148ceee 100644
--- a/entry.c
+++ b/entry.c

@@ -508,3 +508,18 @@ int checkout_entry(struct cache_entry *ce,
 	create_directories(path.buf, path.len, state);
 	return write_entry(ce, path.buf, state, 0);
 }
+
+void unlink_entry(const struct cache_entry *ce)
+{
+	const struct submodule *sub = submodule_from_ce(ce);
+	if (sub) {
+		/* state.force is set at the caller. */
+		submodule_move_head(ce->name, "HEAD", NULL,
+				    SUBMODULE_MOVE_HEAD_FORCE);
+	}
+	if (!check_leading_path(ce->name, ce_namelen(ce)))
+		return;
+	if (remove_or_warn(ce->ce_mode, ce->name))
+		return;
+	schedule_dir_for_removal(ce->name, ce_namelen(ce));
+}

diff --git a/unpack-trees.c b/unpack-trees.c
index 7570df481b..e8d1a6ac50 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -300,25 +300,6 @@ static void load_gitmodules_file(struct index_state *index,
 	}
 }
 
-/*
- * Unlink the last component and schedule the leading directories for
- * removal, such that empty directories get removed.
- */
-static void unlink_entry(const struct cache_entry *ce)
-{
-	const struct submodule *sub = submodule_from_ce(ce);
-	if (sub) {
-		/* state.force is set at the caller. */
-		submodule_move_head(ce->name, "HEAD", NULL,
-				    SUBMODULE_MOVE_HEAD_FORCE);
-	}
-	if (!check_leading_path(ce->name, ce_namelen(ce)))
-		return;
-	if (remove_or_warn(ce->ce_mode, ce->name))
-		return;
-	schedule_dir_for_removal(ce->name, ce_namelen(ce));
-}
-
 static struct progress *get_progress(struct unpack_trees_options *o)
 {
 	unsigned cnt = 0, total = 0;

-- 
2.20.0.405.gbc1bbc6f85

When marking cache entries for removal, and later removing them all at
once using 'remove_marked_cache_entries()', cache entries currently
have to be invalidated manually in the cache tree and in the untracked
cache.

Add an invalidate flag to the function.  With the flag set, the
function will take care of invalidating the path in the cache tree and
in the untracked cache.

This will be useful in a subsequent commit.

Signed-off-by: Thomas Gummerer <redacted>
---

For the two current callsites, unpack-trees seems to do this
invalidation itself internally.  I don't quite understand why we don't
need it in split-index mode though.  I assume it's because the cache
tree in the main index would already have been invalidated?  I didn't
have much time to dig, but couldn't produce any failures with it
either, so I assume not invalidating paths is the right thing to do
here.

 cache.h        | 2 +-
 read-cache.c   | 8 +++++++-
 split-index.c  | 2 +-
 unpack-trees.c | 2 +-
 4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index c1c953e810..1deee48f5b 100644
--- a/cache.h
+++ b/cache.h

@@ -751,7 +751,7 @@ extern void rename_index_entry_at(struct index_state *, int pos, const char *new
 /* Remove entry, return true if there are more entries to go. */
 extern int remove_index_entry_at(struct index_state *, int pos);
 
-extern void remove_marked_cache_entries(struct index_state *istate);
+extern void remove_marked_cache_entries(struct index_state *istate, int invalidate);
 extern int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_VERBOSE 1
 #define ADD_CACHE_PRETEND 2

diff --git a/read-cache.c b/read-cache.c
index 4ca81286c0..d86a06acba 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -590,13 +590,19 @@ int remove_index_entry_at(struct index_state *istate, int pos)
  * CE_REMOVE is set in ce_flags.  This is much more effective than
  * calling remove_index_entry_at() for each entry to be removed.
  */
-void remove_marked_cache_entries(struct index_state *istate)
+void remove_marked_cache_entries(struct index_state *istate, int invalidate)
 {
 	struct cache_entry **ce_array = istate->cache;
 	unsigned int i, j;
 
 	for (i = j = 0; i < istate->cache_nr; i++) {
 		if (ce_array[i]->ce_flags & CE_REMOVE) {
+			if (invalidate) {
+				cache_tree_invalidate_path(istate,
+							   ce_array[i]->name);
+				untracked_cache_remove_from_index(istate,
+								  ce_array[i]->name);
+			}
 			remove_name_hash(istate, ce_array[i]);
 			save_or_free_index_entry(istate, ce_array[i]);
 		}

diff --git a/split-index.c b/split-index.c
index 5820412dc5..8aebc3661b 100644
--- a/split-index.c
+++ b/split-index.c

@@ -162,7 +162,7 @@ void merge_base_index(struct index_state *istate)
 	ewah_each_bit(si->replace_bitmap, replace_entry, istate);
 	ewah_each_bit(si->delete_bitmap, mark_entry_for_delete, istate);
 	if (si->nr_deletions)
-		remove_marked_cache_entries(istate);
+		remove_marked_cache_entries(istate, 0);
 
 	for (i = si->nr_replacements; i < si->saved_cache_nr; i++) {
 		if (!ce_namelen(si->saved_cache[i]))

diff --git a/unpack-trees.c b/unpack-trees.c
index e8d1a6ac50..8e6afa924d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -392,7 +392,7 @@ static int check_updates(struct unpack_trees_options *o)
 				unlink_entry(ce);
 		}
 	}
-	remove_marked_cache_entries(index);
+	remove_marked_cache_entries(index, 0);
 	remove_scheduled_dirs();
 
 	if (should_update_submodules() && o->update && !o->dry_run)

-- 
2.20.0.405.gbc1bbc6f85

Currently 'git checkout' is defined as an overlay operation, which
means that if in 'git checkout <tree-ish> -- [<pathspec>]' we have an
entry in the index that matches <pathspec>, but that doesn't exist in
<tree-ish>, that entry will not be removed from the index or the
working tree.

Introduce a new --{,no-}overlay option, which allows using 'git
checkout' in non-overlay mode, thus removing files from the working
tree if they do not exist in <tree-ish> but match <pathspec>.

Note that 'git checkout -p <tree-ish> -- [<pathspec>]' already works
this way, so no changes are needed for the patch mode.  We disallow
'git checkout --overlay -p' to avoid confusing users who would expect
to be able to force overlay mode in 'git checkout -p' this way.

Signed-off-by: Thomas Gummerer <redacted>
---
 builtin/checkout.c             | 64 +++++++++++++++++++++++++++-------
 t/t2025-checkout-no-overlay.sh | 47 +++++++++++++++++++++++++
 t/t9902-completion.sh          |  1 +
 3 files changed, 99 insertions(+), 13 deletions(-)
 create mode 100755 t/t2025-checkout-no-overlay.sh

diff --git a/builtin/checkout.c b/builtin/checkout.c
index acdafc6e4c..0aef35bbc4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -44,6 +44,7 @@ struct checkout_opts {
 	int ignore_skipworktree;
 	int ignore_other_worktrees;
 	int show_progress;
+	int overlay_mode;
 	/*
 	 * If new checkout options are added, skip_merge_working_tree
 	 * should be updated accordingly.

@@ -132,7 +133,8 @@ static int skip_same_name(const struct cache_entry *ce, int pos)
 	return pos;
 }
 
-static int check_stage(int stage, const struct cache_entry *ce, int pos)
+static int check_stage(int stage, const struct cache_entry *ce, int pos,
+		       int overlay_mode)
 {
 	while (pos < active_nr &&
 	       !strcmp(active_cache[pos]->name, ce->name)) {

@@ -140,6 +142,8 @@ static int check_stage(int stage, const struct cache_entry *ce, int pos)
 			return 0;
 		pos++;
 	}
+	if (!overlay_mode)
+		return 0;
 	if (stage == 2)
 		return error(_("path '%s' does not have our version"), ce->name);
 	else

@@ -165,7 +169,7 @@ static int check_stages(unsigned stages, const struct cache_entry *ce, int pos)
 }
 
 static int checkout_stage(int stage, const struct cache_entry *ce, int pos,
-			  const struct checkout *state)
+			  const struct checkout *state, int overlay_mode)
 {
 	while (pos < active_nr &&
 	       !strcmp(active_cache[pos]->name, ce->name)) {

@@ -173,6 +177,10 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos,
 			return checkout_entry(active_cache[pos], state, NULL);
 		pos++;
 	}
+	if (!overlay_mode) {
+		unlink_entry(ce);
+		return 0;
+	}
 	if (stage == 2)
 		return error(_("path '%s' does not have our version"), ce->name);
 	else

@@ -302,15 +310,29 @@ static int checkout_paths(const struct checkout_opts *opts,
 		ce->ce_flags &= ~CE_MATCHED;
 		if (!opts->ignore_skipworktree && ce_skip_worktree(ce))
 			continue;
-		if (opts->source_tree && !(ce->ce_flags & CE_UPDATE))
-			/*
-			 * "git checkout tree-ish -- path", but this entry
-			 * is in the original index; it will not be checked
-			 * out to the working tree and it does not matter
-			 * if pathspec matched this entry.  We will not do
-			 * anything to this entry at all.
-			 */
-			continue;
+		if (opts->source_tree && !(ce->ce_flags & CE_UPDATE)) {
+			if (!opts->overlay_mode &&
+			    ce_path_match(&the_index, ce, &opts->pathspec, ps_matched)) {
+				/*
+				 * "git checkout --no-overlay <tree-ish> -- path",
+				 * and the path is not in tree-ish, but is in
+				 * the current index, which means that it should 
+				 * be removed.
+				 */
+				ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE;
+				continue;
+			} else {
+				/*
+				 * "git checkout tree-ish -- path", but this
+				 * entry is in the original index; it will not
+				 * be checked out to the working tree and it
+				 * does not matter if pathspec matched this
+				 * entry.  We will not do anything to this entry
+				 * at all.
+				 */
+				continue;
+			}
+		}
 		/*
 		 * Either this entry came from the tree-ish we are
 		 * checking the paths out of, or we are checking out

@@ -348,7 +370,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 			if (opts->force) {
 				warning(_("path '%s' is unmerged"), ce->name);
 			} else if (opts->writeout_stage) {
-				errs |= check_stage(opts->writeout_stage, ce, pos);
+				errs |= check_stage(opts->writeout_stage, ce, pos, opts->overlay_mode);
 			} else if (opts->merge) {
 				errs |= check_stages((1<<2) | (1<<3), ce, pos);
 			} else {

@@ -375,12 +397,14 @@ static int checkout_paths(const struct checkout_opts *opts,
 				continue;
 			}
 			if (opts->writeout_stage)
-				errs |= checkout_stage(opts->writeout_stage, ce, pos, &state);
+				errs |= checkout_stage(opts->writeout_stage, ce, pos, &state, opts->overlay_mode);
 			else if (opts->merge)
 				errs |= checkout_merged(pos, &state);
 			pos = skip_same_name(ce, pos) - 1;
 		}
 	}
+	remove_marked_cache_entries(&the_index, 1);
+	remove_scheduled_dirs();
 	errs |= finish_delayed_checkout(&state);
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))

@@ -542,6 +566,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
 	 * opts->show_progress only impacts output so doesn't require a merge
 	 */
 
+	/*
+	 * opts->overlay_mode cannot be used with switching branches so is
+	 * not tested here
+	 */
+
 	/*
 	 * If we aren't creating a new branch any changes or updates will
 	 * happen in the existing branch.  Since that could only be updating

@@ -1178,6 +1207,10 @@ static int checkout_branch(struct checkout_opts *opts,
 		die(_("'%s' cannot be used with switching branches"),
 		    "--patch");
 
+	if (!opts->overlay_mode)
+		die(_("'%s' cannot be used with switching branches"),
+		    "--no-overlay");
+
 	if (opts->writeout_stage)
 		die(_("'%s' cannot be used with switching branches"),
 		    "--ours/--theirs");

@@ -1266,6 +1299,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			    "checkout", "control recursive updating of submodules",
 			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },
 		OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
+		OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
 		OPT_END(),
 	};
 

@@ -1274,6 +1308,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	opts.overwrite_ignore = 1;
 	opts.prefix = prefix;
 	opts.show_progress = -1;
+	opts.overlay_mode = -1;
 
 	git_config(git_checkout_config, &opts);
 

@@ -1297,6 +1332,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)
 		die(_("-b, -B and --orphan are mutually exclusive"));
 
+	if (opts.overlay_mode == 1 && opts.patch_mode)
+		die(_("-p and --overlay are mutually exclusive"));
+
 	/*
 	 * From here on, new_branch will contain the branch to be checked out,
 	 * and new_branch_force and new_orphan_branch will tell us which one of

diff --git a/t/t2025-checkout-no-overlay.sh b/t/t2025-checkout-no-overlay.sh
new file mode 100755
index 0000000000..3575321382
--- /dev/null
+++ b/t/t2025-checkout-no-overlay.sh

@@ -0,0 +1,47 @@
+#!/bin/sh
+
+test_description='checkout --no-overlay <tree-ish> -- <pathspec>'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	git commit --allow-empty -m "initial"
+'
+
+test_expect_success 'checkout --no-overlay deletes files not in <tree>' '
+	>file &&
+	mkdir dir &&
+	>dir/file1 &&
+	git add file dir/file1 &&
+	git checkout --no-overlay HEAD -- file &&
+	test_path_is_missing file &&
+	test_path_is_file dir/file1
+'
+
+test_expect_success 'checkout --no-overlay removing last file from directory' '
+	git checkout --no-overlay HEAD -- dir/file1 &&
+	test_path_is_missing dir
+'
+
+test_expect_success 'checkout -p --overlay is disallowed' '
+	test_must_fail git checkout -p --overlay HEAD 2>actual &&
+	test_i18ngrep "fatal: -p and --overlay are mutually exclusive" actual
+'
+
+test_expect_success '--no-overlay --theirs with M/D conflict deletes file' '
+	test_commit file1 file1 &&
+	test_commit file2 file2 &&
+	git rm --cached file1 &&
+	echo 1234 >file1 &&
+	F1=$(git rev-parse HEAD:file1) &&
+	F2=$(git rev-parse HEAD:file2) &&
+	{
+		echo "100644 $F1 1	file1" &&
+		echo "100644 $F2 2	file1"
+	} | git update-index --index-info &&
+	test_path_is_file file1 &&
+	git checkout --theirs --no-overlay -- file1 &&
+	test_path_is_missing file1
+'
+
+test_done

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 175f83d704..a3fd9a9630 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh

@@ -1436,6 +1436,7 @@ test_expect_success 'double dash "git checkout"' '
 	--progress Z
 	--no-quiet Z
 	--no-... Z
+	--overlay Z
 	EOF
 '
 

-- 
2.20.0.405.gbc1bbc6f85

Add a new --cached option to git checkout, which works only on the
index, but not the working tree, similar to what 'git reset <tree-ish>
-- <pathspec>... does.  Indeed the tests are adapted from the 'git
reset' tests.

In the longer term the idea is to potentially deprecate 'git reset
<tree-ish> -- <pathspec>...', so the 'git reset' command becomes only
about re-pointing the HEAD, and not also about copying entries from
<tree-ish> to the index.

Note that 'git checkout' by default works in overlay mode, meaning
files that match the pathspec that don't exist in <tree-ish>, but
exist in the index would not be removed.  'git checkout --no-overlay
--cached' can be used to get the same behaviour as 'git reset
<tree-ish> -- <pathspec>'.

One thing this patch doesn't currently deal with is conflicts.
Currently 'git checkout --{ours,theirs} -- <file-with-conflicts>'
doesn't do anything with the index, so the --cached option just
mirrors that behaviour.  But given it doesn't even deal with
conflicts, the '--cached' option doesn't make much sense when no
<tree-ish> is given.  As it operates only on the index, it's always a
no-op if no tree-ish is given.

Signed-off-by: Thomas Gummerer <redacted>
---

Maybe we can just disallow --cached without <tree-ish> given for now,
and possibly later allow it with some different behaviour for
conflicts, not sure what the best way forward here is.  We can also
just make it update the index as appropriate, and have it behave
different than 'git checkout' curerntly does when handling conflicts?

 builtin/checkout.c         |  26 ++++++++--
 t/t2016-checkout-patch.sh  |   8 +++
 t/t2026-checkout-cached.sh | 103 +++++++++++++++++++++++++++++++++++++
 t/t9902-completion.sh      |   1 +
 4 files changed, 135 insertions(+), 3 deletions(-)
 create mode 100755 t/t2026-checkout-cached.sh

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0aef35bbc4..6ba85e9de5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -45,6 +45,7 @@ struct checkout_opts {
 	int ignore_other_worktrees;
 	int show_progress;
 	int overlay_mode;
+	int cached;
 	/*
 	 * If new checkout options are added, skip_merge_working_tree
 	 * should be updated accordingly.

@@ -288,6 +289,10 @@ static int checkout_paths(const struct checkout_opts *opts,
 		die(_("Cannot update paths and switch to branch '%s' at the same time."),
 		    opts->new_branch);
 
+	if (opts->patch_mode && opts->cached)
+		return run_add_interactive(revision, "--patch=reset",
+					   &opts->pathspec);
+
 	if (opts->patch_mode)
 		return run_add_interactive(revision, "--patch=checkout",
 					   &opts->pathspec);

@@ -319,7 +324,9 @@ static int checkout_paths(const struct checkout_opts *opts,
 				 * the current index, which means that it should 
 				 * be removed.
 				 */
-				ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE;
+				ce->ce_flags |= CE_MATCHED | CE_REMOVE;
+				if (!opts->cached)
+					ce->ce_flags |= CE_WT_REMOVE;
 				continue;
 			} else {
 				/*

@@ -392,6 +399,9 @@ static int checkout_paths(const struct checkout_opts *opts,
 	for (pos = 0; pos < active_nr; pos++) {
 		struct cache_entry *ce = active_cache[pos];
 		if (ce->ce_flags & CE_MATCHED) {
+			if (opts->cached) {
+				continue;
+			}
 			if (!ce_stage(ce)) {
 				errs |= checkout_entry(ce, &state, NULL);
 				continue;

@@ -571,6 +581,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
 	 * not tested here
 	 */
 
+	/*
+	 * opts->cached cannot be used with switching branches so is
+	 * not tested here
+	 */
+
 	/*
 	 * If we aren't creating a new branch any changes or updates will
 	 * happen in the existing branch.  Since that could only be updating

@@ -1207,9 +1222,13 @@ static int checkout_branch(struct checkout_opts *opts,
 		die(_("'%s' cannot be used with switching branches"),
 		    "--patch");
 
-	if (!opts->overlay_mode)
+	if (opts->overlay_mode != -1)
+		die(_("'%s' cannot be used with switching branches"),
+		    "--overlay/--no-overlay");
+
+	if (opts->cached)
 		die(_("'%s' cannot be used with switching branches"),
-		    "--no-overlay");
+		    "--cached");
 
 	if (opts->writeout_stage)
 		die(_("'%s' cannot be used with switching branches"),

@@ -1300,6 +1319,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },
 		OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
 		OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
+		OPT_BOOL(0, "cached", &opts.cached, N_("work on the index only")),
 		OPT_END(),
 	};
 

diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 47aeb0b167..e8774046e0 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh

@@ -108,6 +108,14 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
 	verify_state dir/foo head head
 '
 
+test_expect_success PERL 'git checkout --cached -p' '
+	set_and_save_state dir/foo work work &&
+	test_write_lines n y | git checkout --cached -p >output &&
+	verify_state dir/foo work head &&
+	verify_saved_state bar &&
+	test_i18ngrep "Unstage" output
+'
+
 test_expect_success PERL 'none of this moved HEAD' '
 	verify_saved_head
 '

diff --git a/t/t2026-checkout-cached.sh b/t/t2026-checkout-cached.sh
new file mode 100755
index 0000000000..1b66192727
--- /dev/null
+++ b/t/t2026-checkout-cached.sh

@@ -0,0 +1,103 @@
+#!/bin/sh
+
+test_description='checkout --cached <pathspec>'
+
+. ./test-lib.sh
+
+test_expect_success 'checkout --cached <pathspec>' '
+	echo 1 >file1 &&
+	echo 2 >file2 &&
+	git add file1 file2 &&
+	test_tick &&
+	git commit -m files &&
+	git rm file2 &&
+	echo 3 >file3 &&
+	echo 4 >file1 &&
+	git add file1 file3 &&
+	git checkout --cached HEAD -- file1 file2 &&
+	test_must_fail git diff --quiet &&
+
+	cat >expect <<-\EOF &&
+	diff --git a/file1 b/file1
+	index d00491f..b8626c4 100644
+	--- a/file1
+	+++ b/file1
+	@@ -1 +1 @@
+	-1
+	+4
+	diff --git a/file2 b/file2
+	deleted file mode 100644
+	index 0cfbf08..0000000
+	--- a/file2
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-2
+	EOF
+	git diff >actual &&
+	test_cmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	diff --git a/file3 b/file3
+	new file mode 100644
+	index 0000000..00750ed
+	--- /dev/null
+	+++ b/file3
+	@@ -0,0 +1 @@
+	+3
+	EOF
+	git diff --cached >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'checking out an unmodified path is a no-op' '
+	git reset --hard &&
+	git checkout --cached HEAD -- file1 &&
+	git diff-files --exit-code &&
+	git diff-index --cached --exit-code HEAD
+'
+
+test_expect_success 'checking out specific path that is unmerged' '
+	test_commit file3 file3 &&
+	git rm --cached file2 &&
+	echo 1234 >file2 &&
+	F1=$(git rev-parse HEAD:file1) &&
+	F2=$(git rev-parse HEAD:file2) &&
+	F3=$(git rev-parse HEAD:file3) &&
+	{
+		echo "100644 $F1 1	file2" &&
+		echo "100644 $F2 2	file2" &&
+		echo "100644 $F3 3	file2"
+	} | git update-index --index-info &&
+	git ls-files -u &&
+	git checkout --cached HEAD file2 &&
+	test_must_fail git diff --quiet &&
+	git diff-index --exit-code --cached HEAD
+'
+
+test_expect_success '--cached without --no-overlay does not remove entry from index' '
+	test_must_fail git checkout --cached HEAD^ file3 &&
+	git ls-files --error-unmatch -- file3
+'
+
+test_expect_success 'file is removed from the index with --no-overlay' '
+	git checkout --cached --no-overlay HEAD^ file3 &&
+	test_path_is_file file3 &&
+	test_must_fail git ls-files --error-unmatch -- file3
+'
+
+test_expect_success 'test checkout --cached --no-overlay at given paths' '
+	mkdir sub &&
+	>sub/file1 &&
+	>sub/file2 &&
+	git update-index --add sub/file1 sub/file2 &&
+	T=$(git write-tree) &&
+	git checkout --cached --no-overlay HEAD sub/file2 &&
+	test_must_fail git diff --quiet &&
+	U=$(git write-tree) &&
+	echo "$T" &&
+	echo "$U" &&
+	test_must_fail git diff-index --cached --exit-code "$T" &&
+	test "$T" != "$U"
+'
+
+test_done

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a3fd9a9630..cbc304ace8 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh

@@ -1437,6 +1437,7 @@ test_expect_success 'double dash "git checkout"' '
 	--no-quiet Z
 	--no-... Z
 	--overlay Z
+	--cached Z
 	EOF
 '
 

-- 
2.20.0.405.gbc1bbc6f85

Currently when 'git checkout -- <pathspec>...' is invoked with
multiple pathspecs, where one or more of the pathspecs don't match
anything, checkout errors out.

This can be inconvenient in some cases, such as when using git
checkout from a script.  Introduce a new --ignore-unmatched option,
which which allows us to ignore a non-matching pathspec instead of
erroring out.

In a subsequent commit we're going to start using 'git checkout' in
'git stash' and are going to make use of this feature.

Signed-off-by: Thomas Gummerer <redacted>
---
 builtin/checkout.c        | 10 +++++++++-
 t/t2022-checkout-paths.sh |  9 +++++++++
 t/t9902-completion.sh     |  1 +
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6ba85e9de5..7e7b5cd1d3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -46,6 +46,7 @@ struct checkout_opts {
 	int show_progress;
 	int overlay_mode;
 	int cached;
+	int ignore_unmatched;
 	/*
 	 * If new checkout options are added, skip_merge_working_tree
 	 * should be updated accordingly.

@@ -358,7 +359,8 @@ static int checkout_paths(const struct checkout_opts *opts,
 			ce->ce_flags |= CE_MATCHED;
 	}
 
-	if (report_path_error(ps_matched, &opts->pathspec, opts->prefix)) {
+	if (!opts->ignore_unmatched &&
+	    report_path_error(ps_matched, &opts->pathspec, opts->prefix)) {
 		free(ps_matched);
 		return 1;
 	}

@@ -586,6 +588,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
 	 * not tested here
 	 */
 
+	/*
+	 * opts->ignore_unmatched cannot be used with switching branches so is
+	 * not tested here
+	 */
+
 	/*
 	 * If we aren't creating a new branch any changes or updates will
 	 * happen in the existing branch.  Since that could only be updating

@@ -1320,6 +1327,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
 		OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
 		OPT_BOOL(0, "cached", &opts.cached, N_("work on the index only")),
+		OPT_BOOL(0, "ignore-unmatched", &opts.ignore_unmatched, N_("don't error on unmatched pathspecs")),
 		OPT_END(),
 	};
 

diff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh
index fc3eb43b89..b44cdf7b63 100755
--- a/t/t2022-checkout-paths.sh
+++ b/t/t2022-checkout-paths.sh

@@ -78,4 +78,13 @@ test_expect_success 'do not touch files that are already up-to-date' '
 	test_cmp expect actual
 '
 
+test_expect_success 'checkout --ignore-unmatched' '
+	test_commit file1 &&
+	echo changed >file1.t &&
+	git checkout --ignore-unmatched -- file1.t unknown-file &&
+	echo file1 >expect &&
+	test_cmp expect file1.t
+
+'
+
 test_done

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cbc304ace8..475debcf95 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh

@@ -1438,6 +1438,7 @@ test_expect_success 'double dash "git checkout"' '
 	--no-... Z
 	--overlay Z
 	--cached Z
+	--ignore-unmatched Z
 	EOF
 '
 

-- 
2.20.0.405.gbc1bbc6f85

Now that we have 'git checkout --no-overlay', we can use it in git
stash, making the codepaths for 'git stash push' with and without
pathspec more similar, and thus easier to follow.

Signed-off-by: Thomas Gummerer <redacted>
---

As mentioned in the cover letter, not sure if we want to apply this
now.  There are two reasons I did this:
- Showing the new functionality of git checkout
- Increased test coverage, as we are running the new code with all git
  stash tests for free, which helped look at some cases that I was
  missing initially.

 git-stash.sh | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 94793c1a91..67be04d996 100755
--- a/git-stash.sh
+++ b/git-stash.sh

@@ -314,19 +314,15 @@ push_stash () {
 
 	if test -z "$patch_mode"
 	then
-		test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
-		if test -n "$untracked" && test $# = 0
+		test "$untracked" = "all" && CLEAN_X_OPTION=-X || CLEAN_X_OPTION=
+		if test -n "$untracked"
 		then
-			git clean --force --quiet -d $CLEAN_X_OPTION
+			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
 		fi
 
 		if test $# != 0
 		then
-			test -z "$untracked" && UPDATE_OPTION="-u" || UPDATE_OPTION=
-			test "$untracked" = "all" && FORCE_OPTION="--force" || FORCE_OPTION=
-			git add $UPDATE_OPTION $FORCE_OPTION -- "$@"
-			git diff-index -p --cached --binary HEAD -- "$@" |
-			git apply --index -R
+			git checkout --quiet --no-overlay --ignore-unmatched HEAD -- "$@"
 		else
 			git reset --hard -q
 		fi

-- 
2.20.0.405.gbc1bbc6f85

On Sun, Dec 9, 2018 at 9:04 PM Thomas Gummerer [off-list ref] wrote:

The 'git worktree' command used to be just another mode in 'git
checkout', namely 'git checkout --to'.  When the tests for the latter
were retrofitted for the former, the test name was adjusted, but the
test number was kept, even though the test is testing a different
command now.  t/README states: "Second digit tells the particular
command we are testing.", so 'git worktree' should have a separate
number just for itself.

Move the worktree tests to t24* to adhere to that guideline. We're
going to make use of the free'd up numbers in a subsequent commit.

Signed-off-by: Thomas Gummerer <redacted>
---
 t/{t2025-worktree-add.sh => t2400-worktree-add.sh}     | 0
 t/{t2026-worktree-prune.sh => t2401-worktree-prune.sh} | 0
 t/{t2027-worktree-list.sh => t2402-worktree-list.sh}   | 0
 3 files changed, 0 insertions(+), 0 deletions(-)
 rename t/{t2025-worktree-add.sh => t2400-worktree-add.sh} (100%)
 rename t/{t2026-worktree-prune.sh => t2401-worktree-prune.sh} (100%)
 rename t/{t2027-worktree-list.sh => t2402-worktree-list.sh} (100%)

Heh.. I did the same thing (in my unsent switch-branch/restore-files
series) and even used the same 24xx range :D You probably want to move
t2028 and t2029 too (not sure if they have landed on 'master')
-- 
Duy

On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer [off-list ref] wrote:

I'm torn. We try to remove 'extern' but I can see you may want to add
it here to be consistent with others. And removing extern even from
functions from entry.c only would cause some conflicts.

I wonder if we should move the 'removal' variable in symlinks to
'struct checkout' to reduce another global variable. But I guess
that's the problem for another day. It's not the focus of this series.
-- 
Duy

On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer [off-list ref] wrote:

This makes the path counting in nd/checkout-noisy less accurate. But
it's not your fault of course.

Junio, do you still want to merge that series down to 'next' or drop
it? If it will be merged down, I'll keep a note and fix it once this
one lands too.

+
        if (topath)
                return write_entry(ce, topath, state, 1);

--
2.20.0.405.gbc1bbc6f85

-- 
Duy

On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer [off-list ref] wrote:

When marking cache entries for removal, and later removing them all at
once using 'remove_marked_cache_entries()', cache entries currently
have to be invalidated manually in the cache tree and in the untracked
cache.

Add an invalidate flag to the function.  With the flag set, the
function will take care of invalidating the path in the cache tree and
in the untracked cache.

This will be useful in a subsequent commit.

Signed-off-by: Thomas Gummerer <redacted>
---

For the two current callsites, unpack-trees seems to do this
invalidation itself internally.

I'm still a bit scared of this invalidation business in unpack-trees.
The thing is, we handle two separate index_state there, src_index and
result and invalidation has to be done on the right one (because index
extensions are on src_index until the very end of unpack-trees;
invalidating on 'result' would be no-op and wrong).
remove_marked_cache_entries() seems to be called on 'result' while
invalidate_ce_path() is on src_index, hm....

I don't quite understand why we don't
need it in split-index mode though.  I assume it's because the cache
tree in the main index would already have been invalidated?  I didn't
have much time to dig, but couldn't produce any failures with it
either, so I assume not invalidating paths is the right thing to do
here.

Yeah I think it's because cache-tree and untracked cache are already
properly invalidated. This merge base thingy is done when we load the
index files up, not when we write them down. The "front" index may
record that a few paths in the base index are no longer valid and need
to be deleted. But untracked cache and cache-tree both should have
recorded that same info when these paths are marked for delete at
index write time.
-- 
Duy

On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer [off-list ref] wrote:

In non-overlay mode but when pathspec does not match, we come here too.

+                               /*
+                                * "git checkout tree-ish -- path", but this
+                                * entry is in the original index; it will not

I think the missing key point in this comment block is "..is in the
original index _and it's not in tree-ish_". In non-overlay mode, if
pathspec does not match then it's safe to ignore too. But this logic
starts too get to complex and hurt my brain.

+                                * be checked out to the working tree and it
+                                * does not matter if pathspec matched this
+                                * entry.  We will not do anything to this entry
+                                * at all.
+                                */
+                               continue;
+                       }
+               }
                /*
                 * Either this entry came from the tree-ish we are
                 * checking the paths out of, or we are checking out

maybe add " (default)" to the help string.

                OPT_END(),
        };

-- 
Duy

On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer [off-list ref] wrote:

Add a new --cached option to git checkout, which works only on the
index, but not the working tree, similar to what 'git reset <tree-ish>
-- <pathspec>... does.

Elijah wanted another mode (and I agree) that modifies worktree but
leaves the index alone. This is most useful (or least confusing) when
used with <tree-ish> and would be default in restore-files. I'm not
saying you have to implement it, but how do the new command line
options are designed to make sense?

I guess if --cached is "update index only" then --no-cached goes back
to the default "update both worktree and index" and we need a another
option for "worktree only"? Can we have one option with three possible
values (index-only, index-and-worktree, worktree-only) maybe?
-- 
Duy

On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer [off-list ref] wrote:

Currently when 'git checkout -- <pathspec>...' is invoked with
multiple pathspecs, where one or more of the pathspecs don't match
anything, checkout errors out.

This can be inconvenient in some cases, such as when using git
checkout from a script.

Wait, should scripts go with read-tree, checkout-index or other
plumbing commands instead?
-- 
Duy

On Sun, Dec 9, 2018 at 9:04 PM Thomas Gummerer [off-list ref] wrote:

My hope is also that the no-overlay mode could become the new default
in the restore-files command Duy is currently working on.

I already wrote something like that in git-restore-files.txt even
though the implementation is not there :D

There will be a hell of conflicts when the two series enter 'pu' (or
even worse, when the third one to update worktree only appears) so I'm
going to send the switch-branch/restore-files series out but mostly to
gather comments and will rebase once the other series land.

(Alternatively I'll split my series and let the switch-branch part
land first, may be simpler)
-- 
Duy

On Sun, Dec 9, 2018 at 12:04 PM Thomas Gummerer [off-list ref] wrote:

Here's the series I mentioned a couple of times on the list already,
introducing a no-overlay mode in 'git checkout'.  The inspiration for
this came from Junios message in [*1*].

Basically the idea is to also delete files when the match <pathspec>
in 'git checkout <tree-ish> -- <pathspec>' in the current tree, but
don't match <pathspec> in <tree-ish>.  The rest of the cases are
already properly taken care of by 'git checkout'.

Yes, but I'd put it a little differently:

"""
Basically, the idea is when the user run "git checkout --no-overlay
<tree-ish> -- <pathspec>" that the given pathspecs should exactly
match <tree-ish> after the operation completes.  This means that we
also want to delete files that match <pathspec> if those paths are not
found in <tree-ish>.
"""

...and maybe even toss in some comments about the fact that this is
the way git checkout should have always behaved, it just traditionally
hasn't.  (You could also work in comments about how with this new mode
the user can run git diff afterward with the given commit-ish and
pathspecs and get back an empty diff, as expected, which wasn't true
before.  But maybe I'm belaboring the point.)

The final step in the series is to actually make use of this in 'git
stash', which simplifies the code there a bit.  I am however happy to
hold off on this step until the stash-in-C series is merged, so we
don't delay that further.

In addition to the no-overlay mode, we also add a --cached mode, which
works only on the index, thus similar to 'git reset <tree-ish> -- <pathspec>'.

If you're adding a --cached mode to make it only work on the index,
should there be a similar mode to allow it to only work on the working
tree?  (I'm not as concerned with that here, but I really think the
new restore-files command by default should only operate on the
working tree, and then have options to affect the index either in
addition or instead of the working tree.)

Actually deprecating 'git reset <tree-ish> -- <pathspec>' should come
later, probably not before Duy's restore-files command lands, as 'git
checkout --no-overlay <tree-ish> -- <pathspec>' is a bit cumbersome to
type compared to 'git reset <tree-ish> -- <pathspec>'.

Makes sense.

My hope is also that the no-overlay mode could become the new default
in the restore-files command Duy is currently working on.

Absolutely, yes.  I don't want another broken command.  :-)

No documentation yet, as I wanted to get this out for review first.
I'm not familiar with most of the code I touched here, so there may
well be much better ways to implement some of this, that I wasn't able
to figure out.  I'd be very happy with some feedback around that.

Another thing I'm not sure about is how to deal with conflicts.  In
the cached mode this patch series is not dealing with it at all, as
'git checkout -- <pathspec>' when pathspec matches a file with
conflicts doesn't update the index.  For the no-overlay mode, the file
is removed if the corresponding stage is not found in the index.  I'm
however not sure this is the right thing to do in all cases?

Here's how I'd go about analyzing that...

If the user passes a <tree-ish>, then the answer about what to do is
pretty obvious; the <tree-ish> didn't have conflicts, so conflicted
paths in the index that match the pathspec should be overwritten with
whatever version of those paths existed in <tree-ish> (possibly
implying deletion of some paths).

Also, as you point out, --cached means only modify the index and not
the working tree; so if they specify both --cached and provide no
tree, then they've specified a no-op.

So it's only interesting when you have conflicts in the index and
specify --no-overlay without a <tree-ish> or --cached.  This boils
down to "how do we update the working tree to match the index, when
the index is conflicted?"  A couple points to consider:
  * This is somewhat of an edge case
  * In the normal case --no-overlay is only different from --overlay
behavior for directories; it'd be nice if that extended to all cases
  * How does this command behave without a <tree-ish> when
--no-overlay is specified and a directory is given for a <pathspec>
and there aren't any conflicts?  Are we being consistent with that
behavior?


However, I think it turns out that the answer is much simpler than all
that initial analysis or what you say you've implemented.  Here's why:

If <pathspec> is a file which is present in both the working tree and
the index and it has conflicts, then "git checkout -- <pathspec>" will
currently throw an error:

$ git checkout -- subdir/counting
error: path 'subdir/counting' is unmerged

In fact, even if every entry in subdir/ is a path that is in both the
index and the working tree (so that --no-overlay and --overlay ought
to behave the same), if any one of the files in subdir is conflicted,
attempting to checkout the subdir will abort with this same error
message and no paths will be updated at all:

$ git checkout -- subdir
error: path 'subdir/counting' is unmerged

as such, the answer with what to do with --no-overlay mode is pretty
clear: if the <pathspec> matches _any_ path that is conflicted, simply
throw an error and abort the operation without making any changes at
all.

On Mon, Dec 10, 2018 at 7:50 AM Duy Nguyen [off-list ref] wrote:

On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer [off-list ref] wrote:

quoted

Factor out the 'unlink_entry()' function from unpack-trees.c to
entry.c.  It will be used in other places as well in subsequent
steps.

As it's no longer a static function, also move the documentation to
the header file to make it more discoverable.

I also started using unlink_entry() in another place in a local patch
series that I haven't submitted yet (and which I need to get back to
at some point).  So this will help me too.  :-)

quoted

Signed-off-by: Thomas Gummerer <redacted>
---
 cache.h        |  5 +++++
 entry.c        | 15 +++++++++++++++
 unpack-trees.c | 19 -------------------
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/cache.h b/cache.h
index ca36b44ee0..c1c953e810 100644
--- a/cache.h
+++ b/cache.h

@@ -1542,6 +1542,11 @@ struct checkout {
 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
 extern void enable_delayed_checkout(struct checkout *state);
 extern int finish_delayed_checkout(struct checkout *state);
+/*
+ * Unlink the last component and schedule the leading directories for
+ * removal, such that empty directories get removed.
+ */
+extern void unlink_entry(const struct cache_entry *ce);

I'm torn. We try to remove 'extern' but I can see you may want to add
it here to be consistent with others. And removing extern even from
functions from entry.c only would cause some conflicts.

I wonder if we should move the 'removal' variable in symlinks to
'struct checkout' to reduce another global variable. But I guess
that's the problem for another day. It's not the focus of this series.

"move the 'removal' variable in symlinks"?  I'm having a really hard
time parsing that phrase and the sentence it's embedded in.  Could you
reword for me Duy?

On Mon, Dec 10, 2018 at 6:23 PM Elijah Newren [off-list ref] wrote:

quoted

I wonder if we should move the 'removal' variable in symlinks to
'struct checkout' to reduce another global variable. But I guess
that's the problem for another day. It's not the focus of this series.

"move the 'removal' variable in symlinks"?  I'm having a really hard
time parsing that phrase and the sentence it's embedded in.  Could you
reword for me Duy?

Sorry s/in symlinks/&.c/. There's a global variable named 'removal' in
symlinks.c which is used by schedule_dir_for_removal() and this
function in turn is used by unlink_entry().
-- 
Duy

On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer [off-list ref] wrote:

Minor nit: This error message is kinda hard to parse, for someone not
that familiar with all the *_entry functions, like myself.  Maybe add
a comment before this line:
    /* No content and thus no path to create, so we have no pathname
to return */
or reword the error slightly?  Or maybe it's fine and I was just
confused from lack of code familiarity, but I'll throw it out there
since I stumbled on it a bit.

+               unlink_entry(ce);
+               return 0;
+       }
+
        if (topath)
                return write_entry(ce, topath, state, 1);

--
2.20.0.405.gbc1bbc6f85

On Mon, Dec 10, 2018 at 8:09 AM Duy Nguyen [off-list ref] wrote:

On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer [off-list ref] wrote:

quoted

When marking cache entries for removal, and later removing them all at
once using 'remove_marked_cache_entries()', cache entries currently
have to be invalidated manually in the cache tree and in the untracked
cache.

Add an invalidate flag to the function.  With the flag set, the
function will take care of invalidating the path in the cache tree and
in the untracked cache.

This will be useful in a subsequent commit.

Signed-off-by: Thomas Gummerer <redacted>
---

For the two current callsites, unpack-trees seems to do this
invalidation itself internally.

I'm still a bit scared of this invalidation business in unpack-trees.
The thing is, we handle two separate index_state there, src_index and
result and invalidation has to be done on the right one (because index
extensions are on src_index until the very end of unpack-trees;
invalidating on 'result' would be no-op and wrong).
remove_marked_cache_entries() seems to be called on 'result' while
invalidate_ce_path() is on src_index, hm....

Is Thomas avoiding problems here simply because merge is the only
caller of unpack_trees with src_index != dst_index?  Or does src_index
== dst_index for checkout not actually help?

If that does help with the checkout case, then allow me to find a
different way to muddy the waters...  I think I might want to make use
of this function in the merge machinery at some point, so I either
need to figure out how to convince you to verify if all this cache
tree invalidation stuff is sane, or somehow figure out all the
cache_tree stuff stuff myself so I can figure out what is right here.
:-)

quoted

I don't quite understand why we don't
need it in split-index mode though.  I assume it's because the cache
tree in the main index would already have been invalidated?  I didn't
have much time to dig, but couldn't produce any failures with it
either, so I assume not invalidating paths is the right thing to do
here.

Yeah I think it's because cache-tree and untracked cache are already
properly invalidated. This merge base thingy is done when we load the
index files up, not when we write them down. The "front" index may
record that a few paths in the base index are no longer valid and need
to be deleted. But untracked cache and cache-tree both should have
recorded that same info when these paths are marked for delete at
index write time.

On Mon, Dec 10, 2018 at 7:09 PM Elijah Newren [off-list ref] wrote:

quoted

quoted

For the two current callsites, unpack-trees seems to do this
invalidation itself internally.

I'm still a bit scared of this invalidation business in unpack-trees.
The thing is, we handle two separate index_state there, src_index and
result and invalidation has to be done on the right one (because index
extensions are on src_index until the very end of unpack-trees;
invalidating on 'result' would be no-op and wrong).
remove_marked_cache_entries() seems to be called on 'result' while
invalidate_ce_path() is on src_index, hm....

Is Thomas avoiding problems here simply because merge is the only
caller of unpack_trees with src_index != dst_index?  Or does src_index
== dst_index for checkout not actually help?

I think it would not help. 'result' is a temporary index where we copy
things to (and it does not have anything from the beginning). If you
invalidate stuff in there, you invalidate nothing, regardless whether
dst_index == src_index.

If that does help with the checkout case, then allow me to find a
different way to muddy the waters...  I think I might want to make use
of this function in the merge machinery at some point, so I either
need to figure out how to convince you to verify if all this cache
tree invalidation stuff is sane, or somehow figure out all the
cache_tree stuff stuff myself so I can figure out what is right here.
:-)

I'm not the unpack-trees man (I think that would still be Junio). And
I'm not saying it's sane either. I think it's just some leftover
things since Linus split "the index" in unpack-tree operation to
'src', 'result' and 'dst' many years ago and nobody was brave enough
to clean it up (then I piled on with untracked cache and split index,
but I did not see it clearly either). That person could be you ;-)
-- 
Duy

On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer [off-list ref] wrote:

Currently 'git checkout' is defined as an overlay operation, which
means that if in 'git checkout <tree-ish> -- [<pathspec>]' we have an
entry in the index that matches <pathspec>, but that doesn't exist in
<tree-ish>, that entry will not be removed from the index or the
working tree.

Introduce a new --{,no-}overlay option, which allows using 'git
checkout' in non-overlay mode, thus removing files from the working
tree if they do not exist in <tree-ish> but match <pathspec>.

Note that 'git checkout -p <tree-ish> -- [<pathspec>]' already works
this way, so no changes are needed for the patch mode.  We disallow
'git checkout --overlay -p' to avoid confusing users who would expect
to be able to force overlay mode in 'git checkout -p' this way.

Whoa...that's interesting.  To me, that argues even further that the
traditional checkout behavior was wrong all along and the choice of
--overlay vs. --no-overlay in the original implementation was a total
oversight.  I'm really tempted to say that --no-overlay should just be
the default in checkout too...but maybe that's too high a hill to
climb, at least for now.

Making --overlap and -p incompatible is a reasonable first step.  But
you should probably add a comment to the -p option documentation that
it implies --no-overlay.