[PATCH 0/2] Sparse index: fetch, pull, ls-files

[PATCH 0/2] Sparse index: fetch, pull, ls-files

[Derrick Stolee via GitGitGadget]

This is based on ld/sparse-index-blame (merged with 'master' due to an
unrelated build issue).

Here are two relatively-simple patches that further the sparse index
integrations.

Did you know that 'fetch' and 'pull' read the index? I didn't, or this would
have been an integration much earlier in the cycle. They read the index to
look for the .gitmodules file in case there are submodules that need to be
fetched. Since looking for a file by name is already protected, we only need
to disable 'command_requires_full_index' and we are done.

The 'ls-files' builtin is useful when debugging the index, and some scripts
use it, too. We are not changing the default behavior which expands a sparse
index in order to show all of the cached blobs. Instead, we add a '--sparse'
option that allows us to see the sparse directory entries upon request.
Combined with --debug, we can see a lot of index details, such as:

$ git ls-files --debug --sparse
LICENSE
  ctime: 1634910503:287405820
  mtime: 1634910503:287405820
  dev: 16777220 ino: 119325319
  uid: 501  gid: 20
  size: 1098    flags: 200000
README.md
  ctime: 1634910503:288090279
  mtime: 1634910503:288090279
  dev: 16777220 ino: 119325320
  uid: 501  gid: 20
  size: 934 flags: 200000
bin/index.js
  ctime: 1634910767:828434033
  mtime: 1634910767:828434033
  dev: 16777220 ino: 119325520
  uid: 501  gid: 20
  size: 7292    flags: 200000
examples/
  ctime: 0:0
  mtime: 0:0
  dev: 0    ino: 0
  uid: 0    gid: 0
  size: 0   flags: 40004000
package.json
  ctime: 1634910503:288676330
  mtime: 1634910503:288676330
  dev: 16777220 ino: 119325321
  uid: 501  gid: 20
  size: 680 flags: 200000


(In this example, the 'examples/' directory is sparse.)

Thanks!

Derrick Stolee (2):
  fetch/pull: use the sparse index
  ls-files: add --sparse option

 Documentation/git-ls-files.txt           |  4 ++
 builtin/fetch.c                          |  2 +
 builtin/ls-files.c                       | 12 ++++-
 builtin/pull.c                           |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 57 ++++++++++++++++++++++++
 5 files changed, 75 insertions(+), 2 deletions(-)


base-commit: ce9c834d5de7026a5271e289ab4518e569464171
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1080%2Fderrickstolee%2Fsparse-index%2Ffetch-pull-ls-files-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1080/derrickstolee/sparse-index/fetch-pull-ls-files-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1080
-- 
gitgitgadget

[PATCH 1/2] fetch/pull: use the sparse index

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The 'git fetch' and 'git pull' commands parse the index in order to
determine if submodules exist. Without command_requires_full_index=0,
this will expand a sparse index, causing slow performance even when
there is no new data to fetch.

The .gitmodules file will never be inside a sparse directory entry, and
even if it was, the index_name_pos() method would expand the sparse
index if needed as we search for the path by name. These commands do not
iterate over the index, which is the typical thing we are careful about
when integrating with the sparse index.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/fetch.c                          |  2 ++
 builtin/pull.c                           |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 10 ++++++++++
 3 files changed, 14 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f7abbc31ff1..1696b7791d4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1993,6 +1993,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	}
 
 	git_config(git_fetch_config, NULL);
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_fetch_options, builtin_fetch_usage, 0);
diff --git a/builtin/pull.c b/builtin/pull.c
index 127798ba84e..9f6174f4433 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -993,6 +993,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		set_reflog_message(argc, argv);
 
 	git_config(git_pull_config, NULL);
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
 
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index d75f64aad56..f8a8dde60af 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -932,6 +932,16 @@ test_expect_success 'sparse index is not expanded: blame' '
 	ensure_not_expanded blame deep/deeper1/deepest/a
 '
 
+test_expect_success 'sparse index is not expanded: fetch/pull' '
+	init_repos &&
+
+	git -C sparse-index remote add full "file://$(pwd)/full-checkout" &&
+	ensure_not_expanded fetch full &&
+	git -C full-checkout commit --allow-empty -m "for pull merge" &&
+	git -C sparse-index commit --allow-empty -m "for pull merge" &&
+	ensure_not_expanded pull full base
+'
+
 # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
 # in this scenario, but it shouldn't.
 test_expect_success 'reset mixed and checkout orphan' '
-- 
gitgitgadget

[PATCH 2/2] ls-files: add --sparse option

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Existing callers to 'git ls-files' are expecting file names, not
directories. It is best to expand a sparse index to show all of the
contained files in this case.

However, expert users may want to inspect the contents of the index
itself including which directories are sparse. Add a --sparse option to
allow users to request this information.

During testing, I noticed that options such as --modified did not affect
the output when the files in question were outside the sparse-checkout
definition. Tests are added to document this preexisting behavior and
how it remains unchanged with the sparse index and the --sparse option.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-ls-files.txt           |  4 ++
 builtin/ls-files.c                       | 12 +++++-
 t/t1092-sparse-checkout-compatibility.sh | 47 ++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 6d11ab506b7..1c5d5f85ec5 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -187,6 +187,10 @@ Both the <eolinfo> in the index ("i/<eolinfo>")
 and in the working tree ("w/<eolinfo>") are shown for regular files,
 followed by the  ("attr/<eolattr>").
 
+--sparse::
+	If the index is sparse, show the sparse directories without expanding
+	to the contained files.
+
 \--::
 	Do not interpret any more arguments as options.
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 031fef1bcaa..c151eb1fb77 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -37,6 +37,7 @@ static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
 static int skipping_duplicates;
+static int show_sparse_dirs;
 
 static const char *prefix;
 static int max_prefix_len;
@@ -315,8 +316,10 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 
 	if (!(show_cached || show_stage || show_deleted || show_modified))
 		return;
-	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(repo->index);
+
+	if (!show_sparse_dirs)
+		ensure_full_index(repo->index);
+
 	for (i = 0; i < repo->index->cache_nr; i++) {
 		const struct cache_entry *ce = repo->index->cache[i];
 		struct stat st;
@@ -670,6 +673,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
 		OPT_BOOL(0, "deduplicate", &skipping_duplicates,
 			 N_("suppress duplicate entries")),
+		OPT_BOOL(0, "sparse", &show_sparse_dirs,
+			 N_("show sparse directories in the presence of a sparse index")),
 		OPT_END()
 	};
 	int ret = 0;
@@ -677,6 +682,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(ls_files_usage, builtin_ls_files_options);
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	prefix = cmd_prefix;
 	if (prefix)
 		prefix_len = strlen(prefix);
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index f8a8dde60af..ffb6052ff60 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -814,6 +814,12 @@ test_expect_success 'sparse-index is expanded and converted back' '
 	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
 		git -C sparse-index reset -- folder1/a &&
 	test_region index convert_to_sparse trace2.txt &&
+	test_region index ensure_full_index trace2.txt &&
+
+	# ls-files expands on read, but does not write.
+	rm trace2.txt &&
+	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
+		git -C sparse-index ls-files &&
 	test_region index ensure_full_index trace2.txt
 '
 
@@ -838,6 +844,7 @@ test_expect_success 'sparse-index is not expanded' '
 	init_repos &&
 
 	ensure_not_expanded status &&
+	ensure_not_expanded ls-files --sparse &&
 	ensure_not_expanded commit --allow-empty -m empty &&
 	echo >>sparse-index/a &&
 	ensure_not_expanded commit -a -m a &&
@@ -942,6 +949,46 @@ test_expect_success 'sparse index is not expanded: fetch/pull' '
 	ensure_not_expanded pull full base
 '
 
+test_expect_success 'ls-files' '
+	init_repos &&
+
+	# Behavior agrees by default. Sparse index is expanded.
+	test_all_match git ls-files &&
+
+	# With --sparse, the sparse index data changes behavior.
+	git -C sparse-index ls-files --sparse >sparse-index-out &&
+	grep "^folder1/\$" sparse-index-out &&
+	grep "^folder2/\$" sparse-index-out &&
+
+	# With --sparse and no sparse index, nothing changes.
+	git -C sparse-checkout ls-files --sparse >sparse-checkout-out &&
+	grep "^folder1/0/0/0\$" sparse-checkout-out &&
+	! grep "/\$" sparse-checkout-out &&
+
+	write_script edit-content <<-\EOF &&
+	mkdir folder1 &&
+	echo content >>folder1/a
+	EOF
+	run_on_sparse ../edit-content &&
+
+	# ls-files does not notice modified files whose
+	# cache entries are marked SKIP_WORKTREE.
+	test_sparse_match git ls-files --modified &&
+	test_must_be_empty sparse-checkout-out &&
+	test_must_be_empty sparse-index-out &&
+
+	git -C sparse-index ls-files --sparse --modified >sparse-index-out &&
+	test_must_be_empty sparse-index-out &&
+
+	run_on_sparse git sparse-checkout add folder1 &&
+	test_sparse_match git ls-files --modified &&
+	grep "^folder1/a\$" sparse-checkout-out &&
+	grep "^folder1/a\$" sparse-index-out &&
+
+	# Double-check index expansion
+	ensure_not_expanded ls-files --sparse
+'
+
 # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
 # in this scenario, but it shouldn't.
 test_expect_success 'reset mixed and checkout orphan' '
-- 
gitgitgadget

Re: [PATCH 2/2] ls-files: add --sparse option

[Elijah Newren]

On Tue, Nov 16, 2021 at 7:38 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Existing callers to 'git ls-files' are expecting file names, not
> directories. It is best to expand a sparse index to show all of the
> contained files in this case.
>
> However, expert users may want to inspect the contents of the index
> itself including which directories are sparse. Add a --sparse option to
> allow users to request this information.
>
> During testing, I noticed that options such as --modified did not affect
> the output when the files in question were outside the sparse-checkout
> definition. Tests are added to document this preexisting behavior and
> how it remains unchanged with the sparse index and the --sparse option.

Sure, 'git diff' and 'git status' don't show anything for such files
either; we're being consistent.  However, this feels like it's
normalizing an erroneous condition, possibly starting to cement one of
the bigger UI problems we have the sparse-checkout (and sparse-index):

I assert that: Having tracked files be SKIP_WORKTREE despite having a
file present in the working directory is typically an *error*, and one
that we need to help users prevent/discover/recover-from.

* I've seen users get into this condition by doing the equivalent of
either 'git sparse-checkout disable' or 'git sparse-checkout set
$NEW_PATHS' and then hit Ctrl-C in the middle of its operation.
* Users also just occasionally copy files from other places (maybe
even `git show REVISION:FILE >FILE`), and potentially edit.

In either of the above cases:
* There is no command provided for discovering these files; not diff,
status, ls-files, or anything
* There is no command provided for correcting the problem; not clean,
not reset, or anything.  They have to do it by hand
* There are ample opportunities for the error to propagate and grow,
due to the fact that these files will not be reported or updated.[1]

[1] For example, the user may first either switch to another branch or
resets to some other commit.  Neither of those operations update these
erroneous present-despite-SKIP_WORKTREE files.  And there will still
be nothing that reports them.  But now the files are out-of-date with
respect to the new commit.  Now if the user disables or changes the
sparse checkout, they suddenly have several "changed" files in their
working tree -- which they didn't touch.  In the best case, they run
diff or status or something and notice the files and then correct it,
and just get perplexed at where the changes came from.  But with
enough users, some percentage of them are just going to commit (some
of) those changes.  When someone else asks why they modified those
files, they'll (correctly, as it turns out) claim they never did and
that no program on their system did.  They might even think those
files weren't part of the commit and claim that git modified the
commit behind their back, which would be wrong, but there won't be any
reasonable logs to check to prove what happened.


I know this issue is out of scope for your series here, but the
addition of testcases that purposely set up an erroneous condition
makes it feel like we're trying to normalize that situation and not
treat it like an error, and are perhap undercutting future attempts to
try to fix it.  I'd rather have it explicitly stated that we're
setting up an erroneous condition in our tests, in order to make sure
we handle it as best as we can so far -- in a manner in line with diff
and grep -- but also note that later solutions to this deeper issue
may affect one or more of these commands.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-ls-files.txt           |  4 ++
>  builtin/ls-files.c                       | 12 +++++-
>  t/t1092-sparse-checkout-compatibility.sh | 47 ++++++++++++++++++++++++
>  3 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 6d11ab506b7..1c5d5f85ec5 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -187,6 +187,10 @@ Both the <eolinfo> in the index ("i/<eolinfo>")
>  and in the working tree ("w/<eolinfo>") are shown for regular files,
>  followed by the  ("attr/<eolattr>").
>
> +--sparse::
> +       If the index is sparse, show the sparse directories without expanding
> +       to the contained files.
> +
>  \--::
>         Do not interpret any more arguments as options.
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 031fef1bcaa..c151eb1fb77 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -37,6 +37,7 @@ static int debug_mode;
>  static int show_eol;
>  static int recurse_submodules;
>  static int skipping_duplicates;
> +static int show_sparse_dirs;
>
>  static const char *prefix;
>  static int max_prefix_len;
> @@ -315,8 +316,10 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
>
>         if (!(show_cached || show_stage || show_deleted || show_modified))
>                 return;
> -       /* TODO: audit for interaction with sparse-index. */
> -       ensure_full_index(repo->index);
> +
> +       if (!show_sparse_dirs)
> +               ensure_full_index(repo->index);
> +
>         for (i = 0; i < repo->index->cache_nr; i++) {
>                 const struct cache_entry *ce = repo->index->cache[i];
>                 struct stat st;
> @@ -670,6 +673,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>                 OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
>                 OPT_BOOL(0, "deduplicate", &skipping_duplicates,
>                          N_("suppress duplicate entries")),
> +               OPT_BOOL(0, "sparse", &show_sparse_dirs,
> +                        N_("show sparse directories in the presence of a sparse index")),
>                 OPT_END()
>         };
>         int ret = 0;
> @@ -677,6 +682,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>         if (argc == 2 && !strcmp(argv[1], "-h"))
>                 usage_with_options(ls_files_usage, builtin_ls_files_options);
>
> +       prepare_repo_settings(the_repository);
> +       the_repository->settings.command_requires_full_index = 0;
> +
>         prefix = cmd_prefix;
>         if (prefix)
>                 prefix_len = strlen(prefix);
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index f8a8dde60af..ffb6052ff60 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -814,6 +814,12 @@ test_expect_success 'sparse-index is expanded and converted back' '
>         GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
>                 git -C sparse-index reset -- folder1/a &&
>         test_region index convert_to_sparse trace2.txt &&
> +       test_region index ensure_full_index trace2.txt &&
> +
> +       # ls-files expands on read, but does not write.
> +       rm trace2.txt &&
> +       GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
> +               git -C sparse-index ls-files &&
>         test_region index ensure_full_index trace2.txt
>  '
>
> @@ -838,6 +844,7 @@ test_expect_success 'sparse-index is not expanded' '
>         init_repos &&
>
>         ensure_not_expanded status &&
> +       ensure_not_expanded ls-files --sparse &&

Do we also want a
    ensure_not_expanded ls-files &&
?  We don't expect ls-files to write a new index file in any scenario, right?

>         ensure_not_expanded commit --allow-empty -m empty &&
>         echo >>sparse-index/a &&
>         ensure_not_expanded commit -a -m a &&
> @@ -942,6 +949,46 @@ test_expect_success 'sparse index is not expanded: fetch/pull' '
>         ensure_not_expanded pull full base
>  '
>
> +test_expect_success 'ls-files' '
> +       init_repos &&
> +
> +       # Behavior agrees by default. Sparse index is expanded.
> +       test_all_match git ls-files &&
> +
> +       # With --sparse, the sparse index data changes behavior.
> +       git -C sparse-index ls-files --sparse >sparse-index-out &&
> +       grep "^folder1/\$" sparse-index-out &&
> +       grep "^folder2/\$" sparse-index-out &&
> +
> +       # With --sparse and no sparse index, nothing changes.
> +       git -C sparse-checkout ls-files --sparse >sparse-checkout-out &&
> +       grep "^folder1/0/0/0\$" sparse-checkout-out &&
> +       ! grep "/\$" sparse-checkout-out &&

I'd be tempted to split the test up to this point from the rest of the test.

> +
> +       write_script edit-content <<-\EOF &&
> +       mkdir folder1 &&
> +       echo content >>folder1/a
> +       EOF
> +       run_on_sparse ../edit-content &&

As above, since folder1/a is a tracked file, I'd rather we explicitly
called out that we're intentionally setting up an erroneous condition.

> +
> +       # ls-files does not notice modified files whose
> +       # cache entries are marked SKIP_WORKTREE.

...nor does diff, status, etc., as per my lengthy comment from the
beginning of the email.

> +       test_sparse_match git ls-files --modified &&
> +       test_must_be_empty sparse-checkout-out &&
> +       test_must_be_empty sparse-index-out &&
> +
> +       git -C sparse-index ls-files --sparse --modified >sparse-index-out &&
> +       test_must_be_empty sparse-index-out &&
> +
> +       run_on_sparse git sparse-checkout add folder1 &&
> +       test_sparse_match git ls-files --modified &&
> +       grep "^folder1/a\$" sparse-checkout-out &&
> +       grep "^folder1/a\$" sparse-index-out &&
> +
> +       # Double-check index expansion
> +       ensure_not_expanded ls-files --sparse
> +'
> +
>  # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
>  # in this scenario, but it shouldn't.
>  test_expect_success 'reset mixed and checkout orphan' '
> --
> gitgitgadget


Other than my concerns about careful messages with erroneous
conditions (and the minor question about also having a
ensure_not_expanded without the --sparse flag), the patch looks good
to me.

Re: [PATCH 2/2] ls-files: add --sparse option

[Derrick Stolee]

On 11/22/2021 1:36 PM, Elijah Newren wrote:
> On Tue, Nov 16, 2021 at 7:38 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Existing callers to 'git ls-files' are expecting file names, not
>> directories. It is best to expand a sparse index to show all of the
>> contained files in this case.
>>
>> However, expert users may want to inspect the contents of the index
>> itself including which directories are sparse. Add a --sparse option to
>> allow users to request this information.
>>
>> During testing, I noticed that options such as --modified did not affect
>> the output when the files in question were outside the sparse-checkout
>> definition. Tests are added to document this preexisting behavior and
>> how it remains unchanged with the sparse index and the --sparse option.
> 
> Sure, 'git diff' and 'git status' don't show anything for such files
> either; we're being consistent.  However, this feels like it's
> normalizing an erroneous condition, possibly starting to cement one of
> the bigger UI problems we have the sparse-checkout (and sparse-index):
> 
> I assert that: Having tracked files be SKIP_WORKTREE despite having a
> file present in the working directory is typically an *error*, and one
> that we need to help users prevent/discover/recover-from.

I do think we need to tread carefully here, since some users use
'git update-index --skip-worktree' to mark a file in their
worktree as "ignored" so they can change it from HEAD without it
ever being staged. This is usually independent of the sparse-
checkout feature, so you might want to focus your efforts to paths
that exist in the worktree but are outside of the sparse-checkout
patterns.

> * I've seen users get into this condition by doing the equivalent of
> either 'git sparse-checkout disable' or 'git sparse-checkout set
> $NEW_PATHS' and then hit Ctrl-C in the middle of its operation.
> * Users also just occasionally copy files from other places (maybe
> even `git show REVISION:FILE >FILE`), and potentially edit.
> 
> In either of the above cases:
> * There is no command provided for discovering these files; not diff,
> status, ls-files, or anything
> * There is no command provided for correcting the problem; not clean,
> not reset, or anything.  They have to do it by hand
> * There are ample opportunities for the error to propagate and grow,
> due to the fact that these files will not be reported or updated.[1]
> 
> [1] For example, the user may first either switch to another branch or
> resets to some other commit.  Neither of those operations update these
> erroneous present-despite-SKIP_WORKTREE files.  And there will still
> be nothing that reports them.  But now the files are out-of-date with
> respect to the new commit.  Now if the user disables or changes the
> sparse checkout, they suddenly have several "changed" files in their
> working tree -- which they didn't touch.  In the best case, they run
> diff or status or something and notice the files and then correct it,
> and just get perplexed at where the changes came from.  But with
> enough users, some percentage of them are just going to commit (some
> of) those changes.  When someone else asks why they modified those
> files, they'll (correctly, as it turns out) claim they never did and
> that no program on their system did.  They might even think those
> files weren't part of the commit and claim that git modified the
> commit behind their back, which would be wrong, but there won't be any
> reasonable logs to check to prove what happened.
> 
> 
> I know this issue is out of scope for your series here, but the
> addition of testcases that purposely set up an erroneous condition
> makes it feel like we're trying to normalize that situation and not
> treat it like an error, and are perhap undercutting future attempts to
> try to fix it.  I'd rather have it explicitly stated that we're
> setting up an erroneous condition in our tests, in order to make sure
> we handle it as best as we can so far -- in a manner in line with diff
> and grep -- but also note that later solutions to this deeper issue
> may affect one or more of these commands.

I appreciate the attention to this scenario, but I also think
that a patch series whose goal is to change how we react when
in this case already needs to make a case for changing the
behavior. Having tests that exist documenting the previous
behavior form a good basis for "it did this before, but now it
will do this," which hopefully further justifies the change.

Personally, I think of _every_ test as "this is what the code
does right now" and the purpose of a test is to show that we
don't change things _unintentionally_. Every test can be changed
if there is sufficient evidence that it should.

>> @@ -838,6 +844,7 @@ test_expect_success 'sparse-index is not expanded' '
>>         init_repos &&
>>
>>         ensure_not_expanded status &&
>> +       ensure_not_expanded ls-files --sparse &&
> 
> Do we also want a
>     ensure_not_expanded ls-files &&
> ?  We don't expect ls-files to write a new index file in any scenario, right?

When the index is sparse, 'git ls-files' will expand before
listing all of the contents, to avoid listing a sparse
directory entry. One way to avoid ensure_full_index() would
be to do trickery with read_tree_at() whenever we find a
sparse directory entry and use the callback to output what
ls-files would have written. However, that's pretty much the
same amount of work as what ensure_full_index() is doing, so
there is likely little benefit to complicating the code for
this reason.

