In-tree sparse-checkout definitions

From: Derrick Stolee <[email protected]>

The loop in warn_conflicted_path() that checks for the count of entries with the same path uses "i+count" for the array entry. However, the loop only verifies that the value of count is below the array size. Fix this by adding i to the condition.

I hit this condition during a test of the in-tree sparse-checkout feature, so it is exercised by the end of the series.

Signed-off-by Derrick Stolee <[email protected]>

---
 unpack-trees.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

unpack-trees.c +0 -0

index 9a3ccd9d083..4f880f2da90 100644

unpack-trees.c static int warn_conflicted_path(struct index_state *istate, · line 569

Quoted 13 lines · Derrick Stolee via GitGitGadget

unpack-trees.c +1 -1

index 9a3ccd9d083..4f880f2da90 100644
@@ -563,10 +563,11 @@ static int warn_conflicted_path(struct index_state *istate,
 	add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
 
 	/* Find out how many higher stage entries at same path */
-	while (++count < istate->cache_nr &&
+	while (i + ++count < istate->cache_nr &&
 	       !strcmp(conflicting_path,
 		       istate->cache[i+count]->name))
 		/* do nothing */;

Eek. Yes, it is obvious that the original is wrong once you point it out. But "i + ++count" looks like a line noise, and funny way that lines are wrapped in the original does not help X-<.

We may want to fix the style and the grammar while we are at it, perhaps like the attached.

In any case, thanks for a fix.

 unpack-trees.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

unpack-trees.c +0 -0

index 6bbf58d28e..c38938d96c 100644
@@ -562,11 +562,11 @@ static int warn_conflicted_path(struct index_state *istate,
 
 	add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
 

As I mentioned in the cover letter, this is worth taking on its own. Could you queue the collaborative patch? I'll eject it from the next version of this series.

Quoted 22 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

The loop in warn_conflicted_path() that checks for the count of entries with the same path uses "i+count" for the array entry. However, the loop only verifies that the value of count is below the array size. Fix this by adding i to the condition.

I hit this condition during a test of the in-tree sparse-checkout feature, so it is exercised by the end of the series.

Signed-off-by Derrick Stolee <[email protected]>

---
 unpack-trees.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

unpack-trees.c +0 -0

index 9a3ccd9d083..4f880f2da90 100644
@@ -563,10 +563,11 @@ static int warn_conflicted_path(struct index_state *istate,
      add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
 

unpack-trees.c static int warn_conflicted_path(struct index_state *istate, · line 569

Quoted 6 lines

unpack-trees.c +1 -1

      /* Find out how many higher stage entries at same path */
-     while (++count < istate->cache_nr &&
+     while (i + ++count < istate->cache_nr &&
             !strcmp(conflicting_path,
                     istate->cache[i+count]->name))
              /* do nothing */;

Eek. Yes, it is obvious that the original is wrong once you point it out. But "i + ++count" looks like a line noise, and funny way that lines are wrapped in the original does not help X-<.

We may want to fix the style and the grammar while we are at it, perhaps like the attached.

In any case, thanks for a fix.

 unpack-trees.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

unpack-trees.c +0 -2

index 6bbf58d28e..c38938d96c 100644
@@ -562,11 +562,11 @@ static int warn_conflicted_path(struct index_state *istate,
 
        add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path);
 
-       /* Find out how many higher stage entries at same path */
-       while (++count < istate->cache_nr &&

One of the difficulties of using the sparse-checkout feature is not knowing which directories are absolutely needed for working in a portion of the repository. Some of this can be documented in README files or included in a bootstrapping tool along with the repository. This is done in an ad-hoc way by every project that wants to use it.

Let's make this process easier for users by creating a way to define a useful sparse-checkout definition inside the Git tree data. This has several benefits. In particular, the data is available to anyone who has a copy of the repository without needing a different data source. Second, the needs of the repository can change over time and Git can present a way to automatically update the working directory as these sparse-checkout definitions change over time.

To use this feature, add the "--in-tree" option when setting or adding directories to the sparse-checkout definition. For example:

  $ git sparse-checkout set --in-tree .sparse/base
  $ git sparse-checkout add --in-tree .sparse/extra

These commands add values to the multi-valued config setting "sparse.inTree". When updating the sparse-checkout definition, these values describe paths in the repository to find the sparse-checkout data. After the commands listed earlier, we expect to see the following in .git/config.worktree:

[sparse]
intree = .sparse/base
intree = .sparse/extra

When applying the sparse-checkout definitions from this config, the blobs at HEAD:.sparse/base and HEAD:.sparse/extra are loaded.

In those
files, the multi-valued config values "sparse.dir" are considered as the directories to construct a cone mode sparse-checkout file. The end result is as if these paths were provided to "git sparse-checkout set" in cone mode.

For example, suppose .sparse/base had the following content:

[sparse]
dir = A
dir = B/C
dir = D/E/F

and .sparse/extra had the following content:

[sparse]
dir = D
dir = X

Then, the output of "git sparse-checkout list" would be

	A
	B/C
	D
	X

Note that since "D" contains "D/E/F", that directory replaces the position of "D/E/F" in the list.

Since these are parsed using the config library, the parser is robust enough to understand comments and complicated string values.

The key benefit to this approach is that it can be extended by defining new config values. In a later change, we will introduce "sparse.inherit" to point to another file in the tree. This will solve the problem of editing many files when core dependencies change.

Quoted 6 lines · Derrick Stolee via GitGitGadget

One of the difficulties of using the sparse-checkout feature is not knowing which directories are absolutely needed for working in a portion of the repository. Some of this can be documented in README files or included in a bootstrapping tool along with the repository. This is done in an ad-hoc way by every project that wants to use it.

Quoted 7 lines

Let's make this process easier for users by creating a way to define a useful sparse-checkout definition inside the Git tree data. This has several benefits. In particular, the data is available to anyone who has a copy of the repository without needing a different data source. Second, the needs of the repository can change over time and Git can present a way to automatically update the working directory as these sparse-checkout definitions change over time.

And two lines of development can merge them together?

Any time a new "feature" pops up that would eventually affect how "git clone" and "git checkout" work based on untrusted user data, we need to make sure there is no negative security implications.

If it only boils down to "we have files that can record list of leading directory names and without offering extra 'flexibility'", I guess there aren't all that much that a malicious sparse definition can do and we would be safe, though.

Quoted 12 lines

To use this feature, add the "--in-tree" option when setting or adding directories to the sparse-checkout definition. For example:

  $ git sparse-checkout set --in-tree .sparse/base
  $ git sparse-checkout add --in-tree .sparse/extra

These commands add values to the multi-valued config setting "sparse.inTree". When updating the sparse-checkout definition, these values describe paths in the repository to find the sparse-checkout data. After the commands listed earlier, we expect to see the following in .git/config.worktree:

Quoted 3 lines

[sparse]
intree = .sparse/base
intree = .sparse/extra

What does this say in human words? "These two tracked files specify which paths should be in the working tree"? Spelling it out here would help readers of this commit.

Quoted 2 lines

When applying the sparse-checkout definitions from this config, the blobs at HEAD:.sparse/base and HEAD:.sparse/extra are loaded.

OK, so end-user edit to the working tree copy or what is added to the index does not count and only the committed version gets used.

That makes it simple---I was wondering how we would operate when merging a branch with different contents in the .sparse/* files until the conflicts are resolved.

Quoted 5 lines

In those
files, the multi-valued config values "sparse.dir" are considered as the directories to construct a cone mode sparse-checkout file. The end result is as if these paths were provided to "git sparse-checkout set" in cone mode.

OK.

Quoted 23 lines

For example, suppose .sparse/base had the following content:

[sparse]
dir = A
dir = B/C
dir = D/E/F

and .sparse/extra had the following content:

[sparse]
dir = D
dir = X

Then, the output of "git sparse-checkout list" would be

	A
	B/C
	D
	X

Note that since "D" contains "D/E/F", that directory replaces the position of "D/E/F" in the list.

Quoted 7 lines

Since these are parsed using the config library, the parser is robust enough to understand comments and complicated string values.

The key benefit to this approach is that it can be extended by defining new config values. In a later change, we will introduce "sparse.inherit" to point to another file in the tree. This will solve the problem of editing many files when core dependencies change.

With only a multi-valued sparse.dir elements allowed in these in-tree .sparse/* files, I guess there isn't much mischeaf a malicious .sparse/* file can do. Can it try to [includeIf] some paths external to the repository to cause a remote attacker to learn about the paths on the local system, perhaps?

Documentation/config/sparse.txt line 6

Quoted 13 lines

Documentation/config/sparse.txt new file +6 -0

new file mode 100644
index 00000000000..c1fce87cd33
@@ -0,0 +1,15 @@
+sparse.inTree::
+	The `core.sparseCheckout` config option enables the `sparse-checkout`
+	feature, but if there are any values for the multi-valued
+	`sparse.inTree` config option, then the sparse-checkout patterns are
+	defined by parsing the files listed in these values. See
+	linkgit:git-sparse-checkout[1] for more information.

Does "... but if ... then" mean "by parsing the files and ignoring all other things that may otherwise define patterns"?

Documentation/config/sparse.txt line 15

Quoted 8 lines

Documentation/config/sparse.txt new file +8 -0

+sparse.dir::
+	This config setting is ignored if present in the repository config.
+	Instead, this multi-valued option is present in the files listed by
+	`sparse.inTree` and specifies the directories needed in the
+	working directory. The union of all `sparse.dir` values across all
+	`sparse.inTree` files forms the input for `git sparse-checkout set`
+	in cone mode.  See linkgit:git-sparse-checkout[1] for more
+	information.

If this is *not* a config in the usual sense, we probably should not include it in this document and in the "git config --help" output. That will allow us to drop the first sentence.

Those .sparse/* in-tree files are like .gitmodules in the sense that they happen to use the same syntax so that the parser can be shared, but they are not allowed to affect end-user configuration (e.g. writing "[diff] external=rm -fr ." in the file has no effect) at all, right?

And we should describe what we can write in these in-tree files separately and make it clear that they are _different_ from the configuration variables.

Quoted 18 lines · Junio C Hamano

Quoted 6 lines · Derrick Stolee via GitGitGadget

One of the difficulties of using the sparse-checkout feature is not knowing which directories are absolutely needed for working in a portion of the repository. Some of this can be documented in README files or included in a bootstrapping tool along with the repository. This is done in an ad-hoc way by every project that wants to use it.

Quoted 7 lines

Let's make this process easier for users by creating a way to define a useful sparse-checkout definition inside the Git tree data. This has several benefits. In particular, the data is available to anyone who has a copy of the repository without needing a different data source. Second, the needs of the repository can change over time and Git can present a way to automatically update the working directory as these sparse-checkout definitions change over time.

And two lines of development can merge them together?

Quoted 8 lines

Any time a new "feature" pops up that would eventually affect how "git clone" and "git checkout" work based on untrusted user data, we need to make sure there is no negative security implications.

If it only boils down to "we have files that can record list of leading directory names and without offering extra 'flexibility'", I guess there aren't all that much that a malicious sparse definition can do and we would be safe, though.

Yes. I hope that we can be extremely careful with this feature. The RFC status of this series implicitly includes the question "Should we do this at all?" I think the benefits outweigh the risks, but we can minimize those risks with very careful design and implementation.

Quoted 12 lines

Quoted 12 lines

To use this feature, add the "--in-tree" option when setting or adding directories to the sparse-checkout definition. For example:

  $ git sparse-checkout set --in-tree .sparse/base
  $ git sparse-checkout add --in-tree .sparse/extra

These commands add values to the multi-valued config setting "sparse.inTree". When updating the sparse-checkout definition, these values describe paths in the repository to find the sparse-checkout data. After the commands listed earlier, we expect to see the following in .git/config.worktree:

Quoted 7 lines

Quoted 3 lines

[sparse]
intree = .sparse/base
intree = .sparse/extra

What does this say in human words? "These two tracked files specify which paths should be in the working tree"? Spelling it out here would help readers of this commit.

You got it. Sounds good.

Quoted 3 lines

Quoted 2 lines

When applying the sparse-checkout definitions from this config, the blobs at HEAD:.sparse/base and HEAD:.sparse/extra are loaded.

Quoted 6 lines

OK, so end-user edit to the working tree copy or what is added to the index does not count and only the committed version gets used.

That makes it simple---I was wondering how we would operate when merging a branch with different contents in the .sparse/* files until the conflicts are resolved.

It's worth testing this case so we can be sure what happens.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

MERGE_HEAD

Quoted 60 lines · Derrick Stolee

Quoted 18 lines · Junio C Hamano

Quoted 6 lines · Derrick Stolee via GitGitGadget

One of the difficulties of using the sparse-checkout feature is not knowing which directories are absolutely needed for working in a portion of the repository. Some of this can be documented in README files or included in a bootstrapping tool along with the repository. This is done in an ad-hoc way by every project that wants to use it.

Quoted 7 lines

Let's make this process easier for users by creating a way to define a useful sparse-checkout definition inside the Git tree data. This has several benefits. In particular, the data is available to anyone who has a copy of the repository without needing a different data source. Second, the needs of the repository can change over time and Git can present a way to automatically update the working directory as these sparse-checkout definitions change over time.

And two lines of development can merge them together?

Quoted 8 lines

Any time a new "feature" pops up that would eventually affect how "git clone" and "git checkout" work based on untrusted user data, we need to make sure there is no negative security implications.

If it only boils down to "we have files that can record list of leading directory names and without offering extra 'flexibility'", I guess there aren't all that much that a malicious sparse definition can do and we would be safe, though.

Yes. I hope that we can be extremely careful with this feature. The RFC status of this series implicitly includes the question "Should we do this at all?" I think the benefits outweigh the risks, but we can minimize those risks with very careful design and implementation.

Quoted 12 lines

Quoted 12 lines

To use this feature, add the "--in-tree" option when setting or adding directories to the sparse-checkout definition. For example:

  $ git sparse-checkout set --in-tree .sparse/base
  $ git sparse-checkout add --in-tree .sparse/extra

These commands add values to the multi-valued config setting "sparse.inTree". When updating the sparse-checkout definition, these values describe paths in the repository to find the sparse-checkout data. After the commands listed earlier, we expect to see the following in .git/config.worktree:

Quoted 7 lines

Quoted 3 lines

[sparse]
intree = .sparse/base
intree = .sparse/extra

What does this say in human words? "These two tracked files specify which paths should be in the working tree"? Spelling it out here would help readers of this commit.

You got it. Sounds good.

Quoted 3 lines

Quoted 2 lines

When applying the sparse-checkout definitions from this config, the blobs at HEAD:.sparse/base and HEAD:.sparse/extra are loaded.

Quoted 8 lines

Quoted 6 lines

OK, so end-user edit to the working tree copy or what is added to the index does not count and only the committed version gets used.

That makes it simple---I was wondering how we would operate when merging a branch with different contents in the .sparse/* files until the conflicts are resolved.

It's worth testing this case so we can be sure what happens.

During a merge or rebase or checkout -m, what happens if .sparse/extra has the following working tree content:

[sparse]
dir = D
dir = X
<<<<<< HEAD
dir = Y
|||||| MERGE_BASE
======
inherit = .sparse/tools

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

MERGE_HEAD

inherit = .sparse/base

and, of course, three different entries in the index?

Also, do we use the version of the --in-tree file from the latest commit, from the index, or from the working tree? (This is a question not only for merge and rebase, but also checkout with dirty changes and even checkout -m.) Which one "wins"?

And what if the user updates and commits an ill-formed version of the file -- is it equivalent to getting an empty cone with just the toplevel directory, equivalent to getting a complete checkout of everything, or something else?

Son pointed out that mercurial has a 'sparse' extension that has some possible ideas of things we could do here; see
https://lore.kernel.org/git/CABPp-BGLBmWXrmPsTogyBFMgwYbHjN39oWbU=qDWroU1_fJaoQ@mail.gmail.com/
for some further discussion.

From: Derrick Stolee <[email protected]>

A benefit of having sparse-checkouts defined in the tree data is that the sparse-checkout definition can change over time. To take advantage of that data, a user could run "git sparse-checkout reapply" whenever they thought the sparse-checkout definition was out of date. That is likely too manual of a process, since a user would probably only discover this after a failure to accomplish their goal, such as a build failure.

Prevent user frustration by automatically updating the sparse-checkout definition after changing the index when an in-tree definition is provided by config. This will happen during every index change, including every step of a rebase, including conflict states.

Special care was needed around the --no-sparse-checkout option in "git read-tree". This previously relied on changing the skip_sparse_checkout option in "struct unpack_trees_options" to prevent applying the skip-worktree bits. However, now that we make a second update to the index focusing on the skip-worktree bits, this needs to be prevented in another way. The simplest thing to do was disable the feature through the core_apply_sparse_checkout global variable.

Signed-off-by Derrick Stolee <[email protected]>

---
 Documentation/git-sparse-checkout.txt |  7 ++++---
 builtin/commit.c                      |  4 +++-
 builtin/read-tree.c                   |  4 ++++
 read-cache.c                          |  8 +++++---
 sparse-checkout.c                     | 26 +++++++++++++++++++++++++-
 sparse-checkout.h                     |  1 +
 t/t1091-sparse-checkout-builtin.sh    | 19 +++++++++++++++----
 7 files changed, 57 insertions(+), 12 deletions(-)

Documentation/git-sparse-checkout.txt +4 -3

index da9322c5e41..c1713ebb1d2 100644
@@ -233,9 +233,10 @@ Use `git sparse-checkout set --in-tree <path>` to initialize the patterns
 to those included in the file at `<path>`. This will override any existing
 patterns you have in your sparse-checkout file.
 
-After switching between commits with different versions of this file, run
-`git sparse-checkout reapply` to adjust the sparse-checkout patterns to
-the new definition.
+As Git switches between commits, it will update the in-tree sparse-checkout
+definition according to the files available at the new commit. If any of
+the specified files do not exist at the new commit, then the sparse-checkout
+definition will not change.
 
 
 SUBMODULES

builtin/commit.c +1 -0

index 7ba33a3bec4..0eab8d74469 100644
@@ -36,6 +36,7 @@
 #include "help.h"
 #include "commit-reach.h"
 #include "commit-graph.h"
+#include "sparse-checkout.h"
 
 static const char * const builtin_commit_usage[] = {
        N_("git commit [<options>] [--] <pathspec>..."),

builtin/read-tree.c +4 -0

index af7424b94c8..9ae81ffffa1 100644
@@ -247,6 +247,10 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
                parse_tree(tree);
                init_tree_desc(t+i, tree->buffer, tree->size);
        }
+
+       if (opts.skip_sparse_checkout)
+               core_apply_sparse_checkout = 0;
+
        if (unpack_trees(nr_trees, t, &opts))
                return 128;
 

read-cache.c +5 -3

index aa427c5c170..150e73feb0d 100644
@@ -25,6 +25,7 @@
 #include "fsmonitor.h"
 #include "thread-utils.h"
 #include "progress.h"
+#include "sparse-checkout.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 

@@ -3074,9 +3075,10 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
 
        if (ret)
                return ret;
-       if (flags & COMMIT_LOCK)
-               ret = commit_locked_index(lock);
-       else
+       if (flags & COMMIT_LOCK) {
+               ret = commit_locked_index(lock) ||
+                     update_in_tree_sparse_checkout();
+       } else
                ret = close_lock_file_gently(lock);
 
        run_hook_le(NULL, "post-index-change",

sparse-checkout.c +25 -1

index d6c27ca19c4..6c58fda9722 100644
@@ -92,9 +92,12 @@ int load_in_tree_pattern_list(struct repository *r,
                 * Exit silently, as this is likely the case where Git
                 * changed branches to a location where the inherit file
                 * does not exist. Do not update the sparse-checkout.
+                *
+                * Use -1 return to ensure populate_from_existing_patterns()
+                * skips the sparse-checkout updates.
                 */
                if (pos < 0)
-                       return 1;
+                       return -1;
 
                oid = &istate->cache[pos]->oid;
                type = oid_object_info(r, oid, NULL);

@@ -145,6 +148,7 @@ int populate_sparse_checkout_patterns(struct pattern_list *pl)
        return result;
 }
 
+static int updating_sparse_checkout = 0;
 int update_working_directory(struct pattern_list *pl)
 {
        enum update_sparsity_result result;

@@ -152,6 +156,10 @@ int update_working_directory(struct pattern_list *pl)
        struct lock_file lock_file = LOCK_INIT;
        struct repository *r = the_repository;
 
+       if (updating_sparse_checkout)
+               return 0;
+       updating_sparse_checkout = 1;
+
        memset(&o, 0, sizeof(o));
        o.verbose_update = isatty(2);
        o.update = 1;

@@ -180,9 +188,24 @@ int update_working_directory(struct pattern_list *pl)
        else
                rollback_lock_file(&lock_file);
 
+       updating_sparse_checkout = 0;
        return result;
 }
 
+int update_in_tree_sparse_checkout(void)
+{
+       const char *first_value;
+
+       if (!core_apply_sparse_checkout)
+               return 0;
+
+       /* only update if doing so due to sparse.inTree. */
+       if (!git_config_get_value(SPARSE_CHECKOUT_IN_TREE, &first_value) &&
+           first_value)
+               return update_working_directory(NULL);
+       return 0;
+}
+
 static char *escaped_pattern(char *pattern)
 {
        char *p = pattern;

@@ -273,6 +296,7 @@ int write_patterns_and_update(struct pattern_list *pl)
                free(sparse_filename);
                clear_pattern_list(pl);
                update_working_directory(NULL);
+               updating_sparse_checkout = 0;
                return result;
        }
 

sparse-checkout.h +1 -0

index 993a5701a60..fb0ba48524a 100644
@@ -13,6 +13,7 @@ char *get_sparse_checkout_filename(void);
 int populate_sparse_checkout_patterns(struct pattern_list *pl);
 void write_patterns_to_file(FILE *fp, struct pattern_list *pl);
 int update_working_directory(struct pattern_list *pl);
+int update_in_tree_sparse_checkout(void);
 int write_patterns(struct pattern_list *pl, int and_update);
 int write_patterns_and_update(struct pattern_list *pl);
 void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *path);

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

index 1040bf9c261..fdaafba5377 100755
@@ -605,6 +605,7 @@ test_expect_success MINGW 'cone mode replaces backslashes with slashes' '
 '
 
 test_expect_success 'basis of --in-tree' '
+       git -C repo branch no-in-tree &&
        git -C repo config auto.crlf false &&
        cat >folder1 <<-\EOF &&
        [sparse]

@@ -690,18 +691,28 @@ test_expect_success '"add" with --in-tree' '
        check_files repo a deep folder1
 '
 
-test_expect_success 'reapply after updating in-tree file' '
+test_expect_success 'automatically change after updating in-tree file' '
        git -C repo sparse-checkout set --in-tree .sparse/sparse &&
        check_files repo a &&
        test_path_is_dir repo/.sparse &&
-       echo "\tdir = folder1" >>repo/.sparse/sparse &&
+       printf "\tdir = folder1\n" >>repo/.sparse/sparse &&
        git -C repo commit -a -m "Update sparse file" &&
-       git -C repo sparse-checkout reapply &&
        check_files repo a folder1 &&
        test_path_is_dir repo/.sparse &&
        git -C repo checkout HEAD~1 &&
-       git -C repo sparse-checkout reapply &&
        check_files repo a &&
+       test_path_is_dir repo/.sparse &&
+       git -C repo checkout - &&
+       check_files repo a folder1 &&
+       test_path_is_dir repo/.sparse
+'
+
+test_expect_success 'keep definition when in-tree file is missing' '
+       git -C repo checkout no-in-tree &&
+       check_files repo a folder1 &&
+       test_path_is_missing repo/.sparse &&
+       git -C repo checkout - &&
+       check_files repo a folder1 &&
        test_path_is_dir repo/.sparse
 '
 
--

gitgitgadget

From: Derrick Stolee <[email protected]>

As we parse the in-tree config files that store the sparse.dir values used to create an in-tree sparse-checkout definition, we can easily avoid parsing the same file multiple times by using an oidset on those blobs. We only parse if the oid is new to the oidset.

This is unlikely to have a major performance benefit right now, but will be extremely important when we introduce the sparse.inherit options to link multiple files in a directed graph. This oidset will prevent infinite loops when cycles exist in that digraph, or exponential blowups even in the case of a directed acyclic graph.

Signed-off-by Derrick Stolee <[email protected]>

---
 sparse-checkout.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

sparse-checkout.c +19 -6

index 6c58fda9722..d01f4d7b525 100644
@@ -9,6 +9,7 @@
 #include "string-list.h"
 #include "unpack-trees.h"
 #include "object-store.h"
+#include "oidset.h"
 
 char *get_sparse_checkout_filename(void)
 {

@@ -77,9 +78,12 @@ int load_in_tree_pattern_list(struct repository *r,
                              struct string_list *sl,
                              struct pattern_list *pl)
 {
+       int result = 0;
        struct index_state *istate = r->index;
        struct string_list_item *item;
        struct strbuf path = STRBUF_INIT;
+       struct oidset set;
+       oidset_init(&set, 16);
 
        pl->use_cone_patterns = 1;
 

@@ -96,24 +100,34 @@ int load_in_tree_pattern_list(struct repository *r,
                 * Use -1 return to ensure populate_from_existing_patterns()
                 * skips the sparse-checkout updates.
                 */
-               if (pos < 0)
-                       return -1;
+               if (pos < 0) {
+                       result = -1;
+                       goto cleanup;
+               }
 
                oid = &istate->cache[pos]->oid;
+
+               if (oidset_contains(&set, oid))
+                       continue;
+
+               oidset_insert(&set, oid);
+
                type = oid_object_info(r, oid, NULL);
 
                if (type != OBJ_BLOB) {
                        warning(_("expected a file at '%s'; not updating sparse-checkout"),
                                oid_to_hex(oid));
-                       return 1;
+                       result = 1;
+                       goto cleanup;
                }
 
                load_in_tree_from_blob(pl, oid);
        }
 
+cleanup:
        strbuf_release(&path);
-
-       return 0;
+       oidset_clear(&set);
+       return result;
 }
 
 int populate_sparse_checkout_patterns(struct pattern_list *pl)
--

gitgitgadget

Quoted 8 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

As we parse the in-tree config files that store the sparse.dir values used to create an in-tree sparse-checkout definition, we can easily avoid parsing the same file multiple times by using an oidset on those blobs. We only parse if the oid is new to the oidset.

Quoted 5 lines

This is unlikely to have a major performance benefit right now, but will be extremely important when we introduce the sparse.inherit options to link multiple files in a directed graph. This oidset will prevent infinite loops when cycles exist in that digraph, or exponential blowups even in the case of a directed acyclic graph.

I'm still not sure if I like the idea of having a mirror dependency structure separate from (and duplicative of) the build code; I'm still mulling that over.

It's good that you've protected against infinite loops.

Is there any reason to prefer swallowing infinite loops rather than warning or flagging as an error? (I'm not sure, just thinking my questions out loud.)

Replying to my own questions...

Quoted 17 lines · Elijah Newren

Quoted 8 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

As we parse the in-tree config files that store the sparse.dir values used to create an in-tree sparse-checkout definition, we can easily avoid parsing the same file multiple times by using an oidset on those blobs. We only parse if the oid is new to the oidset.

Quoted 5 lines

This is unlikely to have a major performance benefit right now, but will be extremely important when we introduce the sparse.inherit options to link multiple files in a directed graph. This oidset will prevent infinite loops when cycles exist in that digraph, or exponential blowups even in the case of a directed acyclic graph.

Quoted 3 lines

I'm still not sure if I like the idea of having a mirror dependency structure separate from (and duplicative of) the build code; I'm still mulling that over.

I mentioned this to a few other buildsystem folks at $DAYJOB. They were strongly opposed to having more than one source of truth, but generating the git in-tree sparse values from the official build system files, with commit hooks and build system checks to make sure they get updated seemed like it'd be fine or not concern them much.

Quoted 5 lines

It's good that you've protected against infinite loops.

Is there any reason to prefer swallowing infinite loops rather than warning or flagging as an error? (I'm not sure, just thinking my questions out loud.)

The buildsystem folks also reminded me that we have cylic dependencies already, and although it's absolutely ugly, it is somewhat forced on us by a combination of different external tools that we can't change. As such, warnings or errors would be really annoying and we'd be one of the ones to want to turn them off. So drop that idea from me. :-)

From: Derrick Stolee <[email protected]>

As mentioned in the definition of the in-tree sparse-checkout definitions, a large repository could have many different roles for the users of that repository. That means that many different in-tree sparse-checkout definitions need to exist. If a directory needs to be added to many of these roles, then many of these files need to be edited. This is too much work to scale properly.

Instead, let's make these definitions easier to maintain by following basic principles of build dependencies. Add simple links between these in-tree files using the new "sparse.inherit" config key. This multi- valued config setting specifies more files that are needed to define the sparse-checkout for a role. By separating out the common directories into a base file, the more specialized roles can focus their files on the directories only needed by those roles and leave the common directories to that base file.

For example, suppose that .sparse/base has the following data:

[sparse]
dir = A
dir = B/C
dir = D/E/F

and .sparse/extra has the following data:

[sparse]
dir = D
dir = X
inherit = .sparse/base

Now, a user can run "git sparse-checkout set --in-tree .sparse/extra" and the resulting directories will define their cone-mode sparse-checkout patterns:

        A
        B/C
        D
        X

Thus, the resulting sparse-checkout definition is the union of the definitions from .sparse/base and .sparse/extra, but the user only has one value of sparse.inTree of ".sparse/extra".

It is simple to modify our existing logic to explore the directed graph created by the sparse.inherit values. We simply need to append to the string_list containing the list of in-tree files. Since we never repeat parsing on the same blob oid, this will not lead to infinite loops or exponential blowups in the parsing time.

Signed-off-by Derrick Stolee <[email protected]>

---
 Documentation/git-sparse-checkout.txt | 26 ++++++++++++++++++++++
 sparse-checkout.c                     | 27 ++++++++++++++++++-----
 sparse-checkout.h                     |  1 +
 t/t1091-sparse-checkout-builtin.sh    | 31 +++++++++++++++++++++++++++
 4 files changed, 80 insertions(+), 5 deletions(-)

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

index c1713ebb1d2..941658e0011 100644
@@ -238,6 +238,32 @@ definition according to the files available at the new commit. If any of
 the specified files do not exist at the new commit, then the sparse-checkout
 definition will not change.
 
+In a very large repository, there may be need to have multiple of these
+in-tree sparse-checkout definitions to fit the roles of multiple types of
+users. As each definition grows and the number of user types grow, it can
+be difficult to manage updating all of the definitions when a common core
+is modified somehow. For this reason, the in-tree pattern sets can inherit
+the directories from other in-tree pattern sets. Use the `sparse.inherit`
+option to specify other files in the tree.

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

+
+Then, if you run `git sparse-checkout set --in-tree .sparse/extra`, the
+sparse-checkout definition will include `X` and `Y/Z` from `.sparse/extra`
+as well as `A`, `B/C`, and `D/E/F` from `.sparse/core`. This is similar
+to specifying both `.sparse/core` and `.sparse/extra` in the `set`
+subcommand, but has a slight advantage. If the `.sparse/extra` file changes
+the set of `inherit` files, then your sparse-checkout definition will
+update accordingly as you switch between commits.
+
 
 SUBMODULES
 ----------

sparse-checkout.c +0 -0

index d01f4d7b525..4edeab49a10 100644

sparse-checkout.c +16 -5

+
+       return 0;
+}
+
 static int load_in_tree_from_blob(struct pattern_list *pl,
-                                 struct object_id *oid)
+                                 struct object_id *oid,
+                                 struct string_list *inherit)
 {
-       return git_config_from_blob_oid(sparse_dir_cb,
-                                       SPARSE_CHECKOUT_DIR,
-                                       oid, pl);
+       if (git_config_from_blob_oid(sparse_dir_cb,
+                                    SPARSE_CHECKOUT_DIR,
+                                    oid, pl))
+               return 1;
+       if (git_config_from_blob_oid(sparse_inherit_cb,
+                                    SPARSE_CHECKOUT_INHERIT,
+                                    oid, inherit))
+               return 1;
+       return 0;
 }
 
 int load_in_tree_pattern_list(struct repository *r,

@@ -121,7 +138,7 @@ int load_in_tree_pattern_list(struct repository *r,
                        goto cleanup;
                }
 
-               load_in_tree_from_blob(pl, oid);
+               load_in_tree_from_blob(pl, oid, sl);
        }
 
 cleanup:

sparse-checkout.h +1 -0

index fb0ba48524a..8b766ea38fb 100644
@@ -5,6 +5,7 @@
 #include "repository.h"
 
 #define SPARSE_CHECKOUT_DIR "sparse.dir"
+#define SPARSE_CHECKOUT_INHERIT "sparse.inherit"
 #define SPARSE_CHECKOUT_IN_TREE "sparse.intree"
 
 struct pattern_list;

t/t1091-sparse-checkout-builtin.sh +31 -1

index fdaafba5377..b2389e5b5e6 100755
@@ -716,4 +716,35 @@ test_expect_success 'keep definition when in-tree file is missing' '
        test_path_is_dir repo/.sparse
 '
 
+test_expect_success 'inherit definition from other files' '
+       cat >repo/.sparse/inherit <<-EOF &&
+       [sparse]
+               inherit = .sparse/sparse
+               inherit = .sparse/deep
+               inherit = .sparse/deeper1
+       EOF
+       git -C repo add .sparse &&
+       git -C repo commit -m "Add inherited file" &&
+       git -C repo sparse-checkout set --in-tree .sparse/inherit &&
+       check_files repo a deep folder1 &&
+       check_files repo/deep a deeper1 deeper2 &&
+       test_path_is_dir repo/.sparse &&
+       cat >repo/.sparse/sparse <<-EOF &&
+       [sparse]
+               dir = .sparse
+       EOF
+       git -C repo commit -a -m "drop folder1 from sparse" &&
+       check_files repo a deep &&
+       check_files repo/deep a deeper1 deeper2 &&
+       test_path_is_dir repo/.sparse
+'
+
+test_expect_success 'inherit files can have cycles' '
+       echo "\tinherit = .sparse/inherit" >>repo/.sparse/sparse &&
+       git -C repo commit -a -m "create inherit cycle" &&
+       check_files repo a deep &&
+       check_files repo/deep a deeper1 deeper2 &&
+       test_path_is_dir repo/.sparse
+'
+
 test_done
--

gitgitgadget

Quoted 23 lines · Derrick Stolee via GitGitGadget

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

@@ -238,6 +238,32 @@ definition according to the files available at the new commit. If any of
 the specified files do not exist at the new commit, then the sparse-checkout
 definition will not change.
 
+In a very large repository, there may be need to have multiple of these
+in-tree sparse-checkout definitions to fit the roles of multiple types of
+users. As each definition grows and the number of user types grow, it can
+be difficult to manage updating all of the definitions when a common core
+is modified somehow. For this reason, the in-tree pattern sets can inherit
+the directories from other in-tree pattern sets. Use the `sparse.inherit`
+option to specify other files in the tree.
+
+For example, suppose the file listed earlier is at `.sparse/core`. Another
+file could be stored as `.sparse/extra` with contents
+
+----------------------------------
+[sparse]
+       dir = X
+       dir = Y/Z
+       inherit = .sparse/core
+----------------------------------

What happens if the user specifies a non-existent path, or perhaps equivalently, didn't understand and specified the path using a relative path specification? What if the user specifies a path that doesn't exist in the commit, but happens to exist in someones's

working directory? (And maybe even did it intentionally as a way to cheat and add user-defined additional paths to include? Do we want to allow that, or do we want to enforce that extra includes use some kind of pre-defined path?) What if they specify some path that is invalid to record in a git commit (.git/mydeps or ../../../../../../../otherdeps) but happens to exist on some machines?
(Are there future security vulnerabilities of any sort going down
this path?)

This is based on ds/sparse-allow-empty-working-tree.

BACKGROUND
==========

After discussing the future of sparse-checkout at the Git Contributor Summit, Minh Tai caught me during the break to introduce a very interesting
idea: in-tree sparse-checkout definitions.

Here is the context: I was talking about how the Office team is building a tool to update the sparse-checkout definition dynamically as their build definitions change. They have a very compartmentalized build system, and certain "projects" depend on each other. To ensure that builds do not break as dependencies change, the plan was to have this tool run as a hook when HEAD changes. This is a bit expensive to do, and we were discussing how much we need this to be automated versus a part of the build process.

This kind of idea would need to be replicated for every team that wants to use sparse-checkout in this way. That doesn't bode well for wide adoption of the feature.

IN-TREE SPARSE-CHECKOUT DEFINITIONS
===================================

Minh's idea was simple: have sparse-checkout files in the working directory and use config to point to them. As these in-tree files update, we can automatically update the sparse-checkout definition accordingly. Now, the only thing to do would be to ensure that the sparse-checkout files are updated when someone updates the build definitions. This requires some extra build validation, but would not require special tools built on every client.

As soon as I sat down to implement this idea, I discovered some complications to that basic idea. Each of these sparse-checkout definitions should fit a "role" on the team. To continue with the Office analogy, suppose we had a definition for developers working on each of Word, PowerPoint, and Excel. What happens when someone wants to port a feature from Excel to Word? That user would want to take the union of the two sparse-checkout definitions. But what does that mean when we are working with arbitrary sparse-checkout files?

This leads to the restriction that I built into this feature: we only care about "cone mode" definitions. These rely on directory-based prefix matches that are very fast. With this restriction, it is simple to understand what the "union" operation should do: take all directories that would be included by either.

Once we restrict to cone mode, there is no reason to continue storing files using the sparse-checkout file format. The list of patterns is larger than the input to "git sparse-checkout set" or output from "git sparse-checkout list" in cone mode, and more prone to user error. Thus, let's be simpler and use a list of directories to specify the sparse-checkout definition.

This leads to the second issue that complicates things. If a build dependency changes to a core library, but there are N "roles" that depend on that library, then the person changing that library also needs to change and validate N sparse-checkout definitions! This does not scale well. So, let's take a page from build dependencies and allow these in-tree sparse-checkout definitions to depend on each other!

The end result is a mechanism that should be flexible enough to future needs but simple enough to build using existing Git features. The format re-uses the config file format. This provides good re-use of code as well as being something easy to extend in the future. In Patch 4, I tried to describe several alternatives and why this is the best of those options. I'm open to suggestions here.

OUTLINE
=======

* Patch 1 is a bugfix that should work on its own. I caught it in tests after Patch 5.

* Patches 2-7 implement the feature.

* Patches 8-9 make the Git build system a bit more robust to missing directories.

* Patch 10 adds a .sparse directory to Git as well as two in-tree sparse-checkout definitions. One is for a bare-bones Linux developer and the other adds the compat code for Windows on top.

As mentioned in patch 10, this allows the following workflow for Git contributors that want to "eat our own dogfood" with the partial clone and sparse-checkout features:

 $ git clone --sparse --filter=blob:none https://github.com/git/git
$ cd git
$ git sparse-checkout set --in-tree .sparse/base.deps

Then, we have a smaller working directory but can still build and test the code.

FUTURE DIRECTIONS
=================

There are definitely ways to make this feature more seamless, or more useful to us.

* It would be nice to extend git clone to have a string value to --sparse=<file> and possibly have it multi-valued. This would initialize the in-tree sparse-checkout definition immediately upon cloning.

* I think there are some ways we could reorganize the Git codebase to make our "most basic" sparse-checkout file have even fewer files. Mostly, it would require modifying the build to ask "do these files exist on-disk? if not, then disable this build option." This would also require adding test dependencies that also disable some tests when those do not exist.

* Currently, if we have an in-tree sparse-checkout definition, then we can't add more directories to it manually. If someone runs git sparse-checkout set --in-tree <file> followed by git sparse-checkout add <dir>, then will disappear as it is overridden by the in-tree definition. The only way I can think to get around this would be to add these directories through Git config, since we were using the sparse-checkout file as the storage of "what directories do I care about?" but it is no longer the source of truth in this situation. I'm open to ideas here, and it would be nice to have this interaction hardened before moving the series out of RFC status.

IN-TREE SPARSE-CHECKOUT DEFINITIONS
===================================

Minh's idea was simple: have sparse-checkout files in the working directory and use config to point to them. As these in-tree files update, we can automatically update the sparse-checkout definition accordingly. Now, the only thing to do would be to ensure that the sparse-checkout files are updated when someone updates the build definitions. This requires some extra build validation, but would not require special tools built on every client.

Hi,

Another late addition...

Quoted 9 lines · Derrick Stolee via GitGitGadget

IN-TREE SPARSE-CHECKOUT DEFINITIONS
===================================

Minh's idea was simple: have sparse-checkout files in the working directory and use config to point to them. As these in-tree files update, we can automatically update the sparse-checkout definition accordingly. Now, the only thing to do would be to ensure that the sparse-checkout files are updated when someone updates the build definitions. This requires some extra build validation, but would not require special tools built on every client.

"In-tree" still bugs me after a few weeks; the wording seems slightly awkward. I don't have a good suggestion, but I'm curious if there's a better term.

But I really came here to comment on another issue I think I glossed over the first time around. I'm curious if all module definition files have to exist in the working directory, as possibly suggested above, or if we can allow them to just exist in the index. To give you a flavor for what I mean, with my sparsify tool people can do things like:

    ./sparsify --modules MODULE_A

which provides MODULE_A and it's dependencies while removing all other directories. If MODULE_B, is not a dependency (direct or transitive) of MODULE_A, it will not exist in the working directory after this step. Our equivalent of the "in-tree" definition of MODULE_B exists *in* the directory for MODULE_B, because it seems to make sense for us. I want people to be able to do

    ./sparsify --modules MODULE_B

and have it correctly check out all the necessary files even though the definition of MODULE_B wasn't even in the working directory at the time the command ran. (The sparsify script knows to check the working directory first, then fall back to the index).

Quoted 17 lines · Elijah Newren

Hi,

Another late addition...

Quoted 9 lines · Derrick Stolee via GitGitGadget

IN-TREE SPARSE-CHECKOUT DEFINITIONS
===================================

Minh's idea was simple: have sparse-checkout files in the working directory and use config to point to them. As these in-tree files update, we can automatically update the sparse-checkout definition accordingly. Now, the only thing to do would be to ensure that the sparse-checkout files are updated when someone updates the build definitions. This requires some extra build validation, but would not require special tools built on every client.

Quoted 3 lines

"In-tree" still bugs me after a few weeks; the wording seems slightly awkward. I don't have a good suggestion, but I'm curious if there's a better term.

I am open to suggestions. It reminds me of the two hardest problems in software engineering:

1. concurrency
2. naming things
3. off-by-one errors

Quoted 13 lines

But I really came here to comment on another issue I think I glossed over the first time around. I'm curious if all module definition files have to exist in the working directory, as possibly suggested above, or if we can allow them to just exist in the index. To give you a flavor for what I mean, with my sparsify tool people can do things like:

    ./sparsify --modules MODULE_A

which provides MODULE_A and it's dependencies while removing all other directories. If MODULE_B, is not a dependency (direct or transitive) of MODULE_A, it will not exist in the working directory after this step. Our equivalent of the "in-tree" definition of MODULE_B exists *in* the directory for MODULE_B, because it seems to make sense for us. I want people to be able to do

Quoted 5 lines

    ./sparsify --modules MODULE_B

and have it correctly check out all the necessary files even though the definition of MODULE_B wasn't even in the working directory at the time the command ran. (The sparsify script knows to check the working directory first, then fall back to the index).

I think one tricky part of my RFC is that it _only_ looks at the index. This allows us to read the contents even when the files are not part of the current sparse-checkout definition.

You mentioned in another thread that it is a bit unwieldy for a user to rely on a committed (or staged?) file, so adding the ability to check the working directory first is interesting. I wonder how the timing comes into play when changing HEAD to a new commit? Seems tricky, but solvable.

Quoted 7 lines · Derrick Stolee

On 6/17/2020 7:14 PM, Elijah Newren wrote:
You mentioned in another thread that it is a bit unwieldy for a user to rely on a committed (or staged?) file, so adding the ability to check the working directory first is interesting. I wonder how the timing comes into play when changing HEAD to a new commit? Seems tricky, but solvable.

Isn't that essentially the same timing issue that comes into play if you only look at the index, and are changing HEAD to a new commit?

Quoted 11 lines · Elijah Newren

Quoted 7 lines · Derrick Stolee

On 6/17/2020 7:14 PM, Elijah Newren wrote:
You mentioned in another thread that it is a bit unwieldy for a user to rely on a committed (or staged?) file, so adding the ability to check the working directory first is interesting. I wonder how the timing comes into play when changing HEAD to a new commit? Seems tricky, but solvable.

Isn't that essentially the same timing issue that comes into play if you only look at the index, and are changing HEAD to a new commit?

It adds to the complexity. We can inspect the new index for the in-tree definition and update the skip-worktree bits before actually changing the working directory to match the new index. However, if we trust the working directory copy over the index copy, then we need to also decide if the working directory copy _would_ change in this move before using it.

Of course, maybe I'm making it over-complicated. I still know so little about the index. I got into this feature due to a simple pattern-matching problem that I could understand, but now I'm getting lost in index states and dirty statuses. ;)