Sparse checkout improvements -- improved sparsity updating

This series provides a replacement for the git read-tree -mu HEAD-based sparsity updating used by sparse-checkout, fixing the myriad problems it causes. The most important bit is patch 5 and its lengthy commit message explaining the current state and rationale.

Consider the series RFC for now; I probably need to add some tests, get some people at work who are willing to test it in real cases, and get some community feedback particularly on the choices/comments highlighted in the commit message to patch 5. But it's at least basically functional so I wanted to send it out to the community for feedback.

From: Elijah Newren <[email protected]>

If a path is dirty, removing from the working tree risks losing data. As such, we want to make sure any such path is not marked with SKIP_WORKTREE. While the current callers of this code detect this case and re-populate with a previous set of sparsity patterns, we want to allow some paths to be marked with SKIP_WORKTREE while others are left unmarked without it being considered an error. The reason this shouldn't be considered an error is that SKIP_WORKTREE has always been an advisory-only setting; merge and rebase for example were free to materialize paths and clear the SKIP_WORKTREE bit in order to accomplish their work even though they kept the SKIP_WORKTREE bit set for other paths. Leaving dirty working files in the working tree is thus a natural extension of what we have already been doing.

Signed-off-by Elijah Newren <[email protected]>

---
 unpack-trees.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

unpack-trees.c +0 -0

index 853d843b17a..8f10ac91ce1 100644
@@ -504,8 +504,11 @@ static int apply_sparse_checkout(struct index_state *istate,
 		 * also stat info may have lost after merged_entry() so calling
 		 * verify_uptodate() again may fail

From: Elijah Newren <[email protected]>

Remove the equivalent of 'git read-tree -mu HEAD' in the sparse-checkout codepaths for setting the SKIP_WORKTREE bits and instead use the new update_sparsity() function.

Signed-off-by Elijah Newren <[email protected]>

---
 builtin/sparse-checkout.c          | 39 +++++++-----------------------
 t/t1091-sparse-checkout-builtin.sh | 32 +++++++++++++++++-------
 2 files changed, 32 insertions(+), 39 deletions(-)

builtin/sparse-checkout.c +0 -8

index 740da4b6d54..5d3ec2e6be9 100644
@@ -97,47 +97,32 @@ static int update_working_directory(struct pattern_list *pl)
 	int result = 0;
 	struct unpack_trees_options o;
 	struct lock_file lock_file = LOCK_INIT;
-	struct object_id oid;
-	struct tree *tree;
-	struct tree_desc t;
 	struct repository *r = the_repository;
 
-	if (repo_read_index_unmerged(r))
-		die(_("you need to resolve your current index first"));
-
-	if (get_oid("HEAD", &oid))
-		return 0;

builtin/sparse-checkout.c +0 -2

-	resolve_undo_clear_index(r->index);
 	setup_work_tree();
 
-	cache_tree_free(&r->index->cache_tree);

builtin/sparse-checkout.c +7 -5

+		/*
+		 * We don't do any special handling of warnings from untracked
+		 * files in the way or dirty entries that can't be removed.
+		 */
+		result = 0;
+	if (!result)
 		write_locked_index(r->index, &lock_file, COMMIT_LOCK);
-	} else
+	else
 		rollback_lock_file(&lock_file);
 
 	return result;

@@ -304,8 +289,6 @@ static int sparse_checkout_init(int argc, const char **argv)
 	};
 
 	repo_read_index(the_repository);
-	require_clean_work_tree(the_repository,
-				N_("initialize sparse-checkout"), NULL, 1, 0);
 
 	argc = parse_options(argc, argv, NULL,
 			     builtin_sparse_checkout_init_options,

@@ -560,8 +543,6 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
 	};
 
 	repo_read_index(the_repository);
-	require_clean_work_tree(the_repository,
-				N_("set sparse-checkout patterns"), NULL, 1, 0);
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_sparse_checkout_set_options,

builtin/sparse-checkout.c +0 -0

 	memset(&pl, 0, sizeof(pl));
 	hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);

t/t1091-sparse-checkout-builtin.sh +16 -9

index 8607a8e6d1a..0d93d3983e0 100755
@@ -277,16 +277,23 @@ test_expect_success 'cone mode: add parent path' '
 	check_files repo a deep folder1
 '
 
-test_expect_success 'revert to old sparse-checkout on bad update' '
+test_expect_success 'not-up-to-date does not block rest of sparsification' '
 	test_when_finished git -C repo sparse-checkout disable &&
 	test_when_finished git -C repo reset --hard &&
 	git -C repo sparse-checkout set deep &&
+
 	echo update >repo/deep/deeper2/a &&
 	cp repo/.git/info/sparse-checkout expect &&
-	test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
-	test_i18ngrep "cannot set sparse-checkout patterns" err &&
-	test_cmp repo/.git/info/sparse-checkout expect &&
-	check_files repo/deep a deeper1 deeper2
+	test_write_lines "!/deep/*/" "/deep/deeper1/" >>expect &&
+
+	git -C repo sparse-checkout set deep/deeper1 2>err &&
+
+	test_i18ngrep "Cannot update sparse checkout" err &&
+	test_cmp expect repo/.git/info/sparse-checkout &&
+	check_files repo/deep a deeper1 deeper2 &&
+	check_files repo/deep/deeper1 a deepest &&
+	check_files repo/deep/deeper1/deepest a &&
+	check_files repo/deep/deeper2 a
 '
 
 test_expect_success 'revert to old sparse-checkout on empty update' '

@@ -316,12 +323,19 @@ test_expect_success '.gitignore should not warn about cone mode' '
 	test_i18ngrep ! "disabling cone patterns" err
 '
 
-test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status' '
+test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' '
 	git clone repo dirty &&
 	echo dirty >dirty/folder1/a &&
-	test_must_fail git -C dirty sparse-checkout init &&
-	test_must_fail git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
-	test_must_fail git -C dirty sparse-checkout disable &&
+
+	git -C dirty sparse-checkout init 2>err &&
+	test_i18ngrep "warning.*Cannot update sparse checkout" err &&

From: Elijah Newren <[email protected]>

If commands like merge or rebase materialize files as part of their work, or a previous sparse-checkout command failed to update individual files due to dirty changes, users may want a command to simply 'reapply' the sparsity rules. Provide one.

Signed-off-by Elijah Newren <[email protected]>

---
 Documentation/git-sparse-checkout.txt | 10 ++++++++++
 builtin/sparse-checkout.c             | 10 +++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

Documentation/git-sparse-checkout.txt +10 -0

index c0342e53938..27f4392489f 100644
@@ -70,6 +70,16 @@ C-style quoted strings.
 	`core.sparseCheckoutCone` is enabled, the given patterns are interpreted
 	as directory names as in the 'set' subcommand.
 