>>         ensure_not_expanded commit --allow-empty -m empty &&
>>         echo >>sparse-index/a &&
>>         ensure_not_expanded commit -a -m a &&
>> @@ -942,6 +949,46 @@ test_expect_success 'sparse index is not expanded: fetch/pull' '
>>         ensure_not_expanded pull full base
>>  '
>>
>> +test_expect_success 'ls-files' '
>> +       init_repos &&
>> +
>> +       # Behavior agrees by default. Sparse index is expanded.
>> +       test_all_match git ls-files &&
>> +
>> +       # With --sparse, the sparse index data changes behavior.
>> +       git -C sparse-index ls-files --sparse >sparse-index-out &&
>> +       grep "^folder1/\$" sparse-index-out &&
>> +       grep "^folder2/\$" sparse-index-out &&
>> +
>> +       # With --sparse and no sparse index, nothing changes.
>> +       git -C sparse-checkout ls-files --sparse >sparse-checkout-out &&
>> +       grep "^folder1/0/0/0\$" sparse-checkout-out &&
>> +       ! grep "/\$" sparse-checkout-out &&
> 
> I'd be tempted to split the test up to this point from the rest of the test.

Every instance of "init_repos" adds to the cost of this test script,
so when possible I'd err on the side of grouping them. It also keeps
test able to isolate with "--run=1,<N>".

>> +
>> +       write_script edit-content <<-\EOF &&
>> +       mkdir folder1 &&
>> +       echo content >>folder1/a
>> +       EOF
>> +       run_on_sparse ../edit-content &&
> 
> As above, since folder1/a is a tracked file, I'd rather we explicitly
> called out that we're intentionally setting up an erroneous condition.
> 
>> +
>> +       # ls-files does not notice modified files whose
>> +       # cache entries are marked SKIP_WORKTREE.
> 
> ...nor does diff, status, etc., as per my lengthy comment from the
> beginning of the email.

I think the existence of this comment points out that "this is
such a strange situation that it requires explanation." A slight
comment change such as

	# In this strange situation where a tracked file
	# exists in the worktree but is marked with the
	# SKIP_WORKTREE bit in the index, Git ignores the
	# worktree contents.

That both points out that this case is unusual, but also that
ls-files isn't the only command that does this.
> Other than my concerns about careful messages with erroneous
> conditions (and the minor question about also having a
> ensure_not_expanded without the --sparse flag), the patch looks good
> to me.
 
Thanks for your careful read!
-Stolee

Re: [PATCH 2/2] ls-files: add --sparse option

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 16 2021, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
> [...]
> +test_expect_success 'ls-files' '
> +	init_repos &&
> +
> +	# Behavior agrees by default. Sparse index is expanded.
> +	test_all_match git ls-files &&
> +
> +	# With --sparse, the sparse index data changes behavior.
> +	git -C sparse-index ls-files --sparse >sparse-index-out &&
> +	grep "^folder1/\$" sparse-index-out &&
> +	grep "^folder2/\$" sparse-index-out &&
> +
> +	# With --sparse and no sparse index, nothing changes.
> +	git -C sparse-checkout ls-files --sparse >sparse-checkout-out &&
> +	grep "^folder1/0/0/0\$" sparse-checkout-out &&
> +	! grep "/\$" sparse-checkout-out &&

I think all of this would be much clearer both in terms of explaining
this change, and also for future test relability if it did away with the
selective grepping, and simply ran tls-files with and without --sparse,
and then test_cmp'd the full output (after munging away the OIDs).

I.e. the sort of output that's in my just-sent reply to the CL:
https://lore.kernel.org/git/211123.86lf1fwrq5.gmgdl@evledraar.gmail.com/

We really don't need to optimize for lines of tests added, and having
~30 lines of plainly understood diff output is IMO preferrable to even 5
lines of tricky positive & negative grep invocations that take some time
to reason about and understand.

I.e. something like:

    cat >expected <<-\EOF &&
     100644 blob OID   e
     100644 blob OID   folder1-
     100644 blob OID   folder1.x
    -040000 tree OID   folder1/
    +100644 blob OID   folder1/0/0/0
    +100644 blob OID   folder1/0/1
    +100644 blob OID   folder1/a
     100644 blob OID   folder10
    -040000 tree OID   folder2/
    +100644 blob OID   folder2/0/0/0
    +100644 blob OID   folder2/0/1
    +100644 blob OID   folder2/a
     100644 blob OID   g
    -040000 tree OID   x/
    +100644 blob OID   x/a
     100644 blob OID   z
    EOF
    git [...] ls-files --sparse >actual.raw &&
    [munge away OIDs] <actual.raw >actual &&
    test_cmp expected actual

Would test everything you're trying to test here and more (would need 2x
of those..), and would be easier to read & understand.

Re: [PATCH 2/2] ls-files: add --sparse option

[Derrick Stolee]

On 11/22/2021 9:07 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Nov 16 2021, Derrick Stolee via GitGitGadget wrote:

Things in the dependent topics are starting to simmer down, so I'm
back revisiting this topic.

>> From: Derrick Stolee <dstolee@microsoft.com>
>> [...]
>> +test_expect_success 'ls-files' '
>> +	init_repos &&
>> +
>> +	# Behavior agrees by default. Sparse index is expanded.
>> +	test_all_match git ls-files &&
>> +
>> +	# With --sparse, the sparse index data changes behavior.
>> +	git -C sparse-index ls-files --sparse >sparse-index-out &&
>> +	grep "^folder1/\$" sparse-index-out &&
>> +	grep "^folder2/\$" sparse-index-out &&
>> +
>> +	# With --sparse and no sparse index, nothing changes.
>> +	git -C sparse-checkout ls-files --sparse >sparse-checkout-out &&
>> +	grep "^folder1/0/0/0\$" sparse-checkout-out &&
>> +	! grep "/\$" sparse-checkout-out &&
> 
> I think all of this would be much clearer both in terms of explaining
> this change, and also for future test relability if it did away with the
> selective grepping, and simply ran tls-files with and without --sparse,
> and then test_cmp'd the full output (after munging away the OIDs).
> 
> I.e. the sort of output that's in my just-sent reply to the CL:
> https://lore.kernel.org/git/211123.86lf1fwrq5.gmgdl@evledraar.gmail.com/
> 
> We really don't need to optimize for lines of tests added, and having
> ~30 lines of plainly understood diff output is IMO preferrable to even 5
> lines of tricky positive & negative grep invocations that take some time
> to reason about and understand.
> 
> I.e. something like:
> 
>     cat >expected <<-\EOF &&
>      100644 blob OID   e
>      100644 blob OID   folder1-
>      100644 blob OID   folder1.x
>     -040000 tree OID   folder1/
>     +100644 blob OID   folder1/0/0/0
>     +100644 blob OID   folder1/0/1
>     +100644 blob OID   folder1/a
>      100644 blob OID   folder10
>     -040000 tree OID   folder2/
>     +100644 blob OID   folder2/0/0/0
>     +100644 blob OID   folder2/0/1
>     +100644 blob OID   folder2/a
>      100644 blob OID   g
>     -040000 tree OID   x/
>     +100644 blob OID   x/a
>      100644 blob OID   z
>     EOF
>     git [...] ls-files --sparse >actual.raw &&
>     [munge away OIDs] <actual.raw >actual &&
>     test_cmp expected actual
> 
> Would test everything you're trying to test here and more (would need 2x
> of those..), and would be easier to read & understand.

I don't think it is that hard to understand "I expect to see these
lines and not these lines" but I am open to more fully verifying
the full output and demonstrating the change that happens when the
flag is added.

Taking your idea and applying it to 'ls-files' (without --stage to
avoid OIDs which would change depending on the hash algorithm), the
start of the test looks like this:

test_expect_success 'ls-files' '
	init_repos &&

	# Behavior agrees by default. Sparse index is expanded.
	test_all_match git ls-files &&

	# With --sparse, the sparse index data changes behavior.
	git -C sparse-index ls-files --stage >out &&
	git -C sparse-index ls-files --stage --sparse >sparse &&

	cat >expect <<-\EOF &&
	 e
	 folder1-
	 folder1.x
	-folder1/0/0/0
	-folder1/0/1
	-folder1/a
	+folder1/
	 folder10
	-folder2/0/0/0
	-folder2/0/1
	-folder2/a
	+folder2/
	 g
	-x/a
	+x/
	 z
	EOF

	diff -u out sparse | tail -n 16 >actual &&
	test_cmp expect actual
'

I can make similar adjustments throughout the test to match
this style.

Thanks,
-Stolee

Re: [PATCH 2/2] ls-files: add --sparse option

[Derrick Stolee]

On 12/8/2021 10:14 AM, Derrick Stolee wrote:

> Taking your idea and applying it to 'ls-files' (without --stage to
> avoid OIDs which would change depending on the hash algorithm), the
> start of the test looks like this:
> 
> test_expect_success 'ls-files' '
> 	init_repos &&
> 
> 	# Behavior agrees by default. Sparse index is expanded.
> 	test_all_match git ls-files &&
> 
> 	# With --sparse, the sparse index data changes behavior.
> 	git -C sparse-index ls-files --stage >out &&
> 	git -C sparse-index ls-files --stage --sparse >sparse &&

Of course, I had a stale version here... "--stage" should not be here.
 
Thanks,
-Stolee

Re: [PATCH 2/2] ls-files: add --sparse option

[Elijah Newren]

On Wed, Dec 8, 2021 at 7:14 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 11/22/2021 9:07 PM, Ævar Arnfjörð Bjarmason wrote:
> >
> > On Tue, Nov 16 2021, Derrick Stolee via GitGitGadget wrote:
>
> Things in the dependent topics are starting to simmer down, so I'm
> back revisiting this topic.
>
> >> From: Derrick Stolee <dstolee@microsoft.com>
> >> [...]
> >> +test_expect_success 'ls-files' '
> >> +    init_repos &&
> >> +
> >> +    # Behavior agrees by default. Sparse index is expanded.
> >> +    test_all_match git ls-files &&
> >> +
> >> +    # With --sparse, the sparse index data changes behavior.
> >> +    git -C sparse-index ls-files --sparse >sparse-index-out &&
> >> +    grep "^folder1/\$" sparse-index-out &&
> >> +    grep "^folder2/\$" sparse-index-out &&
> >> +
> >> +    # With --sparse and no sparse index, nothing changes.
> >> +    git -C sparse-checkout ls-files --sparse >sparse-checkout-out &&
> >> +    grep "^folder1/0/0/0\$" sparse-checkout-out &&
> >> +    ! grep "/\$" sparse-checkout-out &&
> >
> > I think all of this would be much clearer both in terms of explaining
> > this change, and also for future test relability if it did away with the
> > selective grepping, and simply ran tls-files with and without --sparse,
> > and then test_cmp'd the full output (after munging away the OIDs).
> >
> > I.e. the sort of output that's in my just-sent reply to the CL:
> > https://lore.kernel.org/git/211123.86lf1fwrq5.gmgdl@evledraar.gmail.com/
> >
> > We really don't need to optimize for lines of tests added, and having
> > ~30 lines of plainly understood diff output is IMO preferrable to even 5
> > lines of tricky positive & negative grep invocations that take some time
> > to reason about and understand.
> >
> > I.e. something like:
> >
> >     cat >expected <<-\EOF &&
> >      100644 blob OID   e
> >      100644 blob OID   folder1-
> >      100644 blob OID   folder1.x
> >     -040000 tree OID   folder1/
> >     +100644 blob OID   folder1/0/0/0
> >     +100644 blob OID   folder1/0/1
> >     +100644 blob OID   folder1/a
> >      100644 blob OID   folder10
> >     -040000 tree OID   folder2/
> >     +100644 blob OID   folder2/0/0/0
> >     +100644 blob OID   folder2/0/1
> >     +100644 blob OID   folder2/a
> >      100644 blob OID   g
> >     -040000 tree OID   x/
> >     +100644 blob OID   x/a
> >      100644 blob OID   z
> >     EOF
> >     git [...] ls-files --sparse >actual.raw &&
> >     [munge away OIDs] <actual.raw >actual &&
> >     test_cmp expected actual
> >
> > Would test everything you're trying to test here and more (would need 2x
> > of those..), and would be easier to read & understand.

The loss of checking for trees would be bad; the point of testing a
sparse-index is that it has tree objects in it.  However, the basic
suggestion inspired Stolee's variant below that does check for trees.
So...

> I don't think it is that hard to understand "I expect to see these
> lines and not these lines" but I am open to more fully verifying
> the full output and demonstrating the change that happens when the
> flag is added.
>
> Taking your idea and applying it to 'ls-files' (without --stage to
> avoid OIDs which would change depending on the hash algorithm), the
> start of the test looks like this:
>
> test_expect_success 'ls-files' '
>         init_repos &&
>
>         # Behavior agrees by default. Sparse index is expanded.
>         test_all_match git ls-files &&
>
>         # With --sparse, the sparse index data changes behavior.
>         git -C sparse-index ls-files --stage >out &&
>         git -C sparse-index ls-files --stage --sparse >sparse &&
>
>         cat >expect <<-\EOF &&
>          e
>          folder1-
>          folder1.x
>         -folder1/0/0/0
>         -folder1/0/1
>         -folder1/a
>         +folder1/
>          folder10
>         -folder2/0/0/0
>         -folder2/0/1
>         -folder2/a
>         +folder2/
>          g
>         -x/a
>         +x/
>          z
>         EOF
>
>         diff -u out sparse | tail -n 16 >actual &&
>         test_cmp expect actual
> '

This actually looks quite nice, though the magic '16' is kind of
annoying.  Could we get rid of that -- perhaps using something to rip
out the diff header, or using comm instead?

Also, perhaps 'dense' rather than 'out'?

Re: [PATCH 2/2] ls-files: add --sparse option

[Derrick Stolee]

On 12/8/2021 12:04 PM, Elijah Newren wrote:
> On Wed, Dec 8, 2021 at 7:14 AM Derrick Stolee <stolee@gmail.com> wrote:

>> Taking your idea and applying it to 'ls-files' (without --stage to
>> avoid OIDs which would change depending on the hash algorithm), the
>> start of the test looks like this:
>>
>> test_expect_success 'ls-files' '
>>         init_repos &&
>>
>>         # Behavior agrees by default. Sparse index is expanded.
>>         test_all_match git ls-files &&
>>
>>         # With --sparse, the sparse index data changes behavior.
>>         git -C sparse-index ls-files --stage >out &&
>>         git -C sparse-index ls-files --stage --sparse >sparse &&
>>
>>         cat >expect <<-\EOF &&
>>          e
>>          folder1-
>>          folder1.x
>>         -folder1/0/0/0
>>         -folder1/0/1
>>         -folder1/a
>>         +folder1/
>>          folder10
>>         -folder2/0/0/0
>>         -folder2/0/1
>>         -folder2/a
>>         +folder2/
>>          g
>>         -x/a
>>         +x/
>>          z
>>         EOF
>>
>>         diff -u out sparse | tail -n 16 >actual &&
>>         test_cmp expect actual
>> '
> 
> This actually looks quite nice, though the magic '16' is kind of
> annoying.  Could we get rid of that -- perhaps using something to rip
> out the diff header, or using comm instead?

What I really want is "remove the first two lines of this file"
but perhaps "tail -n $(wc -l expect)" would suffice to avoid a
magic number?

> Also, perhaps 'dense' rather than 'out'?

Sounds good.

Thanks,
-Stolee

Re: [PATCH 2/2] ls-files: add --sparse option

[Elijah Newren]

On Wed, Dec 8, 2021 at 10:23 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 12/8/2021 12:04 PM, Elijah Newren wrote:
> > On Wed, Dec 8, 2021 at 7:14 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> >> Taking your idea and applying it to 'ls-files' (without --stage to
> >> avoid OIDs which would change depending on the hash algorithm), the
> >> start of the test looks like this:
> >>
> >> test_expect_success 'ls-files' '
> >>         init_repos &&
> >>
> >>         # Behavior agrees by default. Sparse index is expanded.
> >>         test_all_match git ls-files &&
> >>
> >>         # With --sparse, the sparse index data changes behavior.
> >>         git -C sparse-index ls-files --stage >out &&
> >>         git -C sparse-index ls-files --stage --sparse >sparse &&
> >>
> >>         cat >expect <<-\EOF &&
> >>          e
> >>          folder1-
> >>          folder1.x
> >>         -folder1/0/0/0
> >>         -folder1/0/1
> >>         -folder1/a
> >>         +folder1/
> >>          folder10
> >>         -folder2/0/0/0
> >>         -folder2/0/1
> >>         -folder2/a
> >>         +folder2/
> >>          g
> >>         -x/a
> >>         +x/
> >>          z
> >>         EOF
> >>
> >>         diff -u out sparse | tail -n 16 >actual &&
> >>         test_cmp expect actual
> >> '
> >
> > This actually looks quite nice, though the magic '16' is kind of
> > annoying.  Could we get rid of that -- perhaps using something to rip
> > out the diff header, or using comm instead?
>
> What I really want is "remove the first two lines of this file"

Is `tail -n +3` portable?  Looks like we have five uses of tail -n +N
in the testsuite, so it should be okay to use.

> but perhaps "tail -n $(wc -l expect)" would suffice to avoid a
> magic number?

That works too.

> > Also, perhaps 'dense' rather than 'out'?
>
> Sounds good.
>
> Thanks,
> -Stolee

Re: [PATCH 2/2] ls-files: add --sparse option

[Derrick Stolee]

On 12/8/2021 1:36 PM, Elijah Newren wrote:
> On Wed, Dec 8, 2021 at 10:23 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 12/8/2021 12:04 PM, Elijah Newren wrote:
>>>
>>> This actually looks quite nice, though the magic '16' is kind of
>>> annoying.  Could we get rid of that -- perhaps using something to rip
>>> out the diff header, or using comm instead?
>>
>> What I really want is "remove the first two lines of this file"
> 
> Is `tail -n +3` portable?  Looks like we have five uses of tail -n +N
> in the testsuite, so it should be okay to use.

Ah, that's the magic incantation. Sounds good.

>> but perhaps "tail -n $(wc -l expect)" would suffice to avoid a
>> magic number?
> 
> That works too.

If the "-n +X" syntax works, then I'll opt for that.

Thanks!

Re: [PATCH 2/2] ls-files: add --sparse option

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 08 2021, Derrick Stolee wrote:

> On 12/8/2021 1:36 PM, Elijah Newren wrote:
>> On Wed, Dec 8, 2021 at 10:23 AM Derrick Stolee <stolee@gmail.com> wrote:
>>>
>>> On 12/8/2021 12:04 PM, Elijah Newren wrote:
>>>>
>>>> This actually looks quite nice, though the magic '16' is kind of
>>>> annoying.  Could we get rid of that -- perhaps using something to rip
>>>> out the diff header, or using comm instead?
>>>
>>> What I really want is "remove the first two lines of this file"
>> 
>> Is `tail -n +3` portable?  Looks like we have five uses of tail -n +N
>> in the testsuite, so it should be okay to use.
>
> Ah, that's the magic incantation. Sounds good.
>
>>> but perhaps "tail -n $(wc -l expect)" would suffice to avoid a
>>> magic number?
>> 
>> That works too.
>
> If the "-n +X" syntax works, then I'll opt for that.

I think it should be per
https://pubs.opengroup.org/onlinepubs/009695299/utilities/tail.html

But isn't that "diff -u" non-portable, per GIT_TEST_CMP. I.e. sometimes
we'll fall back on "cmp" etc.

Just pre-munging both "a" and "b" and doing a:

    test_cmp a b

Should work around that & make it portable.

I realize that's also a problem with my initial example, usually we'd
compare expected/actual, not the diff between the two.

But if that's more readable we could also just use "git diff --no-index
a b >actual" and "test_cmp" that v.s. "expected".

    

Re: [PATCH 2/2] ls-files: add --sparse option

[Derrick Stolee]

On 12/9/2021 7:50 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Dec 08 2021, Derrick Stolee wrote:
> 
>> On 12/8/2021 1:36 PM, Elijah Newren wrote:
>>> On Wed, Dec 8, 2021 at 10:23 AM Derrick Stolee <stolee@gmail.com> wrote:
>>>>
>>>> On 12/8/2021 12:04 PM, Elijah Newren wrote:
>>>>>
>>>>> This actually looks quite nice, though the magic '16' is kind of
>>>>> annoying.  Could we get rid of that -- perhaps using something to rip
>>>>> out the diff header, or using comm instead?
>>>>
>>>> What I really want is "remove the first two lines of this file"
>>>
>>> Is `tail -n +3` portable?  Looks like we have five uses of tail -n +N
>>> in the testsuite, so it should be okay to use.
>>
>> Ah, that's the magic incantation. Sounds good.
>>
>>>> but perhaps "tail -n $(wc -l expect)" would suffice to avoid a
>>>> magic number?
>>>
>>> That works too.
>>
>> If the "-n +X" syntax works, then I'll opt for that.
> 
> I think it should be per
> https://pubs.opengroup.org/onlinepubs/009695299/utilities/tail.html
> 
> But isn't that "diff -u" non-portable, per GIT_TEST_CMP. I.e. sometimes
> we'll fall back on "cmp" etc.

You're talking about this hunk, right?

if test -z "$GIT_TEST_CMP"
then
	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
	then
		GIT_TEST_CMP="$DIFF -c"
	else
		GIT_TEST_CMP="$DIFF -u"
	fi
fi

This only switches from "diff -u" to "diff -c" if the
GIT_TEST_CMP_USE_COPIED_CONTEXT variable is set, but it is not set
by default. Thus, we are using "diff -u" by default throughout.

Please let me know if I'm misreading this.

Thanks,
-Stolee

Re: [PATCH 2/2] ls-files: add --sparse option

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 10 2021, Derrick Stolee wrote:

> On 12/9/2021 7:50 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Wed, Dec 08 2021, Derrick Stolee wrote:
>> 
>>> On 12/8/2021 1:36 PM, Elijah Newren wrote:
>>>> On Wed, Dec 8, 2021 at 10:23 AM Derrick Stolee <stolee@gmail.com> wrote:
>>>>>
>>>>> On 12/8/2021 12:04 PM, Elijah Newren wrote:
>>>>>>
>>>>>> This actually looks quite nice, though the magic '16' is kind of
>>>>>> annoying.  Could we get rid of that -- perhaps using something to rip
>>>>>> out the diff header, or using comm instead?
>>>>>
>>>>> What I really want is "remove the first two lines of this file"
>>>>
>>>> Is `tail -n +3` portable?  Looks like we have five uses of tail -n +N
>>>> in the testsuite, so it should be okay to use.
>>>
>>> Ah, that's the magic incantation. Sounds good.
>>>
>>>>> but perhaps "tail -n $(wc -l expect)" would suffice to avoid a
>>>>> magic number?
>>>>
>>>> That works too.
>>>
>>> If the "-n +X" syntax works, then I'll opt for that.
>> 
>> I think it should be per
>> https://pubs.opengroup.org/onlinepubs/009695299/utilities/tail.html
>> 
>> But isn't that "diff -u" non-portable, per GIT_TEST_CMP. I.e. sometimes
>> we'll fall back on "cmp" etc.
>
> You're talking about this hunk, right?
>
> if test -z "$GIT_TEST_CMP"
> then
> 	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
> 	then
> 		GIT_TEST_CMP="$DIFF -c"
> 	else
> 		GIT_TEST_CMP="$DIFF -u"
> 	fi
> fi
>
> This only switches from "diff -u" to "diff -c" if the
> GIT_TEST_CMP_USE_COPIED_CONTEXT variable is set, but it is not set
> by default. Thus, we are using "diff -u" by default throughout.
>
> Please let me know if I'm misreading this.

Yes and no. Yes it's not on by default, but the reason (well, maybe not
*the* reason, but definitely one reason) we have GIT_TEST_CMP is because
-u isn't portable.

It's not in POSIX, and not just in the theoretical sense, e.g. when I
run tests on HP/UX it'll yell at me about not knowing about -u until I
adjust GIT_TEST_CMP.

Thus you can't use "diff -u" as you did upthread without breaking the
test on those platforms, and you can't execute "$GIT_TEST_CMP" either
because it might use -c, and then your "skip the first three lines"
might not hold.

So even if we avoid using "git" for the actual test helpers themselves
usually using it in this case is probably fine, and a neat way out of
this particular hassle without worrying about portability.

So I don't know if we want a diff here, but if so I think it would be
neat to wrap that in say a "test_cmp_diff" which we'd just use as:

	test_cmp_diff a b <<-\EOF
        <unified diff here>
	EOF

Where under the hood we'd just run:

	git diff --no-index a b >actual
	cat >expect &&
	[ munge diff header here? ] &&
	test_cmp expect actual

Re: [PATCH 2/2] ls-files: add --sparse option

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> You're talking about this hunk, right?
>
> if test -z "$GIT_TEST_CMP"
> then
> 	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
> 	then
> 		GIT_TEST_CMP="$DIFF -c"
> 	else
> 		GIT_TEST_CMP="$DIFF -u"
> 	fi
> fi
>
> This only switches from "diff -u" to "diff -c" if the
> GIT_TEST_CMP_USE_COPIED_CONTEXT variable is set, but it is not set
> by default. Thus, we are using "diff -u" by default throughout.

That it can be set merely means that somebody needed to work around
the lack of "-u" format in their implementation of "diff".  It came
to POSIX only at the Issue 7 of the standard (cf. [*1*]).

Unconditional use of "diff -u" is a breaking change for them.

If these people still exist, that is ;-)


[Reference]

*1* https://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html
The Open Group Base Specifications Issue 7, 2018 edition

Re: [PATCH 2/2] ls-files: add --sparse option

[Derrick Stolee]

On 12/13/2021 2:16 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> You're talking about this hunk, right?
>>
>> if test -z "$GIT_TEST_CMP"
>> then
>> 	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
>> 	then
>> 		GIT_TEST_CMP="$DIFF -c"
>> 	else
>> 		GIT_TEST_CMP="$DIFF -u"
>> 	fi
>> fi
>>
>> This only switches from "diff -u" to "diff -c" if the
>> GIT_TEST_CMP_USE_COPIED_CONTEXT variable is set, but it is not set
>> by default. Thus, we are using "diff -u" by default throughout.
> 
> That it can be set merely means that somebody needed to work around
> the lack of "-u" format in their implementation of "diff".  It came
> to POSIX only at the Issue 7 of the standard (cf. [*1*]).
> 
> Unconditional use of "diff -u" is a breaking change for them.

Yes, noted. I'm currently waiting for ld/sparse-diff-blame to
merge to 'master' before sending a new version, since there are
unrelated build failures without taking newer changes in 'master'.

Thanks,
-Stolee

Re: [PATCH 0/2] Sparse index: fetch, pull, ls-files

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This is based on ld/sparse-index-blame (merged with 'master' due to an
> unrelated build issue).
>
> Here are two relatively-simple patches that further the sparse index
> integrations.
>
> Did you know that 'fetch' and 'pull' read the index? I didn't, or this would
> have been an integration much earlier in the cycle. They read the index to
> look for the .gitmodules file in case there are submodules that need to be
> fetched. Since looking for a file by name is already protected, we only need
> to disable 'command_requires_full_index' and we are done.

This reminds me of one thing.  Our strategy so far has been:

 - start with blindly calling ensure_full();

 - audit the code and adjust each code path that needs to walk to

   . keep walking the full index, but narrow the scope of
     ensure_full_index() if we can; or

   . stop assuming we need to walk the full index, but teach it to
     "skip" the tree-in-index that we are not interested in.

 - declare victory and drop the blind call to ensure_full().

But what makes sure, after all of the above happens, that no new
changes that assume it can walk the full index enters in the
codebase?

In other words, after "fetch" is declared "sparse clean" with patch
[1/2], what effort from us will it take to stay clean?

Thanks.

Re: [PATCH 0/2] Sparse index: fetch, pull, ls-files

[Derrick Stolee]

On 11/17/2021 4:29 AM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> This is based on ld/sparse-index-blame (merged with 'master' due to an
>> unrelated build issue).
>>
>> Here are two relatively-simple patches that further the sparse index
>> integrations.
>>
>> Did you know that 'fetch' and 'pull' read the index? I didn't, or this would
>> have been an integration much earlier in the cycle. They read the index to
>> look for the .gitmodules file in case there are submodules that need to be
>> fetched. Since looking for a file by name is already protected, we only need
>> to disable 'command_requires_full_index' and we are done.
> 
> This reminds me of one thing.  Our strategy so far has been:
> 
>  - start with blindly calling ensure_full();
> 
>  - audit the code and adjust each code path that needs to walk to
> 
>    . keep walking the full index, but narrow the scope of
>      ensure_full_index() if we can; or
> 
>    . stop assuming we need to walk the full index, but teach it to
>      "skip" the tree-in-index that we are not interested in.
> 
>  - declare victory and drop the blind call to ensure_full().

This is a fair summary of the approach.

> But what makes sure, after all of the above happens, that no new
> changes that assume it can walk the full index enters in the
> codebase?
> 
> In other words, after "fetch" is declared "sparse clean" with patch
> [1/2], what effort from us will it take to stay clean?

The tests in t1092 that use the "ensure_not_expanded" helper are
intended to be regression tests that would start failing if the
sparse index starts expanding in a new way. I think this is what
you mean by staying "sparse clean".

The rest of the tests are attempting to verify that the behavior
is correct when the sparse index is enabled, and that is hopefully
a more obvious situation when something goes wrong. We've tried to
create a robust set of tests here, but I'm sure we will add new
ones as more users start using it. (We will soon have a large set
of real users of the sparse index, and that should be informative.)

Do you see a test gap that would be prudent to address?

One direction I could see is that as new features are contributed
that change how the index is used, these features are not
automatically tested with sparse-checkout and the sparse index.
In this case, we will need to increase our awareness when reviewing
such features to ensure that they could fit within the sparse index
model (or are sufficiently protected by ensure_full_index() in their
first version).

I spent some time thinking about how we could add a
GIT_TEST_SPARSE_INDEX mode to the test suite that would automatically
test the sparse index in the existing tests. However, the feature is
only enabled when in cone-mode sparse-checkout, so we would need to
modify the test repos in a way that some sparse directory entries
exist but also don't collide with the test expectations. I never
found a way to get around that issue, which is why t1092 is such a
big test script.

Thanks,
-Stolee

Re: [PATCH 0/2] Sparse index: fetch, pull, ls-files

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

>> But what makes sure, after all of the above happens, that no new
>> changes that assume it can walk the full index enters in the
>> codebase?
>> 
>> In other words, after "fetch" is declared "sparse clean" with patch
>> [1/2], what effort from us will it take to stay clean?
>
> The tests in t1092 that use the "ensure_not_expanded" helper are
> intended to be regression tests that would start failing if the
> sparse index starts expanding in a new way. I think this is what
> you mean by staying "sparse clean".

It is more like how would we help a new piece of code, which does
not even consider the possibility that the in-core index might be
sparse, and instead blindly walks the in-core index entries, doing
something unexpected (to both the author of the new code and to
those who are aware of the sparse-index feature) when it sees an
"tree" entry.  We'll hopefully see a breakage and the developer who
added such a new piece of code can keep both halves?

> One direction I could see is that as new features are contributed
> that change how the index is used, these features are not
> automatically tested with sparse-checkout and the sparse index.
> In this case, we will need to increase our awareness when reviewing
> such features to ensure that they could fit within the sparse index
> model (or are sufficiently protected by ensure_full_index() in their
> first version).

Yup, making developers aware of the issue is probably the good first
thing to do.

Re: [PATCH 0/2] Sparse index: fetch, pull, ls-files

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 16 2021, Derrick Stolee via GitGitGadget wrote:

> The 'ls-files' builtin is useful when debugging the index, and some scripts
> use it, too. We are not changing the default behavior which expands a sparse
> index in order to show all of the cached blobs. Instead, we add a '--sparse'
> option that allows us to see the sparse directory entries upon request.
> Combined with --debug, we can see a lot of index details, such as:
>
> $ git ls-files --debug --sparse
> LICENSE
>   ctime: 1634910503:287405820
>   mtime: 1634910503:287405820
>   dev: 16777220 ino: 119325319
>   uid: 501  gid: 20
>   size: 1098    flags: 200000
> README.md
>   ctime: 1634910503:288090279
>   mtime: 1634910503:288090279
>   dev: 16777220 ino: 119325320
>   uid: 501  gid: 20
>   size: 934 flags: 200000
> bin/index.js
>   ctime: 1634910767:828434033
>   mtime: 1634910767:828434033
>   dev: 16777220 ino: 119325520
>   uid: 501  gid: 20
>   size: 7292    flags: 200000
> examples/
>   ctime: 0:0
>   mtime: 0:0
>   dev: 0    ino: 0
>   uid: 0    gid: 0
>   size: 0   flags: 40004000
> package.json
>   ctime: 1634910503:288676330
>   mtime: 1634910503:288676330
>   dev: 16777220 ino: 119325321
>   uid: 501  gid: 20
>   size: 680 flags: 200000
>
>
> (In this example, the 'examples/' directory is sparse.)
>
> Thanks!

That's useful, and seems to be closing the same feature gap that the RFC
series I sent back in March[1], but at the time you went with adding
this ability to t/helper/test-read-cache.c.

With your series diffing the two with the data used in your new test
shows that we had this with the helper beore:
    
     diff -u <(t/helper/test-tool -C sparse-index read-cache --table) <(t/helper/test-tool -C sparse-index read-cache --table --expand)
    --- /dev/fd/63  2021-11-23 02:57:00.980651400 +0100
    +++ /dev/fd/62  2021-11-23 02:57:00.980651400 +0100
    @@ -13,9 +13,13 @@
     100644 blob cebd1739abee3e524d72ca9f51465a94d5a71daf   e
     100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391   folder1-
     100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391   folder1.x
    -040000 tree e0f2d30b633eb781d675fedd78808135103fe1a0   folder1/
    +100644 blob 8b137891791fe96927ad78e64b0aad7bded08bdc   folder1/0/0/0
    +100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391   folder1/0/1
    +100644 blob 78981922613b2afb6025042ff6bd878ac1994e85   folder1/a
     100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391   folder10
    -040000 tree 123706f6fc38949628eaf0483edbf97ba21123ae   folder2/
    +100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391   folder2/0/0/0
    +100644 blob 8b137891791fe96927ad78e64b0aad7bded08bdc   folder2/0/1
    +100644 blob 78981922613b2afb6025042ff6bd878ac1994e85   folder2/a
     100644 blob 18912c9a915632d7b3344ec25f349dc8b4b9bf27   g
    -040000 tree aaff74984cccd156a469afa7d9ab10e4777beb24   x/
    +100644 blob 78981922613b2afb6025042ff6bd878ac1994e85   x/a
     100644 blob 2d8c856aebeb20da61bd5112d6fa46ff3f56a9e8   z

Which we can now get out of ls-files:
    
    $ diff -u <(git -C sparse-index ls-files --stage --sparse) <(git -C sparse-index ls-files --stage)
    --- /dev/fd/63  2021-11-23 02:55:13.329613255 +0100
    +++ /dev/fd/62  2021-11-23 02:55:13.329613255 +0100
    @@ -13,9 +13,13 @@
     100644 cebd1739abee3e524d72ca9f51465a94d5a71daf 0      e
     100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0      folder1-
     100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0      folder1.x
    -040000 e0f2d30b633eb781d675fedd78808135103fe1a0 0      folder1/
    +100644 8b137891791fe96927ad78e64b0aad7bded08bdc 0      folder1/0/0/0
    +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0      folder1/0/1
    +100644 78981922613b2afb6025042ff6bd878ac1994e85 0      folder1/a
     100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0      folder10
    -040000 123706f6fc38949628eaf0483edbf97ba21123ae 0      folder2/
    +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0      folder2/0/0/0
    +100644 8b137891791fe96927ad78e64b0aad7bded08bdc 0      folder2/0/1
    +100644 78981922613b2afb6025042ff6bd878ac1994e85 0      folder2/a
     100644 18912c9a915632d7b3344ec25f349dc8b4b9bf27 0      g
    -040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0      x/
    +100644 78981922613b2afb6025042ff6bd878ac1994e85 0      x/a
     100644 2d8c856aebeb20da61bd5112d6fa46ff3f56a9e8 0      z

So that's neat, but unless I'm wrong about there still being some
special-sauce in t/helper/test-read-cache.c that's needed this series
seems incomplete without migrating the existing test users of it to this
new ls-files --sparse, followed by a cherry-pick (or equivalent) of [2].

Is there any reason we wouldn't use ls-files instead of the test-tool in
the tests now? I understood from you at the time that the reason for the
that mode of the test-tool existing was an interim state until ls-files
learned to emit this sort of output, but that you wanted to add that
later. It seems we've arrived at that "later" :)

1. https://lore.kernel.org/git/20210317132814.30175-1-avarab@gmail.com/
2. https://lore.kernel.org/git/20210317132814.30175-5-avarab@gmail.com/

[PATCH v2 0/5] Sparse index: fetch, pull, ls-files

[Derrick Stolee via GitGitGadget]

This is based on ld/sparse-index-blame (merged with 'master' due to an
unrelated build issue).

Here are two relatively-simple patches that further the sparse index
integrations.

Did you know that 'fetch' and 'pull' read the index? I didn't, or this would
have been an integration much earlier in the cycle. They read the index to
look for the .gitmodules file in case there are submodules that need to be
fetched. Since looking for a file by name is already protected, we only need
to disable 'command_requires_full_index' and we are done.

The 'ls-files' builtin is useful when debugging the index, and some scripts
use it, too. We are not changing the default behavior which expands a sparse
index in order to show all of the cached blobs. Instead, we add a '--sparse'
option that allows us to see the sparse directory entries upon request.
Combined with --debug, we can see a lot of index details, such as:

$ git ls-files --debug --sparse
LICENSE
  ctime: 1634910503:287405820
  mtime: 1634910503:287405820
  dev: 16777220 ino: 119325319
  uid: 501  gid: 20
  size: 1098    flags: 200000
README.md
  ctime: 1634910503:288090279
  mtime: 1634910503:288090279
  dev: 16777220 ino: 119325320
  uid: 501  gid: 20
  size: 934 flags: 200000
bin/index.js
  ctime: 1634910767:828434033
  mtime: 1634910767:828434033
  dev: 16777220 ino: 119325520
  uid: 501  gid: 20
  size: 7292    flags: 200000
examples/
  ctime: 0:0
  mtime: 0:0
  dev: 0    ino: 0
  uid: 0    gid: 0
  size: 0   flags: 40004000
package.json
  ctime: 1634910503:288676330
  mtime: 1634910503:288676330
  dev: 16777220 ino: 119325321
  uid: 501  gid: 20
  size: 680 flags: 200000


(In this example, the 'examples/' directory is sparse.)

Thanks!


Updates in v2
=============

 * Rebased onto latest ld/sparse-index-blame without issue.
 * Updated the test to use diff-of-diffs instead of a sequence of greps.
 * Added patches that remove the use of 'test-tool read-cache --table' and
   its implementation.

Derrick Stolee (5):
  fetch/pull: use the sparse index
  ls-files: add --sparse option
  t1092: replace 'read-cache --table' with 'ls-files --sparse'
  t1091/t3705: remove 'test-tool read-cache --table'
  test-read-cache: remove --table, --expand options

 Documentation/git-ls-files.txt           |   4 +
 builtin/fetch.c                          |   2 +
 builtin/ls-files.c                       |  12 ++-
 builtin/pull.c                           |   2 +
 t/helper/test-read-cache.c               |  64 ++---------
 t/t1091-sparse-checkout-builtin.sh       |  25 ++++-
 t/t1092-sparse-checkout-compatibility.sh | 129 ++++++++++++++++++++---
 t/t3705-add-sparse-checkout.sh           |   8 +-
 8 files changed, 165 insertions(+), 81 deletions(-)


base-commit: 3fffe69d24e4ecc95246766f5396303a953695ff
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1080%2Fderrickstolee%2Fsparse-index%2Ffetch-pull-ls-files-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1080/derrickstolee/sparse-index/fetch-pull-ls-files-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1080

Range-diff vs v1:

 1:  451056e1a77 ! 1:  f72001638d1 fetch/pull: use the sparse index
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
      
       ## t/t1092-sparse-checkout-compatibility.sh ##
      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is not expanded: blame' '
     - 	ensure_not_expanded blame deep/deeper1/deepest/a
     + 	done
       '
       
      +test_expect_success 'sparse index is not expanded: fetch/pull' '
 2:  e42c0feec94 ! 2:  58b5eca4835 ls-files: add --sparse option
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
      +	test_all_match git ls-files &&
      +
      +	# With --sparse, the sparse index data changes behavior.
     -+	git -C sparse-index ls-files --sparse >sparse-index-out &&
     -+	grep "^folder1/\$" sparse-index-out &&
     -+	grep "^folder2/\$" sparse-index-out &&
     ++	git -C sparse-index ls-files >dense &&
     ++	git -C sparse-index ls-files --sparse >sparse &&
     ++
     ++	cat >expect <<-\EOF &&
     ++	@@ -13,13 +13,9 @@
     ++	 e
     ++	 folder1-
     ++	 folder1.x
     ++	-folder1/0/0/0
     ++	-folder1/0/1
     ++	-folder1/a
     ++	+folder1/
     ++	 folder10
     ++	-folder2/0/0/0
     ++	-folder2/0/1
     ++	-folder2/a
     ++	+folder2/
     ++	 g
     ++	-x/a
     ++	+x/
     ++	 z
     ++	EOF
     ++
     ++	diff -u dense sparse | tail -n +3 >actual &&
     ++	test_cmp expect actual &&
      +
      +	# With --sparse and no sparse index, nothing changes.
     -+	git -C sparse-checkout ls-files --sparse >sparse-checkout-out &&
     -+	grep "^folder1/0/0/0\$" sparse-checkout-out &&
     -+	! grep "/\$" sparse-checkout-out &&
     ++	git -C sparse-checkout ls-files >dense &&
     ++	git -C sparse-checkout ls-files --sparse >sparse &&
     ++	test_cmp dense sparse &&
      +
      +	write_script edit-content <<-\EOF &&
      +	mkdir folder1 &&
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
      +	git -C sparse-index ls-files --sparse --modified >sparse-index-out &&
      +	test_must_be_empty sparse-index-out &&
      +
     -+	run_on_sparse git sparse-checkout add folder1 &&
     ++	# Add folder1 to the sparse-checkout cone and
     ++	# check that ls-files shows the expanded files.
     ++	test_sparse_match git sparse-checkout add folder1 &&
      +	test_sparse_match git ls-files --modified &&
     -+	grep "^folder1/a\$" sparse-checkout-out &&
     -+	grep "^folder1/a\$" sparse-index-out &&
      +
     -+	# Double-check index expansion
     ++	git -C sparse-index ls-files >dense &&
     ++	git -C sparse-index ls-files --sparse >sparse &&
     ++
     ++	cat >expect <<-\EOF &&
     ++	@@ -17,9 +17,7 @@
     ++	 folder1/0/1
     ++	 folder1/a
     ++	 folder10
     ++	-folder2/0/0/0
     ++	-folder2/0/1
     ++	-folder2/a
     ++	+folder2/
     ++	 g
     ++	-x/a
     ++	+x/
     ++	 z
     ++	EOF
     ++
     ++	diff -u dense sparse | tail -n +3 >actual &&
     ++	test_cmp expect actual &&
     ++
     ++	# Double-check index expansion is avoided
      +	ensure_not_expanded ls-files --sparse
      +'
      +
 -:  ----------- > 3:  5ffae2a03ae t1092: replace 'read-cache --table' with 'ls-files --sparse'
 -:  ----------- > 4:  b98e5e6d2bc t1091/t3705: remove 'test-tool read-cache --table'
 -:  ----------- > 5:  f31a24eeb9b test-read-cache: remove --table, --expand options

-- 
gitgitgadget

[PATCH v2 1/5] fetch/pull: use the sparse index

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The 'git fetch' and 'git pull' commands parse the index in order to
determine if submodules exist. Without command_requires_full_index=0,
this will expand a sparse index, causing slow performance even when
there is no new data to fetch.

The .gitmodules file will never be inside a sparse directory entry, and
even if it was, the index_name_pos() method would expand the sparse
index if needed as we search for the path by name. These commands do not
iterate over the index, which is the typical thing we are careful about
when integrating with the sparse index.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/fetch.c                          |  2 ++
 builtin/pull.c                           |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 10 ++++++++++
 3 files changed, 14 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f7abbc31ff1..1696b7791d4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1993,6 +1993,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	}
 
 	git_config(git_fetch_config, NULL);
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_fetch_options, builtin_fetch_usage, 0);
diff --git a/builtin/pull.c b/builtin/pull.c
index 1cfaf9f3436..7bce3bd80f9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -994,6 +994,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		set_reflog_message(argc, argv);
 
 	git_config(git_pull_config, NULL);
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
 
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 83971174ac3..3ba19ba1c14 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -964,6 +964,16 @@ test_expect_success 'sparse index is not expanded: blame' '
 	done
 '
 