+'update'::
+	Update the sparseness of paths in the working tree based on the
+	existing patterns.  Commands like merge or rebase can materialize
+	paths to do their work (e.g. in order to show you a conflict), and
+	other sparse-checkout commands might fail to sparsify an individual
+	file (e.g. because it has unstaged changes or conflicts).  In such
+	cases, it can make sense to run `git sparse-checkout update` later
+	after cleaning up affected paths (e.g. resolving conflicts, undoing
+	or committing changes, etc.).
+
 'disable'::
 	Disable the `core.sparseCheckout` config setting, and restore the
 	working directory to include all files. Leaves the sparse-checkout

builtin/sparse-checkout.c +1 -1

index 5d3ec2e6be9..2ae21011dfd 100644
@@ -18,7 +18,7 @@
 static const char *empty_base = "";
 
 static char const * const builtin_sparse_checkout_usage[] = {
-	N_("git sparse-checkout (init|list|set|add|disable) <options>"),
+	N_("git sparse-checkout (init|list|set|add|update|disable) <options>"),
 	NULL
 };
 

From: Elijah Newren <[email protected]>

Previously, the only way to update the SKIP_WORKTREE bits for various paths was invoking git read-tree -mu HEAD or calling the same code that this codepath invoked. This however had a number of problems. Let's look at all the various cases:

Flipping SKIP_WORKTREE -> !SKIP_WORKTREE (materializing files):
A) There is no file in the way
B) There is an untracked file in the way
Flipping !SKIP_WORKTREE -> SKIP_WORKTREE (removing files):
C) The path is clean
D) The path is unmerged
E) The path has unstaged changes
F) The path has staged changes (differs from HEAD)

For git read-tree -mu HEAD, the behavior is as follows:
A) Materialize the file, as expected
B) Refuse to make *any* changes (to this path or any others)
C) Remove the file, as expected
D) Refuse to make *any* changes (to this path or any others)
E) Refuse to make *any* changes (to this path or any others)
F) Removes the file from the working copy AND resets the index to match HEAD

Refusing to modify files that could result in data loss as in cases B, D, and E is good, but refusing to work on any other paths is very problematic for users. If they are in the middle of a rebase or have made modifications to files that bring in more dependencies, then for their build to work they need to update the sparse paths -- which they are prevented from doing. Sometimes they stage the files and re-try, at which point they run into case F and believe that their changes are lost. (Even if they come ask an expert, they have to trawl through their loose objects looking for which ones match which files.)

Add a new update_sparsity() function which behaves as follows in these cases:
A) Materialize the file, as expected
B) Leave the file in the working copy alone, clear the SKIP_WORKTREE bit, and print a warning (thus leaving them in a state where status will report the file as modified, which seems logical).
C) Remove the file, as expected
D) Do NOT mark this path as SKIP_WORKTREE, but allow others paths to be updated.
E) Do NOT mark this path as SKIP_WORKTREE and print a warning about the dirty path, but allow other paths to be updated.
F) Mark the path as SKIP_WORKTREE, but do not revert the version stored in the index to match HEAD; leave the contents alone.

I tried a different behavior for B (leave the SKIP_WORKTREE bit set), but found it very surprising and counter-intuitive (e.g. the user sees it is present along with all the other files in that directory, tries to stage it, but git add ignores it since the SKIP_WORKTREE bit is set).

A, B, C, and E all seem like optimal behavior to me. D may be as well, though I wonder if printing a warning would be an improvement. Some might be slightly surprised by F at first, but given that it does the right thing with git commit and even git commit -a (add ignores entries that are marked SKIP_WORKTREE and thus doesn't delete them, and commit -a is similar), it seems logical to me.

(quoted diff) +14 -0

+			for (i = 0; i < rejects->nr; i++)
+				strbuf_addstr(&path, rejects->items[i].string);
+			warning(ERRORMSG(o, e), super_prefixed(path.buf));
+			strbuf_release(&path);
+		}
+		string_list_clear(rejects, 0);
+	}
+}
+/*
+ * Update SKIP_WORKTREE bits according to sparsity patterns, and update
+ * working directory to match.
+ *
+ * Returns
+ *   0: success with no warnings

unpack-trees.c +4 -0

+int update_sparsity(struct unpack_trees_options *o)
+{
+	struct pattern_list pl;
+	int i, empty_worktree, ret = 0;

unpack-trees.c +5 -0

+
+	trace_performance_enter();
+
+	if (!o->pl) {
+		char *sparse = git_pathdup("info/sparse-checkout");

unpack-trees.c +14 -0

+		goto done;
+	}
+
+skip_sparse_checkout:
+	if (check_updates(o, o->src_index))
+		ret = -2;
+
+done:
+	display_warning_msgs(o);
+	o->show_all_errors = old_show_all_errors;
+	trace_performance_leave("update_sparsity");
+	return ret;
+}
+
 /* Here come the merge functions */
 
 static int reject_merge(const struct cache_entry *ce,

unpack-trees.h +0 -0

index ae1557fb804..5d1a77ae249 100644

Quoted 30 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Remove the equivalent of 'git read-tree -mu HEAD' in the sparse-checkout codepaths for setting the SKIP_WORKTREE bits and instead use the new update_sparsity() function.

Signed-off-by Elijah Newren <[email protected]>

---
 builtin/sparse-checkout.c          | 39 +++++++-----------------------
 t/t1091-sparse-checkout-builtin.sh | 32 +++++++++++++++++-------
 2 files changed, 32 insertions(+), 39 deletions(-)

builtin/sparse-checkout.c +0 -8

index 740da4b6d54..5d3ec2e6be9 100644
@@ -97,47 +97,32 @@ static int update_working_directory(struct pattern_list *pl)
      int result = 0;
      struct unpack_trees_options o;
      struct lock_file lock_file = LOCK_INIT;
-     struct object_id oid;
-     struct tree *tree;
-     struct tree_desc t;
      struct repository *r = the_repository;
 
-     if (repo_read_index_unmerged(r))
-             die(_("you need to resolve your current index first"));
-
-     if (get_oid("HEAD", &oid))
-             return 0;

builtin/sparse-checkout.c static int update_working_directory(struct pattern_list *pl) · line 119

Quoted 10 lines

builtin/sparse-checkout.c +0 -7

-
-     tree = parse_tree_indirect(&oid);
-     parse_tree(tree);
-     init_tree_desc(&t, tree->buffer, tree->size);
-
      memset(&o, 0, sizeof(o));
      o.verbose_update = isatty(2);
-     o.merge = 1;
      o.update = 1;
-     o.fn = oneway_merge;

I'm glad this is getting much simpler.

builtin/sparse-checkout.c static int update_working_directory(struct pattern_list *pl) · line 125

Quoted 6 lines

builtin/sparse-checkout.c +0 -1

      o.head_idx = -1;
      o.src_index = r->index;
      o.dst_index = r->index;
      o.skip_sparse_checkout = 0;
      o.pl = pl;
-     o.keep_pattern_list = !!pl;

I'm making a mental note to see if this is necessary or not. I'm guessing that update_sparsity() expects a pattern list as input and will never free it, so this option is useless.

Quoted 11 lines

builtin/sparse-checkout.c +1 -5

-     resolve_undo_clear_index(r->index);
      setup_work_tree();
 
-     cache_tree_free(&r->index->cache_tree);
-
      repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR);
 