+test_expect_success 'sparse index is not expanded: fetch/pull' '
+	init_repos &&
+
+	git -C sparse-index remote add full "file://$(pwd)/full-checkout" &&
+	ensure_not_expanded fetch full &&
+	git -C full-checkout commit --allow-empty -m "for pull merge" &&
+	git -C sparse-index commit --allow-empty -m "for pull merge" &&
+	ensure_not_expanded pull full base
+'
+
 # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
 # in this scenario, but it shouldn't.
 test_expect_success 'reset mixed and checkout orphan' '
-- 
gitgitgadget

[PATCH v2 2/5] ls-files: add --sparse option

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Existing callers to 'git ls-files' are expecting file names, not
directories. It is best to expand a sparse index to show all of the
contained files in this case.

However, expert users may want to inspect the contents of the index
itself including which directories are sparse. Add a --sparse option to
allow users to request this information.

During testing, I noticed that options such as --modified did not affect
the output when the files in question were outside the sparse-checkout
definition. Tests are added to document this preexisting behavior and
how it remains unchanged with the sparse index and the --sparse option.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-ls-files.txt           |  4 ++
 builtin/ls-files.c                       | 12 +++-
 t/t1092-sparse-checkout-compatibility.sh | 90 ++++++++++++++++++++++++
 3 files changed, 104 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 6d11ab506b7..1c5d5f85ec5 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -187,6 +187,10 @@ Both the <eolinfo> in the index ("i/<eolinfo>")
 and in the working tree ("w/<eolinfo>") are shown for regular files,
 followed by the  ("attr/<eolattr>").
 
+--sparse::
+	If the index is sparse, show the sparse directories without expanding
+	to the contained files.
+
 \--::
 	Do not interpret any more arguments as options.
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 031fef1bcaa..c151eb1fb77 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -37,6 +37,7 @@ static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
 static int skipping_duplicates;
+static int show_sparse_dirs;
 
 static const char *prefix;
 static int max_prefix_len;
@@ -315,8 +316,10 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 
 	if (!(show_cached || show_stage || show_deleted || show_modified))
 		return;
-	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(repo->index);
+
+	if (!show_sparse_dirs)
+		ensure_full_index(repo->index);
+
 	for (i = 0; i < repo->index->cache_nr; i++) {
 		const struct cache_entry *ce = repo->index->cache[i];
 		struct stat st;
@@ -670,6 +673,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
 		OPT_BOOL(0, "deduplicate", &skipping_duplicates,
 			 N_("suppress duplicate entries")),
+		OPT_BOOL(0, "sparse", &show_sparse_dirs,
+			 N_("show sparse directories in the presence of a sparse index")),
 		OPT_END()
 	};
 	int ret = 0;
@@ -677,6 +682,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(ls_files_usage, builtin_ls_files_options);
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	prefix = cmd_prefix;
 	if (prefix)
 		prefix_len = strlen(prefix);
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 3ba19ba1c14..bf2c6b169b9 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -802,6 +802,12 @@ test_expect_success 'sparse-index is expanded and converted back' '
 	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
 		git -C sparse-index reset -- folder1/a &&
 	test_region index convert_to_sparse trace2.txt &&
+	test_region index ensure_full_index trace2.txt &&
+
+	# ls-files expands on read, but does not write.
+	rm trace2.txt &&
+	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
+		git -C sparse-index ls-files &&
 	test_region index ensure_full_index trace2.txt
 '
 
@@ -826,6 +832,7 @@ test_expect_success 'sparse-index is not expanded' '
 	init_repos &&
 
 	ensure_not_expanded status &&
+	ensure_not_expanded ls-files --sparse &&
 	ensure_not_expanded commit --allow-empty -m empty &&
 	echo >>sparse-index/a &&
 	ensure_not_expanded commit -a -m a &&
@@ -974,6 +981,89 @@ test_expect_success 'sparse index is not expanded: fetch/pull' '
 	ensure_not_expanded pull full base
 '
 
+test_expect_success 'ls-files' '
+	init_repos &&
+
+	# Behavior agrees by default. Sparse index is expanded.
+	test_all_match git ls-files &&
+
+	# With --sparse, the sparse index data changes behavior.
+	git -C sparse-index ls-files >dense &&
+	git -C sparse-index ls-files --sparse >sparse &&
+
+	cat >expect <<-\EOF &&
+	@@ -13,13 +13,9 @@
+	 e
+	 folder1-
+	 folder1.x
+	-folder1/0/0/0
+	-folder1/0/1
+	-folder1/a
+	+folder1/
+	 folder10
+	-folder2/0/0/0
+	-folder2/0/1
+	-folder2/a
+	+folder2/
+	 g
+	-x/a
+	+x/
+	 z
+	EOF
+
+	diff -u dense sparse | tail -n +3 >actual &&
+	test_cmp expect actual &&
+
+	# With --sparse and no sparse index, nothing changes.
+	git -C sparse-checkout ls-files >dense &&
+	git -C sparse-checkout ls-files --sparse >sparse &&
+	test_cmp dense sparse &&
+
+	write_script edit-content <<-\EOF &&
+	mkdir folder1 &&
+	echo content >>folder1/a
+	EOF
+	run_on_sparse ../edit-content &&
+
+	# ls-files does not notice modified files whose
+	# cache entries are marked SKIP_WORKTREE.
+	test_sparse_match git ls-files --modified &&
+	test_must_be_empty sparse-checkout-out &&
+	test_must_be_empty sparse-index-out &&
+
+	git -C sparse-index ls-files --sparse --modified >sparse-index-out &&
+	test_must_be_empty sparse-index-out &&
+
+	# Add folder1 to the sparse-checkout cone and
+	# check that ls-files shows the expanded files.
+	test_sparse_match git sparse-checkout add folder1 &&
+	test_sparse_match git ls-files --modified &&
+
+	git -C sparse-index ls-files >dense &&
+	git -C sparse-index ls-files --sparse >sparse &&
+
+	cat >expect <<-\EOF &&
+	@@ -17,9 +17,7 @@
+	 folder1/0/1
+	 folder1/a
+	 folder10
+	-folder2/0/0/0
+	-folder2/0/1
+	-folder2/a
+	+folder2/
+	 g
+	-x/a
+	+x/
+	 z
+	EOF
+
+	diff -u dense sparse | tail -n +3 >actual &&
+	test_cmp expect actual &&
+
+	# Double-check index expansion is avoided
+	ensure_not_expanded ls-files --sparse
+'
+
 # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
 # in this scenario, but it shouldn't.
 test_expect_success 'reset mixed and checkout orphan' '
-- 
gitgitgadget

Re: [PATCH v2 2/5] ls-files: add --sparse option

[Elijah Newren]

On Wed, Dec 8, 2021 at 11:39 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Existing callers to 'git ls-files' are expecting file names, not
> directories. It is best to expand a sparse index to show all of the
> contained files in this case.
>
> However, expert users may want to inspect the contents of the index
> itself including which directories are sparse. Add a --sparse option to
> allow users to request this information.
>
> During testing, I noticed that options such as --modified did not affect
> the output when the files in question were outside the sparse-checkout
> definition. Tests are added to document this preexisting behavior and
> how it remains unchanged with the sparse index and the --sparse option.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-ls-files.txt           |  4 ++
>  builtin/ls-files.c                       | 12 +++-
>  t/t1092-sparse-checkout-compatibility.sh | 90 ++++++++++++++++++++++++
>  3 files changed, 104 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 6d11ab506b7..1c5d5f85ec5 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -187,6 +187,10 @@ Both the <eolinfo> in the index ("i/<eolinfo>")
>  and in the working tree ("w/<eolinfo>") are shown for regular files,
>  followed by the  ("attr/<eolattr>").
>
> +--sparse::
> +       If the index is sparse, show the sparse directories without expanding
> +       to the contained files.
> +
>  \--::
>         Do not interpret any more arguments as options.
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 031fef1bcaa..c151eb1fb77 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -37,6 +37,7 @@ static int debug_mode;
>  static int show_eol;
>  static int recurse_submodules;
>  static int skipping_duplicates;
> +static int show_sparse_dirs;
>
>  static const char *prefix;
>  static int max_prefix_len;
> @@ -315,8 +316,10 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
>
>         if (!(show_cached || show_stage || show_deleted || show_modified))
>                 return;
> -       /* TODO: audit for interaction with sparse-index. */
> -       ensure_full_index(repo->index);
> +
> +       if (!show_sparse_dirs)
> +               ensure_full_index(repo->index);
> +
>         for (i = 0; i < repo->index->cache_nr; i++) {
>                 const struct cache_entry *ce = repo->index->cache[i];
>                 struct stat st;
> @@ -670,6 +673,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>                 OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
>                 OPT_BOOL(0, "deduplicate", &skipping_duplicates,
>                          N_("suppress duplicate entries")),
> +               OPT_BOOL(0, "sparse", &show_sparse_dirs,
> +                        N_("show sparse directories in the presence of a sparse index")),
>                 OPT_END()
>         };
>         int ret = 0;
> @@ -677,6 +682,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>         if (argc == 2 && !strcmp(argv[1], "-h"))
>                 usage_with_options(ls_files_usage, builtin_ls_files_options);
>
> +       prepare_repo_settings(the_repository);
> +       the_repository->settings.command_requires_full_index = 0;
> +
>         prefix = cmd_prefix;
>         if (prefix)
>                 prefix_len = strlen(prefix);
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 3ba19ba1c14..bf2c6b169b9 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -802,6 +802,12 @@ test_expect_success 'sparse-index is expanded and converted back' '
>         GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
>                 git -C sparse-index reset -- folder1/a &&
>         test_region index convert_to_sparse trace2.txt &&
> +       test_region index ensure_full_index trace2.txt &&
> +
> +       # ls-files expands on read, but does not write.
> +       rm trace2.txt &&
> +       GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
> +               git -C sparse-index ls-files &&
>         test_region index ensure_full_index trace2.txt
>  '
>
> @@ -826,6 +832,7 @@ test_expect_success 'sparse-index is not expanded' '
>         init_repos &&
>
>         ensure_not_expanded status &&
> +       ensure_not_expanded ls-files --sparse &&
>         ensure_not_expanded commit --allow-empty -m empty &&
>         echo >>sparse-index/a &&
>         ensure_not_expanded commit -a -m a &&
> @@ -974,6 +981,89 @@ test_expect_success 'sparse index is not expanded: fetch/pull' '
>         ensure_not_expanded pull full base
>  '
>
> +test_expect_success 'ls-files' '
> +       init_repos &&
> +
> +       # Behavior agrees by default. Sparse index is expanded.
> +       test_all_match git ls-files &&
> +
> +       # With --sparse, the sparse index data changes behavior.
> +       git -C sparse-index ls-files >dense &&
> +       git -C sparse-index ls-files --sparse >sparse &&
> +
> +       cat >expect <<-\EOF &&
> +       @@ -13,13 +13,9 @@
> +        e
> +        folder1-
> +        folder1.x
> +       -folder1/0/0/0
> +       -folder1/0/1
> +       -folder1/a
> +       +folder1/
> +        folder10
> +       -folder2/0/0/0
> +       -folder2/0/1
> +       -folder2/a
> +       +folder2/
> +        g
> +       -x/a
> +       +x/
> +        z
> +       EOF
> +
> +       diff -u dense sparse | tail -n +3 >actual &&
> +       test_cmp expect actual &&
> +
> +       # With --sparse and no sparse index, nothing changes.
> +       git -C sparse-checkout ls-files >dense &&
> +       git -C sparse-checkout ls-files --sparse >sparse &&
> +       test_cmp dense sparse &&
> +

I still believe this next section deserves a comment; something like

# Set up an erroneous condition: folder1/a present despite SKIP_WORKTREE.
# We do this just to verify consistency between sparse-index and
non-sparse-index
# with such files.

> +       write_script edit-content <<-\EOF &&
> +       mkdir folder1 &&
> +       echo content >>folder1/a
> +       EOF
> +       run_on_sparse ../edit-content &&
> +
> +       # ls-files does not notice modified files whose
> +       # cache entries are marked SKIP_WORKTREE.

I also believe this still deserves a slight change, e.g.

# ls-files does not currently notice modified files whose
# cache entries are marked SKIP_WORKTREE; this
# may change in the future, but we test here that
# sparse-index and non-sparse-index at least match.


> +       test_sparse_match git ls-files --modified &&
> +       test_must_be_empty sparse-checkout-out &&
> +       test_must_be_empty sparse-index-out &&
> +
> +       git -C sparse-index ls-files --sparse --modified >sparse-index-out &&
> +       test_must_be_empty sparse-index-out &&
> +
> +       # Add folder1 to the sparse-checkout cone and
> +       # check that ls-files shows the expanded files.
> +       test_sparse_match git sparse-checkout add folder1 &&
> +       test_sparse_match git ls-files --modified &&
> +
> +       git -C sparse-index ls-files >dense &&
> +       git -C sparse-index ls-files --sparse >sparse &&
> +
> +       cat >expect <<-\EOF &&
> +       @@ -17,9 +17,7 @@
> +        folder1/0/1
> +        folder1/a
> +        folder10
> +       -folder2/0/0/0
> +       -folder2/0/1
> +       -folder2/a
> +       +folder2/
> +        g
> +       -x/a
> +       +x/
> +        z
> +       EOF
> +
> +       diff -u dense sparse | tail -n +3 >actual &&
> +       test_cmp expect actual &&
> +
> +       # Double-check index expansion is avoided
> +       ensure_not_expanded ls-files --sparse
> +'
> +
>  # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
>  # in this scenario, but it shouldn't.
>  test_expect_success 'reset mixed and checkout orphan' '
> --
> gitgitgadget

Re: [PATCH v2 2/5] ls-files: add --sparse option

[Derrick Stolee]

On 12/9/2021 12:08 AM, Elijah Newren wrote:
> On Wed, Dec 8, 2021 at 11:39 AM Derrick Stolee via GitGitGadget

>> +       # With --sparse and no sparse index, nothing changes.
>> +       git -C sparse-checkout ls-files >dense &&
>> +       git -C sparse-checkout ls-files --sparse >sparse &&
>> +       test_cmp dense sparse &&
>> +
> 
> I still believe this next section deserves a comment; something like
> 
> # Set up an erroneous condition: folder1/a present despite SKIP_WORKTREE.
> # We do this just to verify consistency between sparse-index and
> non-sparse-index
> # with such files.
> 
>> +       write_script edit-content <<-\EOF &&
>> +       mkdir folder1 &&
>> +       echo content >>folder1/a
>> +       EOF
>> +       run_on_sparse ../edit-content &&
>> +
>> +       # ls-files does not notice modified files whose
>> +       # cache entries are marked SKIP_WORKTREE.
> 
> I also believe this still deserves a slight change, e.g.
> 
> # ls-files does not currently notice modified files whose
> # cache entries are marked SKIP_WORKTREE; this
> # may change in the future, but we test here that
> # sparse-index and non-sparse-index at least match.
> 
> 
>> +       test_sparse_match git ls-files --modified &&
>> +       test_must_be_empty sparse-checkout-out &&
>> +       test_must_be_empty sparse-index-out &&

Ok. Sorry I missed these in the gap between v1 and v2.

Thanks,
-Stolee

[PATCH v2 3/5] t1092: replace 'read-cache --table' with 'ls-files --sparse'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Now that 'git ls-files --sparse' exists, we can use it to verify the
state of a sparse index instead of 'test-tool read-cache table'. Replace
these usages within t1092-sparse-checkout-compatibility.sh.

The important changes are due to the different output format. We need to
use the '--stage' output to get a file mode and OID, but it also
includes a stage value and drops the object type. This leads to some
differences in how we handle looking for specific entries.

Some places where we previously looked for no 'tree' entries, we can
instead directly compare the output across the repository with a sparse
index and the one without.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 29 +++++++++++-------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index bf2c6b169b9..4a6832ea3c5 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -200,45 +200,42 @@ test_sparse_unstaged () {
 test_expect_success 'sparse-index contents' '
 	init_repos &&
 
-	test-tool -C sparse-index read-cache --table >cache &&
+	git -C sparse-index ls-files --sparse --stage >cache &&
 	for dir in folder1 folder2 x
 	do
 		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
-		grep "040000 tree $TREE	$dir/" cache \
+		grep "040000 $TREE 0	$dir/" cache \
 			|| return 1
 	done &&
 
 	git -C sparse-index sparse-checkout set folder1 &&
 
-	test-tool -C sparse-index read-cache --table >cache &&
+	git -C sparse-index ls-files --sparse --stage >cache &&
 	for dir in deep folder2 x
 	do
 		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
-		grep "040000 tree $TREE	$dir/" cache \
+		grep "040000 $TREE 0	$dir/" cache \
 			|| return 1
 	done &&
 
 	git -C sparse-index sparse-checkout set deep/deeper1 &&
 
-	test-tool -C sparse-index read-cache --table >cache &&
+	git -C sparse-index ls-files --sparse --stage >cache &&
 	for dir in deep/deeper2 folder1 folder2 x
 	do
 		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
-		grep "040000 tree $TREE	$dir/" cache \
+		grep "040000 $TREE 0	$dir/" cache \
 			|| return 1
 	done &&
 
 	# Disabling the sparse-index removes tree entries with full ones
 	git -C sparse-index sparse-checkout init --no-sparse-index &&
-
-	test-tool -C sparse-index read-cache --table >cache &&
-	! grep "040000 tree" cache &&
-	test_sparse_match test-tool read-cache --table
+	test_sparse_match git ls-files --stage --sparse
 '
 
 test_expect_success 'expanded in-memory index matches full index' '
 	init_repos &&
-	test_sparse_match test-tool read-cache --expand --table
+	test_sparse_match git ls-files --stage
 '
 
 test_expect_success 'status with options' '
@@ -787,9 +784,9 @@ test_expect_success 'submodule handling' '
 
 	# having a submodule prevents "modules" from collapse
 	test_sparse_match git sparse-checkout set deep/deeper1 &&
-	test-tool -C sparse-index read-cache --table >cache &&
-	grep "100644 blob .*	modules/a" cache &&
-	grep "160000 commit $(git -C initial-repo rev-parse HEAD)	modules/sub" cache
+	git -C sparse-index ls-files --sparse --stage >cache &&
+	grep "100644 .*	modules/a" cache &&
+	grep "160000 $(git -C initial-repo rev-parse HEAD) 0	modules/sub" cache
 '
 
 # When working with a sparse index, some commands will need to expand the
@@ -1079,13 +1076,13 @@ test_expect_success 'reset mixed and checkout orphan' '
 	# the sparse checkouts skip "adding" the other side of
 	# the conflict.
 	test_sparse_match git reset --mixed HEAD~1 &&
-	test_sparse_match test-tool read-cache --table --expand &&
+	test_sparse_match git ls-files --stage &&
 	test_sparse_match git status --porcelain=v2 &&
 
 	# At this point, sparse-checkouts behave differently
 	# from the full-checkout.
 	test_sparse_match git checkout --orphan new-branch &&
-	test_sparse_match test-tool read-cache --table --expand &&
+	test_sparse_match git ls-files --stage &&
 	test_sparse_match git status --porcelain=v2
 '
 
-- 
gitgitgadget

Re: [PATCH v2 3/5] t1092: replace 'read-cache --table' with 'ls-files --sparse'

[Elijah Newren]

On Wed, Dec 8, 2021 at 11:39 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Now that 'git ls-files --sparse' exists, we can use it to verify the
> state of a sparse index instead of 'test-tool read-cache table'. Replace
> these usages within t1092-sparse-checkout-compatibility.sh.
>
> The important changes are due to the different output format. We need to
> use the '--stage' output to get a file mode and OID, but it also
> includes a stage value and drops the object type. This leads to some
> differences in how we handle looking for specific entries.
>
> Some places where we previously looked for no 'tree' entries, we can
> instead directly compare the output across the repository with a sparse
> index and the one without.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t1092-sparse-checkout-compatibility.sh | 29 +++++++++++-------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index bf2c6b169b9..4a6832ea3c5 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -200,45 +200,42 @@ test_sparse_unstaged () {
>  test_expect_success 'sparse-index contents' '
>         init_repos &&
>
> -       test-tool -C sparse-index read-cache --table >cache &&
> +       git -C sparse-index ls-files --sparse --stage >cache &&
>         for dir in folder1 folder2 x
>         do
>                 TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
> -               grep "040000 tree $TREE $dir/" cache \
> +               grep "040000 $TREE 0    $dir/" cache \
>                         || return 1
>         done &&
>
>         git -C sparse-index sparse-checkout set folder1 &&
>
> -       test-tool -C sparse-index read-cache --table >cache &&
> +       git -C sparse-index ls-files --sparse --stage >cache &&
>         for dir in deep folder2 x
>         do
>                 TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
> -               grep "040000 tree $TREE $dir/" cache \
> +               grep "040000 $TREE 0    $dir/" cache \
>                         || return 1
>         done &&
>
>         git -C sparse-index sparse-checkout set deep/deeper1 &&
>
> -       test-tool -C sparse-index read-cache --table >cache &&
> +       git -C sparse-index ls-files --sparse --stage >cache &&
>         for dir in deep/deeper2 folder1 folder2 x
>         do
>                 TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
> -               grep "040000 tree $TREE $dir/" cache \
> +               grep "040000 $TREE 0    $dir/" cache \
>                         || return 1
>         done &&
>
>         # Disabling the sparse-index removes tree entries with full ones

Not the fault of this patch, but perhaps worth fixing "removes" ->
"replaces" while you're making fixes in this area?


>         git -C sparse-index sparse-checkout init --no-sparse-index &&
> -
> -       test-tool -C sparse-index read-cache --table >cache &&
> -       ! grep "040000 tree" cache &&
> -       test_sparse_match test-tool read-cache --table
> +       test_sparse_match git ls-files --stage --sparse
>  '
>
>  test_expect_success 'expanded in-memory index matches full index' '
>         init_repos &&
> -       test_sparse_match test-tool read-cache --expand --table
> +       test_sparse_match git ls-files --stage
>  '
>
>  test_expect_success 'status with options' '
> @@ -787,9 +784,9 @@ test_expect_success 'submodule handling' '
>
>         # having a submodule prevents "modules" from collapse
>         test_sparse_match git sparse-checkout set deep/deeper1 &&
> -       test-tool -C sparse-index read-cache --table >cache &&
> -       grep "100644 blob .*    modules/a" cache &&
> -       grep "160000 commit $(git -C initial-repo rev-parse HEAD)       modules/sub" cache
> +       git -C sparse-index ls-files --sparse --stage >cache &&
> +       grep "100644 .* modules/a" cache &&
> +       grep "160000 $(git -C initial-repo rev-parse HEAD) 0    modules/sub" cache
>  '
>
>  # When working with a sparse index, some commands will need to expand the
> @@ -1079,13 +1076,13 @@ test_expect_success 'reset mixed and checkout orphan' '
>         # the sparse checkouts skip "adding" the other side of
>         # the conflict.
>         test_sparse_match git reset --mixed HEAD~1 &&
> -       test_sparse_match test-tool read-cache --table --expand &&
> +       test_sparse_match git ls-files --stage &&
>         test_sparse_match git status --porcelain=v2 &&
>
>         # At this point, sparse-checkouts behave differently
>         # from the full-checkout.
>         test_sparse_match git checkout --orphan new-branch &&
> -       test_sparse_match test-tool read-cache --table --expand &&
> +       test_sparse_match git ls-files --stage &&
>         test_sparse_match git status --porcelain=v2
>  '
>
> --
> gitgitgadget
>

[PATCH v2 4/5] t1091/t3705: remove 'test-tool read-cache --table'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Now that 'git ls-files --sparse' exists, we can use it to verify the
state of a sparse index instead of 'test-tool read-cache table'. Replace
these usages within t1091-sparse-checkout-builtin.sh and
t3705-add-sparse-checkout.sh.

The important changes are due to the different output format. In t3705,
wWe need to use the '--stage' output to get a file mode and OID, but
it also includes a stage value and drops the object type. This leads
to some differences in how we handle looking for specific entries.

In t1091, the test focuses on enabling the sparse index, so we do not
need the --stage flag to demonstrate how the index changes, and instead
can use a diff.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1091-sparse-checkout-builtin.sh | 25 ++++++++++++++++++++-----
 t/t3705-add-sparse-checkout.sh     |  8 ++++----
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 272ba1b566b..680e0043c36 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -212,12 +212,27 @@ test_expect_success 'sparse-index enabled and disabled' '
 
 		git -C repo sparse-checkout init --cone --sparse-index &&
 		test_cmp_config -C repo true index.sparse &&
-		test-tool -C repo read-cache --table >cache &&
-		grep " tree " cache &&
-
+		git -C repo ls-files --sparse >sparse &&
 		git -C repo sparse-checkout disable &&
-		test-tool -C repo read-cache --table >cache &&
-		! grep " tree " cache &&
+		git -C repo ls-files --sparse >full &&
+
+		cat >expect <<-\EOF &&
+		@@ -1,4 +1,7 @@
+		 a
+		-deep/
+		-folder1/
+		-folder2/
+		+deep/a
+		+deep/deeper1/a
+		+deep/deeper1/deepest/a
+		+deep/deeper2/a
+		+folder1/a
+		+folder2/a
+		EOF
+
+		diff -u sparse full | tail -n +3 >actual &&
+		test_cmp expect actual &&
+
 		git -C repo config --list >config &&
 		! grep index.sparse config
 	)
diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
index f3143c92908..81f3384eeed 100755
--- a/t/t3705-add-sparse-checkout.sh
+++ b/t/t3705-add-sparse-checkout.sh
@@ -181,13 +181,13 @@ test_expect_success 'git add fails outside of sparse-checkout definition' '
 	# Avoid munging CRLFs to avoid an error message
 	git -c core.autocrlf=input add --sparse sparse_entry 2>stderr &&
 	test_must_be_empty stderr &&
-	test-tool read-cache --table >actual &&
-	grep "^100644 blob.*sparse_entry\$" actual &&
+	git ls-files --stage >actual &&
+	grep "^100644 .*sparse_entry\$" actual &&
 
 	git add --sparse --chmod=+x sparse_entry 2>stderr &&
 	test_must_be_empty stderr &&
-	test-tool read-cache --table >actual &&
-	grep "^100755 blob.*sparse_entry\$" actual &&
+	git ls-files --stage >actual &&
+	grep "^100755 .*sparse_entry\$" actual &&
 
 	git reset &&
 
-- 
gitgitgadget

Re: [PATCH v2 4/5] t1091/t3705: remove 'test-tool read-cache --table'

[Elijah Newren]

On Wed, Dec 8, 2021 at 11:40 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Now that 'git ls-files --sparse' exists, we can use it to verify the
> state of a sparse index instead of 'test-tool read-cache table'. Replace
> these usages within t1091-sparse-checkout-builtin.sh and
> t3705-add-sparse-checkout.sh.
>
> The important changes are due to the different output format. In t3705,
> wWe need to use the '--stage' output to get a file mode and OID, but

s/wWe/we/


> it also includes a stage value and drops the object type. This leads
> to some differences in how we handle looking for specific entries.
>
> In t1091, the test focuses on enabling the sparse index, so we do not
> need the --stage flag to demonstrate how the index changes, and instead
> can use a diff.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t1091-sparse-checkout-builtin.sh | 25 ++++++++++++++++++++-----
>  t/t3705-add-sparse-checkout.sh     |  8 ++++----
>  2 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 272ba1b566b..680e0043c36 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -212,12 +212,27 @@ test_expect_success 'sparse-index enabled and disabled' '
>
>                 git -C repo sparse-checkout init --cone --sparse-index &&
>                 test_cmp_config -C repo true index.sparse &&
> -               test-tool -C repo read-cache --table >cache &&
> -               grep " tree " cache &&
> -
> +               git -C repo ls-files --sparse >sparse &&
>                 git -C repo sparse-checkout disable &&
> -               test-tool -C repo read-cache --table >cache &&
> -               ! grep " tree " cache &&
> +               git -C repo ls-files --sparse >full &&
> +
> +               cat >expect <<-\EOF &&
> +               @@ -1,4 +1,7 @@
> +                a
> +               -deep/
> +               -folder1/
> +               -folder2/
> +               +deep/a
> +               +deep/deeper1/a
> +               +deep/deeper1/deepest/a
> +               +deep/deeper2/a
> +               +folder1/a
> +               +folder2/a
> +               EOF
> +
> +               diff -u sparse full | tail -n +3 >actual &&
> +               test_cmp expect actual &&
> +
>                 git -C repo config --list >config &&
>                 ! grep index.sparse config
>         )
> diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
> index f3143c92908..81f3384eeed 100755
> --- a/t/t3705-add-sparse-checkout.sh
> +++ b/t/t3705-add-sparse-checkout.sh
> @@ -181,13 +181,13 @@ test_expect_success 'git add fails outside of sparse-checkout definition' '
>         # Avoid munging CRLFs to avoid an error message
>         git -c core.autocrlf=input add --sparse sparse_entry 2>stderr &&
>         test_must_be_empty stderr &&
> -       test-tool read-cache --table >actual &&
> -       grep "^100644 blob.*sparse_entry\$" actual &&
> +       git ls-files --stage >actual &&
> +       grep "^100644 .*sparse_entry\$" actual &&
>
>         git add --sparse --chmod=+x sparse_entry 2>stderr &&
>         test_must_be_empty stderr &&
> -       test-tool read-cache --table >actual &&
> -       grep "^100755 blob.*sparse_entry\$" actual &&
> +       git ls-files --stage >actual &&
> +       grep "^100755 .*sparse_entry\$" actual &&
>
>         git reset &&
>
> --
> gitgitgadget

[PATCH v2 5/5] test-read-cache: remove --table, --expand options

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

This commit effectively reverts 2782db3 (test-tool: don't force full
index, 2021-03-30) and e2df6c3 (test-read-cache: print cache entries
with --table, 2021-03-30) to remove the --table and --expand options
from 'test-tool read-cache'. The previous changes already removed these
options from the test suite in favor of 'git ls-files --sparse'.

The initial thought of creating these options was to allow for tests to
see additional information with every cache entry. In particular, the
object type is still not mirrored in 'git ls-files'. Since sparse
directory entries always end with a slash, the object type is not
critical to verify the sparse index is enabled. It was thought that it
would be helpful to have additional information, such as flags, but that
was not needed for the FS Monitor integration and hasn't been needed
since.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/helper/test-read-cache.c | 64 ++++++--------------------------------
 1 file changed, 10 insertions(+), 54 deletions(-)

diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index 0d9f08931a1..b736ef16421 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -1,83 +1,39 @@
 #include "test-tool.h"
 #include "cache.h"
 #include "config.h"
-#include "blob.h"
-#include "commit.h"
-#include "tree.h"
-#include "sparse-index.h"
-
-static void print_cache_entry(struct cache_entry *ce)
-{
-	const char *type;
-	printf("%06o ", ce->ce_mode & 0177777);
-
-	if (S_ISSPARSEDIR(ce->ce_mode))
-		type = tree_type;
-	else if (S_ISGITLINK(ce->ce_mode))
-		type = commit_type;
-	else
-		type = blob_type;
-
-	printf("%s %s\t%s\n",
-	       type,
-	       oid_to_hex(&ce->oid),
-	       ce->name);
-}
-
-static void print_cache(struct index_state *istate)
-{
-	int i;
-	for (i = 0; i < istate->cache_nr; i++)
-		print_cache_entry(istate->cache[i]);
-}
 
 int cmd__read_cache(int argc, const char **argv)
 {
-	struct repository *r = the_repository;
 	int i, cnt = 1;
 	const char *name = NULL;
-	int table = 0, expand = 0;
 
 	initialize_the_repository();
 
-	for (++argv, --argc; *argv && starts_with(*argv, "--"); ++argv, --argc) {
-		if (skip_prefix(*argv, "--print-and-refresh=", &name))
-			continue;
-		if (!strcmp(*argv, "--table"))
-			table = 1;
-		else if (!strcmp(*argv, "--expand"))
-			expand = 1;
+	if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
+		argc--;
+		argv++;
 	}
 
-	if (argc == 1)
-		cnt = strtol(argv[0], NULL, 0);
+	if (argc == 2)
+		cnt = strtol(argv[1], NULL, 0);
 	setup_git_directory();
 	git_config(git_default_config, NULL);
 
-	prepare_repo_settings(r);
-	r->settings.command_requires_full_index = 0;
-
 	for (i = 0; i < cnt; i++) {
-		repo_read_index(r);
-
-		if (expand)
-			ensure_full_index(r->index);
-
+		read_cache();
 		if (name) {
 			int pos;
 
-			refresh_index(r->index, REFRESH_QUIET,
+			refresh_index(&the_index, REFRESH_QUIET,
 				      NULL, NULL, NULL);
-			pos = index_name_pos(r->index, name, strlen(name));
+			pos = index_name_pos(&the_index, name, strlen(name));
 			if (pos < 0)
 				die("%s not in index", name);
 			printf("%s is%s up to date\n", name,
-			       ce_uptodate(r->index->cache[pos]) ? "" : " not");
+			       ce_uptodate(the_index.cache[pos]) ? "" : " not");
 			write_file(name, "%d\n", i);
 		}
-		if (table)
-			print_cache(r->index);
-		discard_index(r->index);
+		discard_cache();
 	}
 	return 0;
 }
-- 
gitgitgadget

Re: [PATCH v2 0/5] Sparse index: fetch, pull, ls-files

[Elijah Newren]

On Wed, Dec 8, 2021 at 11:39 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This is based on ld/sparse-index-blame (merged with 'master' due to an
> unrelated build issue).
>
> Here are two relatively-simple patches that further the sparse index
> integrations.
>
> Did you know that 'fetch' and 'pull' read the index? I didn't, or this would
> have been an integration much earlier in the cycle. They read the index to
> look for the .gitmodules file in case there are submodules that need to be
> fetched. Since looking for a file by name is already protected, we only need
> to disable 'command_requires_full_index' and we are done.
>
> The 'ls-files' builtin is useful when debugging the index, and some scripts
> use it, too. We are not changing the default behavior which expands a sparse
> index in order to show all of the cached blobs. Instead, we add a '--sparse'
> option that allows us to see the sparse directory entries upon request.
> Combined with --debug, we can see a lot of index details, such as:
>
> $ git ls-files --debug --sparse
> LICENSE
>   ctime: 1634910503:287405820
>   mtime: 1634910503:287405820
>   dev: 16777220 ino: 119325319
>   uid: 501  gid: 20
>   size: 1098    flags: 200000
> README.md
>   ctime: 1634910503:288090279
>   mtime: 1634910503:288090279
>   dev: 16777220 ino: 119325320
>   uid: 501  gid: 20
>   size: 934 flags: 200000
> bin/index.js
>   ctime: 1634910767:828434033
>   mtime: 1634910767:828434033
>   dev: 16777220 ino: 119325520
>   uid: 501  gid: 20
>   size: 7292    flags: 200000
> examples/
>   ctime: 0:0
>   mtime: 0:0
>   dev: 0    ino: 0
>   uid: 0    gid: 0
>   size: 0   flags: 40004000
> package.json
>   ctime: 1634910503:288676330
>   mtime: 1634910503:288676330
>   dev: 16777220 ino: 119325321
>   uid: 501  gid: 20
>   size: 680 flags: 200000
>
>
> (In this example, the 'examples/' directory is sparse.)
>
> Thanks!
>
>
> Updates in v2
> =============
>
>  * Rebased onto latest ld/sparse-index-blame without issue.
>  * Updated the test to use diff-of-diffs instead of a sequence of greps.
>  * Added patches that remove the use of 'test-tool read-cache --table' and
>    its implementation.

I still think a couple things in patch 2 deserve some comments about
the expectations.  Other than that, though, the series reads nicely
and I was only able to spot a few other very minor items.

> Derrick Stolee (5):
>   fetch/pull: use the sparse index
>   ls-files: add --sparse option
>   t1092: replace 'read-cache --table' with 'ls-files --sparse'
>   t1091/t3705: remove 'test-tool read-cache --table'
>   test-read-cache: remove --table, --expand options
>
>  Documentation/git-ls-files.txt           |   4 +
>  builtin/fetch.c                          |   2 +
>  builtin/ls-files.c                       |  12 ++-
>  builtin/pull.c                           |   2 +
>  t/helper/test-read-cache.c               |  64 ++---------
>  t/t1091-sparse-checkout-builtin.sh       |  25 ++++-
>  t/t1092-sparse-checkout-compatibility.sh | 129 ++++++++++++++++++++---
>  t/t3705-add-sparse-checkout.sh           |   8 +-
>  8 files changed, 165 insertions(+), 81 deletions(-)
>
>
> base-commit: 3fffe69d24e4ecc95246766f5396303a953695ff
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1080%2Fderrickstolee%2Fsparse-index%2Ffetch-pull-ls-files-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1080/derrickstolee/sparse-index/fetch-pull-ls-files-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1080
>
> Range-diff vs v1:
>
>  1:  451056e1a77 ! 1:  f72001638d1 fetch/pull: use the sparse index
>      @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
>
>        ## t/t1092-sparse-checkout-compatibility.sh ##
>       @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is not expanded: blame' '
>      -  ensure_not_expanded blame deep/deeper1/deepest/a
>      +  done
>        '
>
>       +test_expect_success 'sparse index is not expanded: fetch/pull' '
>  2:  e42c0feec94 ! 2:  58b5eca4835 ls-files: add --sparse option
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
>       + test_all_match git ls-files &&
>       +
>       + # With --sparse, the sparse index data changes behavior.
>      -+ git -C sparse-index ls-files --sparse >sparse-index-out &&
>      -+ grep "^folder1/\$" sparse-index-out &&
>      -+ grep "^folder2/\$" sparse-index-out &&
>      ++ git -C sparse-index ls-files >dense &&
>      ++ git -C sparse-index ls-files --sparse >sparse &&
>      ++
>      ++ cat >expect <<-\EOF &&
>      ++ @@ -13,13 +13,9 @@
>      ++  e
>      ++  folder1-
>      ++  folder1.x
>      ++ -folder1/0/0/0
>      ++ -folder1/0/1
>      ++ -folder1/a
>      ++ +folder1/
>      ++  folder10
>      ++ -folder2/0/0/0
>      ++ -folder2/0/1
>      ++ -folder2/a
>      ++ +folder2/
>      ++  g
>      ++ -x/a
>      ++ +x/
>      ++  z
>      ++ EOF
>      ++
>      ++ diff -u dense sparse | tail -n +3 >actual &&
>      ++ test_cmp expect actual &&
>       +
>       + # With --sparse and no sparse index, nothing changes.
>      -+ git -C sparse-checkout ls-files --sparse >sparse-checkout-out &&
>      -+ grep "^folder1/0/0/0\$" sparse-checkout-out &&
>      -+ ! grep "/\$" sparse-checkout-out &&
>      ++ git -C sparse-checkout ls-files >dense &&
>      ++ git -C sparse-checkout ls-files --sparse >sparse &&
>      ++ test_cmp dense sparse &&
>       +
>       + write_script edit-content <<-\EOF &&
>       + mkdir folder1 &&
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
>       + git -C sparse-index ls-files --sparse --modified >sparse-index-out &&
>       + test_must_be_empty sparse-index-out &&
>       +
>      -+ run_on_sparse git sparse-checkout add folder1 &&
>      ++ # Add folder1 to the sparse-checkout cone and
>      ++ # check that ls-files shows the expanded files.
>      ++ test_sparse_match git sparse-checkout add folder1 &&
>       + test_sparse_match git ls-files --modified &&
>      -+ grep "^folder1/a\$" sparse-checkout-out &&
>      -+ grep "^folder1/a\$" sparse-index-out &&
>       +
>      -+ # Double-check index expansion
>      ++ git -C sparse-index ls-files >dense &&
>      ++ git -C sparse-index ls-files --sparse >sparse &&
>      ++
>      ++ cat >expect <<-\EOF &&
>      ++ @@ -17,9 +17,7 @@
>      ++  folder1/0/1
>      ++  folder1/a
>      ++  folder10
>      ++ -folder2/0/0/0
>      ++ -folder2/0/1
>      ++ -folder2/a
>      ++ +folder2/
>      ++  g
>      ++ -x/a
>      ++ +x/
>      ++  z
>      ++ EOF
>      ++
>      ++ diff -u dense sparse | tail -n +3 >actual &&
>      ++ test_cmp expect actual &&
>      ++
>      ++ # Double-check index expansion is avoided
>       + ensure_not_expanded ls-files --sparse
>       +'
>       +
>  -:  ----------- > 3:  5ffae2a03ae t1092: replace 'read-cache --table' with 'ls-files --sparse'
>  -:  ----------- > 4:  b98e5e6d2bc t1091/t3705: remove 'test-tool read-cache --table'
>  -:  ----------- > 5:  f31a24eeb9b test-read-cache: remove --table, --expand options
>
> --
> gitgitgadget

[PATCH v3 0/5] Sparse index: fetch, pull, ls-files

[Derrick Stolee via GitGitGadget]

This is based on ld/sparse-index-blame (merged with 'master' due to an
unrelated build issue).

Here are two relatively-simple patches that further the sparse index
integrations.

Did you know that 'fetch' and 'pull' read the index? I didn't, or this would
have been an integration much earlier in the cycle. They read the index to
look for the .gitmodules file in case there are submodules that need to be
fetched. Since looking for a file by name is already protected, we only need
to disable 'command_requires_full_index' and we are done.

The 'ls-files' builtin is useful when debugging the index, and some scripts
use it, too. We are not changing the default behavior which expands a sparse
index in order to show all of the cached blobs. Instead, we add a '--sparse'
option that allows us to see the sparse directory entries upon request.
Combined with --debug, we can see a lot of index details, such as:

$ git ls-files --debug --sparse
LICENSE
  ctime: 1634910503:287405820
  mtime: 1634910503:287405820
  dev: 16777220 ino: 119325319
  uid: 501  gid: 20
  size: 1098    flags: 200000
README.md
  ctime: 1634910503:288090279
  mtime: 1634910503:288090279
  dev: 16777220 ino: 119325320
  uid: 501  gid: 20
  size: 934 flags: 200000
bin/index.js
  ctime: 1634910767:828434033
  mtime: 1634910767:828434033
  dev: 16777220 ino: 119325520
  uid: 501  gid: 20
  size: 7292    flags: 200000
examples/
  ctime: 0:0
  mtime: 0:0
  dev: 0    ino: 0
  uid: 0    gid: 0
  size: 0   flags: 40004000
package.json
  ctime: 1634910503:288676330
  mtime: 1634910503:288676330
  dev: 16777220 ino: 119325321
  uid: 501  gid: 20
  size: 680 flags: 200000


(In this example, the 'examples/' directory is sparse.)

Thanks!


Updates in v2
=============

 * Rebased onto latest ld/sparse-index-blame without issue.
 * Updated the test to use diff-of-diffs instead of a sequence of greps.
 * Added patches that remove the use of 'test-tool read-cache --table' and
   its implementation.


Updates in v3
=============

 * Fixed typo in commit message.
 * Added comments around doing strange things in an ls-files test.
 * Fixed adjacent typo in a test comment.

Derrick Stolee (5):
  fetch/pull: use the sparse index
  ls-files: add --sparse option
  t1092: replace 'read-cache --table' with 'ls-files --sparse'
  t1091/t3705: remove 'test-tool read-cache --table'
  test-read-cache: remove --table, --expand options

 Documentation/git-ls-files.txt           |   4 +
 builtin/fetch.c                          |   2 +
 builtin/ls-files.c                       |  12 +-
 builtin/pull.c                           |   2 +
 t/helper/test-read-cache.c               |  64 ++---------
 t/t1091-sparse-checkout-builtin.sh       |  25 ++++-
 t/t1092-sparse-checkout-compatibility.sh | 137 ++++++++++++++++++++---
 t/t3705-add-sparse-checkout.sh           |   8 +-
 8 files changed, 172 insertions(+), 82 deletions(-)


base-commit: 3fffe69d24e4ecc95246766f5396303a953695ff
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1080%2Fderrickstolee%2Fsparse-index%2Ffetch-pull-ls-files-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1080/derrickstolee/sparse-index/fetch-pull-ls-files-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1080