-     core_apply_sparse_checkout = 1;
-     result = unpack_trees(1, &t, &o);
+     result = update_sparsity(&o);
 

builtin/sparse-checkout.c static int update_working_directory(struct pattern_list *pl) · line 117

Quoted 3 lines

builtin/sparse-checkout.c +1 -2

-     if (!result) {
-             prime_cache_tree(r, r->index, tree);
+     if (result == 1)

I wonder if it is appropriate to return an enum in update_sparsity() so this statement is clearer out-of-context. For example, I'm looking at this patch before looking closely at the update_sparsity() patch so I know how it will be used before I check its implementation.

Quoted 31 lines

builtin/sparse-checkout.c +7 -5

+             /*
+              * We don't do any special handling of warnings from untracked
+              * files in the way or dirty entries that can't be removed.
+              */
+             result = 0;
+     if (!result)
              write_locked_index(r->index, &lock_file, COMMIT_LOCK);
-     } else
+     else
              rollback_lock_file(&lock_file);
 
      return result;

@@ -304,8 +289,6 @@ static int sparse_checkout_init(int argc, const char **argv)
      };
 
      repo_read_index(the_repository);
-     require_clean_work_tree(the_repository,
-                             N_("initialize sparse-checkout"), NULL, 1, 0);
 
      argc = parse_options(argc, argv, NULL,
                           builtin_sparse_checkout_init_options,

@@ -560,8 +543,6 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
      };
 
      repo_read_index(the_repository);
-     require_clean_work_tree(the_repository,
-                             N_("set sparse-checkout patterns"), NULL, 1, 0);
 
      argc = parse_options(argc, argv, prefix,
                           builtin_sparse_checkout_set_options,

builtin/sparse-checkout.c static int sparse_checkout_disable(int argc, const char **argv) · line 581

Quoted 5 lines

builtin/sparse-checkout.c +0 -2

@@ -577,8 +558,6 @@ static int sparse_checkout_disable(int argc, const char **argv)
      struct strbuf match_all = STRBUF_INIT;
 
      repo_read_index(the_repository);
-     require_clean_work_tree(the_repository,
-                             N_("disable sparse-checkout"), NULL, 1, 0);

In the three hunks above, do we still need repo_read_index()?

Quoted 49 lines

builtin/sparse-checkout.c +0 -0

      memset(&pl, 0, sizeof(pl));
      hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);

t/t1091-sparse-checkout-builtin.sh +16 -9

index 8607a8e6d1a..0d93d3983e0 100755
@@ -277,16 +277,23 @@ test_expect_success 'cone mode: add parent path' '
      check_files repo a deep folder1
 '
 
-test_expect_success 'revert to old sparse-checkout on bad update' '
+test_expect_success 'not-up-to-date does not block rest of sparsification' '
      test_when_finished git -C repo sparse-checkout disable &&
      test_when_finished git -C repo reset --hard &&
      git -C repo sparse-checkout set deep &&
+
      echo update >repo/deep/deeper2/a &&
      cp repo/.git/info/sparse-checkout expect &&
-     test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
-     test_i18ngrep "cannot set sparse-checkout patterns" err &&
-     test_cmp repo/.git/info/sparse-checkout expect &&
-     check_files repo/deep a deeper1 deeper2
+     test_write_lines "!/deep/*/" "/deep/deeper1/" >>expect &&
+
+     git -C repo sparse-checkout set deep/deeper1 2>err &&
+
+     test_i18ngrep "Cannot update sparse checkout" err &&
+     test_cmp expect repo/.git/info/sparse-checkout &&
+     check_files repo/deep a deeper1 deeper2 &&
+     check_files repo/deep/deeper1 a deepest &&
+     check_files repo/deep/deeper1/deepest a &&
+     check_files repo/deep/deeper2 a
 '
 
 test_expect_success 'revert to old sparse-checkout on empty update' '

@@ -316,12 +323,19 @@ test_expect_success '.gitignore should not warn about cone mode' '
      test_i18ngrep ! "disabling cone patterns" err
 '
 
-test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status' '
+test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' '
      git clone repo dirty &&
      echo dirty >dirty/folder1/a &&
-     test_must_fail git -C dirty sparse-checkout init &&
-     test_must_fail git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
-     test_must_fail git -C dirty sparse-checkout disable &&
+
+     git -C dirty sparse-checkout init 2>err &&
+     test_i18ngrep "warning.*Cannot update sparse checkout" err &&

t/t1091-sparse-checkout-builtin.sh test_expect_success '.gitignore should not warn about cone mode' ' · line 341

Quoted 10 lines

t/t1091-sparse-checkout-builtin.sh +7 -0

+
+     git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
+     test_i18ngrep "warning.*Cannot update sparse checkout" err &&
+
+     git -C dirty sparse-checkout disable &&
+     test_i18ngrep "warning.*Cannot update sparse checkout" err &&
+
      git -C dirty reset --hard &&
      git -C dirty sparse-checkout init &&
      git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&

I like that the test changes here are limited to updating existing tests. You mentioned adding more tests, which I would also like, but those can come in a follow-up patch that is dedicated to all the fun situations we can get into, and how we should respond.

Here are a few that I've tested manually:

1. Start: no sparse-checkout
a. Get into rebase conflict in a directory.
b. "git sparse-checkout init --cone"
c. See that the file is on-disk with conflict markers.
d. Resolve conflict and "git add"
e. "git sparse-checkout set <dir-without-conflict>"
f. Sparse-checkout is set as expected, file is not on disk, but still staged. g. "git rebase --continue" succeeds.

2. Start: sparse-checkout set to "dir1"
a. Get into merge conflict in "dir2"
b. Resolve conflict, "git add"
c. "git sparse-checkout update" (available after next patch) d. dir1 gone, dir1/file staged.
e. git merge --continue succeeds

Those are the really big ones to me, but I'm sure we will find others. I'm really happy that this is working as we expect.

Quoted 6 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

If commands like merge or rebase materialize files as part of their work, or a previous sparse-checkout command failed to update individual files due to dirty changes, users may want a command to simply 'reapply' the sparsity rules. Provide one.

I was actually thinking "refresh" would be a better name, but also you use "reapply" which is good, too. I'm concerned that "update" may imply that the sparse-checkout patterns can change, but you really mean to re-do the work from a previous "git sparse-checkout (set|add)".