Range-diff vs v2:

 1:  f72001638d1 = 1:  f72001638d1 fetch/pull: use the sparse index
 2:  58b5eca4835 ! 2:  b81174ba54b ls-files: add --sparse option
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
      +	git -C sparse-checkout ls-files --sparse >sparse &&
      +	test_cmp dense sparse &&
      +
     ++	# Set up a strange condition of having a file edit
     ++	# outside of the sparse-checkout cone. This is just
     ++	# to verify that sparse-checkout and sparse-index
     ++	# behave the same in this case.
      +	write_script edit-content <<-\EOF &&
      +	mkdir folder1 &&
      +	echo content >>folder1/a
      +	EOF
      +	run_on_sparse ../edit-content &&
      +
     -+	# ls-files does not notice modified files whose
     -+	# cache entries are marked SKIP_WORKTREE.
     ++	# ls-files does not currently notice modified files whose
     ++	# cache entries are marked SKIP_WORKTREE. This may change
     ++	# in the future, but here we test that sparse index does
     ++	# not accidentally create a change of behavior.
      +	test_sparse_match git ls-files --modified &&
      +	test_must_be_empty sparse-checkout-out &&
      +	test_must_be_empty sparse-index-out &&
 3:  5ffae2a03ae ! 3:  2a6a1c5a39c t1092: replace 'read-cache --table' with 'ls-files --sparse'
     @@ t/t1092-sparse-checkout-compatibility.sh: test_sparse_unstaged () {
       			|| return 1
       	done &&
       
     - 	# Disabling the sparse-index removes tree entries with full ones
     +-	# Disabling the sparse-index removes tree entries with full ones
     ++	# Disabling the sparse-index replaces tree entries with full ones
       	git -C sparse-index sparse-checkout init --no-sparse-index &&
      -
      -	test-tool -C sparse-index read-cache --table >cache &&
 4:  b98e5e6d2bc ! 4:  f0143686754 t1091/t3705: remove 'test-tool read-cache --table'
     @@ Commit message
          t3705-add-sparse-checkout.sh.
      
          The important changes are due to the different output format. In t3705,
     -    wWe need to use the '--stage' output to get a file mode and OID, but
     +    we need to use the '--stage' output to get a file mode and OID, but
          it also includes a stage value and drops the object type. This leads
          to some differences in how we handle looking for specific entries.
      
 5:  f31a24eeb9b = 5:  9227dc54165 test-read-cache: remove --table, --expand options

-- 
gitgitgadget

[PATCH v3 1/5] fetch/pull: use the sparse index

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The 'git fetch' and 'git pull' commands parse the index in order to
determine if submodules exist. Without command_requires_full_index=0,
this will expand a sparse index, causing slow performance even when
there is no new data to fetch.

The .gitmodules file will never be inside a sparse directory entry, and
even if it was, the index_name_pos() method would expand the sparse
index if needed as we search for the path by name. These commands do not
iterate over the index, which is the typical thing we are careful about
when integrating with the sparse index.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/fetch.c                          |  2 ++
 builtin/pull.c                           |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 10 ++++++++++
 3 files changed, 14 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f7abbc31ff1..1696b7791d4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1993,6 +1993,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	}
 
 	git_config(git_fetch_config, NULL);
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_fetch_options, builtin_fetch_usage, 0);
diff --git a/builtin/pull.c b/builtin/pull.c
index 1cfaf9f3436..7bce3bd80f9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -994,6 +994,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		set_reflog_message(argc, argv);
 
 	git_config(git_pull_config, NULL);
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
 
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 83971174ac3..3ba19ba1c14 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -964,6 +964,16 @@ test_expect_success 'sparse index is not expanded: blame' '
 	done
 '
 
+test_expect_success 'sparse index is not expanded: fetch/pull' '
+	init_repos &&
+
+	git -C sparse-index remote add full "file://$(pwd)/full-checkout" &&
+	ensure_not_expanded fetch full &&
+	git -C full-checkout commit --allow-empty -m "for pull merge" &&
+	git -C sparse-index commit --allow-empty -m "for pull merge" &&
+	ensure_not_expanded pull full base
+'
+
 # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
 # in this scenario, but it shouldn't.
 test_expect_success 'reset mixed and checkout orphan' '
-- 
gitgitgadget

[PATCH v3 2/5] ls-files: add --sparse option

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Existing callers to 'git ls-files' are expecting file names, not
directories. It is best to expand a sparse index to show all of the
contained files in this case.

However, expert users may want to inspect the contents of the index
itself including which directories are sparse. Add a --sparse option to
allow users to request this information.

During testing, I noticed that options such as --modified did not affect
the output when the files in question were outside the sparse-checkout
definition. Tests are added to document this preexisting behavior and
how it remains unchanged with the sparse index and the --sparse option.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-ls-files.txt           |  4 +
 builtin/ls-files.c                       | 12 ++-
 t/t1092-sparse-checkout-compatibility.sh | 96 ++++++++++++++++++++++++
 3 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 6d11ab506b7..1c5d5f85ec5 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -187,6 +187,10 @@ Both the <eolinfo> in the index ("i/<eolinfo>")
 and in the working tree ("w/<eolinfo>") are shown for regular files,
 followed by the  ("attr/<eolattr>").
 
+--sparse::
+	If the index is sparse, show the sparse directories without expanding
+	to the contained files.
+
 \--::
 	Do not interpret any more arguments as options.
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 031fef1bcaa..c151eb1fb77 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -37,6 +37,7 @@ static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
 static int skipping_duplicates;
+static int show_sparse_dirs;
 
 static const char *prefix;
 static int max_prefix_len;
@@ -315,8 +316,10 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 
 	if (!(show_cached || show_stage || show_deleted || show_modified))
 		return;
-	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(repo->index);
+
+	if (!show_sparse_dirs)
+		ensure_full_index(repo->index);
+
 	for (i = 0; i < repo->index->cache_nr; i++) {
 		const struct cache_entry *ce = repo->index->cache[i];
 		struct stat st;
@@ -670,6 +673,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
 		OPT_BOOL(0, "deduplicate", &skipping_duplicates,
 			 N_("suppress duplicate entries")),
+		OPT_BOOL(0, "sparse", &show_sparse_dirs,
+			 N_("show sparse directories in the presence of a sparse index")),
 		OPT_END()
 	};
 	int ret = 0;
@@ -677,6 +682,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(ls_files_usage, builtin_ls_files_options);
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	prefix = cmd_prefix;
 	if (prefix)
 		prefix_len = strlen(prefix);
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 3ba19ba1c14..29b97667378 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -802,6 +802,12 @@ test_expect_success 'sparse-index is expanded and converted back' '
 	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
 		git -C sparse-index reset -- folder1/a &&
 	test_region index convert_to_sparse trace2.txt &&
+	test_region index ensure_full_index trace2.txt &&
+
+	# ls-files expands on read, but does not write.
+	rm trace2.txt &&
+	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
+		git -C sparse-index ls-files &&
 	test_region index ensure_full_index trace2.txt
 '
 
@@ -826,6 +832,7 @@ test_expect_success 'sparse-index is not expanded' '
 	init_repos &&
 
 	ensure_not_expanded status &&
+	ensure_not_expanded ls-files --sparse &&
 	ensure_not_expanded commit --allow-empty -m empty &&
 	echo >>sparse-index/a &&
 	ensure_not_expanded commit -a -m a &&
@@ -974,6 +981,95 @@ test_expect_success 'sparse index is not expanded: fetch/pull' '
 	ensure_not_expanded pull full base
 '
 
+test_expect_success 'ls-files' '
+	init_repos &&
+
+	# Behavior agrees by default. Sparse index is expanded.
+	test_all_match git ls-files &&
+
+	# With --sparse, the sparse index data changes behavior.
+	git -C sparse-index ls-files >dense &&
+	git -C sparse-index ls-files --sparse >sparse &&
+
+	cat >expect <<-\EOF &&
+	@@ -13,13 +13,9 @@
+	 e
+	 folder1-
+	 folder1.x
+	-folder1/0/0/0
+	-folder1/0/1
+	-folder1/a
+	+folder1/
+	 folder10
+	-folder2/0/0/0
+	-folder2/0/1
+	-folder2/a
+	+folder2/
+	 g
+	-x/a
+	+x/
+	 z
+	EOF
+
+	diff -u dense sparse | tail -n +3 >actual &&
+	test_cmp expect actual &&
+
+	# With --sparse and no sparse index, nothing changes.
+	git -C sparse-checkout ls-files >dense &&
+	git -C sparse-checkout ls-files --sparse >sparse &&
+	test_cmp dense sparse &&
+
+	# Set up a strange condition of having a file edit
+	# outside of the sparse-checkout cone. This is just
+	# to verify that sparse-checkout and sparse-index
+	# behave the same in this case.
+	write_script edit-content <<-\EOF &&
+	mkdir folder1 &&
+	echo content >>folder1/a
+	EOF
+	run_on_sparse ../edit-content &&
+
+	# ls-files does not currently notice modified files whose
+	# cache entries are marked SKIP_WORKTREE. This may change
+	# in the future, but here we test that sparse index does
+	# not accidentally create a change of behavior.
+	test_sparse_match git ls-files --modified &&
+	test_must_be_empty sparse-checkout-out &&
+	test_must_be_empty sparse-index-out &&
+
+	git -C sparse-index ls-files --sparse --modified >sparse-index-out &&
+	test_must_be_empty sparse-index-out &&
+
+	# Add folder1 to the sparse-checkout cone and
+	# check that ls-files shows the expanded files.
+	test_sparse_match git sparse-checkout add folder1 &&
+	test_sparse_match git ls-files --modified &&
+
+	git -C sparse-index ls-files >dense &&
+	git -C sparse-index ls-files --sparse >sparse &&
+
+	cat >expect <<-\EOF &&
+	@@ -17,9 +17,7 @@
+	 folder1/0/1
+	 folder1/a
+	 folder10
+	-folder2/0/0/0
+	-folder2/0/1
+	-folder2/a
+	+folder2/
+	 g
+	-x/a
+	+x/
+	 z
+	EOF
+
+	diff -u dense sparse | tail -n +3 >actual &&
+	test_cmp expect actual &&
+
+	# Double-check index expansion is avoided
+	ensure_not_expanded ls-files --sparse
+'
+
 # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
 # in this scenario, but it shouldn't.
 test_expect_success 'reset mixed and checkout orphan' '
-- 
gitgitgadget

[PATCH v3 3/5] t1092: replace 'read-cache --table' with 'ls-files --sparse'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Now that 'git ls-files --sparse' exists, we can use it to verify the
state of a sparse index instead of 'test-tool read-cache table'. Replace
these usages within t1092-sparse-checkout-compatibility.sh.

The important changes are due to the different output format. We need to
use the '--stage' output to get a file mode and OID, but it also
includes a stage value and drops the object type. This leads to some
differences in how we handle looking for specific entries.

Some places where we previously looked for no 'tree' entries, we can
instead directly compare the output across the repository with a sparse
index and the one without.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 31 +++++++++++-------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 29b97667378..d4a0d8ce825 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -200,45 +200,42 @@ test_sparse_unstaged () {
 test_expect_success 'sparse-index contents' '
 	init_repos &&
 
-	test-tool -C sparse-index read-cache --table >cache &&
+	git -C sparse-index ls-files --sparse --stage >cache &&
 	for dir in folder1 folder2 x
 	do
 		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
-		grep "040000 tree $TREE	$dir/" cache \
+		grep "040000 $TREE 0	$dir/" cache \
 			|| return 1
 	done &&
 
 	git -C sparse-index sparse-checkout set folder1 &&
 
-	test-tool -C sparse-index read-cache --table >cache &&
+	git -C sparse-index ls-files --sparse --stage >cache &&
 	for dir in deep folder2 x
 	do
 		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
-		grep "040000 tree $TREE	$dir/" cache \
+		grep "040000 $TREE 0	$dir/" cache \
 			|| return 1
 	done &&
 
 	git -C sparse-index sparse-checkout set deep/deeper1 &&
 
-	test-tool -C sparse-index read-cache --table >cache &&
+	git -C sparse-index ls-files --sparse --stage >cache &&
 	for dir in deep/deeper2 folder1 folder2 x
 	do
 		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
-		grep "040000 tree $TREE	$dir/" cache \
+		grep "040000 $TREE 0	$dir/" cache \
 			|| return 1
 	done &&
 
-	# Disabling the sparse-index removes tree entries with full ones
+	# Disabling the sparse-index replaces tree entries with full ones
 	git -C sparse-index sparse-checkout init --no-sparse-index &&
-
-	test-tool -C sparse-index read-cache --table >cache &&
-	! grep "040000 tree" cache &&
-	test_sparse_match test-tool read-cache --table
+	test_sparse_match git ls-files --stage --sparse
 '
 
 test_expect_success 'expanded in-memory index matches full index' '
 	init_repos &&
-	test_sparse_match test-tool read-cache --expand --table
+	test_sparse_match git ls-files --stage
 '
 
 test_expect_success 'status with options' '
@@ -787,9 +784,9 @@ test_expect_success 'submodule handling' '
 
 	# having a submodule prevents "modules" from collapse
 	test_sparse_match git sparse-checkout set deep/deeper1 &&
-	test-tool -C sparse-index read-cache --table >cache &&
-	grep "100644 blob .*	modules/a" cache &&
-	grep "160000 commit $(git -C initial-repo rev-parse HEAD)	modules/sub" cache
+	git -C sparse-index ls-files --sparse --stage >cache &&
+	grep "100644 .*	modules/a" cache &&
+	grep "160000 $(git -C initial-repo rev-parse HEAD) 0	modules/sub" cache
 '
 
 # When working with a sparse index, some commands will need to expand the
@@ -1085,13 +1082,13 @@ test_expect_success 'reset mixed and checkout orphan' '
 	# the sparse checkouts skip "adding" the other side of
 	# the conflict.
 	test_sparse_match git reset --mixed HEAD~1 &&
-	test_sparse_match test-tool read-cache --table --expand &&
+	test_sparse_match git ls-files --stage &&
 	test_sparse_match git status --porcelain=v2 &&
 
 	# At this point, sparse-checkouts behave differently
 	# from the full-checkout.
 	test_sparse_match git checkout --orphan new-branch &&
-	test_sparse_match test-tool read-cache --table --expand &&
+	test_sparse_match git ls-files --stage &&
 	test_sparse_match git status --porcelain=v2
 '
 
-- 
gitgitgadget

[PATCH v3 4/5] t1091/t3705: remove 'test-tool read-cache --table'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Now that 'git ls-files --sparse' exists, we can use it to verify the
state of a sparse index instead of 'test-tool read-cache table'. Replace
these usages within t1091-sparse-checkout-builtin.sh and
t3705-add-sparse-checkout.sh.

The important changes are due to the different output format. In t3705,
we need to use the '--stage' output to get a file mode and OID, but
it also includes a stage value and drops the object type. This leads
to some differences in how we handle looking for specific entries.

In t1091, the test focuses on enabling the sparse index, so we do not
need the --stage flag to demonstrate how the index changes, and instead
can use a diff.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1091-sparse-checkout-builtin.sh | 25 ++++++++++++++++++++-----
 t/t3705-add-sparse-checkout.sh     |  8 ++++----
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 272ba1b566b..680e0043c36 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -212,12 +212,27 @@ test_expect_success 'sparse-index enabled and disabled' '
 
 		git -C repo sparse-checkout init --cone --sparse-index &&
 		test_cmp_config -C repo true index.sparse &&
-		test-tool -C repo read-cache --table >cache &&
-		grep " tree " cache &&
-
+		git -C repo ls-files --sparse >sparse &&
 		git -C repo sparse-checkout disable &&
-		test-tool -C repo read-cache --table >cache &&
-		! grep " tree " cache &&
+		git -C repo ls-files --sparse >full &&
+
+		cat >expect <<-\EOF &&
+		@@ -1,4 +1,7 @@
+		 a
+		-deep/
+		-folder1/
+		-folder2/
+		+deep/a
+		+deep/deeper1/a
+		+deep/deeper1/deepest/a
+		+deep/deeper2/a
+		+folder1/a
+		+folder2/a
+		EOF
+
+		diff -u sparse full | tail -n +3 >actual &&
+		test_cmp expect actual &&
+
 		git -C repo config --list >config &&
 		! grep index.sparse config
 	)
diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
index f3143c92908..81f3384eeed 100755
--- a/t/t3705-add-sparse-checkout.sh
+++ b/t/t3705-add-sparse-checkout.sh
@@ -181,13 +181,13 @@ test_expect_success 'git add fails outside of sparse-checkout definition' '
 	# Avoid munging CRLFs to avoid an error message
 	git -c core.autocrlf=input add --sparse sparse_entry 2>stderr &&
 	test_must_be_empty stderr &&
-	test-tool read-cache --table >actual &&
-	grep "^100644 blob.*sparse_entry\$" actual &&
+	git ls-files --stage >actual &&
+	grep "^100644 .*sparse_entry\$" actual &&
 
 	git add --sparse --chmod=+x sparse_entry 2>stderr &&
 	test_must_be_empty stderr &&
-	test-tool read-cache --table >actual &&
-	grep "^100755 blob.*sparse_entry\$" actual &&
+	git ls-files --stage >actual &&
+	grep "^100755 .*sparse_entry\$" actual &&
 
 	git reset &&
 
-- 
gitgitgadget

[PATCH v3 5/5] test-read-cache: remove --table, --expand options

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

This commit effectively reverts 2782db3 (test-tool: don't force full
index, 2021-03-30) and e2df6c3 (test-read-cache: print cache entries
with --table, 2021-03-30) to remove the --table and --expand options
from 'test-tool read-cache'. The previous changes already removed these
options from the test suite in favor of 'git ls-files --sparse'.

The initial thought of creating these options was to allow for tests to
see additional information with every cache entry. In particular, the
object type is still not mirrored in 'git ls-files'. Since sparse
directory entries always end with a slash, the object type is not
critical to verify the sparse index is enabled. It was thought that it
would be helpful to have additional information, such as flags, but that
was not needed for the FS Monitor integration and hasn't been needed
since.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/helper/test-read-cache.c | 64 ++++++--------------------------------
 1 file changed, 10 insertions(+), 54 deletions(-)

diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index 0d9f08931a1..b736ef16421 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -1,83 +1,39 @@
 #include "test-tool.h"
 #include "cache.h"
 #include "config.h"
-#include "blob.h"
-#include "commit.h"
-#include "tree.h"
-#include "sparse-index.h"
-
-static void print_cache_entry(struct cache_entry *ce)
-{
-	const char *type;
-	printf("%06o ", ce->ce_mode & 0177777);
-
-	if (S_ISSPARSEDIR(ce->ce_mode))
-		type = tree_type;
-	else if (S_ISGITLINK(ce->ce_mode))
-		type = commit_type;
-	else
-		type = blob_type;
-
-	printf("%s %s\t%s\n",
-	       type,
-	       oid_to_hex(&ce->oid),
-	       ce->name);
-}
-
-static void print_cache(struct index_state *istate)
-{
-	int i;
-	for (i = 0; i < istate->cache_nr; i++)
-		print_cache_entry(istate->cache[i]);
-}
 
 int cmd__read_cache(int argc, const char **argv)
 {
-	struct repository *r = the_repository;
 	int i, cnt = 1;
 	const char *name = NULL;
-	int table = 0, expand = 0;
 
 	initialize_the_repository();
 
-	for (++argv, --argc; *argv && starts_with(*argv, "--"); ++argv, --argc) {
-		if (skip_prefix(*argv, "--print-and-refresh=", &name))
-			continue;
-		if (!strcmp(*argv, "--table"))
-			table = 1;
-		else if (!strcmp(*argv, "--expand"))
-			expand = 1;
+	if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
+		argc--;
+		argv++;
 	}
 
-	if (argc == 1)
-		cnt = strtol(argv[0], NULL, 0);
+	if (argc == 2)
+		cnt = strtol(argv[1], NULL, 0);
 	setup_git_directory();
 	git_config(git_default_config, NULL);
 
-	prepare_repo_settings(r);
-	r->settings.command_requires_full_index = 0;
-
 	for (i = 0; i < cnt; i++) {
-		repo_read_index(r);
-
-		if (expand)
-			ensure_full_index(r->index);
-
+		read_cache();
 		if (name) {
 			int pos;
 
-			refresh_index(r->index, REFRESH_QUIET,
+			refresh_index(&the_index, REFRESH_QUIET,
 				      NULL, NULL, NULL);
-			pos = index_name_pos(r->index, name, strlen(name));
+			pos = index_name_pos(&the_index, name, strlen(name));
 			if (pos < 0)
 				die("%s not in index", name);
 			printf("%s is%s up to date\n", name,
-			       ce_uptodate(r->index->cache[pos]) ? "" : " not");
+			       ce_uptodate(the_index.cache[pos]) ? "" : " not");
 			write_file(name, "%d\n", i);
 		}
-		if (table)
-			print_cache(r->index);
-		discard_index(r->index);
+		discard_cache();
 	}
 	return 0;
 }
-- 
gitgitgadget

Re: [PATCH v3 0/5] Sparse index: fetch, pull, ls-files

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 10 2021, Derrick Stolee via GitGitGadget wrote:

> Updates in v3
> =============
>
>  * Fixed typo in commit message.
>  * Added comments around doing strange things in an ls-files test.
>  * Fixed adjacent typo in a test comment.

Yay, I'm happy to see 5/5. Not because I didn't like the helper, but
that sparse is getting mature enough that we're getting ls-files to emit
information about it. Thanks.

There's the small "diff -u" portability issue noted in my just-sent
<211210.86zgp8bi48.gmgdl@evledraar.gmail.com>.

Other than that 2/5 adds this documentation about ls-files --sparse:

	If the index is sparse, show the sparse directories without expanding
	to the contained files.

Shouldn't we at least add:

	Sparse directories will be shown with a trailing slash,
	e.g. "x/" for a sparse directory "x".q

In addition to that I think this may have a buggy/unexpected interaction
with the --eol option:

    040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       i/      w/      attr/                   x/

I.e. should we be saying anything about the EOL state of these? OTOHO I
tried adding a submodule and it says the same, which seems similarly
odd, so maybe it's either correct, or this isn't updated for those
either.

Is the behavior of:

    $ git -C sparse-index ls-files --stage --sparse -- 'folder2/a'
    $ echo $?
    0

Expected? I.e. accepting /a when we'd just print "folder2/" and not
e.g. erroring (probably, just asking)?

How about:

    $ ls -l sparse-index/x
    ls: cannot access 'sparse-index/x': No such file or directory
    $ git -C sparse-index ls-files --stage 'x/*'
    100644 78981922613b2afb6025042ff6bd878ac1994e85 0       x/a
    $ git -C sparse-index ls-files --stage --no-empty-directory 'x/*' 
    100644 78981922613b2afb6025042ff6bd878ac1994e85 0       x/a
    $ git -C sparse-index ls-files --stage --no-empty-directory --sparse 'x/*' 
    040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       x/

The answer is probably "yes that's fine" because I've got no idea how
sparse really works, but just checking..

So it's very nice to have the new diff test in 2/5, but would be much
nicer/assuring to have that split into a trivial function followed by
seeing how the diff looked in combination with each of the other option
that "ls-files" accepts.

Re: [PATCH v3 0/5] Sparse index: fetch, pull, ls-files

[Elijah Newren]

On Fri, Dec 10, 2021 at 8:31 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Fri, Dec 10 2021, Derrick Stolee via GitGitGadget wrote:
>
> > Updates in v3
> > =============
> >
> >  * Fixed typo in commit message.
> >  * Added comments around doing strange things in an ls-files test.
> >  * Fixed adjacent typo in a test comment.
>
> Yay, I'm happy to see 5/5. Not because I didn't like the helper, but
> that sparse is getting mature enough that we're getting ls-files to emit
> information about it. Thanks.
>
> There's the small "diff -u" portability issue noted in my just-sent
> <211210.86zgp8bi48.gmgdl@evledraar.gmail.com>.

Yeah, that one is an important point.

> Other than that 2/5 adds this documentation about ls-files --sparse:
>
>         If the index is sparse, show the sparse directories without expanding
>         to the contained files.
>
> Shouldn't we at least add:
>
>         Sparse directories will be shown with a trailing slash,
>         e.g. "x/" for a sparse directory "x".q

Makes sense.  Except I don't understand the trailing 'q' -- typo?

>
> In addition to that I think this may have a buggy/unexpected interaction
> with the --eol option:
>
>     040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       i/      w/      attr/                   x/
>
> I.e. should we be saying anything about the EOL state of these? OTOHO I
> tried adding a submodule and it says the same, which seems similarly
> odd, so maybe it's either correct, or this isn't updated for those
> either.

If it matches what we do for submodules, for which eol values are also
non-sensical, then I think we're good enough for this series.  Perhaps
we just shouldn't print anything eol related for directories with
--eol, but that sounds like an orthogonal series rather than something
that should go in this one.

> Is the behavior of:
>
>     $ git -C sparse-index ls-files --stage --sparse -- 'folder2/a'
>     $ echo $?
>     0
>
> Expected? I.e. accepting /a when we'd just print "folder2/" and not
> e.g. erroring (probably, just asking)?

Fair question.  I think it's fine; by way of comparison:

$ git rm --cached removed-and-no-longer-tracked-file
$ git ls-files --stage -- non-existent-file
removed-and-no-longer-tracked-file untracked-file
$ echo $?
0

So it also shows nothing and displays nothing when asked for file(s)
that are not in the index.

Yes, there is a slight semantic difference in that in your example we
have a "folder2/" entry which *could be* expanded, but I am quite
happy with the literal interpretation of the command that there is no
"folder2/a" in the index.  Said another way, I'm happy with ls-files
showing what is in the index right now, rather than what could be in
it, or listing things that HEAD contains that we don't for whatever
reason.

> How about:
>
>     $ ls -l sparse-index/x
>     ls: cannot access 'sparse-index/x': No such file or directory
>     $ git -C sparse-index ls-files --stage 'x/*'
>     100644 78981922613b2afb6025042ff6bd878ac1994e85 0       x/a
>     $ git -C sparse-index ls-files --stage --no-empty-directory 'x/*'
>     100644 78981922613b2afb6025042ff6bd878ac1994e85 0       x/a
>     $ git -C sparse-index ls-files --stage --no-empty-directory --sparse 'x/*'
>     040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       x/
>
> The answer is probably "yes that's fine" because I've got no idea how
> sparse really works, but just checking..

You should read the docs for this option you are trying: "Do not list
empty directories. Has no effect without --directory."    (Also,
--directory only takes effect with --other, which you are also
missing.)

So yeah, that flag is irrelevant.  Perhaps ls-files should print a
warning when flags are passed but ignored due to other flags not being
passed, but that would belong in an orthogonal series rather than this
one.

> So it's very nice to have the new diff test in 2/5, but would be much
> nicer/assuring to have that split into a trivial function followed by
> seeing how the diff looked in combination with each of the other option
> that "ls-files" accepts.

There's no point testing in combination with flags that only affect
untracked files.  And I'm very dubious of adding testing for a case
where we would need to add an explicit disclaimer that "We have no
idea what the output should be but we are testing it anyway".  So the
options you suggest at least are things I'd rather not see us trying
to add to the testing here.

Re: [PATCH v3 0/5] Sparse index: fetch, pull, ls-files

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 10 2021, Elijah Newren wrote:

> On Fri, Dec 10, 2021 at 8:31 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> [...]
>> Other than that 2/5 adds this documentation about ls-files --sparse:
>>
>>         If the index is sparse, show the sparse directories without expanding
>>         to the contained files.
>>
>> Shouldn't we at least add:
>>
>>         Sparse directories will be shown with a trailing slash,
>>         e.g. "x/" for a sparse directory "x".q
>
> Makes sense.  Except I don't understand the trailing 'q' -- typo?

Yes, sorry.

>>
>> In addition to that I think this may have a buggy/unexpected interaction
>> with the --eol option:
>>
>>     040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       i/      w/      attr/                   x/
>>
>> I.e. should we be saying anything about the EOL state of these? OTOHO I
>> tried adding a submodule and it says the same, which seems similarly
>> odd, so maybe it's either correct, or this isn't updated for those
>> either.
>
> If it matches what we do for submodules, for which eol values are also
> non-sensical, then I think we're good enough for this series.  Perhaps
> we just shouldn't print anything eol related for directories with
> --eol, but that sounds like an orthogonal series rather than something
> that should go in this one.

*nod*, probably. 

>> Is the behavior of:
>>
>>     $ git -C sparse-index ls-files --stage --sparse -- 'folder2/a'
>>     $ echo $?
>>     0
>>
>> Expected? I.e. accepting /a when we'd just print "folder2/" and not
>> e.g. erroring (probably, just asking)?
>
> Fair question.  I think it's fine; by way of comparison:
>
> $ git rm --cached removed-and-no-longer-tracked-file
> $ git ls-files --stage -- non-existent-file
> removed-and-no-longer-tracked-file untracked-file
> $ echo $?
> 0
>
> So it also shows nothing and displays nothing when asked for file(s)
> that are not in the index.
>
> Yes, there is a slight semantic difference in that in your example we
> have a "folder2/" entry which *could be* expanded, but I am quite
> happy with the literal interpretation of the command that there is no
> "folder2/a" in the index.  Said another way, I'm happy with ls-files
> showing what is in the index right now, rather than what could be in
> it, or listing things that HEAD contains that we don't for whatever
> reason.

Sounds good.

>> How about:
>>
>>     $ ls -l sparse-index/x
>>     ls: cannot access 'sparse-index/x': No such file or directory
>>     $ git -C sparse-index ls-files --stage 'x/*'
>>     100644 78981922613b2afb6025042ff6bd878ac1994e85 0       x/a
>>     $ git -C sparse-index ls-files --stage --no-empty-directory 'x/*'
>>     100644 78981922613b2afb6025042ff6bd878ac1994e85 0       x/a
>>     $ git -C sparse-index ls-files --stage --no-empty-directory --sparse 'x/*'
>>     040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       x/
>>
>> The answer is probably "yes that's fine" because I've got no idea how
>> sparse really works, but just checking..
>
> You should read the docs for this option you are trying: "Do not list
> empty directories. Has no effect without --directory."    (Also,
> --directory only takes effect with --other, which you are also
> missing.)
>
> So yeah, that flag is irrelevant.  Perhaps ls-files should print a
> warning when flags are passed but ignored due to other flags not being
> passed, but that would belong in an orthogonal series rather than this
> one.

...

>> So it's very nice to have the new diff test in 2/5, but would be much
>> nicer/assuring to have that split into a trivial function followed by
>> seeing how the diff looked in combination with each of the other option
>> that "ls-files" accepts.
>
> There's no point testing in combination with flags that only affect
> untracked files.  And I'm very dubious of adding testing for a case
> where we would need to add an explicit disclaimer that "We have no
> idea what the output should be but we are testing it anyway".  So the
> options you suggest at least are things I'd rather not see us trying
> to add to the testing here.

This series is adding a new flag to ls-files, it doesn't error out when
combined with other existing flags, and observably changes their output.

I think erroring out would be fine, or doing whatever it's doing now,
but either way the gap in test coverage should be closed, shouldn't it?

I'd think the easiest and probably most prudent fix would just be to say
that we don't think some of these make sense with --sparse and have them
error out if they're combined, no?

Re: [PATCH v3 0/5] Sparse index: fetch, pull, ls-files

[Elijah Newren]

On Fri, Dec 10, 2021 at 6:28 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Fri, Dec 10 2021, Elijah Newren wrote:
>
> > On Fri, Dec 10, 2021 at 8:31 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> > [...]
> >> So it's very nice to have the new diff test in 2/5, but would be much
> >> nicer/assuring to have that split into a trivial function followed by
> >> seeing how the diff looked in combination with each of the other option
> >> that "ls-files" accepts.
> >
> > There's no point testing in combination with flags that only affect
> > untracked files.  And I'm very dubious of adding testing for a case
> > where we would need to add an explicit disclaimer that "We have no
> > idea what the output should be but we are testing it anyway".  So the
> > options you suggest at least are things I'd rather not see us trying
> > to add to the testing here.
>
> This series is adding a new flag to ls-files, it doesn't error out when
> combined with other existing flags, and observably changes their output.

Ah, I think you had a misunderstanding here.  If what you say here
were true, then indeed we would need some testing and it'd suggest
some kind of bug.  But the combination here does not observably change
the output.  You were missing an important testcase for comparison.
Let me repeat your testing and sprinkle in some commentary:

> >>     $ ls -l sparse-index/x
> >>     ls: cannot access 'sparse-index/x': No such file or directory

Right, this is a sparse directory; good to double check.

> >>     $ git -C sparse-index ls-files --stage 'x/*'
> >>     100644 78981922613b2afb6025042ff6bd878ac1994e85 0       x/a
> >>     $ git -C sparse-index ls-files --stage --no-empty-directory 'x/*'
> >>     100644 78981922613b2afb6025042ff6bd878ac1994e85 0       x/a

Right, --no-empty-directory by itself is a useless option that won't
affect the output of ls-files; it only takes affect with --directory,
which in turn only takes affect with other options.  Since that
options is useless, the output is the same for both of these.

> >>     $ git -C sparse-index ls-files --stage --no-empty-directory --sparse 'x/*'
> >>     040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       x/

Here you added --sparse, but you neglected what it would show without
--no-empty-directory, which is a critical comparison point.  So let me
fill it in:

     $ git -C sparse-index ls-files --stage --sparse 'x/*'
     040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       x/

Now, this case I added in comparison to the one three above it shows
that, yes, --sparse does indeed change the output relative to --stage.
And it does so by design.  Now if you compare my added case to the
last one you showed, you can verify that adding --no-empty-directory
to that mix does not change the output further; --no-empty-directory
is a useless/ignored option unless you also include other flags that
were not involved here.

> I think erroring out would be fine, or doing whatever it's doing now,
> but either way the gap in test coverage should be closed, shouldn't it?
>
> I'd think the easiest and probably most prudent fix would just be to say
> that we don't think some of these make sense with --sparse and have them
> error out if they're combined, no?

ls-files offers several options that allow you to either slice and
dice or tweak the output, and function on two kinds of files: tracked,
and not tracked.  Several examples of such flags:
   * tracked: --cached, --stage, --unmerged, --modified, --sparse (and
I think --error-unmatch)
   * not tracked: --others, --ignored, --exclude, --exclude-from,
--exclude-standard, --directory, --no-empty-directory

Now, in particular, specifying any of --exclude, --exclude-from,
--exclude-standard, --directory, or --no-empty-directory is a complete
waste of breath and will do nothing unless you also specify --others
or --ignored.  None of these options interact in any way with any of
the flags from the --tracked category.

I don't think we want an n! permutation of all combinations tested.  I
don't even think an n^2 pair-wise combination makes sense when we know
that some flags have no effect on their own.  What would make sense is
perhaps adding a warning to ls-files when specified flags will have no
utility due to depending on other flags that have not been specified.
But that's in no way specific to --sparse and does not make sense to
me to make part of this topic.

Re: [PATCH v3 0/5] Sparse index: fetch, pull, ls-files

[Elijah Newren]

On Fri, Dec 10, 2021 at 7:13 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This is based on ld/sparse-index-blame (merged with 'master' due to an
> unrelated build issue).
>
> Here are two relatively-simple patches that further the sparse index
> integrations.
>
> Did you know that 'fetch' and 'pull' read the index? I didn't, or this would
> have been an integration much earlier in the cycle. They read the index to
> look for the .gitmodules file in case there are submodules that need to be
> fetched. Since looking for a file by name is already protected, we only need
> to disable 'command_requires_full_index' and we are done.
>
> The 'ls-files' builtin is useful when debugging the index, and some scripts
> use it, too. We are not changing the default behavior which expands a sparse
> index in order to show all of the cached blobs. Instead, we add a '--sparse'
> option that allows us to see the sparse directory entries upon request.
> Combined with --debug, we can see a lot of index details, such as:
>
> $ git ls-files --debug --sparse
> LICENSE
>   ctime: 1634910503:287405820
>   mtime: 1634910503:287405820
>   dev: 16777220 ino: 119325319
>   uid: 501  gid: 20
>   size: 1098    flags: 200000
> README.md
>   ctime: 1634910503:288090279
>   mtime: 1634910503:288090279
>   dev: 16777220 ino: 119325320
>   uid: 501  gid: 20
>   size: 934 flags: 200000
> bin/index.js
>   ctime: 1634910767:828434033
>   mtime: 1634910767:828434033
>   dev: 16777220 ino: 119325520
>   uid: 501  gid: 20
>   size: 7292    flags: 200000
> examples/
>   ctime: 0:0
>   mtime: 0:0
>   dev: 0    ino: 0
>   uid: 0    gid: 0
>   size: 0   flags: 40004000
> package.json
>   ctime: 1634910503:288676330
>   mtime: 1634910503:288676330
>   dev: 16777220 ino: 119325321
>   uid: 501  gid: 20
>   size: 680 flags: 200000
>
>
> (In this example, the 'examples/' directory is sparse.)
>
> Thanks!
>
>
> Updates in v2
> =============
>
>  * Rebased onto latest ld/sparse-index-blame without issue.
>  * Updated the test to use diff-of-diffs instead of a sequence of greps.
>  * Added patches that remove the use of 'test-tool read-cache --table' and
>    its implementation.
>
>
> Updates in v3
> =============
>
>  * Fixed typo in commit message.
>  * Added comments around doing strange things in an ls-files test.
>  * Fixed adjacent typo in a test comment.

Thanks, this round addresses all my previous feedback.  However, there
are two things Ævar has brought up that I think are important:
   * cannot rely on `diff -u` for portability reasons[1] (his
suggestion of git diff --no-index sounds good, or you can use comm(1))
   * have documentation mention the trailing slash that sparse
directory entries are mentioned with[2]

[1] https://lore.kernel.org/git/211210.86zgp8bi48.gmgdl@evledraar.gmail.com/
[2] https://lore.kernel.org/git/211210.86v8zwbev9.gmgdl@evledraar.gmail.com/

[PATCH v4 0/5] Sparse index: fetch, pull, ls-files

[Derrick Stolee via GitGitGadget]

This is now based on 'master'.

Did you know that 'fetch' and 'pull' read the index? I didn't, or this would
have been an integration much earlier in the cycle. They read the index to
look for the .gitmodules file in case there are submodules that need to be
fetched. Since looking for a file by name is already protected, we only need
to disable 'command_requires_full_index' and we are done.

The 'ls-files' builtin is useful when debugging the index, and some scripts
use it, too. We are not changing the default behavior which expands a sparse
index in order to show all of the cached blobs. Instead, we add a '--sparse'
option that allows us to see the sparse directory entries upon request.
Combined with --debug, we can see a lot of index details, such as:

$ git ls-files --debug --sparse
LICENSE
  ctime: 1634910503:287405820
  mtime: 1634910503:287405820
  dev: 16777220 ino: 119325319
  uid: 501  gid: 20
  size: 1098    flags: 200000
README.md
  ctime: 1634910503:288090279
  mtime: 1634910503:288090279
  dev: 16777220 ino: 119325320
  uid: 501  gid: 20
  size: 934 flags: 200000
bin/index.js
  ctime: 1634910767:828434033
  mtime: 1634910767:828434033
  dev: 16777220 ino: 119325520
  uid: 501  gid: 20
  size: 7292    flags: 200000
examples/
  ctime: 0:0
  mtime: 0:0
  dev: 0    ino: 0
  uid: 0    gid: 0
  size: 0   flags: 40004000
package.json
  ctime: 1634910503:288676330
  mtime: 1634910503:288676330
  dev: 16777220 ino: 119325321
  uid: 501  gid: 20
  size: 680 flags: 200000


(In this example, the 'examples/' directory is sparse.)

Thanks!


Updates in v2
=============

 * Rebased onto latest ld/sparse-index-blame without issue.
 * Updated the test to use diff-of-diffs instead of a sequence of greps.
 * Added patches that remove the use of 'test-tool read-cache --table' and
   its implementation.


Updates in v3
=============

 * Fixed typo in commit message.
 * Added comments around doing strange things in an ls-files test.
 * Fixed adjacent typo in a test comment.


Updates in v4
=============

 * Rebased on to 'master' now that ld/sparse-index-blame is merged.
 * Change testing strategy to check exact output instead of using 'diff -u'.
 * Updated documentation to state that directories have a trailing slash.

Derrick Stolee (5):
  fetch/pull: use the sparse index
  ls-files: add --sparse option
  t1092: replace 'read-cache --table' with 'ls-files --sparse'
  t1091/t3705: remove 'test-tool read-cache --table'
  test-read-cache: remove --table, --expand options

 Documentation/git-ls-files.txt           |   5 +
 builtin/fetch.c                          |   2 +
 builtin/ls-files.c                       |  12 ++-
 builtin/pull.c                           |   2 +
 t/helper/test-read-cache.c               |  64 ++---------
 t/t1091-sparse-checkout-builtin.sh       |  25 ++++-
 t/t1092-sparse-checkout-compatibility.sh | 132 ++++++++++++++++++++---
 t/t3705-add-sparse-checkout.sh           |   8 +-
 8 files changed, 168 insertions(+), 82 deletions(-)


base-commit: 597af311a2899bfd6640b9b107622c5795d5f998
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1080%2Fderrickstolee%2Fsparse-index%2Ffetch-pull-ls-files-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1080/derrickstolee/sparse-index/fetch-pull-ls-files-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1080

Range-diff vs v3:

 1:  f72001638d1 = 1:  1043a1927d2 fetch/pull: use the sparse index
 2:  b81174ba54b ! 2:  e1ec52881d9 ls-files: add --sparse option
     @@ Documentation/git-ls-files.txt: Both the <eolinfo> in the index ("i/<eolinfo>")
       
      +--sparse::
      +	If the index is sparse, show the sparse directories without expanding
     -+	to the contained files.
     ++	to the contained files. Sparse directories will be shown with a
     ++	trailing slash, such as "x/" for a sparse directory "x".
      +
       \--::
       	Do not interpret any more arguments as options.
     @@ builtin/ls-files.c: int cmd_ls_files(int argc, const char **argv, const char *cm
      
       ## t/t1092-sparse-checkout-compatibility.sh ##
      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is expanded and converted back' '
     - 	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
     + 	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
       		git -C sparse-index reset -- folder1/a &&
       	test_region index convert_to_sparse trace2.txt &&
      +	test_region index ensure_full_index trace2.txt &&
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
      +test_expect_success 'ls-files' '
      +	init_repos &&
      +
     ++	# Use a smaller sparse-checkout for reduced output
     ++	test_sparse_match git sparse-checkout set &&
     ++
      +	# Behavior agrees by default. Sparse index is expanded.
      +	test_all_match git ls-files &&
      +
      +	# With --sparse, the sparse index data changes behavior.
     -+	git -C sparse-index ls-files >dense &&
     -+	git -C sparse-index ls-files --sparse >sparse &&
     ++	git -C sparse-index ls-files --sparse >actual &&
      +
      +	cat >expect <<-\EOF &&
     -+	@@ -13,13 +13,9 @@
     -+	 e
     -+	 folder1-
     -+	 folder1.x
     -+	-folder1/0/0/0
     -+	-folder1/0/1
     -+	-folder1/a
     -+	+folder1/
     -+	 folder10
     -+	-folder2/0/0/0
     -+	-folder2/0/1
     -+	-folder2/a
     -+	+folder2/
     -+	 g
     -+	-x/a
     -+	+x/
     -+	 z
     ++	a
     ++	deep/
     ++	e
     ++	folder1-
     ++	folder1.x
     ++	folder1/
     ++	folder10
     ++	folder2/
     ++	g
     ++	x/
     ++	z
      +	EOF
      +
     -+	diff -u dense sparse | tail -n +3 >actual &&
      +	test_cmp expect actual &&
      +
      +	# With --sparse and no sparse index, nothing changes.
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is n
      +	test_sparse_match git sparse-checkout add folder1 &&
      +	test_sparse_match git ls-files --modified &&
      +
     -+	git -C sparse-index ls-files >dense &&
     -+	git -C sparse-index ls-files --sparse >sparse &&
     ++	test_all_match git ls-files &&
     ++	git -C sparse-index ls-files --sparse >actual &&
      +
      +	cat >expect <<-\EOF &&
     -+	@@ -17,9 +17,7 @@
     -+	 folder1/0/1
     -+	 folder1/a
     -+	 folder10
     -+	-folder2/0/0/0
     -+	-folder2/0/1
     -+	-folder2/a
     -+	+folder2/
     -+	 g
     -+	-x/a
     -+	+x/
     -+	 z
     ++	a
     ++	deep/
     ++	e
     ++	folder1-
     ++	folder1.x
     ++	folder1/0/0/0
     ++	folder1/0/1
     ++	folder1/a
     ++	folder10
     ++	folder2/
     ++	g
     ++	x/
     ++	z
      +	EOF
      +
     -+	diff -u dense sparse | tail -n +3 >actual &&
      +	test_cmp expect actual &&
      +
      +	# Double-check index expansion is avoided
 3:  2a6a1c5a39c = 3:  0c53bd09ba4 t1092: replace 'read-cache --table' with 'ls-files --sparse'
 4:  f0143686754 = 4:  4952c9e724b t1091/t3705: remove 'test-tool read-cache --table'
 5:  9227dc54165 = 5:  3efffad814c test-read-cache: remove --table, --expand options

-- 
gitgitgadget

[PATCH v4 1/5] fetch/pull: use the sparse index

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The 'git fetch' and 'git pull' commands parse the index in order to
determine if submodules exist. Without command_requires_full_index=0,
this will expand a sparse index, causing slow performance even when
there is no new data to fetch.

The .gitmodules file will never be inside a sparse directory entry, and
even if it was, the index_name_pos() method would expand the sparse
index if needed as we search for the path by name. These commands do not
iterate over the index, which is the typical thing we are careful about
when integrating with the sparse index.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/fetch.c                          |  2 ++
 builtin/pull.c                           |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 10 ++++++++++
 3 files changed, 14 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e45185cf9cf..a0757dd0158 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1999,6 +1999,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	}
 
 	git_config(git_fetch_config, NULL);
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
 
 	argc = parse_options(argc, argv, prefix,
 			     builtin_fetch_options, builtin_fetch_usage, 0);