I also thought of "reset" but that would be a confusing overload.

Quoted 44 lines

Signed-off-by Elijah Newren <[email protected]>

---
 Documentation/git-sparse-checkout.txt | 10 ++++++++++
 builtin/sparse-checkout.c             | 10 +++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

Documentation/git-sparse-checkout.txt +10 -0

index c0342e53938..27f4392489f 100644
@@ -70,6 +70,16 @@ C-style quoted strings.
      `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
      as directory names as in the 'set' subcommand.
 
+'update'::
+     Update the sparseness of paths in the working tree based on the
+     existing patterns.  Commands like merge or rebase can materialize
+     paths to do their work (e.g. in order to show you a conflict), and
+     other sparse-checkout commands might fail to sparsify an individual
+     file (e.g. because it has unstaged changes or conflicts).  In such
+     cases, it can make sense to run `git sparse-checkout update` later
+     after cleaning up affected paths (e.g. resolving conflicts, undoing
+     or committing changes, etc.).
+
 'disable'::
      Disable the `core.sparseCheckout` config setting, and restore the
      working directory to include all files. Leaves the sparse-checkout

builtin/sparse-checkout.c +1 -1

index 5d3ec2e6be9..2ae21011dfd 100644
@@ -18,7 +18,7 @@
 static const char *empty_base = "";
 
 static char const * const builtin_sparse_checkout_usage[] = {
-     N_("git sparse-checkout (init|list|set|add|disable) <options>"),
+     N_("git sparse-checkout (init|list|set|add|update|disable) <options>"),
      NULL
 };
 

@@ -552,6 +552,12 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,
      return modify_pattern_list(argc, argv, m);
 }
 

builtin/sparse-checkout.c static int sparse_checkout_set(int argc, const char **argv, const char *prefix, · line 560

Quoted 6 lines

builtin/sparse-checkout.c +6 -0

+static int sparse_checkout_update(int argc, const char **argv)
+{
+     repo_read_index(the_repository);
+     return update_working_directory(NULL);
+}
+

Short and sweet! I suppose my earlier comment about whether repo_read_index() was necessary is answered here. Perhaps it should be part of update_working_directory()? (And pass a repository pointer to it?)

Quoted 12 lines

builtin/sparse-checkout.c +2 -0

 static int sparse_checkout_disable(int argc, const char **argv)
 {
      struct pattern_list pl;

@@ -601,6 +607,8 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
                      return sparse_checkout_set(argc, argv, prefix, REPLACE);
              if (!strcmp(argv[0], "add"))
                      return sparse_checkout_set(argc, argv, prefix, ADD);
+             if (!strcmp(argv[0], "update"))
+                     return sparse_checkout_update(argc, argv);
              if (!strcmp(argv[0], "disable"))
                      return sparse_checkout_disable(argc, argv);
      }

Quoted 14 lines · Derrick Stolee

builtin/sparse-checkout.c static int sparse_checkout_set(int argc, const char **argv, const char *prefix, · line 560

Quoted 7 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

builtin/sparse-checkout.c +6 -0

+static int sparse_checkout_update(int argc, const char **argv)
+{
+     repo_read_index(the_repository);
+     return update_working_directory(NULL);
+}
+

Short and sweet! I suppose my earlier comment about whether repo_read_index() was necessary is answered here. Perhaps it should be part of update_working_directory()? (And pass a repository pointer to it?)

Good question. Is there a chance we want to make update_working_directory() available to other areas of git outside of sparse-checkout.c? If so, potentially re-reading the index might not be friendly, but if sparse-checkout.c is going to remain the only caller then it probably makes sense to move it inside.

Quoted 16 lines · Elijah Newren

Quoted 14 lines · Derrick Stolee

builtin/sparse-checkout.c static int sparse_checkout_set(int argc, const char **argv, const char *prefix, · line 560

Quoted 7 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

builtin/sparse-checkout.c +6 -0

+static int sparse_checkout_update(int argc, const char **argv)
+{
+     repo_read_index(the_repository);
+     return update_working_directory(NULL);
+}
+

Short and sweet! I suppose my earlier comment about whether repo_read_index() was necessary is answered here. Perhaps it should be part of update_working_directory()? (And pass a repository pointer to it?)

Quoted 5 lines

Good question. Is there a chance we want to make update_working_directory() available to other areas of git outside of sparse-checkout.c? If so, potentially re-reading the index might not be friendly, but if sparse-checkout.c is going to remain the only caller then it probably makes sense to move it inside.

Minh had an interesting idea during side-conversations at the summit:
have a way for an in-tree description of some sparse-checkout cones. The idea was to be able to automatically update the sparse-checkout while moving between commits that may have different dependency configurations. In the world of Office it would mean that there is some file ".sparse/word" that describes the directories required to build Word, and ".sparse/ppt" for building PowerPoint. Then, based on local Git config, we would see that we want our sparse-checkout cone to match the union of the directories in .sparse/word and .sparse/ppt. As we move HEAD, we would want to automatically update the sparse cone when those files change.

I'm working on a design document for how this idea would work, realistically, that I plan to share here and with the Office team to see if it is actually a helpful plan. I think it would reduce the performance cost of the hook we plan to use for this, and would reduce the investment needed for a project to adopt sparse-checkout.

All that is to say, yes we may want to add other callers to update_working_directory() outside of the sparse-checkout builtin. With that in mind, perhaps its name should reflect the fact that we are only updating it according to the sparse cone?

Thanks,
-Stolee

Quoted 7 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Previously, the only way to update the SKIP_WORKTREE bits for various paths was invoking git read-tree -mu HEAD or calling the same code that this codepath invoked. This however had a number of problems. Let's look at all the various cases:

Quoted 8 lines

Flipping SKIP_WORKTREE -> !SKIP_WORKTREE (materializing files):
A) There is no file in the way
B) There is an untracked file in the way
Flipping !SKIP_WORKTREE -> SKIP_WORKTREE (removing files):
C) The path is clean
D) The path is unmerged
E) The path has unstaged changes
F) The path has staged changes (differs from HEAD)

Please also note that cases D, E, and F can appear in any combination. The lack of any of D, E, and F is case "C".

Perhaps instead we should think of the following independent states that can toggle orthogonally on a path:

i. Current SKIP_WORKTREE value
ii. Intended SKIP_WORKTREE value
iii. Path has staged changes.
iv. Path has unstaged changes.
v. Path is unmerged.

with the additional conditional case of:

vi. There is a collision when changing from SKIP_WORKTREE to
!SKIP_WORKTREE. This could be file content difference or a file/directory type conflict.

I believe the intention when refreshing or changing the sparse- checkout definition is to:

0. If a merge conflict happens outside the sparse cone, then the conflicting files are created with the conflict markers. This puts those paths in the state of having unstaged changes.

1. If the path exists and we intend to set SKIP_WORKTREE, then delete the file only if there are no unstaged changes.

2. If the path does not exist and we intend to remove SKIP_WORKTREE, then check for case (vi). Fail with a helpful error if any such case exists, and do not update any other paths.

(This is all before I read the rest, so I'm sorry if I repeat what you have already described.)

Quoted 8 lines

For git read-tree -mu HEAD, the behavior is as follows:
A) Materialize the file, as expected
B) Refuse to make *any* changes (to this path or any others)
C) Remove the file, as expected
D) Refuse to make *any* changes (to this path or any others)
E) Refuse to make *any* changes (to this path or any others)
F) Removes the file from the working copy AND resets the index to match HEAD

This case F is particularly bad, as we lose a staged change and its copy on disk. I believe that was the case that convinced me

to put in the requirement of a clean status before changing the sparse-checkout definition.

Quoted 24 lines

Refusing to modify files that could result in data loss as in cases B, D, and E is good, but refusing to work on any other paths is very problematic for users. If they are in the middle of a rebase or have made modifications to files that bring in more dependencies, then for their build to work they need to update the sparse paths -- which they are prevented from doing. Sometimes they stage the files and re-try, at which point they run into case F and believe that their changes are lost. (Even if they come ask an expert, they have to trawl through their loose objects looking for which ones match which files.)

Add a new update_sparsity() function which behaves as follows in these cases:
A) Materialize the file, as expected
B) Leave the file in the working copy alone, clear the SKIP_WORKTREE bit, and print a warning (thus leaving them in a state where status will report the file as modified, which seems logical).
C) Remove the file, as expected
D) Do NOT mark this path as SKIP_WORKTREE, but allow others paths to be updated.
E) Do NOT mark this path as SKIP_WORKTREE and print a warning about the dirty path, but allow other paths to be updated.
F) Mark the path as SKIP_WORKTREE, but do not revert the version stored in the index to match HEAD; leave the contents alone.

Quoted 4 lines

I tried a different behavior for B (leave the SKIP_WORKTREE bit set), but found it very surprising and counter-intuitive (e.g. the user sees it is present along with all the other files in that directory, tries to stage it, but git add ignores it since the SKIP_WORKTREE bit is set).

That sounds bad! I think you picked the right approach here.

Quoted 6 lines

A, B, C, and E all seem like optimal behavior to me. D may be as well, though I wonder if printing a warning would be an improvement. Some might be slightly surprised by F at first, but given that it does the right thing with git commit and even git commit -a (add ignores entries that are marked SKIP_WORKTREE and thus doesn't delete them, and commit -a is similar), it seems logical to me.

The established usage pattern is that a user can create "colliding" directories and files outside of the sparse cone and Git is expected to ignore them. The problem happens when the user adds those paths to the sparse cone. I think you handle the situations quite well with this plan.

Quoted 4 lines

unpack-trees.c +4 -0

+/*
+ * display all the "error" messages as warnings
+ */
+static void display_warning_msgs(struct unpack_trees_options *o)

unpack-trees.c static void display_error_msgs(struct unpack_trees_options *o) · line 267

Quoted 6 lines

unpack-trees.c +6 -0

+{
+     int e, i;
+     for (e = 0; e < NB_UNPACK_TREES_ERROR_TYPES; e++) {
+             struct string_list *rejects = &o->unpack_rejects[e];
+             if (rejects->nr > 0) {
+                     struct strbuf path = STRBUF_INIT;

nit: Define 'i' within the inner-most scope that it matters.

Quoted 14 lines

(quoted diff) +14 -0

+                     for (i = 0; i < rejects->nr; i++)
+                             strbuf_addstr(&path, rejects->items[i].string);
+                     warning(ERRORMSG(o, e), super_prefixed(path.buf));
+                     strbuf_release(&path);
+             }
+             string_list_clear(rejects, 0);
+     }
+}
+/*
+ * Update SKIP_WORKTREE bits according to sparsity patterns, and update
+ * working directory to match.
+ *
+ * Returns
+ *   0: success with no warnings

Review

Quoted 10 lines

(quoted diff) +10 -0

+ *   1: success with warnings (warnings come either from (a) dirty changes
+ *           present in which case we avoid marking those paths as
+ *           SKIP_WORKTREE, or (b) from untracked files being in the way us
+ *           checking a file out of the index, in which case we leave the file
+ *           in the working tree alone while clearing SKIP_WORKTREE)
+ *   -1: failure to manipulate the resulting index
+ *   -2: failure to reflect the changes to the work tree.
+ *
+ * CE_NEW_SKIP_WORKTREE is used internally.
+ */

I think I mentioned using an enum for this function in another patch, and reading the documentation here makes that even more clear. Anything other than {-1,0,1} return types really could use stronger typing. Even using {-1,0,1} instead of just {0,1} is debatable if it should use an enum instead.

I wonder if this patch could benefit a split where we first return {-2, -1, 0} and don't include any warnings, followed by a patch where the warnings are added (and tested).

Quoted 9 lines

unpack-trees.c +9 -0

+int update_sparsity(struct unpack_trees_options *o)
+{
+     struct pattern_list pl;
+     int i, empty_worktree, ret = 0;
+     unsigned old_show_all_errors;
+
+     old_show_all_errors = o->show_all_errors;
+     o->show_all_errors = 1;
+

unpack-trees.c int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options · line 1759

Quoted 5 lines

unpack-trees.c +5 -0

+     /* Sanity checks */
+     if (!o->update || o->index_only || o->skip_sparse_checkout)
+             BUG("update_sparsity() is for reflecting sparsity patterns in working directory");
+     if (o->src_index != o->dst_index || o->fn)
+             BUG("update_sparsity() called wrong");

I was going to add something about split indexes at the end, but I think this precondition has it covered.

Quoted 14 lines

unpack-trees.c +14 -0

+
+     trace_performance_enter();
+
+     if (!o->pl) {
+             char *sparse = git_pathdup("info/sparse-checkout");
+             memset(&pl, 0, sizeof(pl));
+             pl.use_cone_patterns = core_sparse_checkout_cone;
+             if (add_patterns_from_file_to_list(sparse, "", 0, &pl, NULL) < 0) {
+                     /* FIXME: Skip to check_updates()?? */
+                     o->skip_sparse_checkout = 1;
+                     goto skip_sparse_checkout;
+             } else
+                     o->pl = &pl;
+             free(sparse);

Quoted 1 line

+ }

Outside of the goto, this part looks identical to other parts of the same file where we "fallback" to the sparse-checkout file. Perhaps that could be extracted to a helper method? It could return failure when there is no in-memory pattern and the sparse-checkout file fails to load, and in response we can do the goto.

unpack-trees.c int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options · line 1788

Quoted 13 lines

unpack-trees.c +13 -0

+     /* Set NEW_SKIP_WORKTREE on existing entries. */
+     mark_all_ce_unused(o->src_index);
+     mark_new_skip_worktree(o->pl, o->src_index, 0,
+                            CE_NEW_SKIP_WORKTREE, o->verbose_update);
+
+     /* Then loop over entries and update/remove as needed */
+     ret = 0;
+     empty_worktree = 1;
+     for (i = 0; i < o->src_index->cache_nr; i++) {
+             struct cache_entry *ce = o->src_index->cache[i];
+
+             if (apply_sparse_checkout(o->src_index, ce, o))
+                     ret = 1;

So here, we report that at least one path has a warning.

Quoted 12 lines

unpack-trees.c +12 -0

+             if (!ce_skip_worktree(ce))
+                     empty_worktree = 0;
+
+     }
+
+     /*
+      * Sparse checkout is meant to narrow down checkout area
+      * but it does not make sense to narrow down to empty working
+      * tree. This is usually a mistake in sparse checkout rules.
+      * Do not allow users to do that.
+      */
+     if (o->src_index->cache_nr && empty_worktree) {

Quoted 1 line

+ ret = unpack_failed(o, "Sparse checkout leaves no entry on working directory");

Interesting that we are using the result of unpack_failed() here. I suppose that could be an argument for using an int instead of an enum, but perhaps we translate the result of unpack_failed() here? What sorts of values could it be, anyway? Is this a case where unpack_failed() could return either -1 or -2? Or should we simply set 'ret' to the "-1" case ourselves?

Quoted 26 lines

unpack-trees.c +14 -0

+             goto done;
+     }
+
+skip_sparse_checkout:
+     if (check_updates(o, o->src_index))
+             ret = -2;
+
+done:
+     display_warning_msgs(o);
+     o->show_all_errors = old_show_all_errors;
+     trace_performance_leave("update_sparsity");
+     return ret;
+}
+
 /* Here come the merge functions */
 
 static int reject_merge(const struct cache_entry *ce,

unpack-trees.h +0 -0

index ae1557fb804..5d1a77ae249 100644
@@ -90,6 +90,8 @@ struct unpack_trees_options {
 int unpack_trees(unsigned n, struct tree_desc *t,
               struct unpack_trees_options *options);
 

unpack-trees.h struct unpack_trees_options { · line 94

Quoted 2 lines

unpack-trees.h +2 -0

+int update_sparsity(struct unpack_trees_options *options);
+

Would it not be the pattern to include the documentation comment here in the header file?

In some ways, your update_sparsity() repeats some of the patterns of unpack_trees(), especially with its order of mark_all_ce_unused(), mark_new_skip_worktree(), and unpack_failed(). I think there may be some benefits to adding some of the performance tracing patterns from that method, too. These could be added as follow-up patches.

I'm happy that you decided to create a new method instead of adding yet another mode to unpack_trees(), especially because your version does not involve calling traverse_trees().

After careful inspection of your patch, I cannot find anything wrong. I'm pretty happy with your organization and the approach seems clear. Hopefully my comments in the commit message could help improve that part of the discussion.

At this point, the only thing we really need are tests that demonstrate as many of these cases as possible.

Thanks,
-Stolee

From: Elijah Newren <[email protected]>

commit e091228e17 ("sparse-checkout: update working directory in-process", 2019-11-21) allowed passing a pre-defined set of patterns to unpack_trees(). However, if o->pl was NULL, it would still read the existing patterns and use those. If those patterns were read into a data structure that was allocated, naturally they needed to be free'd. However, despite the same function being responsible for knowing about both the allocation and the free'ing, the logic for tracking whether to free the pattern_list was hoisted to an outer function with an additional flag in unpack_trees_options. Put the logic back in the relevant function and discard the now unnecessary flag.

Signed-off-by Elijah Newren <[email protected]>

---
 builtin/sparse-checkout.c | 1 -
 unpack-trees.c            | 6 ++++--
 unpack-trees.h            | 3 +--
 3 files changed, 5 insertions(+), 5 deletions(-)

builtin/sparse-checkout.c +0 -1

index 740da4b6d54..d102a9697fd 100644
@@ -122,7 +122,6 @@ static int update_working_directory(struct pattern_list *pl)
 	o.dst_index = r->index;
 	o.skip_sparse_checkout = 0;
 	o.pl = pl;
-	o.keep_pattern_list = !!pl;
 
 	resolve_undo_clear_index(r->index);
 	setup_work_tree();

unpack-trees.c +4 -2

index 3af2e126abf..d2863fa0310 100644
@@ -1503,6 +1503,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	int i, ret;
 	static struct cache_entry *dfc;
 	struct pattern_list pl;
+	int free_pattern_list = 0;
 
 	if (len > MAX_UNPACK_TREES)
 		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);

@@ -1519,6 +1520,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		else
 			o->pl = &pl;
 		free(sparse);
+		free_pattern_list = 1;
 	}
 
 	memset(&o->result, 0, sizeof(o->result));

@@ -1686,9 +1688,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	o->src_index = NULL;
 
 done:
-	trace_performance_leave("unpack_trees");
-	if (!o->keep_pattern_list)
+	if (free_pattern_list)
 		clear_pattern_list(&pl);
+	trace_performance_leave("unpack_trees");
 	return ret;
 
 return_failed:

unpack-trees.h +1 -2

index 6d7c7b6c2e0..d3516267f36 100644
@@ -58,8 +58,7 @@ struct unpack_trees_options {
 		     quiet,
 		     exiting_early,
 		     show_all_errors,
-		     dry_run,
-		     keep_pattern_list;
+		     dry_run;
 	const char *prefix;
 	int cache_bottom;
 	struct dir_struct *dir;

From: Elijah Newren <[email protected]>

Previously, the only way to update the SKIP_WORKTREE bits for various paths was invoking git read-tree -mu HEAD or calling the same code that this codepath invoked. This however had a number of problems if the index or working directory were not clean. First, let's consider the case:

Flipping SKIP_WORKTREE -> !SKIP_WORKTREE (materializing files)

If the working tree was clean this was fine, but if there were files or directories or symlinks or whatever already present at the given path then the operation would abort with an error. Let's label this case for later discussion:

A) There is an untracked path in the way

Now let's consider the opposite case:

Flipping !SKIP_WORKTREE -> SKIP_WORKTREE (removing files)

If the index and working tree was clean this was fine, but if there were any unclean paths we would run into problems. There are three different cases to consider:

B) The path is unmerged
C) The path has unstaged changes
D) The path has staged changes (differs from HEAD)

If any path fell into case B or C, then the whole operation would be aborted with an error. With sparse-checkout, the whole operation would be aborted for case D as well, but for its predecessor of using git read-tree -mu HEAD directly, any paths that fell into case D would be removed from the working copy and the index entry for that path would be reset to match HEAD -- which looks and feels like data loss to users (only a few are even aware to ask whether it can be recovered, and even then it requires walking through loose objects trying to match up the right ones).

Refusing to remove files that have unsaved user changes is good, but refusing to work on any other paths is very problematic for users. If the user is in the middle of a rebase or has made modifications to files that bring in more dependencies, then for their build to work they need to update the sparse paths. This logic has been preventing them from doing so. Sometimes in response, the user will stage the files and re-try, to no avail with sparse-checkout or to the horror of losing their changes if they are using its predecessor of git read-tree -mu HEAD.

Add a new update_sparsity() function which will not error out in any of these cases but behaves as follows for the special cases:
A) Leave the file in the working copy alone, clear the SKIP_WORKTREE bit, and print a warning (thus leaving the path in a state where status will report the file as modified, which seems logical).
B) Do NOT mark this path as SKIP_WORKTREE, and leave it as unmerged.
C) Do NOT mark this path as SKIP_WORKTREE and print a warning about the dirty path.
D) Mark the path as SKIP_WORKTREE, but do not revert the version stored in the index to match HEAD; leave the contents alone.

I tried a different behavior for A (leave the SKIP_WORKTREE bit set), but found it very surprising and counter-intuitive (e.g. the user sees it is present along with all the other files in that directory, tries to stage it, but git add ignores it since the SKIP_WORKTREE bit is set). A & C seem like optimal behavior to me. B may be as well, though I wonder if printing a warning would be an improvement. Some might be slightly surprised by D at first, but given that it does the right thing with git commit and even git commit -a (git add ignores entries that are marked SKIP_WORKTREE and thus doesn't delete them, and commit -a is similar), it seems logical to me.

Signed-off-by Elijah Newren <[email protected]>

---
 unpack-trees.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.h |  9 ++++++
 2 files changed, 87 insertions(+)

unpack-trees.c +14 -0

index 4733e7eaf89..6abea555929 100644
@@ -1714,6 +1714,84 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	goto done;
 }
 
+/*
+ * Update SKIP_WORKTREE bits according to sparsity patterns, and update
+ * working directory to match.
+ *
+ * CE_NEW_SKIP_WORKTREE is used internally.
+ */
+enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
+{
+	enum update_sparsity_result ret = UPDATE_SPARSITY_SUCCESS;
+	struct pattern_list pl;
+	int i, empty_worktree;
+	unsigned old_show_all_errors;
+	int free_pattern_list = 0;
+

unpack-trees.c +12 -0

+		populate_from_existing_patterns(o, &pl);
+		if (o->skip_sparse_checkout)
+			goto skip_sparse_checkout;
+	}
+
+	/* Set NEW_SKIP_WORKTREE on existing entries. */
+	mark_all_ce_unused(o->src_index);
+	mark_new_skip_worktree(o->pl, o->src_index, 0,
+			       CE_NEW_SKIP_WORKTREE, o->verbose_update);
+
+	/* Then loop over entries and update/remove as needed */
+	ret = UPDATE_SPARSITY_SUCCESS;

unpack-trees.c +27 -0

+	}
+
+	/*
+	 * Sparse checkout is meant to narrow down checkout area
+	 * but it does not make sense to narrow down to empty working
+	 * tree. This is usually a mistake in sparse checkout rules.
+	 * Do not allow users to do that.
+	 */
+	if (o->src_index->cache_nr && empty_worktree) {
+		unpack_failed(o, "Sparse checkout leaves no entry on working directory");
+		ret = UPDATE_SPARSITY_INDEX_UPDATE_FAILURES;
+		goto done;
+	}
+
+skip_sparse_checkout:
+	if (check_updates(o, o->src_index))
+		ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;
+
+done:
+	display_error_msgs(o);
+	o->show_all_errors = old_show_all_errors;
+	if (free_pattern_list)
+		clear_pattern_list(&pl);
+	trace_performance_leave("update_sparsity");
+	return ret;
+}
+
 /* Here come the merge functions */
 
 static int reject_merge(const struct cache_entry *ce,

unpack-trees.h +0 -0

index d3516267f36..2c5d54cae9f 100644

From: Elijah Newren <[email protected]>

Remove the equivalent of 'git read-tree -mu HEAD' in the sparse-checkout codepaths for setting the SKIP_WORKTREE bits and instead use the new update_sparsity() function.

t/t1091-sparse-checkout-builtin.sh +3 -5

index 8607a8e6d1a..86ae422ff5c 100755
@@ -277,16 +277,23 @@ test_expect_success 'cone mode: add parent path' '
 	check_files repo a deep folder1
 '
 
-test_expect_success 'revert to old sparse-checkout on bad update' '
+test_expect_success 'not-up-to-date does not block rest of sparsification' '
 	test_when_finished git -C repo sparse-checkout disable &&
 	test_when_finished git -C repo reset --hard &&
 	git -C repo sparse-checkout set deep &&
+
 	echo update >repo/deep/deeper2/a &&
 	cp repo/.git/info/sparse-checkout expect &&
-	test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
-	test_i18ngrep "cannot set sparse-checkout patterns" err &&
-	test_cmp repo/.git/info/sparse-checkout expect &&
-	check_files repo/deep a deeper1 deeper2
+	test_write_lines "!/deep/*/" "/deep/deeper1/" >>expect &&

t/t1091-sparse-checkout-builtin.sh +4 -4

 test_expect_success 'revert to old sparse-checkout on empty update' '

@@ -316,12 +323,22 @@ test_expect_success '.gitignore should not warn about cone mode' '
 	test_i18ngrep ! "disabling cone patterns" err
 '
 
-test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status' '
+test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' '
 	git clone repo dirty &&
 	echo dirty >dirty/folder1/a &&
-	test_must_fail git -C dirty sparse-checkout init &&
-	test_must_fail git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
-	test_must_fail git -C dirty sparse-checkout disable &&
+
+	git -C dirty sparse-checkout init 2>err &&
+	test_i18ngrep "error" err &&

unpack-trees.c int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options · line 1767

Quoted 9 lines · Elijah Newren via GitGitGadget

unpack-trees.c +9 -0

+     for (i = 0; i < o->src_index->cache_nr; i++) {
+             struct cache_entry *ce = o->src_index->cache[i];
+
+             if (apply_sparse_checkout(o->src_index, ce, o))
+                     ret = UPDATE_SPARSITY_WARNINGS;
+
+             if (!ce_skip_worktree(ce))
+                     empty_worktree = 0;
+

nit: extra whitespace-only line

Quoted 38 lines

unpack-trees.c +27 -0

+     }
+
+     /*
+      * Sparse checkout is meant to narrow down checkout area
+      * but it does not make sense to narrow down to empty working
+      * tree. This is usually a mistake in sparse checkout rules.
+      * Do not allow users to do that.
+      */
+     if (o->src_index->cache_nr && empty_worktree) {
+             unpack_failed(o, "Sparse checkout leaves no entry on working directory");
+             ret = UPDATE_SPARSITY_INDEX_UPDATE_FAILURES;
+             goto done;
+     }
+
+skip_sparse_checkout:
+     if (check_updates(o, o->src_index))
+             ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;
+
+done:
+     display_error_msgs(o);
+     o->show_all_errors = old_show_all_errors;
+     if (free_pattern_list)
+             clear_pattern_list(&pl);
+     trace_performance_leave("update_sparsity");
+     return ret;
+}
+
 /* Here come the merge functions */
 
 static int reject_merge(const struct cache_entry *ce,

unpack-trees.h +0 -0

index d3516267f36..2c5d54cae9f 100644
@@ -28,6 +28,13 @@ enum unpack_trees_error_types {
      NB_UNPACK_TREES_ERROR_TYPES
 };
 

unpack-trees.h enum unpack_trees_error_types { · line 37

Quoted 7 lines

unpack-trees.h +7 -0

+enum update_sparsity_result {
+     UPDATE_SPARSITY_SUCCESS = 0,
+     UPDATE_SPARSITY_WARNINGS = 1,
+     UPDATE_SPARSITY_INDEX_UPDATE_FAILURES = -1,
+     UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
+};
+

Is there a reason this isn't located just before
update_sparsity()?

unpack-trees.h struct unpack_trees_options { · line 99

Quoted 9 lines

unpack-trees.h +2 -0

 /*
  * Sets the list of user-friendly error messages to be used by the
  * command "cmd" (either merge or checkout), and show_all_errors to 1.

@@ -88,6 +95,8 @@ struct unpack_trees_options {
 int unpack_trees(unsigned n, struct tree_desc *t,
               struct unpack_trees_options *options);
 
+int update_sparsity(struct unpack_trees_options *options);
+

This appears to not use the enum as it should.

Quoted 6 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Remove the equivalent of 'git read-tree -mu HEAD' in the sparse-checkout codepaths for setting the SKIP_WORKTREE bits and instead use the new update_sparsity() function.

[snip]

Quoted 21 lines

t/t1091-sparse-checkout-builtin.sh +3 -5

index 8607a8e6d1a..86ae422ff5c 100755
@@ -277,16 +277,23 @@ test_expect_success 'cone mode: add parent path' '
      check_files repo a deep folder1
 '
 
-test_expect_success 'revert to old sparse-checkout on bad update' '
+test_expect_success 'not-up-to-date does not block rest of sparsification' '
      test_when_finished git -C repo sparse-checkout disable &&
      test_when_finished git -C repo reset --hard &&
      git -C repo sparse-checkout set deep &&
+
      echo update >repo/deep/deeper2/a &&
      cp repo/.git/info/sparse-checkout expect &&
-     test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
-     test_i18ngrep "cannot set sparse-checkout patterns" err &&
-     test_cmp repo/.git/info/sparse-checkout expect &&
-     check_files repo/deep a deeper1 deeper2
+     test_write_lines "!/deep/*/" "/deep/deeper1/" >>expect &&

t/t1091-sparse-checkout-builtin.sh test_expect_success 'cone mode: add parent path' ' · line 297

Quoted 10 lines

t/t1091-sparse-checkout-builtin.sh +9 -0

+
+     git -C repo sparse-checkout set deep/deeper1 2>err &&
+
+     test_i18ngrep "Cannot update sparse checkout" err &&
+     test_cmp expect repo/.git/info/sparse-checkout &&
+     check_files repo/deep a deeper1 deeper2 &&
+     check_files repo/deep/deeper1 a deepest &&
+     check_files repo/deep/deeper1/deepest a &&
+     check_files repo/deep/deeper2 a
 '

This demonstrates some of the value of using update_sparsity. I expect some more specific tests are coming later in the series.

Quoted 20 lines

t/t1091-sparse-checkout-builtin.sh +9 -4

 test_expect_success 'revert to old sparse-checkout on empty update' '

@@ -316,12 +323,22 @@ test_expect_success '.gitignore should not warn about cone mode' '
      test_i18ngrep ! "disabling cone patterns" err
 '
 
-test_expect_success 'sparse-checkout (init|set|disable) fails with dirty status' '
+test_expect_success 'sparse-checkout (init|set|disable) warns with dirty status' '
      git clone repo dirty &&
      echo dirty >dirty/folder1/a &&
-     test_must_fail git -C dirty sparse-checkout init &&
-     test_must_fail git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
-     test_must_fail git -C dirty sparse-checkout disable &&
+
+     git -C dirty sparse-checkout init 2>err &&
+     test_i18ngrep "error" err &&
+     test_i18ngrep "Cannot update sparse checkout" err &&
+
+     git -C dirty sparse-checkout set /folder2/* /deep/deeper1/* &&
+     test_i18ngrep "error" err &&
+     test_i18ngrep "Cannot update sparse checkout" err &&

t/t1091-sparse-checkout-builtin.sh test_expect_success '.gitignore should not warn about cone mode' ' · line 341

Quoted 5 lines

t/t1091-sparse-checkout-builtin.sh +5 -0

+
+     git -C dirty sparse-checkout disable &&
+     test_i18ngrep "error" err &&
+     test_i18ngrep "Cannot update sparse checkout" err &&
+

Should the "error" messages we grep for be more specific? Are they the same as the "Cannot update sparse checkout"?

Quoted 12 lines · Derrick Stolee

unpack-trees.h enum unpack_trees_error_types { · line 37

Quoted 7 lines · Elijah Newren via GitGitGadget

unpack-trees.h +7 -0

+enum update_sparsity_result {
+     UPDATE_SPARSITY_SUCCESS = 0,
+     UPDATE_SPARSITY_WARNINGS = 1,
+     UPDATE_SPARSITY_INDEX_UPDATE_FAILURES = -1,
+     UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
+};
+

Is there a reason this isn't located just before
update_sparsity()?

You mean move it to unpack-trees.c? If I did that, the code in sparse-checkout.c that uses two of these values would fail to compile.

Quoted 4 lines

unpack-trees.h struct unpack_trees_options { · line 99

Quoted 2 lines

unpack-trees.h +2 -0

+int update_sparsity(struct unpack_trees_options *options);
+

This appears to not use the enum as it should.

Whoops! Will fix. (Interesting that the compiler didn't flag any kind of warning based on mismatch of declared function return types for update_sparsity() in the .c and .h files...)

Quoted 3 lines · Derrick Stolee

Should the "error" messages we grep for be more specific? Are they the same as the "Cannot update sparse checkout"?

The two lines are checking for the same message, it's just that the error message is split across multiple lines and thus requires two greps. The error messages are actually a bit of a mess, particularly when they are triggered for multiple paths. That's something that subsequent commits will clean up.

I can add a note about this to the commit message.