diff --git a/builtin/pull.c b/builtin/pull.c
index c8457619d86..100cbf9fb85 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -994,6 +994,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		set_reflog_message(argc, argv);
 
 	git_config(git_pull_config, NULL);
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
 
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 49f70a65692..8d3c21fc84c 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1009,6 +1009,16 @@ test_expect_success 'sparse index is not expanded: blame' '
 	done
 '
 
+test_expect_success 'sparse index is not expanded: fetch/pull' '
+	init_repos &&
+
+	git -C sparse-index remote add full "file://$(pwd)/full-checkout" &&
+	ensure_not_expanded fetch full &&
+	git -C full-checkout commit --allow-empty -m "for pull merge" &&
+	git -C sparse-index commit --allow-empty -m "for pull merge" &&
+	ensure_not_expanded pull full base
+'
+
 # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
 # in this scenario, but it shouldn't.
 test_expect_success 'reset mixed and checkout orphan' '
-- 
gitgitgadget

[PATCH v4 2/5] ls-files: add --sparse option

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Existing callers to 'git ls-files' are expecting file names, not
directories. It is best to expand a sparse index to show all of the
contained files in this case.

However, expert users may want to inspect the contents of the index
itself including which directories are sparse. Add a --sparse option to
allow users to request this information.

During testing, I noticed that options such as --modified did not affect
the output when the files in question were outside the sparse-checkout
definition. Tests are added to document this preexisting behavior and
how it remains unchanged with the sparse index and the --sparse option.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-ls-files.txt           |  5 ++
 builtin/ls-files.c                       | 12 +++-
 t/t1092-sparse-checkout-compatibility.sh | 91 ++++++++++++++++++++++++
 3 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 2e3d695fa21..48cc7c0b6f4 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -187,6 +187,11 @@ Both the <eolinfo> in the index ("i/<eolinfo>")
 and in the working tree ("w/<eolinfo>") are shown for regular files,
 followed by the  ("attr/<eolattr>").
 
+--sparse::
+	If the index is sparse, show the sparse directories without expanding
+	to the contained files. Sparse directories will be shown with a
+	trailing slash, such as "x/" for a sparse directory "x".
+
 \--::
 	Do not interpret any more arguments as options.
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 031fef1bcaa..c151eb1fb77 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -37,6 +37,7 @@ static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
 static int skipping_duplicates;
+static int show_sparse_dirs;
 
 static const char *prefix;
 static int max_prefix_len;
@@ -315,8 +316,10 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
 
 	if (!(show_cached || show_stage || show_deleted || show_modified))
 		return;
-	/* TODO: audit for interaction with sparse-index. */
-	ensure_full_index(repo->index);
+
+	if (!show_sparse_dirs)
+		ensure_full_index(repo->index);
+
 	for (i = 0; i < repo->index->cache_nr; i++) {
 		const struct cache_entry *ce = repo->index->cache[i];
 		struct stat st;
@@ -670,6 +673,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
 		OPT_BOOL(0, "deduplicate", &skipping_duplicates,
 			 N_("suppress duplicate entries")),
+		OPT_BOOL(0, "sparse", &show_sparse_dirs,
+			 N_("show sparse directories in the presence of a sparse index")),
 		OPT_END()
 	};
 	int ret = 0;
@@ -677,6 +682,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(ls_files_usage, builtin_ls_files_options);
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	prefix = cmd_prefix;
 	if (prefix)
 		prefix_len = strlen(prefix);
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 8d3c21fc84c..0c51e9bd3b7 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -816,6 +816,12 @@ test_expect_success 'sparse-index is expanded and converted back' '
 	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
 		git -C sparse-index reset -- folder1/a &&
 	test_region index convert_to_sparse trace2.txt &&
+	test_region index ensure_full_index trace2.txt &&
+
+	# ls-files expands on read, but does not write.
+	rm trace2.txt &&
+	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TRACE2_EVENT_NESTING=10 \
+		git -C sparse-index ls-files &&
 	test_region index ensure_full_index trace2.txt
 '
 
@@ -871,6 +877,7 @@ test_expect_success 'sparse-index is not expanded' '
 	init_repos &&
 
 	ensure_not_expanded status &&
+	ensure_not_expanded ls-files --sparse &&
 	ensure_not_expanded commit --allow-empty -m empty &&
 	echo >>sparse-index/a &&
 	ensure_not_expanded commit -a -m a &&
@@ -1019,6 +1026,90 @@ test_expect_success 'sparse index is not expanded: fetch/pull' '
 	ensure_not_expanded pull full base
 '
 
+test_expect_success 'ls-files' '
+	init_repos &&
+
+	# Use a smaller sparse-checkout for reduced output
+	test_sparse_match git sparse-checkout set &&
+
+	# Behavior agrees by default. Sparse index is expanded.
+	test_all_match git ls-files &&
+
+	# With --sparse, the sparse index data changes behavior.
+	git -C sparse-index ls-files --sparse >actual &&
+
+	cat >expect <<-\EOF &&
+	a
+	deep/
+	e
+	folder1-
+	folder1.x
+	folder1/
+	folder10
+	folder2/
+	g
+	x/
+	z
+	EOF
+
+	test_cmp expect actual &&
+
+	# With --sparse and no sparse index, nothing changes.
+	git -C sparse-checkout ls-files >dense &&
+	git -C sparse-checkout ls-files --sparse >sparse &&
+	test_cmp dense sparse &&
+
+	# Set up a strange condition of having a file edit
+	# outside of the sparse-checkout cone. This is just
+	# to verify that sparse-checkout and sparse-index
+	# behave the same in this case.
+	write_script edit-content <<-\EOF &&
+	mkdir folder1 &&
+	echo content >>folder1/a
+	EOF
+	run_on_sparse ../edit-content &&
+
+	# ls-files does not currently notice modified files whose
+	# cache entries are marked SKIP_WORKTREE. This may change
+	# in the future, but here we test that sparse index does
+	# not accidentally create a change of behavior.
+	test_sparse_match git ls-files --modified &&
+	test_must_be_empty sparse-checkout-out &&
+	test_must_be_empty sparse-index-out &&
+
+	git -C sparse-index ls-files --sparse --modified >sparse-index-out &&
+	test_must_be_empty sparse-index-out &&
+
+	# Add folder1 to the sparse-checkout cone and
+	# check that ls-files shows the expanded files.
+	test_sparse_match git sparse-checkout add folder1 &&
+	test_sparse_match git ls-files --modified &&
+
+	test_all_match git ls-files &&
+	git -C sparse-index ls-files --sparse >actual &&
+
+	cat >expect <<-\EOF &&
+	a
+	deep/
+	e
+	folder1-
+	folder1.x
+	folder1/0/0/0
+	folder1/0/1
+	folder1/a
+	folder10
+	folder2/
+	g
+	x/
+	z
+	EOF
+
+	test_cmp expect actual &&
+
+	# Double-check index expansion is avoided
+	ensure_not_expanded ls-files --sparse
+'
+
 # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
 # in this scenario, but it shouldn't.
 test_expect_success 'reset mixed and checkout orphan' '
-- 
gitgitgadget

[PATCH v4 3/5] t1092: replace 'read-cache --table' with 'ls-files --sparse'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Now that 'git ls-files --sparse' exists, we can use it to verify the
state of a sparse index instead of 'test-tool read-cache table'. Replace
these usages within t1092-sparse-checkout-compatibility.sh.

The important changes are due to the different output format. We need to
use the '--stage' output to get a file mode and OID, but it also
includes a stage value and drops the object type. This leads to some
differences in how we handle looking for specific entries.

Some places where we previously looked for no 'tree' entries, we can
instead directly compare the output across the repository with a sparse
index and the one without.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 31 +++++++++++-------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 0c51e9bd3b7..4ba16177528 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -206,45 +206,42 @@ test_sparse_unstaged () {
 test_expect_success 'sparse-index contents' '
 	init_repos &&
 
-	test-tool -C sparse-index read-cache --table >cache &&
+	git -C sparse-index ls-files --sparse --stage >cache &&
 	for dir in folder1 folder2 x
 	do
 		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
-		grep "040000 tree $TREE	$dir/" cache \
+		grep "040000 $TREE 0	$dir/" cache \
 			|| return 1
 	done &&
 
 	git -C sparse-index sparse-checkout set folder1 &&
 
-	test-tool -C sparse-index read-cache --table >cache &&
+	git -C sparse-index ls-files --sparse --stage >cache &&
 	for dir in deep folder2 x
 	do
 		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
-		grep "040000 tree $TREE	$dir/" cache \
+		grep "040000 $TREE 0	$dir/" cache \
 			|| return 1
 	done &&
 
 	git -C sparse-index sparse-checkout set deep/deeper1 &&
 
-	test-tool -C sparse-index read-cache --table >cache &&
+	git -C sparse-index ls-files --sparse --stage >cache &&
 	for dir in deep/deeper2 folder1 folder2 x
 	do
 		TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
-		grep "040000 tree $TREE	$dir/" cache \
+		grep "040000 $TREE 0	$dir/" cache \
 			|| return 1
 	done &&
 
-	# Disabling the sparse-index removes tree entries with full ones
+	# Disabling the sparse-index replaces tree entries with full ones
 	git -C sparse-index sparse-checkout init --no-sparse-index &&
-
-	test-tool -C sparse-index read-cache --table >cache &&
-	! grep "040000 tree" cache &&
-	test_sparse_match test-tool read-cache --table
+	test_sparse_match git ls-files --stage --sparse
 '
 
 test_expect_success 'expanded in-memory index matches full index' '
 	init_repos &&
-	test_sparse_match test-tool read-cache --expand --table
+	test_sparse_match git ls-files --stage
 '
 
 test_expect_success 'status with options' '
@@ -801,9 +798,9 @@ test_expect_success 'submodule handling' '
 
 	# having a submodule prevents "modules" from collapse
 	test_sparse_match git sparse-checkout set deep/deeper1 &&
-	test-tool -C sparse-index read-cache --table >cache &&
-	grep "100644 blob .*	modules/a" cache &&
-	grep "160000 commit $(git -C initial-repo rev-parse HEAD)	modules/sub" cache
+	git -C sparse-index ls-files --sparse --stage >cache &&
+	grep "100644 .*	modules/a" cache &&
+	grep "160000 $(git -C initial-repo rev-parse HEAD) 0	modules/sub" cache
 '
 
 # When working with a sparse index, some commands will need to expand the
@@ -1125,13 +1122,13 @@ test_expect_success 'reset mixed and checkout orphan' '
 	# the sparse checkouts skip "adding" the other side of
 	# the conflict.
 	test_sparse_match git reset --mixed HEAD~1 &&
-	test_sparse_match test-tool read-cache --table --expand &&
+	test_sparse_match git ls-files --stage &&
 	test_sparse_match git status --porcelain=v2 &&
 
 	# At this point, sparse-checkouts behave differently
 	# from the full-checkout.
 	test_sparse_match git checkout --orphan new-branch &&
-	test_sparse_match test-tool read-cache --table --expand &&
+	test_sparse_match git ls-files --stage &&
 	test_sparse_match git status --porcelain=v2
 '
 
-- 
gitgitgadget

[PATCH v4 4/5] t1091/t3705: remove 'test-tool read-cache --table'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Now that 'git ls-files --sparse' exists, we can use it to verify the
state of a sparse index instead of 'test-tool read-cache table'. Replace
these usages within t1091-sparse-checkout-builtin.sh and
t3705-add-sparse-checkout.sh.

The important changes are due to the different output format. In t3705,
we need to use the '--stage' output to get a file mode and OID, but
it also includes a stage value and drops the object type. This leads
to some differences in how we handle looking for specific entries.

In t1091, the test focuses on enabling the sparse index, so we do not
need the --stage flag to demonstrate how the index changes, and instead
can use a diff.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1091-sparse-checkout-builtin.sh | 25 ++++++++++++++++++++-----
 t/t3705-add-sparse-checkout.sh     |  8 ++++----
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 272ba1b566b..680e0043c36 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -212,12 +212,27 @@ test_expect_success 'sparse-index enabled and disabled' '
 
 		git -C repo sparse-checkout init --cone --sparse-index &&
 		test_cmp_config -C repo true index.sparse &&
-		test-tool -C repo read-cache --table >cache &&
-		grep " tree " cache &&
-
+		git -C repo ls-files --sparse >sparse &&
 		git -C repo sparse-checkout disable &&
-		test-tool -C repo read-cache --table >cache &&
-		! grep " tree " cache &&
+		git -C repo ls-files --sparse >full &&
+
+		cat >expect <<-\EOF &&
+		@@ -1,4 +1,7 @@
+		 a
+		-deep/
+		-folder1/
+		-folder2/
+		+deep/a
+		+deep/deeper1/a
+		+deep/deeper1/deepest/a
+		+deep/deeper2/a
+		+folder1/a
+		+folder2/a
+		EOF
+
+		diff -u sparse full | tail -n +3 >actual &&
+		test_cmp expect actual &&
+
 		git -C repo config --list >config &&
 		! grep index.sparse config
 	)
diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
index f3143c92908..81f3384eeed 100755
--- a/t/t3705-add-sparse-checkout.sh
+++ b/t/t3705-add-sparse-checkout.sh
@@ -181,13 +181,13 @@ test_expect_success 'git add fails outside of sparse-checkout definition' '
 	# Avoid munging CRLFs to avoid an error message
 	git -c core.autocrlf=input add --sparse sparse_entry 2>stderr &&
 	test_must_be_empty stderr &&
-	test-tool read-cache --table >actual &&
-	grep "^100644 blob.*sparse_entry\$" actual &&
+	git ls-files --stage >actual &&
+	grep "^100644 .*sparse_entry\$" actual &&
 
 	git add --sparse --chmod=+x sparse_entry 2>stderr &&
 	test_must_be_empty stderr &&
-	test-tool read-cache --table >actual &&
-	grep "^100755 blob.*sparse_entry\$" actual &&
+	git ls-files --stage >actual &&
+	grep "^100755 .*sparse_entry\$" actual &&
 
 	git reset &&
 
-- 
gitgitgadget

[PATCH v4 5/5] test-read-cache: remove --table, --expand options

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

This commit effectively reverts 2782db3 (test-tool: don't force full
index, 2021-03-30) and e2df6c3 (test-read-cache: print cache entries
with --table, 2021-03-30) to remove the --table and --expand options
from 'test-tool read-cache'. The previous changes already removed these
options from the test suite in favor of 'git ls-files --sparse'.

The initial thought of creating these options was to allow for tests to
see additional information with every cache entry. In particular, the
object type is still not mirrored in 'git ls-files'. Since sparse
directory entries always end with a slash, the object type is not
critical to verify the sparse index is enabled. It was thought that it
would be helpful to have additional information, such as flags, but that
was not needed for the FS Monitor integration and hasn't been needed
since.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/helper/test-read-cache.c | 64 ++++++--------------------------------
 1 file changed, 10 insertions(+), 54 deletions(-)

diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index 0d9f08931a1..b736ef16421 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -1,83 +1,39 @@
 #include "test-tool.h"
 #include "cache.h"
 #include "config.h"
-#include "blob.h"
-#include "commit.h"
-#include "tree.h"
-#include "sparse-index.h"
-
-static void print_cache_entry(struct cache_entry *ce)
-{
-	const char *type;
-	printf("%06o ", ce->ce_mode & 0177777);
-
-	if (S_ISSPARSEDIR(ce->ce_mode))
-		type = tree_type;
-	else if (S_ISGITLINK(ce->ce_mode))
-		type = commit_type;
-	else
-		type = blob_type;
-
-	printf("%s %s\t%s\n",
-	       type,
-	       oid_to_hex(&ce->oid),
-	       ce->name);
-}
-
-static void print_cache(struct index_state *istate)
-{
-	int i;
-	for (i = 0; i < istate->cache_nr; i++)
-		print_cache_entry(istate->cache[i]);
-}
 
 int cmd__read_cache(int argc, const char **argv)
 {
-	struct repository *r = the_repository;
 	int i, cnt = 1;
 	const char *name = NULL;
-	int table = 0, expand = 0;
 
 	initialize_the_repository();
 
-	for (++argv, --argc; *argv && starts_with(*argv, "--"); ++argv, --argc) {
-		if (skip_prefix(*argv, "--print-and-refresh=", &name))
-			continue;
-		if (!strcmp(*argv, "--table"))
-			table = 1;
-		else if (!strcmp(*argv, "--expand"))
-			expand = 1;
+	if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
+		argc--;
+		argv++;
 	}
 
-	if (argc == 1)
-		cnt = strtol(argv[0], NULL, 0);
+	if (argc == 2)
+		cnt = strtol(argv[1], NULL, 0);
 	setup_git_directory();
 	git_config(git_default_config, NULL);
 
-	prepare_repo_settings(r);
-	r->settings.command_requires_full_index = 0;
-
 	for (i = 0; i < cnt; i++) {
-		repo_read_index(r);
-
-		if (expand)
-			ensure_full_index(r->index);
-
+		read_cache();
 		if (name) {
 			int pos;
 
-			refresh_index(r->index, REFRESH_QUIET,
+			refresh_index(&the_index, REFRESH_QUIET,
 				      NULL, NULL, NULL);
-			pos = index_name_pos(r->index, name, strlen(name));
+			pos = index_name_pos(&the_index, name, strlen(name));
 			if (pos < 0)
 				die("%s not in index", name);
 			printf("%s is%s up to date\n", name,
-			       ce_uptodate(r->index->cache[pos]) ? "" : " not");
+			       ce_uptodate(the_index.cache[pos]) ? "" : " not");
 			write_file(name, "%d\n", i);
 		}
-		if (table)
-			print_cache(r->index);
-		discard_index(r->index);
+		discard_cache();
 	}
 	return 0;
 }
-- 
gitgitgadget

Re: [PATCH v4 0/5] Sparse index: fetch, pull, ls-files

[Elijah Newren]

On Wed, Dec 22, 2021 at 6:20 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This is now based on 'master'.
>
> Did you know that 'fetch' and 'pull' read the index? I didn't, or this would
> have been an integration much earlier in the cycle. They read the index to
> look for the .gitmodules file in case there are submodules that need to be
> fetched. Since looking for a file by name is already protected, we only need
> to disable 'command_requires_full_index' and we are done.
>
> The 'ls-files' builtin is useful when debugging the index, and some scripts
> use it, too. We are not changing the default behavior which expands a sparse
> index in order to show all of the cached blobs. Instead, we add a '--sparse'
> option that allows us to see the sparse directory entries upon request.
...
> Updates in v2
> =============
>
>  * Rebased onto latest ld/sparse-index-blame without issue.
>  * Updated the test to use diff-of-diffs instead of a sequence of greps.
>  * Added patches that remove the use of 'test-tool read-cache --table' and
>    its implementation.
>
>
> Updates in v3
> =============
>
>  * Fixed typo in commit message.
>  * Added comments around doing strange things in an ls-files test.
>  * Fixed adjacent typo in a test comment.
>
>
> Updates in v4
> =============
>
>  * Rebased on to 'master' now that ld/sparse-index-blame is merged.
>  * Change testing strategy to check exact output instead of using 'diff -u'.
>  * Updated documentation to state that directories have a trailing slash.

This version looks good to me:

Reviewed-by: Elijah Newren <newren@gmail.com>

Re: [PATCH v4 0/5] Sparse index: fetch, pull, ls-files

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Wed, Dec 22, 2021 at 6:20 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> This is now based on 'master'.
>>
>> Did you know that 'fetch' and 'pull' read the index? I didn't, or this would
>> have been an integration much earlier in the cycle. They read the index to
>> look for the .gitmodules file in case there are submodules that need to be
>> fetched. Since looking for a file by name is already protected, we only need
>> to disable 'command_requires_full_index' and we are done.
>>
>> The 'ls-files' builtin is useful when debugging the index, and some scripts
>> use it, too. We are not changing the default behavior which expands a sparse
>> index in order to show all of the cached blobs. Instead, we add a '--sparse'
>> option that allows us to see the sparse directory entries upon request.
> ...
>> Updates in v2
>> =============
>>
>>  * Rebased onto latest ld/sparse-index-blame without issue.
>>  * Updated the test to use diff-of-diffs instead of a sequence of greps.
>>  * Added patches that remove the use of 'test-tool read-cache --table' and
>>    its implementation.
>>
>>
>> Updates in v3
>> =============
>>
>>  * Fixed typo in commit message.
>>  * Added comments around doing strange things in an ls-files test.
>>  * Fixed adjacent typo in a test comment.
>>
>>
>> Updates in v4
>> =============
>>
>>  * Rebased on to 'master' now that ld/sparse-index-blame is merged.
>>  * Change testing strategy to check exact output instead of using 'diff -u'.
>>  * Updated documentation to state that directories have a trailing slash.
>
> This version looks good to me:
>
> Reviewed-by: Elijah Newren <newren@gmail.com>

Yup, they looked good to me too.

Thanks.