[RFC][PATCH] t1092: add tests for `git diff-files`

[RFC][PATCH] t1092: add tests for `git diff-files`

[Shuqi Liang]

To make sure git diff-files behaves as expected when
inside or outside of sparse-checkout definition.

Add test for git diff-files:
Path is within sparse-checkout cone
Path is outside sparse-checkout cone

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 32 ++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..f4815c619a 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2054,5 +2054,37 @@ test_expect_success 'grep sparse directory within submodules' '
 	git grep --cached --recurse-submodules a -- "*/folder1/*" >actual &&
 	test_cmp actual expect
 '
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>$1
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files  &&
+	test_all_match git diff-files deep/a &&
+	test_all_match git diff-files --find-object=HEAD:a
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>$1
+	EOF
+
+	run_on_sparse mkdir newdirectory &&
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git sparse-checkout set newdirectory &&
+	test_sparse_match git add newdirectory/testfile &&
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git sparse-checkout set &&
+
+	test_sparse_match git diff-files &&
+	test_sparse_match git diff-files newdirectory/testfile &&
+	test_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile
+'
 
 test_done
-- 
2.39.0

Re: [RFC][PATCH] t1092: add tests for `git diff-files`

[Derrick Stolee]

On 3/3/2023 9:57 PM, Shuqi Liang wrote:
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2054,5 +2054,37 @@ test_expect_success 'grep sparse directory within submodules' '
>  	git grep --cached --recurse-submodules a -- "*/folder1/*" >actual &&
>  	test_cmp actual expect
>  '
> +test_expect_success 'diff-files with pathspec inside sparse definition' '

nit: you need an empty line between the previous test's closing quote
and the start of your new test.

> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>$1
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	test_all_match git diff-files  &&
> +	test_all_match git diff-files deep/a &&
> +	test_all_match git diff-files --find-object=HEAD:a
> +'
> +
> +test_expect_success 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>$1
> +	EOF
> +
> +	run_on_sparse mkdir newdirectory &&
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git sparse-checkout set newdirectory &&
> +	test_sparse_match git add newdirectory/testfile &&
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git sparse-checkout set &&

These uses of 'git sparse-checkout set' are probably not necessary
if you use "git add --sparse newdirectory/testfile". It does present
an interesting modification of your test case: what if the file
exists on-disk but outside of the sparse-checkout definition? What
happens in each case? What if the file is different from the staged
version?

> +
> +	test_sparse_match git diff-files &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile
> +'

These tests look like a good start here. I was first confused as
to why you were doing such steps to modify the sparse-checkout
definition, but I see it is critical that you have staged changes
outside of the sparse-checkout cone. These kinds of details, the
"why" you are doing subtle things, are great to add to the commit
message.

> To make sure git diff-files behaves as expected when
> inside or outside of sparse-checkout definition.
> 
> Add test for git diff-files:
> Path is within sparse-checkout cone
> Path is outside sparse-checkout cone

With that in mind, here is a way you could edit your commit
message to be more informative:

  Before integrating the 'git diff-files' builtin with the sparse
  index feature, add tests to t1092-sparse-checkout-compatibility.sh
  to ensure it currently works with sparse-checkout and will still
  work with sparse index after that integration.

  When adding tests against a sparse-checkout definition, we must
  test two modes: all changes are within the sparse-checkout cone
  and some changes are outside the sparse-checkout cone. In order to
  have staged changes outside of the sparse-checkout cone, create a
  'newdirectory/testfile' and add it to the index, while leaving it
  outside of the sparse-checkout definition.

(If you decide to add tests for the case of 'newdirectory/testfile'
being present on-disk with or without modifications, then you can
expand your commit message to include details about those tests,
too.)

Thanks,
-Stolee

[PATCH v2 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Turn on sparse-index feature within `git diff-files` command.
Add necessary modifications and test them.

Changes since v1:

1.Add an empty line between the previous test's closing quote
and the start of new test.

2.Use "git add --sparse newdirectory/testfile" instead of 
'git sparse-checkout set' to have staged changes outside 
of the sparse-checkout cone

3.Edit commit message to be more informative

(sorry to send this patch twice ,I forgot to --inreply-to the origin one)

Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  4 ++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 52 ++++++++++++++++++++++++
 3 files changed, 58 insertions(+)


base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
-- 
2.39.0

[PATCH v2 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin
with the sparse index feature, add tests to
t1092-sparse-checkout-compatibility.sh to ensure it currently works
with sparse-checkout and will still work with sparse index
after that integration.

When adding tests against a sparse-checkout
definition, we test two modes: all changes are
within the sparse-checkout cone and some changes are outside
the sparse-checkout cone.

In order to have staged changes outside of
the sparse-checkout cone, create a 'newdirectory/testfile' and
add it to the index, while leaving it outside of
the sparse-checkout definition.Test 'newdirectory/testfile'
being present on-disk without modifications, then change content inside
'newdirectory/testfile' in order to test 'newdirectory/testfile'
being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 38 ++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..9382428352 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,42 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>$1
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files  &&
+	test_all_match git diff-files deep/a &&
+	test_all_match git diff-files --find-object=HEAD:a
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>$1
+	EOF
+
+	#add file to the index but outside of cone
+	run_on_sparse mkdir newdirectory &&
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git add --sparse newdirectory/testfile &&
+
+	#file present on-disk without modifications
+	test_sparse_match git diff-files &&
+	test_sparse_match git diff-files newdirectory/testfile &&
+	test_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile &&
+
+	#file present on-disk with modifications
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git diff-files &&
+	test_sparse_match git diff-files newdirectory/testfile &&
+	test_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile
+'
+
 test_done
-- 
2.39.0

[PATCH v2 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Remove full index requirement for `git diff-files`
and test to ensure the index is not expanded in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  4 ++++
 t/perf/p2000-sparse-operations.sh        |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 14 ++++++++++++++
 3 files changed, 20 insertions(+)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..360464e6ef 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 3242cfe91a..82751f2ca3 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all
 test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
 test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
 test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 9382428352..7cc02b882b 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2093,4 +2093,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
 	test_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile
 '
 
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>$1
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files  &&
+	ensure_not_expanded diff-files deep/a &&
+	ensure_not_expanded diff-files --find-object=HEAD:a
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v2 1/2] t1092: add tests for `git diff-files`

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 801919009e..9382428352 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2055,4 +2055,42 @@ test_expect_success 'grep sparse directory within submodules' '
>  	test_cmp actual expect
>  '
>  
> +test_expect_success 'diff-files with pathspec inside sparse definition' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>$1
> +	EOF

(Documentation/CodingGuidelines)

 - Redirection operators should be written with space before, but no
   space after them.  In other words, write 'echo test >"$file"'
   instead of 'echo test> $file' or 'echo test > $file'.  Note that
   even though it is not required by POSIX to double-quote the
   redirection target in a variable (as shown above), our code does so
   because some versions of bash issue a warning without the quotes.

> +	#add file to the index but outside of cone

Can you have a SP after "#" here to make it more readable?

> +	run_on_sparse mkdir newdirectory &&
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git add --sparse newdirectory/testfile &&

We create a new directory that is outside the cone, with or without
using the sparse-index feature.  We know we are violating the cone,
and have to override the safety with the "--sparse" option.  OK.

What output do we expect out of "git add" to match in the two cases?

> +	#file present on-disk without modifications
> +	test_sparse_match git diff-files &&
> +	test_sparse_match git diff-files newdirectory/testfile &&

As "diff-files" is about comparing between the index and the working
tree, the new path should not appear in the output when the sparse
checkout feature with or without the sparse-index feature is NOT in
use.  Does the picture get different when we are sparse?  IOW, would
we notice that we now have newdirectory/testfile that is supposed to
be missing in the index and show that in the output?

> +	test_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile &&

What does HEAD:testfile refer to in this test?  This expects "diff-files"
invocation to fail, and perhaps in your test it failed in both test
repositories the same way, but are they failing for the right reason?

In a non-sparse repository whose HEAD commit does not have
'testfile' (e.g. "git" source tree), I get

    $ git diff-files --find-object=HEAD:testfile
    error: unable to resolve 'HEAD:testfile'

without sparse checkout or sparse index.  It is unclear what value
we get out of having this test here.

> +	#file present on-disk with modifications
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git diff-files &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile

Ditto.

> +'
> +
>  test_done

Thanks.

Re: [PATCH v2 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Hi Junio

On Tue, Mar 7, 2023 at 1:53 PM Junio C Hamano <gitster@pobox.com> wrote:

> (Documentation/CodingGuidelines)
>
>  - Redirection operators should be written with space before, but no
>    space after them.  In other words, write 'echo test >"$file"'
>    instead of 'echo test> $file' or 'echo test > $file'.  Note that
>    even though it is not required by POSIX to double-quote the
>    redirection target in a variable (as shown above), our code does so
>    because some versions of bash issue a warning without the quotes.


Thanks for the styling reminders! I should go back and reread CodingGuidelines
more often.


> > +     #add file to the index but outside of cone
>
> Can you have a SP after "#" here to make it more readable?

Will do!


> We create a new directory that is outside the cone, with or without
> using the sparse-index feature.  We know we are violating the cone,
> and have to override the safety with the "--sparse" option.  OK.
>
> What output do we expect out of "git add" to match in the two cases?
>
> > +     #file present on-disk without modifications
> > +     test_sparse_match git diff-files &&
> > +     test_sparse_match git diff-files newdirectory/testfile &&
>
> As "diff-files" is about comparing between the index and the working
> tree, the new path should not appear in the output when the sparse
> checkout feature with or without the sparse-index feature is NOT in
> use.  Does the picture get different when we are sparse?  IOW, would
> we notice that we now have newdirectory/testfile that is supposed to
> be missing in the index and show that in the output?

I'm a bit caught up here.
Do you mean I need to add a test for "git add" also?

when we use "git add" instead of "git add --sparse ", we will get different.
Cause newdirectory/testfile is missing in the index so diff-files will not
work in these cases.


> In a non-sparse repository whose HEAD commit does not have
> 'testfile' (e.g. "git" source tree), I get
>
>     $ git diff-files --find-object=HEAD:testfile
>     error: unable to resolve 'HEAD:testfile'
>
> without sparse checkout or sparse index.  It is unclear what value
> we get out of having this test here.

Thanks for pointing out the error. HEAD:testfile is useless for the test here.

-----------------------------------
Thanks
Shuqi

Re: [PATCH v2 1/2] t1092: add tests for `git diff-files`

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

>> We create a new directory that is outside the cone, with or without
>> using the sparse-index feature.  We know we are violating the cone,
>> and have to override the safety with the "--sparse" option.  OK.
>>
>> What output do we expect out of "git add" to match in the two cases?
>>
>> > +     #file present on-disk without modifications
>> > +     test_sparse_match git diff-files &&
>> > +     test_sparse_match git diff-files newdirectory/testfile &&
>>
>> As "diff-files" is about comparing between the index and the working
>> tree, the new path should not appear in the output when the sparse
>> checkout feature with or without the sparse-index feature is NOT in
>> use.  Does the picture get different when we are sparse?  IOW, would
>> we notice that we now have newdirectory/testfile that is supposed to
>> be missing in the index and show that in the output?
>
> I'm a bit caught up here.
> Do you mean I need to add a test for "git add" also?

Not really.  The above two tests are happy with _any_ output coming
out of "git diff-files" (and "git diff-files nd/tf") as long as they
match between sparse checkouts, one of which uses and the other does
not use the sparse index feature.  I was wondering if we want to be
a bit stricter than that.  Thinks like "not only the two output must
match, they both must be empty".

[PATCH v3 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Changes since v2:

1. According to Documentation/CodingGuidelines

 write 'echo test >"$1"'
 instead of 'echo test> $1'

2. Add a SP after "#" to make sentence more readable.

3. When file present on-disk without modifications, add
test to make sure not only the two output must
match, they both must be empty.

4. Remove the useless test for HEAD:testfile.

Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  4 ++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 53 ++++++++++++++++++++++++
 3 files changed, 59 insertions(+)


base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
-- 
2.39.0

[PATCH v3 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin
with the sparse index feature, add tests to
t1092-sparse-checkout-compatibility.sh to ensure it currently works
with sparse-checkout and will still work with sparse index
after that integration.

When adding tests against a sparse-checkout
definition, we test two modes: all changes are
within the sparse-checkout cone and some changes are outside
the sparse-checkout cone.

In order to have staged changes outside of
the sparse-checkout cone, create a 'newdirectory/testfile' and
add it to the index, while leaving it outside of
the sparse-checkout definition.Test 'newdirectory/testfile'
being present on-disk without modifications, then change content inside
'newdirectory/testfile' in order to test 'newdirectory/testfile'
being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 40 ++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..bdf3cf25d4 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,44 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files  &&
+	test_all_match git diff-files deep/a 
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# add file to the index but outside of cone
+	run_on_sparse mkdir newdirectory &&
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git add --sparse newdirectory/testfile &&
+
+	# file present on-disk without modifications
+	test_sparse_match git diff-files &&
+	! test_file_not_empty sparse-checkout-out &&
+	! test_file_not_empty sparse-index-out &&
+	test_sparse_match git diff-files newdirectory/testfile &&
+	! test_file_not_empty sparse-checkout-out &&
+	! test_file_not_empty sparse-index-out &&
+
+	# file present on-disk with modifications
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git diff-files &&
+	test_sparse_match git diff-files newdirectory/testfile &&
+	test_file_not_empty sparse-checkout-out
+'
+
 test_done
-- 
2.39.0

[PATCH v3 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Remove full index requirement for `git diff-files`
and test to ensure the index is not expanded in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  4 ++++
 t/perf/p2000-sparse-operations.sh        |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 13 +++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..360464e6ef 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 3242cfe91a..82751f2ca3 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all
 test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
 test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
 test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index bdf3cf25d4..bc26c2b82a 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2095,4 +2095,17 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
 	test_file_not_empty sparse-checkout-out
 '
 
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files  &&
+	ensure_not_expanded diff-files deep/a 
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v3 1/2] t1092: add tests for `git diff-files`

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> +	! test_file_not_empty sparse-checkout-out &&

If you looked at existing uses of this test helper, you probably
wouldn't have written this.

    $ git grep -e test_file_not_empty t/t[0-9]\*.sh | wc -l
    34
    $ git grep -e '! test_file_not_empty' t/t[0-9]\*.sh | wc -l
    0

This is because test_file_not_empty is designed to fail loudly with
a complaint when the given file is empty.  Its implementation reads
like so:

        # Check if the file exists and has a size greater than zero
        test_file_not_empty () {
                test "$#" = 2 && BUG "2 param"
                if ! test -s "$1"
                then
                        echo "'$1' is not a non-empty file."
                        false
                fi
        }

In the successful case in your test, you expect the file to be empty
(I didn't check if it should be empty or not---I am just taking your
word for it).  It means that the "! test_file_not_empty" is expected
to keep complaining that it is NOT a non-empty file.

Not very nice, no?

Perhaps test_must_be_empty is what you wanted to use.

> +	! test_file_not_empty sparse-index-out &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	! test_file_not_empty sparse-checkout-out &&
> +	! test_file_not_empty sparse-index-out &&
> +
> +	# file present on-disk with modifications
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git diff-files &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_file_not_empty sparse-checkout-out

Now, are we happy if the file is not empty and has any garbage in
it?  Don't we know what the list of different paths are and what the
common output between the two should look like?

In general "as long as it is not empty, any garbage is fine" is a
poor primitive to use in tests, unless (1) we are testing output
that is deliberately designed to be unstable, or (2) we know the
program that produces output will always show an empty result when
it fails in any way.

> +'
> +
>  test_done

Thanks.

[PATCH v4 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Changes since v3:

1.Use 'test_must_be_empty' instead of '! test_file_not_empty'

2.remove useless 'test_file_not_empty sparse-checkout-out' 
in 'file present on-disk with modifications'

Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  4 ++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 52 ++++++++++++++++++++++++
 3 files changed, 58 insertions(+)


base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
-- 
2.39.0

[PATCH v4 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin
with the sparse index feature, add tests to
t1092-sparse-checkout-compatibility.sh to ensure it currently works
with sparse-checkout and will still work with sparse index
after that integration.

When adding tests against a sparse-checkout
definition, we test two modes: all changes are
within the sparse-checkout cone and some changes are outside
the sparse-checkout cone.

In order to have staged changes outside of
the sparse-checkout cone, create a 'newdirectory/testfile' and
add it to the index, while leaving it outside of
the sparse-checkout definition.Test 'newdirectory/testfile'
being present on-disk without modifications, then change content inside
'newdirectory/testfile' in order to test 'newdirectory/testfile'
being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 39 ++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..18d3b4f313 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,43 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files  &&
+	test_all_match git diff-files deep/a 
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# add file to the index but outside of cone
+	run_on_sparse mkdir newdirectory &&
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git add --sparse newdirectory/testfile &&
+
+	# file present on-disk without modifications
+	test_sparse_match git diff-files &&
+	test_must_be_empty sparse-checkout-out &&
+	test_must_be_empty sparse-index-out &&
+	test_sparse_match git diff-files newdirectory/testfile &&
+	test_must_be_empty sparse-checkout-out &&
+	test_must_be_empty sparse-index-out &&
+
+	# file present on-disk with modifications
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git diff-files &&
+	test_sparse_match git diff-files newdirectory/testfile 
+'
+
 test_done
-- 
2.39.0

[PATCH v4 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Remove full index requirement for `git diff-files`
and test to ensure the index is not expanded in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  4 ++++
 t/perf/p2000-sparse-operations.sh        |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 13 +++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..360464e6ef 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 3242cfe91a..82751f2ca3 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all
 test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
 test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
 test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 18d3b4f313..7cc6287627 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2094,4 +2094,17 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
 	test_sparse_match git diff-files newdirectory/testfile 
 '
 
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files  &&
+	ensure_not_expanded diff-files deep/a 
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v4 1/2] t1092: add tests for `git diff-files`

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> +	run_on_all ../edit-contents deep/a &&
> +
> +	test_all_match git diff-files  &&

An extra space on this line.

> +	test_all_match git diff-files deep/a 

And on this line.

No need to resend only to correct the above two, but if you are
going to reroll to fix something else, please make sure fixing
them.

> +'
> +
> +test_expect_success 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	# add file to the index but outside of cone
> +	run_on_sparse mkdir newdirectory &&
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git add --sparse newdirectory/testfile &&
> +
> +	# file present on-disk without modifications
> +	test_sparse_match git diff-files &&
> +	test_must_be_empty sparse-checkout-out &&
> +	test_must_be_empty sparse-index-out &&

As output from checkout and index are known to be identical (that is
one of the things that test_sparse_match does), I do not think there
is much point checking -out from both sides.

If we know "diff-files" invocation above should never send anything
to the standard error, then checking that sparse-checkout-err is
empty may have value, though.

> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_must_be_empty sparse-checkout-out &&
> +	test_must_be_empty sparse-index-out &&

Ditto.

> +	# file present on-disk with modifications
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git diff-files &&
> +	test_sparse_match git diff-files newdirectory/testfile 

We do not care what the actual output is in this case?

> +'
> +
>  test_done

Thanks.

Re: [PATCH v4 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Hi Junio

On Thu, Mar 9, 2023 at 12:20 PM Junio C Hamano <gitster@pobox.com> wrote:

>
> > +     run_on_all ../edit-contents deep/a &&
> > +
> > +     test_all_match git diff-files  &&
>
> An extra space on this line.
>
> > +     test_all_match git diff-files deep/a
>
> And on this line.

Will do !


> As output from checkout and index are known to be identical (that is
> one of the things that test_sparse_match does), I do not think there
> is much point checking -out from both sides.
>
> If we know "diff-files" invocation above should never send anything
> to the standard error, then checking that sparse-checkout-err is
> empty may have value, though.

Agree!

> > +     # file present on-disk with modifications
> > +     run_on_sparse ../edit-contents newdirectory/testfile &&
> > +     test_sparse_match git diff-files &&
> > +     test_sparse_match git diff-files newdirectory/testfile
>
> We do not care what the actual output is in this case?

I wonder if the method below is good  to test the actual output for '
file present on-disk with modifications' :

    cat >expect  <<-EOF &&
    :100644 100644 8e27be7d6154a1f68ea9160ef0e18691d20560dc
0000000000000000000000000000000000000000 M newdirectory/testfile
    EOF

     # file present on-disk with modifications
     run_on_sparse ../edit-contents newdirectory/testfile &&
     test_sparse_match git diff-files &&
     test_cmp expect sparse-checkout-out &&
     test_sparse_match git diff-files newdirectory/testfile &&
     test_cmp expect sparse-checkout-out

-------------------
Thanks,
Shuqi

Re: [PATCH v4 1/2] t1092: add tests for `git diff-files`

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> I wonder if the method below is good  to test the actual output for '
> file present on-disk with modifications' :
>
>     cat >expect  <<-EOF &&
>     :100644 100644 8e27be7d6154a1f68ea9160ef0e18691d20560dc
> 0000000000000000000000000000000000000000 M newdirectory/testfile
>     EOF

Hardcoding 8e27be assumes we only support SHA-1 but there is a CI
test job that runs everything in SHA-256 mode, so it is likely
to break if you wrote it like so.  Something along the lines of ...

	FN=newdirectory/testfile &&
	OID=$(git hash-object $FN) &&
	ZERO=$(test_oid zero) &&
	echo ":100644 100644 $OID $ZERO M new $FN" >expect

... may have a better chance to be correct, but I didn't test ;-)

[PATCH v5 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Changes since v4:

1. In 'diff-files with pathspec inside sparse definition' '

Add some extra space to make test more readable.

2.In 'diff-files with pathspec outside sparse definition' '

(1)
Remove redundant tests since output from checkout and index 
are known to be identical. 

(2)
Add test to check 
sparse-checkout-err (sparse-index-err) is empty.

(3)
Add test to test the actual output for
'file present on-disk with modifications'.




Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  4 ++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 61 ++++++++++++++++++++++++
 3 files changed, 67 insertions(+)


base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
-- 
2.39.0

[PATCH v5 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin
with the sparse index feature, add tests to
t1092-sparse-checkout-compatibility.sh to ensure it currently works
with sparse-checkout and will still work with sparse index
after that integration.

When adding tests against a sparse-checkout
definition, we test two modes: all changes are
within the sparse-checkout cone and some changes are outside
the sparse-checkout cone.

In order to have staged changes outside of
the sparse-checkout cone, create a 'newdirectory/testfile' and
add it to the index, while leaving it outside of
the sparse-checkout definition.Test 'newdirectory/testfile'
being present on-disk without modifications, then change content inside
'newdirectory/testfile' in order to test 'newdirectory/testfile'
being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 48 ++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..9b71d7f5f9 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,52 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files &&
+
+	test_all_match git diff-files deep/a 
+
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# add file to the index but outside of cone
+	run_on_sparse mkdir newdirectory &&
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git add --sparse newdirectory/testfile &&
+
+	# file present on-disk without modifications
+	test_sparse_match git diff-files &&
+	test_must_be_empty sparse-checkout-out &&
+	test_must_be_empty sparse-checkout-err &&
+	test_sparse_match git diff-files newdirectory/testfile &&
+	test_must_be_empty sparse-checkout-out &&
+	test_must_be_empty sparse-checkout-err &&
+
+	# file present on-disk with modifications
+	FN=newdirectory/testfile &&
+	OID=$(git -C sparse-checkout hash-object $FN) &&
+	ZERO=$(test_oid zero) &&
+	echo ":100644 100644 $OID $ZERO M	$FN" >expect &&
+
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git diff-files &&
+	test_cmp expect sparse-checkout-out &&
+	test_sparse_match git diff-files newdirectory/testfile &&
+	test_cmp expect sparse-checkout-out
+'
+
 test_done
-- 
2.39.0

[PATCH v5 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Remove full index requirement for `git diff-files`
and test to ensure the index is not expanded in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  4 ++++
 t/perf/p2000-sparse-operations.sh        |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 13 +++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..360464e6ef 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 3242cfe91a..82751f2ca3 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all
 test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
 test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
 test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 9b71d7f5f9..4f582164a3 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2103,4 +2103,17 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
 	test_cmp expect sparse-checkout-out
 '
 
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files  &&
+	ensure_not_expanded diff-files deep/a 
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v5 1/2] t1092: add tests for `git diff-files`

[Victoria Dye]

Shuqi Liang wrote:

Hi Shuqi! 

Apologies for taking so long to review; thanks for working on this.

> Before integrating the 'git diff-files' builtin
> with the sparse index feature, add tests to
> t1092-sparse-checkout-compatibility.sh to ensure it currently works
> with sparse-checkout and will still work with sparse index
> after that integration.
> 
> When adding tests against a sparse-checkout
> definition, we test two modes: all changes are
> within the sparse-checkout cone and some changes are outside
> the sparse-checkout cone.
> 
> In order to have staged changes outside of
> the sparse-checkout cone, create a 'newdirectory/testfile' and
> add it to the index, while leaving it outside of
> the sparse-checkout definition.Test 'newdirectory/testfile'

nit: missing space after "definition."

> being present on-disk without modifications, then change content inside
> 'newdirectory/testfile' in order to test 'newdirectory/testfile'
> being present on-disk with modifications.

Generally, you don't need to create a new file to get to this state, and I'd
personally advise against it. Personally, I find that adding a new file can
make the test more cumbersome than is necessary. I'll see if I can suggest
an alternative later on.

> 
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>  t/t1092-sparse-checkout-compatibility.sh | 48 ++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 801919009e..9b71d7f5f9 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2055,4 +2055,52 @@ test_expect_success 'grep sparse directory within submodules' '
>  	test_cmp actual expect
>  '
>  
> +test_expect_success 'diff-files with pathspec inside sparse definition' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	test_all_match git diff-files &&
> +
> +	test_all_match git diff-files deep/a 
> +

I'd be interested in seeing an additional test case for a pathspec with
wildcards or other "magic" [1], e.g. 'git diff-files "deep/*"'. In past
sparse index integrations, there has occasionally been a need for special
handling of those types of pathspecs [2][3], so it would be good for the
test to cover cases like that.

Otherwise, this test looks good.

[1] https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefpathspecapathspec
[2] https://lore.kernel.org/git/822d7344587f698e73abba1ca726c3a905f7b403.1638201164.git.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/20220807041335.1790658-3-shaoxuan.yuan02@gmail.com/

> +'
> +
> +test_expect_success 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&

Before messing with modified files on disk, it'd be nice to show a
"baseline" of correct behavior when a pathspec points to out-of-cone files,
e.g. 'test_all_match git diff-files folder2/a'.

> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	# add file to the index but outside of cone
> +	run_on_sparse mkdir newdirectory &&
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git add --sparse newdirectory/testfile &&
> +
> +	# file present on-disk without modifications

From the comment you've added here, it looks like the state you want to test
is "file outside sparse checkout definition exists on disk". However, since
one of the goals of this test is to verify sparse index behavior once its
integrated with the 'diff-files' command, whether the "outside of sparse-
checkout definition" file belongs to a sparse directory entry is an
important (and fairly nuanced) factor to consider.

The main difference between a "regular" sparse-checkout and a sparse
index-enabled sparse-checkout is the existence of "sparse directory"
entries: index entries with 'SKIP_WORKTREE' that represent directories and
their contents. In the context of these tests, the thing we really want to
verify is that the sparse index-enabled case produces the same results as
the full index when an operation needs to get some information out of a
sparse directory. 

Coming back to this test, the 'newdirectory/testfile' you create, while
outside the sparse-checkout definition, never actually belongs to a sparse
directory because 'newdirectory' is never collapsed - in fact, I don't
think it even gets the SKIP_WORKTREE bit in the index. To ensure you have a
sparse directory & SKIP_WORKTREE, you'd need to run 'git sparse-checkout
reapply' after removing 'newdirectory/testfile' from disk - which doesn't
help if you want to test what happens when the file exists on disk!

In fact, because of built-in safeguards around on-disk files and
sparse-checkout, there isn't really a way in Git to materialize a file
without also expanding its sparse directory and removing SKIP_WORKTREE. If
you want to preserve a sparse directory, you should write the contents of an
existing file inside that sparse directory to disk manually:

	run_on_sparse mkdir folder1 &&
	run_on_sparse cp a folder1/a &&  # `folder1/a` is identical to `a` in the base commit

Git's index will not be touched by doing this, meaning the next command you
invoke (in this case, 'diff-files') will need to reconcile the file on disk
with what it sees in the index.

> +	test_sparse_match git diff-files &&
> +	test_must_be_empty sparse-checkout-out &&
> +	test_must_be_empty sparse-checkout-err &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_must_be_empty sparse-checkout-out &&
> +	test_must_be_empty sparse-checkout-err &&

These checks should be 'test_all_match' rather than 'test_sparse_match'.
Since (through other tests) we're confident that 'git diff-files' on an
unmodified, non-sparse-checkout repository will give us an empty result, you
wouldn't need the additional 'test_must_be_empty' checks.

A bit of a "spoiler": when I tested this out locally, I found that the diff
was *not* empty for the sparse-checkout cases, until I ran 'git status'
(which strips the 'SKIP_WORKTREE' bit from the file and writes it to the
index). That's not the desired behavior, so there's a bug in the
sparse-checkout logic used in 'diff-files' that needs to be fixed (my first
guess would be that 'clear_skip_worktree_from_present_files()' is not being
applied when the index is read).

If you'd like any help investigating this or get stuck, please let me know -
I'd be happy to assist!

> +
> +	# file present on-disk with modifications
> +	FN=newdirectory/testfile &&
> +	OID=$(git -C sparse-checkout hash-object $FN) &&
> +	ZERO=$(test_oid zero) &&
> +	echo ":100644 100644 $OID $ZERO M	$FN" >expect &&
> +
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git diff-files &&
> +	test_cmp expect sparse-checkout-out &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_cmp expect sparse-checkout-out

Similarly, you should use 'run_on_all' to modify the file & 'test_all_match'
to verify that they all match here. It would demonstrate that we don't
expect any "special" behavior from sparse-checkout, meaning you can probably
avoid checking the result verbatim. 

Finally, as with the earlier test, it'd be nice to show that the result is
the same with a wildcard pathspec, e.g. 'git diff-files "folder*/a"'.

> +'
> +
>  test_done

Re: [PATCH v5 2/2] diff-files: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> Remove full index requirement for `git diff-files`
> and test to ensure the index is not expanded in `git diff-files`.
> 
> The `p2000` tests demonstrate a ~96% execution time reduction for 'git
> diff-files' and a ~97% execution time reduction for 'git diff-files'
> for a file using a sparse index:
> 
> Test                                           before  after
> -----------------------------------------------------------------
> 2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
> 2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
> 2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
> 2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
> 2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
> 2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
> 2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
> 2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

These are great performance results!

> 
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>  builtin/diff-files.c                     |  4 ++++
>  t/perf/p2000-sparse-operations.sh        |  2 ++
>  t/t1092-sparse-checkout-compatibility.sh | 13 +++++++++++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/builtin/diff-files.c b/builtin/diff-files.c
> index dc991f753b..360464e6ef 100644
> --- a/builtin/diff-files.c
> +++ b/builtin/diff-files.c
> @@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
>  		usage(diff_files_usage);
>  
>  	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> +
> +	prepare_repo_settings(the_repository);
> +	the_repository->settings.command_requires_full_index = 0;

Looks good.

> +
>  	repo_init_revisions(the_repository, &rev, prefix);
>  	rev.abbrev = 0;
>  
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index 3242cfe91a..82751f2ca3 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all
>  test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
>  test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
>  test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
> +test_perf_on_all git diff-files
> +test_perf_on_all git diff-files $SPARSE_CONE/a
>  
>  test_done
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 9b71d7f5f9..4f582164a3 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2103,4 +2103,17 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
>  	test_cmp expect sparse-checkout-out
>  '
>  
> +test_expect_success 'sparse index is not expanded: diff-files' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	ensure_not_expanded diff-files  &&
> +	ensure_not_expanded diff-files deep/a 

IIRC, in many cases, the internal diff machinery won't expand a sparse index
even if the pathspec matches files outside the sparse-checkout definition.
Does 'ensure_not_expanded diff-files folder1/a' work? What about
'ensure_not_expanded diff-files "*a"'?

> +'
> +
>  test_done

[RFC PATCH v6 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Did not fix the logic of spare-checkout yet. Leave the spare diff-files 
with pathspec outside sparse definition as 'test_expect_failure' now.
but will fix soon.

Changes since v5:

1. Add space after "definition."

2. Add test case for a pathspec with wildcards or other "magic"

3. Before messing with modified files on disk, add a "baseline" 
of correct behavior when a pathspec points to out-of-cone files.

4. Write the contents of an
existing file inside that sparse directory to disk manually

5. Use 'test_all_match' rather than 'test_sparse_match'. 
wouldn't need the additional 'test_must_be_empty' checks.


Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  8 +++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 73 ++++++++++++++++++++++++
 3 files changed, 83 insertions(+)


base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
-- 
2.39.0

[PATCH v6 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin
with the sparse index feature, add tests to
t1092-sparse-checkout-compatibility.sh to ensure it currently works
with sparse-checkout and will still work with sparse index
after that integration.

When adding tests against a sparse-checkout
definition, we test two modes: all changes are
within the sparse-checkout cone and some changes are outside
the sparse-checkout cone.

In order to have staged changes outside of
the sparse-checkout cone, make a directory called 'folder1' and
copy `a` into 'folder1/a'. 'folder1/a' is identical to `a` in the base
commit. These make 'folder1/a' in the index, while leaving it outside of
the sparse-checkout definition. Test 'folder1/a'being present on-disk
without modifications, then change content inside 'folder1/a' in order
to test 'folder1/a' being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 42 ++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..c1329e2f16 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,46 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files &&
+
+	test_all_match git diff-files deep/a && 
+
+	# test wildcard
+	test_all_match git diff-files deep/*
+'
+
+test_expect_failure 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	test_sparse_match test_must_fail git diff-files folder2/a &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# Add file to the index but outside of cone for sparse-checkout cases.
+	# Add file to the index without sparse-checkout cases to ensure all have 
+	# same output.
+	run_on_all mkdir folder1 &&
+	run_on_all cp a folder1/a &&
+
+	# file present on-disk without modifications
+	test_all_match git diff-files &&
+	test_all_match git diff-files folder1/a &&
+
+	# file present on-disk with modifications
+	run_on_all ../edit-contents folder1/a &&
+	test_all_match git diff-files &&
+	test_all_match git diff-files folder1/a
+'
+
 test_done
-- 
2.39.0

[PATCH v6 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Originally, diff-files a pathspec that is out-of-cone in a sparse-index
environment, Git dies with "pathspec '<x>' did not match any files",
mainly because it does not expand the index so nothing is matched.
Expand the index when the <pathspec> needs an expanded index, i.e. the
<pathspec> contains wildcard that may need a full-index or the
<pathspec> is simply outside of sparse-checkout definition.

Remove full index requirement for `git diff-files`
and add test to ensure the index only expanded when necessary
in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  8 ++++++
 t/perf/p2000-sparse-operations.sh        |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 31 ++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..d88875aa07 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
@@ -80,6 +84,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		result = -1;
 		goto cleanup;
 	}
+
+	if (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))
+		ensure_full_index(the_repository->index);
+		
 	result = run_diff_files(&rev, options);
 	result = diff_result_code(&rev.diffopt, result);
 cleanup:
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 3242cfe91a..82751f2ca3 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all
 test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
 test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
 test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index c1329e2f16..6cbbc51a16 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2097,4 +2097,35 @@ test_expect_failure 'diff-files with pathspec outside sparse definition' '
 	test_all_match git diff-files folder1/a
 '
 
+test_expect_success 'diff-files pathspec expands index when necessary' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+	
+	# pathspec that should expand index
+	! ensure_not_expanded diff-files "*/a" &&
+	test_must_be_empty sparse-index-err &&
+
+	! ensure_not_expanded diff-files "**a" &&
+	test_must_be_empty sparse-index-err
+'
+
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files &&
+	ensure_not_expanded diff-files deep/a &&
+	ensure_not_expanded diff-files deep/*
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v5 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

On Fri, Mar 10, 2023 at 1:23 PM Victoria Dye <vdye@github.com> wrote:

 Hi Victoria!

Sorry for the late reply! I was having midterm exams.

Thank you for the feedback on the patch .
I appreciate your time giving me the advice for improvement.

> > In order to have staged changes outside of
> > the sparse-checkout cone, create a 'newdirectory/testfile' and
> > add it to the index, while leaving it outside of
> > the sparse-checkout definition.Test 'newdirectory/testfile'

> nit: missing space after "definition."

Will do!

> I'd be interested in seeing an additional test case for a pathspec with
> wildcards or other "magic" [1], e.g. 'git diff-files "deep/*"'. In past
> sparse index integrations, there has occasionally been a need for special
> handling of those types of pathspecs [2][3], so it would be good for the
> test to cover cases like that.

Will do!

> From the comment you've added here, it looks like the state you want to test
> is "file outside sparse checkout definition exists on disk". However, since
> one of the goals of this test is to verify sparse index behavior once its
> integrated with the 'diff-files' command, whether the "outside of sparse-
> checkout definition" file belongs to a sparse directory entry is an
> important (and fairly nuanced) factor to consider.
>
> The main difference between a "regular" sparse-checkout and a sparse
> index-enabled sparse-checkout is the existence of "sparse directory"
> entries: index entries with 'SKIP_WORKTREE' that represent directories and
> their contents. In the context of these tests, the thing we really want to
> verify is that the sparse index-enabled case produces the same results as
> the full index when an operation needs to get some information out of a
> sparse directory.
>
> Coming back to this test, the 'newdirectory/testfile' you create, while
> outside the sparse-checkout definition, never actually belongs to a sparse
> directory because 'newdirectory' is never collapsed - in fact, I don't
> think it even gets the SKIP_WORKTREE bit in the index. To ensure you have a
> sparse directory & SKIP_WORKTREE, you'd need to run 'git sparse-checkout
> reapply' after removing 'newdirectory/testfile' from disk - which doesn't
> help if you want to test what happens when the file exists on disk!

Thanks for the explanation here! I learn a lot.

> In fact, because of built-in safeguards around on-disk files and
> sparse-checkout, there isn't really a way in Git to materialize a file
> without also expanding its sparse directory and removing SKIP_WORKTREE. If
> you want to preserve a sparse directory, you should write the contents of an
> existing file inside that sparse directory to disk manually:
>
>         run_on_sparse mkdir folder1 &&
>         run_on_sparse cp a folder1/a &&  # `folder1/a` is identical to `a` in the base commit
>
> Git's index will not be touched by doing this, meaning the next command you
> invoke (in this case, 'diff-files') will need to reconcile the file on disk
> with what it sees in the index.
>
> > +     test_sparse_match git diff-files &&
> > +     test_must_be_empty sparse-checkout-out &&
> > +     test_must_be_empty sparse-checkout-err &&
> > +     test_sparse_match git diff-files newdirectory/testfile &&
> > +     test_must_be_empty sparse-checkout-out &&
> > +     test_must_be_empty sparse-checkout-err &&
>
> These checks should be 'test_all_match' rather than 'test_sparse_match'.
> Since (through other tests) we're confident that 'git diff-files' on an
> unmodified, non-sparse-checkout repository will give us an empty result, you
> wouldn't need the additional 'test_must_be_empty' checks.
>
> A bit of a "spoiler": when I tested this out locally, I found that the diff
> was *not* empty for the sparse-checkout cases, until I ran 'git status'
> (which strips the 'SKIP_WORKTREE' bit from the file and writes it to the
> index). That's not the desired behavior, so there's a bug in the
> sparse-checkout logic used in 'diff-files' that needs to be fixed (my first
> guess would be that 'clear_skip_worktree_from_present_files()' is not being
> applied when the index is read).

Yeah. After I use

 run_on_sparse mkdir folder1 &&
 run_on_sparse cp a folder1/a &&  # `folder1/a` is identical to `a` in
the base commit

 diff was *not* empty for the sparse-checkout cases

 when I look into builtin/diff-files.c
'repo_read_index_preload' call 'repo_read_index' which call
 'clear_skip_worktree_from_present_files()' to apply when the index is read.

I got stuck in here and do not have the idea to investigate it. Any
tips would be helpful!


> > +     run_on_sparse ../edit-contents newdirectory/testfile &&
> > +     test_sparse_match git diff-files &&
> > +     test_cmp expect sparse-checkout-out &&
> > +     test_sparse_match git diff-files newdirectory/testfile &&
> > +     test_cmp expect sparse-checkout-out
>
> Similarly, you should use 'run_on_all' to modify the file & 'test_all_match'
> to verify that they all match here. It would demonstrate that we don't
> expect any "special" behavior from sparse-checkout, meaning you can probably
> avoid checking the result verbatim.

> Finally, as with the earlier test, it'd be nice to show that the result is
> the same with a wildcard pathspec, e.g. 'git diff-files "folder*/a"'.

Will do!
----------------------------------------------------------------------------
Thanks
Shuqi

Re: [RFC PATCH v6 0/2] diff-files: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> Did not fix the logic of spare-checkout yet. Leave the spare diff-files 
> with pathspec outside sparse definition as 'test_expect_failure' now.
> but will fix soon.
> 
> Changes since v5:
> 
> 1. Add space after "definition."
> 
> 2. Add test case for a pathspec with wildcards or other "magic"
> 
> 3. Before messing with modified files on disk, add a "baseline" 
> of correct behavior when a pathspec points to out-of-cone files.
> 
> 4. Write the contents of an
> existing file inside that sparse directory to disk manually
> 
> 5. Use 'test_all_match' rather than 'test_sparse_match'. 
> wouldn't need the additional 'test_must_be_empty' checks.
> 
> 
> Shuqi Liang (2):
>   t1092: add tests for `git diff-files`
>   diff-files: integrate with sparse index
> 
>  builtin/diff-files.c                     |  8 +++
>  t/perf/p2000-sparse-operations.sh        |  2 +
>  t/t1092-sparse-checkout-compatibility.sh | 73 ++++++++++++++++++++++++
>  3 files changed, 83 insertions(+)

[forgot to Reply-All - Shuqi, sorry for the duplicate email!]

In future iterations, please also include a range-diff in your version
iterations. The description of changes is useful, but the range-diff
provides a much more detailed and comprehensive summary of the changes
(making it exceptionally helpful for reviewers). I *think* you can just add
the '--range-diff <previous iteration>' option to 'git format-patch' (see
MyFirstContribution [1] for more detailed instructions).

[1] https://git-scm.com/docs/MyFirstContribution#v2-git-send-email

For anyone that's interested, here's the range-diff vs. v5:

1:  fb9ec0901c ! 1:  14bbcf41e0 t1092: add tests for `git diff-files`
    @@ Commit message
         the sparse-checkout cone.
     
         In order to have staged changes outside of
    -    the sparse-checkout cone, create a 'newdirectory/testfile' and
    -    add it to the index, while leaving it outside of
    -    the sparse-checkout definition.Test 'newdirectory/testfile'
    -    being present on-disk without modifications, then change content inside
    -    'newdirectory/testfile' in order to test 'newdirectory/testfile'
    -    being present on-disk with modifications.
    +    the sparse-checkout cone, make a directory called 'folder1' and
    +    copy `a` into 'folder1/a'. 'folder1/a' is identical to `a` in the base
    +    commit. These make 'folder1/a' in the index, while leaving it outside of
    +    the sparse-checkout definition. Test 'folder1/a'being present on-disk
    +    without modifications, then change content inside 'folder1/a' in order
    +    to test 'folder1/a' being present on-disk with modifications.
     
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc
     +
     +	test_all_match git diff-files &&
     +
    -+	test_all_match git diff-files deep/a 
    ++	test_all_match git diff-files deep/a && 
     +
    ++	# test wildcard
    ++	test_all_match git diff-files deep/*
     +'
     +
    -+test_expect_success 'diff-files with pathspec outside sparse definition' '
    ++test_expect_failure 'diff-files with pathspec outside sparse definition' '
     +	init_repos &&
     +
    ++	test_sparse_match test_must_fail git diff-files folder2/a &&
    ++
     +	write_script edit-contents <<-\EOF &&
     +	echo text >>"$1"
     +	EOF
     +
    -+	# add file to the index but outside of cone
    -+	run_on_sparse mkdir newdirectory &&
    -+	run_on_sparse ../edit-contents newdirectory/testfile &&
    -+	test_sparse_match git add --sparse newdirectory/testfile &&
    ++	# Add file to the index but outside of cone for sparse-checkout cases.
    ++	# Add file to the index without sparse-checkout cases to ensure all have 
    ++	# same output.
    ++	run_on_all mkdir folder1 &&
    ++	run_on_all cp a folder1/a &&
     +
     +	# file present on-disk without modifications
    -+	test_sparse_match git diff-files &&
    -+	test_must_be_empty sparse-checkout-out &&
    -+	test_must_be_empty sparse-checkout-err &&
    -+	test_sparse_match git diff-files newdirectory/testfile &&
    -+	test_must_be_empty sparse-checkout-out &&
    -+	test_must_be_empty sparse-checkout-err &&
    ++	test_all_match git diff-files &&
    ++	test_all_match git diff-files folder1/a &&
     +
     +	# file present on-disk with modifications
    -+	FN=newdirectory/testfile &&
    -+	OID=$(git -C sparse-checkout hash-object $FN) &&
    -+	ZERO=$(test_oid zero) &&
    -+	echo ":100644 100644 $OID $ZERO M	$FN" >expect &&
    -+
    -+	run_on_sparse ../edit-contents newdirectory/testfile &&
    -+	test_sparse_match git diff-files &&
    -+	test_cmp expect sparse-checkout-out &&
    -+	test_sparse_match git diff-files newdirectory/testfile &&
    -+	test_cmp expect sparse-checkout-out
    ++	run_on_all ../edit-contents folder1/a &&
    ++	test_all_match git diff-files &&
    ++	test_all_match git diff-files folder1/a
     +'
     +
      test_done
2:  04e24f7db5 ! 2:  734cd24f0c diff-files: integrate with sparse index
    @@ Metadata
      ## Commit message ##
         diff-files: integrate with sparse index
     
    +    Originally, diff-files a pathspec that is out-of-cone in a sparse-index
    +    environment, Git dies with "pathspec '<x>' did not match any files",
    +    mainly because it does not expand the index so nothing is matched.
    +    Expand the index when the <pathspec> needs an expanded index, i.e. the
    +    <pathspec> contains wildcard that may need a full-index or the
    +    <pathspec> is simply outside of sparse-checkout definition.
    +
         Remove full index requirement for `git diff-files`
    -    and test to ensure the index is not expanded in `git diff-files`.
    +    and add test to ensure the index only expanded when necessary
    +    in `git diff-files`.
     
         The `p2000` tests demonstrate a ~96% execution time reduction for 'git
         diff-files' and a ~97% execution time reduction for 'git diff-files'
    @@ builtin/diff-files.c: int cmd_diff_files(int argc, const char **argv, const char
      	repo_init_revisions(the_repository, &rev, prefix);
      	rev.abbrev = 0;
      
    +@@ builtin/diff-files.c: int cmd_diff_files(int argc, const char **argv, const char *prefix)
    + 		result = -1;
    + 		goto cleanup;
    + 	}
    ++
    ++	if (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))
    ++		ensure_full_index(the_repository->index);
    ++		
    + 	result = run_diff_files(&rev, options);
    + 	result = diff_result_code(&rev.diffopt, result);
    + cleanup:
     
      ## t/perf/p2000-sparse-operations.sh ##
     @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all
    @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all
      test_done
     
      ## t/t1092-sparse-checkout-compatibility.sh ##
    -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with pathspec outside sparse definition' '
    - 	test_cmp expect sparse-checkout-out
    +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'diff-files with pathspec outside sparse definition' '
    + 	test_all_match git diff-files folder1/a
      '
      
    ++test_expect_success 'diff-files pathspec expands index when necessary' '
    ++	init_repos &&
    ++
    ++	write_script edit-contents <<-\EOF &&
    ++	echo text >>"$1"
    ++	EOF
    ++
    ++	run_on_all ../edit-contents deep/a &&
    ++	
    ++	# pathspec that should expand index
    ++	! ensure_not_expanded diff-files "*/a" &&
    ++	test_must_be_empty sparse-index-err &&
    ++
    ++	! ensure_not_expanded diff-files "**a" &&
    ++	test_must_be_empty sparse-index-err
    ++'
    ++
     +test_expect_success 'sparse index is not expanded: diff-files' '
     +	init_repos &&
     +
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with p
     +
     +	run_on_all ../edit-contents deep/a &&
     +
    -+	ensure_not_expanded diff-files  &&
    -+	ensure_not_expanded diff-files deep/a 
    ++	ensure_not_expanded diff-files &&
    ++	ensure_not_expanded diff-files deep/a &&
    ++	ensure_not_expanded diff-files deep/*
     +'
     +
      test_done

> 
> 
> base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1

Re: [PATCH v6 1/2] t1092: add tests for `git diff-files`

[Victoria Dye]

Shuqi Liang wrote:
> Before integrating the 'git diff-files' builtin
> with the sparse index feature, add tests to
> t1092-sparse-checkout-compatibility.sh to ensure it currently works
> with sparse-checkout and will still work with sparse index
> after that integration.
> 
> When adding tests against a sparse-checkout
> definition, we test two modes: all changes are
> within the sparse-checkout cone and some changes are outside
> the sparse-checkout cone.
> 
> In order to have staged changes outside of
> the sparse-checkout cone, make a directory called 'folder1' and
> copy `a` into 'folder1/a'. 'folder1/a' is identical to `a` in the base
> commit. These make 'folder1/a' in the index, while leaving it outside of
> the sparse-checkout definition. Test 'folder1/a'being present on-disk
> without modifications, then change content inside 'folder1/a' in order
> to test 'folder1/a' being present on-disk with modifications.

The word wrapping on this message (and your other commits/cover letter) is a
bit strange. By convention, it should be consistently wrapped to 72 columns
per line. Most text editors have some way of configuring that so you don't
need to do it manually.

> 
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>  t/t1092-sparse-checkout-compatibility.sh | 42 ++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 801919009e..c1329e2f16 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2055,4 +2055,46 @@ test_expect_success 'grep sparse directory within submodules' '
>  	test_cmp actual expect
>  '
>  
> +test_expect_success 'diff-files with pathspec inside sparse definition' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	test_all_match git diff-files &&
> +
> +	test_all_match git diff-files deep/a && 
> +
> +	# test wildcard
> +	test_all_match git diff-files deep/*
> +'
> +
> +test_expect_failure 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&
> +
> +	test_sparse_match test_must_fail git diff-files folder2/a &&

Makes sense. In a sparse-checkout, folder2/a isn't in the working tree, so 
'diff-files' (which compares working tree vs. index) doesn't really apply.

> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	# Add file to the index but outside of cone for sparse-checkout cases.
> +	# Add file to the index without sparse-checkout cases to ensure all have 
> +	# same output.
> +	run_on_all mkdir folder1 &&

The test is failing because of this line. It should be 'mkdir -p folder1';
as you have it now, the command fails because 'folder1' already exists in
'full-checkout'. Alternatively, you could use 'run_on_sparse mkdir folder1',
but using '-p' seems like the less fragile approach. 

For reference, the way I figured that out was to run 't1092' as follows:

	cd t && ./t1092-sparse-checkout-compatibility.sh -xvdi --run=1,82

What the options correspond to:
* -x: print the commands called in the test (equivalent of calling 'set -x'
      in the test script).
* -v: print stdout/stderr to the console.
* -d: do not remove the "trash directory" of the test.
* -i: stop test execution once it encounters a failure.
* --run: run only the specified tests (1 is the first test - 'setup' - and
         82 is 'diff-files with pathspec outside sparse definition').

> +	run_on_all cp a folder1/a &&
> +
> +	# file present on-disk without modifications
> +	test_all_match git diff-files &&
> +	test_all_match git diff-files folder1/a &&

The strange thing is, once I fixed the 'mkdir' issue in my local copy of
these patches, these 'test_all_match diff-files' calls succeeded. It turns
out that 'git diff-files' in the 'full-checkout', like in 'sparse-checkout',
reports a difference in 'folder1/a' that doesn't actually exist. So the bug
isn't in sparse-checkout as I initially assumed [1], but rather in
diff-files itself.

At this point, I'd say the diff-files bug is out-of-scope of this sparse
index integration; in your implementation, sparse-checkout and sparse index
work the same as a full checkout, it's just that the "normal" full checkout
behavior is wrong. My recommendation would be that you keep this test as-is
and, to force the failure, add a check that 'full-checkout-out' is empty.
Then, in a "NEEDSWORK" comment on the test (like the one on 'diff with
renames and conflicts') that indicates that the 'diff-files' behavior is
wrong.

I'll try to make some time this week to look into the 'diff-files' bug.
Sorry for the back-and-forth and distraction from your sparse index
integration. Other than the 'mkdir' issue, these updated tests look great!

[1] https://lore.kernel.org/git/b537d855-edb7-4f67-de08-d651868247a5@github.com/

> +
> +	# file present on-disk with modifications
> +	run_on_all ../edit-contents folder1/a &&
> +	test_all_match git diff-files &&
> +	test_all_match git diff-files folder1/a
> +'
> +
>  test_done

Re: [PATCH v6 1/2] t1092: add tests for `git diff-files`

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

> The strange thing is, once I fixed the 'mkdir' issue in my local copy of
> these patches, these 'test_all_match diff-files' calls succeeded. It turns
> out that 'git diff-files' in the 'full-checkout', like in 'sparse-checkout',
> reports a difference in 'folder1/a' that doesn't actually exist. So the bug
> isn't in sparse-checkout as I initially assumed [1], but rather in
> diff-files itself.

Is that a bug, or just a common "ah, you forgot to refresh the index"?

Re: [PATCH v6 1/2] t1092: add tests for `git diff-files`

[Victoria Dye]

Junio C Hamano wrote:
> Victoria Dye <vdye@github.com> writes:
> 
>> The strange thing is, once I fixed the 'mkdir' issue in my local copy of
>> these patches, these 'test_all_match diff-files' calls succeeded. It turns
>> out that 'git diff-files' in the 'full-checkout', like in 'sparse-checkout',
>> reports a difference in 'folder1/a' that doesn't actually exist. So the bug
>> isn't in sparse-checkout as I initially assumed [1], but rather in
>> diff-files itself.
> 
> Is that a bug, or just a common "ah, you forgot to refresh the index"?

Ah, you're right - I completely forgot about 'diff-files' not refreshing the
index (since 'diff', by default, does). The ctime is (often, but not always)
different on the copied file than what's in the index, so 'diff-files' shows
the file as "modified" if the index isn't refreshed. 

Going back to these tests, the goal is to make sure that 'diff-files' finds
the correct index entry (possibly in a sparse directory) and compares that
correctly to what's on disk. But since we want to ignore ctime differences,
we could use '--stat' (or '-p', or '--num-stat':

	run_on_all mkdir -p folder1 &&
	run_on_all cp a folder1/a &&

	# file present on-disk without modifications
	# use `--stat` to ignore file creation time differences in
	# unrefreshed index
	test_all_match git diff-files --stat &&
	test_all_match git diff-files --stat folder1/a &&

I don't think that makes the test any less comprehensive (especially since
later on in the same test, we modify the contents of 'folder1/a' and get the
expected "modified" status in 'git diff-files' without '--stat'), but it
avoids potential breakages related to inconsistency in file creation time.

Re: [PATCH v6 2/2] diff-files: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> Originally, diff-files a pathspec that is out-of-cone in a sparse-index
> environment, Git dies with "pathspec '<x>' did not match any files",
> mainly because it does not expand the index so nothing is matched.
> Expand the index when the <pathspec> needs an expanded index, i.e. the
> <pathspec> contains wildcard that may need a full-index or the
> <pathspec> is simply outside of sparse-checkout definition.

...

> +	if (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))
> +		ensure_full_index(the_repository->index);

Looks good! I'm glad you were able to use the tests to confirm that this
pathspec-based expansion was needed.

> +		
>  	result = run_diff_files(&rev, options);
>  	result = diff_result_code(&rev.diffopt, result);
>  cleanup:
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index 3242cfe91a..82751f2ca3 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all
>  test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
>  test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
>  test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
> +test_perf_on_all git diff-files
> +test_perf_on_all git diff-files $SPARSE_CONE/a
>  
>  test_done
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index c1329e2f16..6cbbc51a16 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2097,4 +2097,35 @@ test_expect_failure 'diff-files with pathspec outside sparse definition' '
>  	test_all_match git diff-files folder1/a
>  '
>  
> +test_expect_success 'diff-files pathspec expands index when necessary' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +	
> +	# pathspec that should expand index
> +	! ensure_not_expanded diff-files "*/a" &&
> +	test_must_be_empty sparse-index-err &&
> +
> +	! ensure_not_expanded diff-files "**a" &&
> +	test_must_be_empty sparse-index-err
> +'

Thanks for adding these, it's a good idea to show when the sparse index *is*
expanded in addition to when it is not. However, checking that the
'sparse-index-err' is empty won't handle silent failures, so it's probably
better to create an 'ensure_expanded' to mirror 'ensure_not_expanded'. The
two functions could share pretty much all of their code except for the last
line ('test_region ...').

> +
> +test_expect_success 'sparse index is not expanded: diff-files' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	ensure_not_expanded diff-files &&
> +	ensure_not_expanded diff-files deep/a &&
> +	ensure_not_expanded diff-files deep/*
> +'
> +
>  test_done

[PATCH v7 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Changes since v6:

1. Fix word wrap in commit message.

2. Use  'mkdir -p folder1' since full-checkout already have folder1.

3. Use `--stat` to ignore file creation time differences in unrefreshed
index.

4. In 'diff-files with pathspec outside sparse definition' add 
'git diff-files "folder*/a" to show that the result is the same with a 
wildcard pathspec.

5. Create an 'ensure_expanded' to handle silent failures.


Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  8 ++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 98 ++++++++++++++++++++++++
 3 files changed, 108 insertions(+)

Range-diff against v6:
1:  2a994e60bc ! 1:  e2dcf9921e t1092: add tests for `git diff-files`
    @@ Metadata
      ## Commit message ##
         t1092: add tests for `git diff-files`
     
    -    Before integrating the 'git diff-files' builtin
    -    with the sparse index feature, add tests to
    -    t1092-sparse-checkout-compatibility.sh to ensure it currently works
    -    with sparse-checkout and will still work with sparse index
    -    after that integration.
    +    Before integrating the 'git diff-files' builtin with the sparse index
    +    feature, add tests to t1092-sparse-checkout-compatibility.sh to ensure
    +    it currently works with sparse-checkout and will still work with sparse
    +    index after that integration.
     
    -    When adding tests against a sparse-checkout
    -    definition, we test two modes: all changes are
    -    within the sparse-checkout cone and some changes are outside
    -    the sparse-checkout cone.
    +    When adding tests against a sparse-checkout definition, we test two
    +    modes: all changes are within the sparse-checkout cone and some changes
    +    are outside the sparse-checkout cone.
     
    -    In order to have staged changes outside of
    -    the sparse-checkout cone, make a directory called 'folder1' and
    -    copy `a` into 'folder1/a'. 'folder1/a' is identical to `a` in the base
    -    commit. These make 'folder1/a' in the index, while leaving it outside of
    -    the sparse-checkout definition. Test 'folder1/a'being present on-disk
    +    In order to have staged changes outside of the sparse-checkout cone,
    +    make a directory called 'folder1' and copy `a` into 'folder1/a'.
    +    'folder1/a' is identical to `a` in the base commit. These make
    +    'folder1/a' in the index, while leaving it outside of the
    +    sparse-checkout definition. Test 'folder1/a'being present on-disk
         without modifications, then change content inside 'folder1/a' in order
         to test 'folder1/a' being present on-disk with modifications.
     
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc
     +	test_all_match git diff-files deep/*
     +'
     +
    -+test_expect_failure 'diff-files with pathspec outside sparse definition' '
    ++test_expect_success 'diff-files with pathspec outside sparse definition' '
     +	init_repos &&
     +
     +	test_sparse_match test_must_fail git diff-files folder2/a &&
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc
     +	# Add file to the index but outside of cone for sparse-checkout cases.
     +	# Add file to the index without sparse-checkout cases to ensure all have 
     +	# same output.
    -+	run_on_all mkdir folder1 &&
    ++	run_on_all mkdir -p folder1 &&
     +	run_on_all cp a folder1/a &&
     +
     +	# file present on-disk without modifications
    -+	test_all_match git diff-files &&
    -+	test_all_match git diff-files folder1/a &&
    ++	# use `--stat` to ignore file creation time differences in
    ++	# unrefreshed index
    ++	test_all_match git diff-files --stat &&
    ++	test_all_match git diff-files --stat folder1/a &&
    ++	test_all_match git diff-files --stat "folder*/a" &&
     +
     +	# file present on-disk with modifications
     +	run_on_all ../edit-contents folder1/a &&
     +	test_all_match git diff-files &&
    -+	test_all_match git diff-files folder1/a
    ++	test_all_match git diff-files folder1/a &&
    ++	test_all_match git diff-files "folder*/a" 
     +'
     +
      test_done
2:  ac730e372d ! 2:  fb8edaf583 diff-files: integrate with sparse index
    @@ Commit message
         <pathspec> contains wildcard that may need a full-index or the
         <pathspec> is simply outside of sparse-checkout definition.
     
    -    Remove full index requirement for `git diff-files`
    -    and add test to ensure the index only expanded when necessary
    -    in `git diff-files`.
    +    Remove full index requirement for `git diff-files`.Create an
    +    'ensure_expanded' to handle silent failures. Add test to
    +    ensure the index only expanded when necessary in `git diff-files`.
     
         The `p2000` tests demonstrate a ~96% execution time reduction for 'git
         diff-files' and a ~97% execution time reduction for 'git diff-files'
    @@ builtin/diff-files.c: int cmd_diff_files(int argc, const char **argv, const char
     +
     +	if (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))
     +		ensure_full_index(the_repository->index);
    -+		
    ++
      	result = run_diff_files(&rev, options);
      	result = diff_result_code(&rev.diffopt, result);
      cleanup:
    @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all
      test_done
     
      ## t/t1092-sparse-checkout-compatibility.sh ##
    -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'diff-files with pathspec outside sparse definition' '
    - 	test_all_match git diff-files folder1/a
    +@@ t/t1092-sparse-checkout-compatibility.sh: ensure_not_expanded () {
    + 	test_region ! index ensure_full_index trace2.txt
    + }
    + 
    ++ensure_expanded () {
    ++	rm -f trace2.txt &&
    ++	if test -z "$WITHOUT_UNTRACKED_TXT"
    ++	then
    ++		echo >>sparse-index/untracked.txt
    ++	fi &&
    ++
    ++	if test "$1" = "!"
    ++	then
    ++		shift &&
    ++		test_must_fail env \
    ++			GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
    ++			git -C sparse-index "$@" \
    ++			>sparse-index-out \
    ++			2>sparse-index-error || return 1
    ++	else
    ++		GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
    ++			git -C sparse-index "$@" \
    ++			>sparse-index-out \
    ++			2>sparse-index-error || return 1
    ++	fi &&
    ++	test_region index ensure_full_index trace2.txt
    ++}
    ++
    + test_expect_success 'sparse-index is not expanded' '
    + 	init_repos &&
    + 
    +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with pathspec outside sparse definition' '
    + 	test_all_match git diff-files "folder*/a" 
      '
      
     +test_expect_success 'diff-files pathspec expands index when necessary' '
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'diff-files with p
     +	run_on_all ../edit-contents deep/a &&
     +	
     +	# pathspec that should expand index
    -+	! ensure_not_expanded diff-files "*/a" &&
    -+	test_must_be_empty sparse-index-err &&
    -+
    -+	! ensure_not_expanded diff-files "**a" &&
    -+	test_must_be_empty sparse-index-err
    ++	ensure_expanded diff-files "*/a" &&
    ++	ensure_expanded diff-files "**a" 
     +'
     +
     +test_expect_success 'sparse index is not expanded: diff-files' '
-- 
2.39.0

[PATCH v7 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin with the sparse index
feature, add tests to t1092-sparse-checkout-compatibility.sh to ensure
it currently works with sparse-checkout and will still work with sparse
index after that integration.

When adding tests against a sparse-checkout definition, we test two
modes: all changes are within the sparse-checkout cone and some changes
are outside the sparse-checkout cone.

In order to have staged changes outside of the sparse-checkout cone,
make a directory called 'folder1' and copy `a` into 'folder1/a'.
'folder1/a' is identical to `a` in the base commit. These make
'folder1/a' in the index, while leaving it outside of the
sparse-checkout definition. Test 'folder1/a'being present on-disk
without modifications, then change content inside 'folder1/a' in order
to test 'folder1/a' being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..d23041e27a 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,50 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files &&
+
+	test_all_match git diff-files deep/a && 
+
+	# test wildcard
+	test_all_match git diff-files deep/*
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	test_sparse_match test_must_fail git diff-files folder2/a &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# Add file to the index but outside of cone for sparse-checkout cases.
+	# Add file to the index without sparse-checkout cases to ensure all have 
+	# same output.
+	run_on_all mkdir -p folder1 &&
+	run_on_all cp a folder1/a &&
+
+	# file present on-disk without modifications
+	# use `--stat` to ignore file creation time differences in
+	# unrefreshed index
+	test_all_match git diff-files --stat &&
+	test_all_match git diff-files --stat folder1/a &&
+	test_all_match git diff-files --stat "folder*/a" &&
+
+	# file present on-disk with modifications
+	run_on_all ../edit-contents folder1/a &&
+	test_all_match git diff-files &&
+	test_all_match git diff-files folder1/a &&
+	test_all_match git diff-files "folder*/a" 
+'
+
 test_done
-- 
2.39.0

[PATCH v7 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Originally, diff-files a pathspec that is out-of-cone in a sparse-index
environment, Git dies with "pathspec '<x>' did not match any files",
mainly because it does not expand the index so nothing is matched.
Expand the index when the <pathspec> needs an expanded index, i.e. the
<pathspec> contains wildcard that may need a full-index or the
<pathspec> is simply outside of sparse-checkout definition.

Remove full index requirement for `git diff-files`.Create an
'ensure_expanded' to handle silent failures. Add test to
ensure the index only expanded when necessary in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  8 ++++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 52 ++++++++++++++++++++++++
 3 files changed, 62 insertions(+)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..db90592090 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
@@ -80,6 +84,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		result = -1;
 		goto cleanup;
 	}
+
+	if (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))
+		ensure_full_index(the_repository->index);
+
 	result = run_diff_files(&rev, options);
 	result = diff_result_code(&rev.diffopt, result);
 cleanup:
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 3242cfe91a..82751f2ca3 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all
 test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
 test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
 test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index d23041e27a..152f3f752e 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1401,6 +1401,30 @@ ensure_not_expanded () {
 	test_region ! index ensure_full_index trace2.txt
 }
 
+ensure_expanded () {
+	rm -f trace2.txt &&
+	if test -z "$WITHOUT_UNTRACKED_TXT"
+	then
+		echo >>sparse-index/untracked.txt
+	fi &&
+
+	if test "$1" = "!"
+	then
+		shift &&
+		test_must_fail env \
+			GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+			git -C sparse-index "$@" \
+			>sparse-index-out \
+			2>sparse-index-error || return 1
+	else
+		GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+			git -C sparse-index "$@" \
+			>sparse-index-out \
+			2>sparse-index-error || return 1
+	fi &&
+	test_region index ensure_full_index trace2.txt
+}
+
 test_expect_success 'sparse-index is not expanded' '
 	init_repos &&
 
@@ -2101,4 +2125,32 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
 	test_all_match git diff-files "folder*/a" 
 '
 
+test_expect_success 'diff-files pathspec expands index when necessary' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+	
+	# pathspec that should expand index
+	ensure_expanded diff-files "*/a" &&
+	ensure_expanded diff-files "**a" 
+'
+
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files &&
+	ensure_not_expanded diff-files deep/a &&
+	ensure_not_expanded diff-files deep/*
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v7 0/2] diff-files: integrate with sparse index

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> 3. Use `--stat` to ignore file creation time differences in unrefreshed
> index.

I am curious about this one.  Why is this a preferred solution over
say "run 'update-index --refresh' before running diff-files"?

Note that this is merely "I am curious", not "I think it is wrong".

Thanks.

Re: [PATCH v7 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

On Wed, Mar 22, 2023 at 7:36 PM Junio C Hamano <gitster@pobox.com> wrote:

> > 3. Use `--stat` to ignore file creation time differences in unrefreshed
> > index.
>
> I am curious about this one.  Why is this a preferred solution over
> say "run 'update-index --refresh' before running diff-files"?
>
> Note that this is merely "I am curious", not "I think it is wrong".

Hi Junio

Thank you for your question, it has prompted me to consider the matter
further =)  I think both solutions, using git diff-files --stat and using git
update-index --refresh before git diff-files, can produce the same output but
in different ways.

When the index file is not up-to-date, git diff-files may show differences
between the working directory and the index that are caused by file creation
time differences, rather than actual changes to the file contents. By using git
diff-files --stat, which ignores file creation time differences.

While 'git update-index --refresh' updates the index file to match the contents
of the working tree. By running this command before git diff-files, we can
ensure that the index file is up-to-date and that the output of git diff-files
accurately reflects the differences between the working directory and the index.

Maybe using git update-index --refresh would be more direct and
straightforward solution.

(Hi Victoria, do you have any comments?  =)


Thanks
Shuqi

Re: [PATCH v7 0/2] diff-files: integrate with sparse index

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> When the index file is not up-to-date, git diff-files may show differences
> between the working directory and the index that are caused by file creation
> time differences, rather than actual changes to the file contents. By using git
> diff-files --stat, which ignores file creation time differences.

Use of "diff-files --stat" would mean that the contents of the blob
registered in the index will be inspected, which can be used to hide
the "stat dirty" condition.

But doesn't it cut both ways?  Starting from a clean index that has
up-to-date stat information for paths, we may want to test what
"stat dirty" changes diff-files reports when we touch paths in the
working tree, both inside and outside the spase cones.  A test with
"--stat" will not achieve that, exactly because it does not pay
attention to and hides the stat dirtiness.

On the other hand, if "update-index --refresh" is used in the test,
we may discover breakages caused by "update-index" not handling
the sparse index correctly.  It would be outside the topic of this
series, so avoiding it would be simpler, but (1) if it is not broken,
then as you said, it would be a more direct way to test diff-files,
and (2) if it is broken, it would need to be fixed anyway, before or
after this series.  So, I dunno...

Thanks.

Re: [PATCH v7 0/2] diff-files: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> On Wed, Mar 22, 2023 at 7:36 PM Junio C Hamano <gitster@pobox.com> wrote:
> 
>>> 3. Use `--stat` to ignore file creation time differences in unrefreshed
>>> index.
>>
>> I am curious about this one.  Why is this a preferred solution over
>> say "run 'update-index --refresh' before running diff-files"?
>>
>> Note that this is merely "I am curious", not "I think it is wrong".
> 
> Hi Junio
> 
> Thank you for your question, it has prompted me to consider the matter
> further =)  I think both solutions, using git diff-files --stat and using git
> update-index --refresh before git diff-files, can produce the same output but
> in different ways.

While they'll (ideally) give the same user-facing result, there is a
difference in how they exercise 'diff-files' because of how 'update-index
--refresh' will affect SKIP_WORKTREE and sparse directories.

Using the same scenario you've set up for your test, suppose I start with a
fresh copy of the 't1092' repo. In the 'sparse-index' repo copy, 'folder1/'
will be a sparse directory:

$ git ls-files -t --sparse folder1/
S folder1/

(note: "S" indicates that SKIP_WORKTREE is applied to the entry)

Now suppose I copy 'a' into 'folder1/' and run 'update-index --refresh'
then 'ls-files' again:

$ git update-index --refresh
$ git ls-files -t --sparse folder1/
S folder1/0/
H folder1/a

(note: "H" indicates that 'folder1/a' does not have SKIP_WORKTREE applied)

The sparse directory has been expanded and SKIP_WORKTREE has been removed
from the file that's now present on-disk. This was an intentional "safety"
measure added in [1] to address the growing volume of bugs and complexities
in scenarios where SKIP_WORKTREE files existed on disk.

Ultimately, the main difference between this test with & without
'update-index' is who applies those index corrections when initially reading
the index: 'update-index' or 'diff-files'. I lean towards the latter because
the former is tested (almost identically) in 'update-index modify outside
sparse definition' earlier in 't1092'.

[1] https://lore.kernel.org/git/pull.1114.v2.git.1642175983.gitgitgadget@gmail.com/

> 
> When the index file is not up-to-date, git diff-files may show differences
> between the working directory and the index that are caused by file creation
> time differences, rather than actual changes to the file contents. By using git
> diff-files --stat, which ignores file creation time differences.

More or less, yes. Internally, 'diff-files' will "see" the file creation
differences, but the '--stat' format doesn't print them.

> 
> While 'git update-index --refresh' updates the index file to match the contents
> of the working tree. By running this command before git diff-files, we can
> ensure that the index file is up-to-date and that the output of git diff-files
> accurately reflects the differences between the working directory and the index.

This isn't quite true - 'update-index' only updates the *contents* of index
entries (or, colloquially, "stage them for commit") for files explicitly
provided as arguments. Separately, though, '--refresh' updates *all* index
entries' cached 'stat' information. 

Going a bit deeper: with no arguments, 'update-index' will read the index,
do nothing to it, then write it only if something has changed. In almost all
cases, reading the index doesn't cause any changes to it, making it a no-op.
However, the removal of SKIP_WORKTREE is done on read (including a refresh
of the entry's stat information), so a even plain 'update-index' *without*
'--refresh' would write a modified index to disk. In your test, that means:

	run_on_sparse mkdir -p folder1 &&
	run_on_sparse cp a folder1/a &&
	run_on_all git update-index &&
	test_all_match git diff-files

would get you the same result as:

	run_on_sparse mkdir -p folder1 &&
	run_on_sparse cp a folder1/a &&
	run_on_all git update-index --refresh &&
	test_all_match git diff-files

> 
> Maybe using git update-index --refresh would be more direct and
> straightforward solution.
> 
> (Hi Victoria, do you have any comments?  =)

I hope the above explanation is helpful. I still think '--stat' is the best
way to test this case, but I'm interested to hear your/others' thoughts on
the matter given the additional context.

> 
> 
> Thanks
> Shuqi

Re: [PATCH v7 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Hi Junio

On Thu, Mar 23, 2023 at 12:03 PM Junio C Hamano <gitster@pobox.com> wrote:

> > When the index file is not up-to-date, git diff-files may show differences
> > between the working directory and the index that are caused by file creation
> > time differences, rather than actual changes to the file contents. By using git
> > diff-files --stat, which ignores file creation time differences.
>
> Use of "diff-files --stat" would mean that the contents of the blob
> registered in the index will be inspected, which can be used to hide
> the "stat dirty" condition.
>
> But doesn't it cut both ways?  Starting from a clean index that has
> up-to-date stat information for paths, we may want to test what
> "stat dirty" changes diff-files reports when we touch paths in the
> working tree, both inside and outside the spase cones.  A test with
> "--stat" will not achieve that, exactly because it does not pay
> attention to and hides the stat dirtiness.

In this case, we can only use 'git diff-files --stat' when files are
present on disk without modifications. Since we know in the
full-checkout case 'diff-files --stat' will give empty output, so
sparse-checkout and sparse-index are also empty. These make
sure that the paths in the working tree are not dirty. So we do not
need to pay attention to 'stat dirty' change.

When 'file present on-disk with modifications'. We use 'git diff-files'
instead of  'git diff-files --stat' so we can get the expected
"modified" status but avoids potential breakages related to
inconsistency in the file creation time.

# file present on-disk without modifications
# use `--stat` to ignore file creation time differences in
# unrefreshed index
test_all_match git diff-files --stat &&
test_all_match git diff-files --stat folder1/a &&
test_all_match git diff-files --stat "folder*/a" &&

# file present on-disk with modifications
run_on_all ../edit-contents folder1/a &&
test_all_match git diff-files &&
test_all_match git diff-files folder1/a &&
test_all_match git diff-files "folder*/a"

> On the other hand, if "update-index --refresh" is used in the test,
> we may discover breakages caused by "update-index" not handling
> the sparse index correctly.  It would be outside the topic of this
> series, so avoiding it would be simpler, but (1) if it is not broken,
> then as you said, it would be a more direct way to test diff-files,
> and (2) if it is broken, it would need to be fixed anyway, before or
> after this series.  So, I dunno...

Thanks
Shuqi

Re: [PATCH v7 0/2] diff-files: integrate with sparse index

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> Changes since v6:
>
> 1. Fix word wrap in commit message.
>
> 2. Use  'mkdir -p folder1' since full-checkout already have folder1.
>
> 3. Use `--stat` to ignore file creation time differences in unrefreshed
> index.
>
> 4. In 'diff-files with pathspec outside sparse definition' add 
> 'git diff-files "folder*/a" to show that the result is the same with a 
> wildcard pathspec.
>
> 5. Create an 'ensure_expanded' to handle silent failures.

It seems that review comments have petered out.

Shall we mark this ready for 'next'?

Thanks.

Re: [PATCH v7 0/2] diff-files: integrate with sparse index

[Victoria Dye]

Junio C Hamano wrote:
> Shuqi Liang <cheskaqiqi@gmail.com> writes:
> 
>> Changes since v6:
>>
>> 1. Fix word wrap in commit message.
>>
>> 2. Use  'mkdir -p folder1' since full-checkout already have folder1.
>>
>> 3. Use `--stat` to ignore file creation time differences in unrefreshed
>> index.
>>
>> 4. In 'diff-files with pathspec outside sparse definition' add 
>> 'git diff-files "folder*/a" to show that the result is the same with a 
>> wildcard pathspec.
>>
>> 5. Create an 'ensure_expanded' to handle silent failures.
> 
> It seems that review comments have petered out.
> 
> Shall we mark this ready for 'next'?

Sorry for the delay - I noticed a couple more things that might warrant
changes before moving to 'next' and am in the process of writing up that
response. I'll send it within the hour.

> 
> Thanks.

Re: [PATCH v7 2/2] diff-files: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> diff --git a/builtin/diff-files.c b/builtin/diff-files.c
> index dc991f753b..db90592090 100644
> --- a/builtin/diff-files.c
> +++ b/builtin/diff-files.c
> @@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
>  		usage(diff_files_usage);
>  
>  	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> +
> +	prepare_repo_settings(the_repository);
> +	the_repository->settings.command_requires_full_index = 0;
> +
>  	repo_init_revisions(the_repository, &rev, prefix);
>  	rev.abbrev = 0;
>  
> @@ -80,6 +84,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
>  		result = -1;
>  		goto cleanup;
>  	}
> +
> +	if (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))
> +		ensure_full_index(the_repository->index);

After reviewing the 'diff-index' integration [1], I'm wondering whether we
actually need pathspec expansion at all in this case. 'diff-files' compares
the working tree and the index, and will output a difference if the file on
disk differs from the index. But, if a file is SKIP_WORKTREE'd, that diff
will (I think?) always be empty. So, why would we need to expand a sparse
directory to match a pathspec to its contents if we *know* that the diff
will be empty?

[1] https://lore.kernel.org/git/20230408112342.404318-1-nanth.raghul@gmail.com/

> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index d23041e27a..152f3f752e 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -1401,6 +1401,30 @@ ensure_not_expanded () {
>  	test_region ! index ensure_full_index trace2.txt
>  }
>  
> +ensure_expanded () {
> +	rm -f trace2.txt &&
> +	if test -z "$WITHOUT_UNTRACKED_TXT"
> +	then
> +		echo >>sparse-index/untracked.txt
> +	fi &&
> +
> +	if test "$1" = "!"
> +	then
> +		shift &&
> +		test_must_fail env \
> +			GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
> +			git -C sparse-index "$@" \
> +			>sparse-index-out \
> +			2>sparse-index-error || return 1
> +	else
> +		GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
> +			git -C sparse-index "$@" \
> +			>sparse-index-out \
> +			2>sparse-index-error || return 1
> +	fi &&
> +	test_region index ensure_full_index trace2.txt
> +}

This implementation duplicates a lot of the code from 'ensure_not_expanded'.
Can 'ensure_expanded' and 'ensure_not_expanded' be refactored to call a
common helper function (which contains the common code) instead?

> +
>  test_expect_success 'sparse-index is not expanded' '
>  	init_repos &&
>  
> @@ -2101,4 +2125,32 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
>  	test_all_match git diff-files "folder*/a" 
>  '
>  
> +test_expect_success 'diff-files pathspec expands index when necessary' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +	
> +	# pathspec that should expand index
> +	ensure_expanded diff-files "*/a" &&
> +	ensure_expanded diff-files "**a" 

Similar to the comments in my 'diff-index' review [2]:

- The '**' in the pathspec doesn't do anything special unless using an
  explicit ':(glob)' pathspec. To make it clear that you're not trying to
  use a glob pathspec, you can use '*a' instead.
- Why are these pathspecs in quotes, but those in 'sparse index is not
  expanded: diff-files' are?

[2] https://lore.kernel.org/git/62821012-4fc3-5ad8-695c-70f7ab14a8c9@github.com/

> +'
> +
> +test_expect_success 'sparse index is not expanded: diff-files' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	ensure_not_expanded diff-files &&
> +	ensure_not_expanded diff-files deep/a &&
> +	ensure_not_expanded diff-files deep/*
> +'
> +
>  test_done

Re: [PATCH v7 1/2] t1092: add tests for `git diff-files`

[Victoria Dye]

Shuqi Liang wrote:
> +test_expect_success 'diff-files with pathspec inside sparse definition' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	test_all_match git diff-files &&
> +
> +	test_all_match git diff-files deep/a && 
> +
> +	# test wildcard
> +	test_all_match git diff-files deep/*

Should this pathspec be quoted (like you do for "folder*/a" below)?

> +'
> +
> +test_expect_success 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&
> +
> +	test_sparse_match test_must_fail git diff-files folder2/a &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	# Add file to the index but outside of cone for sparse-checkout cases.
> +	# Add file to the index without sparse-checkout cases to ensure all have 
> +	# same output.
> +	run_on_all mkdir -p folder1 &&
> +	run_on_all cp a folder1/a &&
> +
> +	# file present on-disk without modifications
> +	# use `--stat` to ignore file creation time differences in
> +	# unrefreshed index
> +	test_all_match git diff-files --stat &&
> +	test_all_match git diff-files --stat folder1/a &&
> +	test_all_match git diff-files --stat "folder*/a" &&
> +
> +	# file present on-disk with modifications
> +	run_on_all ../edit-contents folder1/a &&
> +	test_all_match git diff-files &&
> +	test_all_match git diff-files folder1/a &&
> +	test_all_match git diff-files "folder*/a" 
> +'
> +
>  test_done

Re: [PATCH v7 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Hi Victoria,

Sorry for the late reply. I'm still in the middle of my final exams period.

On Thu, Apr 13, 2023 at 5:55 PM Victoria Dye <vdye@github.com> wrote:
>
> Shuqi Liang wrote:
> > diff --git a/builtin/diff-files.c b/builtin/diff-files.c
> > index dc991f753b..db90592090 100644
> > --- a/builtin/diff-files.c
> > +++ b/builtin/diff-files.c
> > @@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
> >               usage(diff_files_usage);
> >
> >       git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> > +
> > +     prepare_repo_settings(the_repository);
> > +     the_repository->settings.command_requires_full_index = 0;
> > +
> >       repo_init_revisions(the_repository, &rev, prefix);
> >       rev.abbrev = 0;
> >
> > @@ -80,6 +84,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
> >               result = -1;
> >               goto cleanup;
> >       }
> > +
> > +     if (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))
> > +             ensure_full_index(the_repository->index);
>
> After reviewing the 'diff-index' integration [1], I'm wondering whether we
> actually need pathspec expansion at all in this case. 'diff-files' compares
> the working tree and the index, and will output a difference if the file on
> disk differs from the index. But, if a file is SKIP_WORKTREE'd, that diff
> will (I think?) always be empty. So, why would we need to expand a sparse
> directory to match a pathspec to its contents if we *know* that the diff
> will be empty?
>
> [1] https://lore.kernel.org/git/20230408112342.404318-1-nanth.raghul@gmail.com/


It's true that in the case of 'diff-files', expanding the sparse directory to
match a pathspec to its contents might not be necessary. If we don't use
pathspec expansion in this case. It could optimize for performance better.

However, there could be some edge cases. if a user manually modifies the
contents of a SKIP_WORKTREE file in the working tree, the diff between
the working tree and the index would no longer be empty. So I think, In this
case, expanding the sparse directory might still be necessary to ensure the
correct behavior of the 'diff-files' command.



> > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> > index d23041e27a..152f3f752e 100755
> > --- a/t/t1092-sparse-checkout-compatibility.sh
> > +++ b/t/t1092-sparse-checkout-compatibility.sh
> > @@ -1401,6 +1401,30 @@ ensure_not_expanded () {
> >       test_region ! index ensure_full_index trace2.txt
> >  }
> >
> > +ensure_expanded () {
> > +     rm -f trace2.txt &&
> > +     if test -z "$WITHOUT_UNTRACKED_TXT"
> > +     then
> > +             echo >>sparse-index/untracked.txt
> > +     fi &&
> > +
> > +     if test "$1" = "!"
> > +     then
> > +             shift &&
> > +             test_must_fail env \
> > +                     GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
> > +                     git -C sparse-index "$@" \
> > +                     >sparse-index-out \
> > +                     2>sparse-index-error || return 1
> > +     else
> > +             GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
> > +                     git -C sparse-index "$@" \
> > +                     >sparse-index-out \
> > +                     2>sparse-index-error || return 1
> > +     fi &&
> > +     test_region index ensure_full_index trace2.txt
> > +}
>
> This implementation duplicates a lot of the code from 'ensure_not_expanded'.
> Can 'ensure_expanded' and 'ensure_not_expanded' be refactored to call a
> common helper function (which contains the common code) instead?

Will do!

> > +
> >  test_expect_success 'sparse-index is not expanded' '
> >       init_repos &&
> >
> > @@ -2101,4 +2125,32 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
> >       test_all_match git diff-files "folder*/a"
> >  '
> >
> > +test_expect_success 'diff-files pathspec expands index when necessary' '
> > +     init_repos &&
> > +
> > +     write_script edit-contents <<-\EOF &&
> > +     echo text >>"$1"
> > +     EOF
> > +
> > +     run_on_all ../edit-contents deep/a &&
> > +
> > +     # pathspec that should expand index
> > +     ensure_expanded diff-files "*/a" &&
> > +     ensure_expanded diff-files "**a"
>
> Similar to the comments in my 'diff-index' review [2]:
>
> - The '**' in the pathspec doesn't do anything special unless using an
>   explicit ':(glob)' pathspec. To make it clear that you're not trying to
>   use a glob pathspec, you can use '*a' instead.

Will do !

> - Why are these pathspecs in quotes, but those in 'sparse index is not
>   expanded: diff-files' are?
>
> [2] https://lore.kernel.org/git/62821012-4fc3-5ad8-695c-70f7ab14a8c9@github.com/


I quote around the pathspec  to prevent shell expansion  of the pathspec
patterns by the shell before they are passed to the git command. "*a" and
"*/a"  have special characters ' * '. I use quotes to tell the shell
to treat them
as regular characters.

In 'sparse index is not expanded: diff-files''deep/a'  does not contain any
special characters that the shell would try to expand. So I use it without
double quotes. And  I think I need to add a double quote to 'deep/*'.

Thanks,
Shuqi

Re: [PATCH v7 2/2] diff-files: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> Hi Victoria,
> 
> Sorry for the late reply. I'm still in the middle of my final exams period.

No problem at all, thanks for following up!

> It's true that in the case of 'diff-files', expanding the sparse directory to
> match a pathspec to its contents might not be necessary. If we don't use
> pathspec expansion in this case. It could optimize for performance better.
> 
> However, there could be some edge cases. if a user manually modifies the
> contents of a SKIP_WORKTREE file in the working tree, the diff between
> the working tree and the index would no longer be empty. So I think, In this
> case, expanding the sparse directory might still be necessary to ensure the
> correct behavior of the 'diff-files' command.

If a user manually modifies a SKIP_WORKTREE file, SKIP_WORKTREE will be
removed from the file and the index expanded automatically [1]. If that
mechanism is working properly, there would be no need to manually check the
pathspec and expand the index.

Have you tried removing the 'pathspec_needs_expanded_index()' and running
the tests? If so, is 'diff-files' producing incorrect results? 

[1] https://lore.kernel.org/git/11d46a399d26c913787b704d2b7169cafc28d639.1642175983.git.gitgitgadget@gmail.com/

Re: [PATCH v7 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Hi Victoria,

> If a user manually modifies a SKIP_WORKTREE file, SKIP_WORKTREE will be
> removed from the file and the index expanded automatically [1]. If that
> mechanism is working properly, there would be no need to manually check the
> pathspec and expand the index.
>
> Have you tried removing the 'pathspec_needs_expanded_index()' and running
> the tests? If so, is 'diff-files' producing incorrect results?
>
> [1] https://lore.kernel.org/git/11d46a399d26c913787b704d2b7169cafc28d639.1642175983.git.gitgitgadget@gmail.com/

As per your suggestion, I tried removing pathspec_needs_expanded_index()
from the code, and 'diff-files pathspec expands index when necessary'
test failed.

So, I'm thinking about keeping it to ensure everything works properly.
I'd like to know your thoughts on this. Should we keep it or explore
other options?

Thanks,
Shuqi

Re: [PATCH v7 2/2] diff-files: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> Hi Victoria,
> 
>> If a user manually modifies a SKIP_WORKTREE file, SKIP_WORKTREE will be
>> removed from the file and the index expanded automatically [1]. If that
>> mechanism is working properly, there would be no need to manually check the
>> pathspec and expand the index.
>>
>> Have you tried removing the 'pathspec_needs_expanded_index()' and running
>> the tests? If so, is 'diff-files' producing incorrect results?
>>
>> [1] https://lore.kernel.org/git/11d46a399d26c913787b704d2b7169cafc28d639.1642175983.git.gitgitgadget@gmail.com/
> 
> As per your suggestion, I tried removing pathspec_needs_expanded_index()
> from the code, and 'diff-files pathspec expands index when necessary'
> test failed.
> 
> So, I'm thinking about keeping it to ensure everything works properly.
> I'd like to know your thoughts on this. Should we keep it or explore
> other options?

Did the test fail because the index wasn't expanded in a case where you
previously expended it to be expanded? Or because of the returned results of
'diff-files' are invalid?

Only the latter represents incorrect behavior. If we're aren't expanding the
index for a case that was causing index expansion before *and* the
user-facing behavior is as-expected, that's the best-case scenario for a
sparse index integration!

Taking a step back, it's important to remember that the overarching goal of
the project is not just to switch 'command_requires_full_index' to '0'
everywhere, but to find all of the places where Git is working with the
index and make sure that work can be done on a sparse directory.

In most cases, it's possible to adapt an index-related operation to work
with sparse directories (albeit with varying levels of complexity). The use
of 'ensure_full_index()' is reserved for cases where it is _impossible_ to
make Git perform a given action on a sparse directory - expanding the index
completely eliminates the performance gains had by using a sparse index, so
it should be avoided at all costs.

I hope that helps!

> 
> Thanks,
> Shuqi

Re: [PATCH v7 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Hi Victoria,

On Fri, Apr 21, 2023 at 5:26 PM Victoria Dye <vdye@github.com> wrote:
> Only the latter represents incorrect behavior. If we're aren't expanding the
> index for a case that was causing index expansion before *and* the
> user-facing behavior is as-expected, that's the best-case scenario for a
> sparse index integration!
>
> Taking a step back, it's important to remember that the overarching goal of
> the project is not just to switch 'command_requires_full_index' to '0'
> everywhere, but to find all of the places where Git is working with the
> index and make sure that work can be done on a sparse directory.
>
> In most cases, it's possible to adapt an index-related operation to work
> with sparse directories (albeit with varying levels of complexity). The use
> of 'ensure_full_index()' is reserved for cases where it is _impossible_ to
> make Git perform a given action on a sparse directory - expanding the index
> completely eliminates the performance gains had by using a sparse index, so
> it should be avoided at all costs.
>
> I hope that helps!

Thanks for reminding me about the ultimate goal of sparse index
integration! I've learned a lot from it. After looking into the test
failure, it seems that the index didn't expand in cases where I expected
it to. I'll go ahead and update my patch.

Thanks,
Shuqi

[PATCH v8 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Changes since v7:

* Refactor the ensure_expanded and ensure_not_expanded functions by 
introducing a helper function, ensure_index_state.

* Delete the test 'diff-files pathspec expands index when necessary'.

* Delete 'the pathspec_needs_expanded_index' function.

* Add double quotes to "deep/*"

* Change "**a" to "*a"

* Updata commit message.


Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  4 ++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 81 +++++++++++++++++++++++-
 3 files changed, 85 insertions(+), 2 deletions(-)

Range-diff:
1:  e2dcf9921e ! 1:  d7f921c1a6 t1092: add tests for `git diff-files`
    @@ Commit message
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
      ## t/t1092-sparse-checkout-compatibility.sh ##
    -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse directory within submodules' '
    - 	test_cmp actual expect
    +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is not expanded: write-tree' '
    + 	ensure_not_expanded write-tree
      '
      
     +test_expect_success 'diff-files with pathspec inside sparse definition' '
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc
     +	test_all_match git diff-files deep/a && 
     +
     +	# test wildcard
    -+	test_all_match git diff-files deep/*
    ++	test_all_match git diff-files "deep/*"
     +'
     +
     +test_expect_success 'diff-files with pathspec outside sparse definition' '
2:  fb8edaf583 < -:  ---------- diff-files: integrate with sparse index
-:  ---------- > 2:  b44384ac94 diff-files: integrate with sparse index
-- 
2.39.0

[PATCH v8 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin with the sparse index
feature, add tests to t1092-sparse-checkout-compatibility.sh to ensure
it currently works with sparse-checkout and will still work with sparse
index after that integration.

When adding tests against a sparse-checkout definition, we test two
modes: all changes are within the sparse-checkout cone and some changes
are outside the sparse-checkout cone.

In order to have staged changes outside of the sparse-checkout cone,
make a directory called 'folder1' and copy `a` into 'folder1/a'.
'folder1/a' is identical to `a` in the base commit. These make
'folder1/a' in the index, while leaving it outside of the
sparse-checkout definition. Test 'folder1/a'being present on-disk
without modifications, then change content inside 'folder1/a' in order
to test 'folder1/a' being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 0c784813f1..3c140103c5 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2108,4 +2108,50 @@ test_expect_success 'sparse-index is not expanded: write-tree' '
 	ensure_not_expanded write-tree
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files &&
+
+	test_all_match git diff-files deep/a && 
+
+	# test wildcard
+	test_all_match git diff-files "deep/*"
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	test_sparse_match test_must_fail git diff-files folder2/a &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# Add file to the index but outside of cone for sparse-checkout cases.
+	# Add file to the index without sparse-checkout cases to ensure all have 
+	# same output.
+	run_on_all mkdir -p folder1 &&
+	run_on_all cp a folder1/a &&
+
+	# file present on-disk without modifications
+	# use `--stat` to ignore file creation time differences in
+	# unrefreshed index
+	test_all_match git diff-files --stat &&
+	test_all_match git diff-files --stat folder1/a &&
+	test_all_match git diff-files --stat "folder*/a" &&
+
+	# file present on-disk with modifications
+	run_on_all ../edit-contents folder1/a &&
+	test_all_match git diff-files &&
+	test_all_match git diff-files folder1/a &&
+	test_all_match git diff-files "folder*/a" 
+'
+
 test_done
-- 
2.39.0

[PATCH v8 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Remove full index requirement for `git diff-files`. Refactor the
ensure_expanded and ensure_not_expanded functions by introducing a
common helper function, ensure_index_state. Add test to ensure the index
is no expanded in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  4 +++
 t/perf/p2000-sparse-operations.sh        |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 35 ++++++++++++++++++++++--
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..360464e6ef 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 60d1de0662..29165b3493 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -129,5 +129,7 @@ test_perf_on_all git grep --cached bogus -- "f2/f1/f1/*"
 test_perf_on_all git write-tree
 test_perf_on_all git describe --dirty
 test_perf_on_all 'echo >>new && git describe --dirty'
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 3c140103c5..7ebcfe785e 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1377,7 +1377,10 @@ test_expect_success 'index.sparse disabled inline uses full index' '
 	! test_region index ensure_full_index trace2.txt
 '
 
-ensure_not_expanded () {
+ensure_index_state () {
+	local expected_expansion="$1"
+	shift
+
 	rm -f trace2.txt &&
 	if test -z "$WITHOUT_UNTRACKED_TXT"
 	then
@@ -1398,7 +1401,21 @@ ensure_not_expanded () {
 			>sparse-index-out \
 			2>sparse-index-error || return 1
 	fi &&
-	test_region ! index ensure_full_index trace2.txt
+
+	if [ "$expected_expansion" = "expanded" ]
+	then
+		test_region index ensure_full_index trace2.txt
+	else
+		test_region ! index ensure_full_index trace2.txt
+	fi
+}
+
+ensure_expanded () {
+	ensure_index_state "expanded" "$@"
+}
+
+ensure_not_expanded () {
+	ensure_index_state "not_expanded" "$@"
 }
 
 test_expect_success 'sparse-index is not expanded' '
@@ -2154,4 +2171,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
 	test_all_match git diff-files "folder*/a" 
 '
 
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files &&
+	ensure_not_expanded diff-files deep/a &&
+	ensure_not_expanded diff-files "deep/*"
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v8 0/2] diff-files: integrate with sparse index

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> Changes since v7:
>
> * Refactor the ensure_expanded and ensure_not_expanded functions by 
> introducing a helper function, ensure_index_state.
>
> * Delete the test 'diff-files pathspec expands index when necessary'.
>
> * Delete 'the pathspec_needs_expanded_index' function.
>
> * Add double quotes to "deep/*"
>
> * Change "**a" to "*a"
>
> * Updata commit message.

These patches seem to have some whitespace errors.

Re: [PATCH v8 0/2] diff-files: integrate with sparse index

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> Changes since v7:
>
> * Refactor the ensure_expanded and ensure_not_expanded functions by 
> introducing a helper function, ensure_index_state.
>
> * Delete the test 'diff-files pathspec expands index when necessary'.
>
> * Delete 'the pathspec_needs_expanded_index' function.
>
> * Add double quotes to "deep/*"
>
> * Change "**a" to "*a"
>
> * Updata commit message.

This round did not see any reactions; is everybody happy to see us
declare victory and merge it down to 'next'?

Thanks.

Re: [PATCH v8 2/2] diff-files: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 3c140103c5..7ebcfe785e 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -1377,7 +1377,10 @@ test_expect_success 'index.sparse disabled inline uses full index' '
>  	! test_region index ensure_full_index trace2.txt
>  '
>  
> -ensure_not_expanded () {
> +ensure_index_state () {
> +	local expected_expansion="$1"
> +	shift
> +
>  	rm -f trace2.txt &&
>  	if test -z "$WITHOUT_UNTRACKED_TXT"
>  	then
> @@ -1398,7 +1401,21 @@ ensure_not_expanded () {
>  			>sparse-index-out \
>  			2>sparse-index-error || return 1
>  	fi &&
> -	test_region ! index ensure_full_index trace2.txt
> +
> +	if [ "$expected_expansion" = "expanded" ]
> +	then
> +		test_region index ensure_full_index trace2.txt
> +	else
> +		test_region ! index ensure_full_index trace2.txt
> +	fi
> +}
> +
> +ensure_expanded () {
> +	ensure_index_state "expanded" "$@"
> +}
> +
> +ensure_not_expanded () {
> +	ensure_index_state "not_expanded" "$@"
>  }

This still seems a bit more complicated than necessary (mainly due to the
new string comparison & local arg). What about something like this (applied
on top)?

-------- 8< -------- 8< -------- 8< --------
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 9d11d28891..333822f322 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1377,10 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '
 	! test_region index ensure_full_index trace2.txt
 '
 
-ensure_index_state () {
-	local expected_expansion="$1"
-	shift
-
+run_sparse_index_trace2 () {
 	rm -f trace2.txt &&
 	if test -z "$WITHOUT_UNTRACKED_TXT"
 	then
@@ -1400,22 +1397,17 @@ ensure_index_state () {
 			git -C sparse-index "$@" \
 			>sparse-index-out \
 			2>sparse-index-error || return 1
-	fi &&
-
-	if [ "$expected_expansion" = "expanded" ]
-	then
-		test_region index ensure_full_index trace2.txt
-	else
-		test_region ! index ensure_full_index trace2.txt
 	fi
 }
 
 ensure_expanded () {
-	ensure_index_state "expanded" "$@"
+	run_sparse_index_trace2 "$@" &&
+	test_region index ensure_full_index trace2.txt
 }
 
 ensure_not_expanded () {
-	ensure_index_state "not_expanded" "$@"
+	run_sparse_index_trace2 "$@" &&
+	test_region ! index ensure_full_index trace2.txt
 }
 
 test_expect_success 'sparse-index is not expanded' '
-------- >8 -------- >8 -------- >8 --------

That said, given that this is my only complaint with this iteration (and
it's pretty subjective), if others are happy with it then I'm not opposed to
merging to 'next'.

[PATCH v9 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Changes since v8:

* Fix white space problem.

* Simplified the refactored function based on Victoria's suggestion.

Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  4 ++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 73 +++++++++++++++++++++++-
 3 files changed, 77 insertions(+), 2 deletions(-)

Range-diff against v8:
1:  d7f921c1a6 ! 1:  d78513af83 t1092: add tests for `git diff-files`
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
     +
     +	test_all_match git diff-files &&
     +
    -+	test_all_match git diff-files deep/a && 
    ++	test_all_match git diff-files deep/a &&
     +
     +	# test wildcard
     +	test_all_match git diff-files "deep/*"
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
     +	EOF
     +
     +	# Add file to the index but outside of cone for sparse-checkout cases.
    -+	# Add file to the index without sparse-checkout cases to ensure all have 
    ++	# Add file to the index without sparse-checkout cases to ensure all have
     +	# same output.
     +	run_on_all mkdir -p folder1 &&
     +	run_on_all cp a folder1/a &&
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
     +	run_on_all ../edit-contents folder1/a &&
     +	test_all_match git diff-files &&
     +	test_all_match git diff-files folder1/a &&
    -+	test_all_match git diff-files "folder*/a" 
    ++	test_all_match git diff-files "folder*/a"
     +'
     +
      test_done
2:  b44384ac94 ! 2:  a2454befa0 diff-files: integrate with sparse index
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'index.sparse disa
      '
      
     -ensure_not_expanded () {
    -+ensure_index_state () {
    -+	local expected_expansion="$1"
    -+	shift
    -+
    ++run_sparse_index_trace2 () {
      	rm -f trace2.txt &&
      	if test -z "$WITHOUT_UNTRACKED_TXT"
      	then
     @@ t/t1092-sparse-checkout-compatibility.sh: ensure_not_expanded () {
    + 			git -C sparse-index "$@" \
      			>sparse-index-out \
      			2>sparse-index-error || return 1
    - 	fi &&
    --	test_region ! index ensure_full_index trace2.txt
    -+
    -+	if [ "$expected_expansion" = "expanded" ]
    -+	then
    -+		test_region index ensure_full_index trace2.txt
    -+	else
    -+		test_region ! index ensure_full_index trace2.txt
    +-	fi &&
     +	fi
     +}
     +
     +ensure_expanded () {
    -+	ensure_index_state "expanded" "$@"
    ++	run_sparse_index_trace2 "$@" &&
    ++	test_region index ensure_full_index trace2.txt
     +}
     +
     +ensure_not_expanded () {
    -+	ensure_index_state "not_expanded" "$@"
    ++	run_sparse_index_trace2 "$@" &&
    + 	test_region ! index ensure_full_index trace2.txt
      }
      
    - test_expect_success 'sparse-index is not expanded' '
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with pathspec outside sparse definition' '
    - 	test_all_match git diff-files "folder*/a" 
    + 	test_all_match git diff-files "folder*/a"
      '
      
     +test_expect_success 'sparse index is not expanded: diff-files' '
-- 
2.39.0

[PATCH v9 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin with the sparse index
feature, add tests to t1092-sparse-checkout-compatibility.sh to ensure
it currently works with sparse-checkout and will still work with sparse
index after that integration.

When adding tests against a sparse-checkout definition, we test two
modes: all changes are within the sparse-checkout cone and some changes
are outside the sparse-checkout cone.

In order to have staged changes outside of the sparse-checkout cone,
make a directory called 'folder1' and copy `a` into 'folder1/a'.
'folder1/a' is identical to `a` in the base commit. These make
'folder1/a' in the index, while leaving it outside of the
sparse-checkout definition. Test 'folder1/a'being present on-disk
without modifications, then change content inside 'folder1/a' in order
to test 'folder1/a' being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 0c784813f1..053435bb0c 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2108,4 +2108,50 @@ test_expect_success 'sparse-index is not expanded: write-tree' '
 	ensure_not_expanded write-tree
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files &&
+
+	test_all_match git diff-files deep/a &&
+
+	# test wildcard
+	test_all_match git diff-files "deep/*"
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	test_sparse_match test_must_fail git diff-files folder2/a &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# Add file to the index but outside of cone for sparse-checkout cases.
+	# Add file to the index without sparse-checkout cases to ensure all have
+	# same output.
+	run_on_all mkdir -p folder1 &&
+	run_on_all cp a folder1/a &&
+
+	# file present on-disk without modifications
+	# use `--stat` to ignore file creation time differences in
+	# unrefreshed index
+	test_all_match git diff-files --stat &&
+	test_all_match git diff-files --stat folder1/a &&
+	test_all_match git diff-files --stat "folder*/a" &&
+
+	# file present on-disk with modifications
+	run_on_all ../edit-contents folder1/a &&
+	test_all_match git diff-files &&
+	test_all_match git diff-files folder1/a &&
+	test_all_match git diff-files "folder*/a"
+'
+
 test_done
-- 
2.39.0

[PATCH v9 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Remove full index requirement for `git diff-files`. Refactor the
ensure_expanded and ensure_not_expanded functions by introducing a
common helper function, ensure_index_state. Add test to ensure the index
is no expanded in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  4 ++++
 t/perf/p2000-sparse-operations.sh        |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++++++++++++++++--
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..360464e6ef 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 60d1de0662..29165b3493 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -129,5 +129,7 @@ test_perf_on_all git grep --cached bogus -- "f2/f1/f1/*"
 test_perf_on_all git write-tree
 test_perf_on_all git describe --dirty
 test_perf_on_all 'echo >>new && git describe --dirty'
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 053435bb0c..333822f322 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1377,7 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '
 	! test_region index ensure_full_index trace2.txt
 '
 
-ensure_not_expanded () {
+run_sparse_index_trace2 () {
 	rm -f trace2.txt &&
 	if test -z "$WITHOUT_UNTRACKED_TXT"
 	then
@@ -1397,7 +1397,16 @@ ensure_not_expanded () {
 			git -C sparse-index "$@" \
 			>sparse-index-out \
 			2>sparse-index-error || return 1
-	fi &&
+	fi
+}
+
+ensure_expanded () {
+	run_sparse_index_trace2 "$@" &&
+	test_region index ensure_full_index trace2.txt
+}
+
+ensure_not_expanded () {
+	run_sparse_index_trace2 "$@" &&
 	test_region ! index ensure_full_index trace2.txt
 }
 
@@ -2154,4 +2163,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
 	test_all_match git diff-files "folder*/a"
 '
 
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files &&
+	ensure_not_expanded diff-files deep/a &&
+	ensure_not_expanded diff-files "deep/*"
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v9 1/2] t1092: add tests for `git diff-files`

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> +test_expect_success 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&
> +
> +	test_sparse_match test_must_fail git diff-files folder2/a &&

In "sparse" directories at this point of test, "folder2" is outside
the cone(s) of interest and is not instantiated.  The reason why
the command fails is because the command line parsing that is
generic to all users of the revision machinery requires you to have
a disambiguating double-dash before such a pathspec that tries to
match a path that does not exist in the working tree and is not
specific to "diff-files".

I wonder how interesting and useful this test is.  Without
accompanying test that uses disambiguating double-dash properly,
e.g. "git diff-files -- folder2", I doubt it is very much useful.

> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	# Add file to the index but outside of cone for sparse-checkout cases.
> +	# Add file to the index without sparse-checkout cases to ensure all have
> +	# same output.
> +	run_on_all mkdir -p folder1 &&
> +	run_on_all cp a folder1/a &&

Now, "folder1" also has not been instantiated in sparse ones while
the full one of course has it, so "-p" in "mkdir -p" makes sense.
After these commands, all three will share the same "folder1/a".

> +	# file present on-disk without modifications
> +	# use `--stat` to ignore file creation time differences in
> +	# unrefreshed index
> +	test_all_match git diff-files --stat &&
> +	test_all_match git diff-files --stat folder1/a &&
> +	test_all_match git diff-files --stat "folder*/a" &&

Because in all three repositories, "folder1/a" exists in the working
tree, the "you need to disambiguate" error like the first test
(whose utility I questioned) would not trigger.

What does this demonstrate, though?  That instantiating a file on
the working tree, even outside the cone(s) of interest in a sparsely
checked out working tree, makes it part of the interesting set
automatically?  As there is no difference between the indexed
contents and what is in the working tree, we cannot tell from this
test if that is the case (not a complaint, just an observation).

But ...

> +	# file present on-disk with modifications
> +	run_on_all ../edit-contents folder1/a &&
> +	test_all_match git diff-files &&
> +	test_all_match git diff-files folder1/a &&
> +	test_all_match git diff-files "folder*/a"

... it is shown by doing the same test with modified contents?

For consistency with the earlier "the same contents" test, we should
use "--stat" here, too.  Or even "--stat -p".

Alternatively, we could refresh the index before running diff-files
(here and also before the earlier "the same contents" test), I
guess.

> +'
> +
>  test_done

Thanks.

Re: [PATCH v9 1/2] t1092: add tests for `git diff-files`

[Victoria Dye]

Junio C Hamano wrote:
> Shuqi Liang <cheskaqiqi@gmail.com> writes:
> 
>> +test_expect_success 'diff-files with pathspec outside sparse definition' '
>> +	init_repos &&
>> +
>> +	test_sparse_match test_must_fail git diff-files folder2/a &&
> 
> In "sparse" directories at this point of test, "folder2" is outside
> the cone(s) of interest and is not instantiated.  The reason why
> the command fails is because the command line parsing that is
> generic to all users of the revision machinery requires you to have
> a disambiguating double-dash before such a pathspec that tries to
> match a path that does not exist in the working tree and is not
> specific to "diff-files".
> 
> I wonder how interesting and useful this test is.  Without
> accompanying test that uses disambiguating double-dash properly,
> e.g. "git diff-files -- folder2", I doubt it is very much useful.

I agree, this test isn't helpful as-is. With the '--', 'diff-files'
shouldn't fail, so in the interest of avoiding unexpected regressions in the
future (masked by 'test_must_fail'), I think this test should be updated as
you described. 

>> +	# file present on-disk without modifications
>> +	# use `--stat` to ignore file creation time differences in
>> +	# unrefreshed index
>> +	test_all_match git diff-files --stat &&
>> +	test_all_match git diff-files --stat folder1/a &&
>> +	test_all_match git diff-files --stat "folder*/a" &&
> 
> Because in all three repositories, "folder1/a" exists in the working
> tree, the "you need to disambiguate" error like the first test
> (whose utility I questioned) would not trigger.
> 
> What does this demonstrate, though?  That instantiating a file on
> the working tree, even outside the cone(s) of interest in a sparsely
> checked out working tree, makes it part of the interesting set
> automatically?  As there is no difference between the indexed
> contents and what is in the working tree, we cannot tell from this
> test if that is the case (not a complaint, just an observation).

This was meant [1] to check whether there are any issues expanding the index
(specifically, the 'folder1/' sparse directory) before comparing to the 
now-on-disk 'folder1/a'. 

However...

[1] https://lore.kernel.org/git/b537d855-edb7-4f67-de08-d651868247a5@github.com/

> 
> But ...
> 
>> +	# file present on-disk with modifications
>> +	run_on_all ../edit-contents folder1/a &&
>> +	test_all_match git diff-files &&
>> +	test_all_match git diff-files folder1/a &&
>> +	test_all_match git diff-files "folder*/a"
> 
> ... it is shown by doing the same test with modified contents?
> 
> For consistency with the earlier "the same contents" test, we should
> use "--stat" here, too.  Or even "--stat -p".
> 
> Alternatively, we could refresh the index before running diff-files
> (here and also before the earlier "the same contents" test), I
> guess.

...to your point, we probably don't need both the "unmodified folder1/a
diff-files" *and* "modified folder1/a diff-files" tests. In fact, the empty
output of "unmodified folder1/a" could be caused by either "this file is
unmodified" or "this file isn't in the index", so the test might pass even
if there's an issue with index expansion. That isn't a problem in the
"modified folder1/a" case, since we're expecting to see - and comparing the
contents of - a diff.

I think we can drop the 'diff-files --stat' tests and go straight to the
'run_on_all ../edit-contents folder1/a'. Adding '--' here to disambiguate
the pathspecs might be nice as well.

> 
>> +'
>> +
>>  test_done
> 
> Thanks.

Thanks for the detailed review, apologies for missing these issues earlier.

[PATCH v10 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Changes since v9:

* Replace the unhelpful test with a double-dash case to prevent
regressions. 

* Remove the unmodified test because its empty output could pass
even with an index expansion issue.

* Update the relevant commit message.

* Did not add " -- " in modified test as suggested sine 
"folder1/a" exists in the working tree


Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  4 ++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 66 +++++++++++++++++++++++-
 3 files changed, 70 insertions(+), 2 deletions(-)

Range-diff against v9:
1:  d78513af83 ! 1:  3b284bdf3b t1092: add tests for `git diff-files`
    @@ Commit message
         make a directory called 'folder1' and copy `a` into 'folder1/a'.
         'folder1/a' is identical to `a` in the base commit. These make
         'folder1/a' in the index, while leaving it outside of the
    -    sparse-checkout definition. Test 'folder1/a'being present on-disk
    -    without modifications, then change content inside 'folder1/a' in order
    +    sparse-checkout definition. Change content inside 'folder1/a' in order
         to test 'folder1/a' being present on-disk with modifications.
     
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
     +test_expect_success 'diff-files with pathspec outside sparse definition' '
     +	init_repos &&
     +
    -+	test_sparse_match test_must_fail git diff-files folder2/a &&
    ++	test_sparse_match git diff-files -- folder2/a &&
     +
     +	write_script edit-contents <<-\EOF &&
     +	echo text >>"$1"
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
     +	run_on_all mkdir -p folder1 &&
     +	run_on_all cp a folder1/a &&
     +
    -+	# file present on-disk without modifications
    -+	# use `--stat` to ignore file creation time differences in
    -+	# unrefreshed index
    -+	test_all_match git diff-files --stat &&
    -+	test_all_match git diff-files --stat folder1/a &&
    -+	test_all_match git diff-files --stat "folder*/a" &&
    -+
     +	# file present on-disk with modifications
     +	run_on_all ../edit-contents folder1/a &&
     +	test_all_match git diff-files &&
2:  a2454befa0 = 2:  15472db302 diff-files: integrate with sparse index
-- 
2.39.0

[PATCH v10 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin with the sparse index
feature, add tests to t1092-sparse-checkout-compatibility.sh to ensure
it currently works with sparse-checkout and will still work with sparse
index after that integration.

When adding tests against a sparse-checkout definition, we test two
modes: all changes are within the sparse-checkout cone and some changes
are outside the sparse-checkout cone.

In order to have staged changes outside of the sparse-checkout cone,
make a directory called 'folder1' and copy `a` into 'folder1/a'.
'folder1/a' is identical to `a` in the base commit. These make
'folder1/a' in the index, while leaving it outside of the
sparse-checkout definition. Change content inside 'folder1/a' in order
to test 'folder1/a' being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 39 ++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 0c784813f1..eddae7ee08 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2108,4 +2108,43 @@ test_expect_success 'sparse-index is not expanded: write-tree' '
 	ensure_not_expanded write-tree
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files &&
+
+	test_all_match git diff-files deep/a &&
+
+	# test wildcard
+	test_all_match git diff-files "deep/*"
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	test_sparse_match git diff-files -- folder2/a &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# Add file to the index but outside of cone for sparse-checkout cases.
+	# Add file to the index without sparse-checkout cases to ensure all have
+	# same output.
+	run_on_all mkdir -p folder1 &&
+	run_on_all cp a folder1/a &&
+
+	# file present on-disk with modifications
+	run_on_all ../edit-contents folder1/a &&
+	test_all_match git diff-files &&
+	test_all_match git diff-files folder1/a &&
+	test_all_match git diff-files "folder*/a"
+'
+
 test_done
-- 
2.39.0

[PATCH v10 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Remove full index requirement for `git diff-files`. Refactor the
ensure_expanded and ensure_not_expanded functions by introducing a
common helper function, ensure_index_state. Add test to ensure the index
is no expanded in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  4 ++++
 t/perf/p2000-sparse-operations.sh        |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++++++++++++++++--
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..360464e6ef 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 60d1de0662..29165b3493 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -129,5 +129,7 @@ test_perf_on_all git grep --cached bogus -- "f2/f1/f1/*"
 test_perf_on_all git write-tree
 test_perf_on_all git describe --dirty
 test_perf_on_all 'echo >>new && git describe --dirty'
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index eddae7ee08..ed9bf466c2 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1377,7 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '
 	! test_region index ensure_full_index trace2.txt
 '
 
-ensure_not_expanded () {
+run_sparse_index_trace2 () {
 	rm -f trace2.txt &&
 	if test -z "$WITHOUT_UNTRACKED_TXT"
 	then
@@ -1397,7 +1397,16 @@ ensure_not_expanded () {
 			git -C sparse-index "$@" \
 			>sparse-index-out \
 			2>sparse-index-error || return 1
-	fi &&
+	fi
+}
+
+ensure_expanded () {
+	run_sparse_index_trace2 "$@" &&
+	test_region index ensure_full_index trace2.txt
+}
+
+ensure_not_expanded () {
+	run_sparse_index_trace2 "$@" &&
 	test_region ! index ensure_full_index trace2.txt
 }
 
@@ -2147,4 +2156,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
 	test_all_match git diff-files "folder*/a"
 '
 
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files &&
+	ensure_not_expanded diff-files deep/a &&
+	ensure_not_expanded diff-files "deep/*"
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v10 1/2] t1092: add tests for `git diff-files`

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> +	test_sparse_match git diff-files -- folder2/a &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF

> +	# Add file to the index but outside of cone for sparse-checkout cases.
> +	# Add file to the index without sparse-checkout cases to ensure all have
> +	# same output.

Are these two sentences supposed to explain the following two
commands that are run in all three repositories?  As far as I can
tell, no command in this test adds anything to the index.  Perhaps
it is a leftover/stale comment from previous rounds or something?

> +	run_on_all mkdir -p folder1 &&
> +	run_on_all cp a folder1/a &&
> +
> +	# file present on-disk with modifications
> +	run_on_all ../edit-contents folder1/a &&

With the above three commands taken together, we have made folder1/a
on the working tree different from what is in the index, so we can
expect to see differences between the index and the working tree
files.  "# file present on-disk with modifications" is a good way to
summarize a half of what we are trying to achieve, with the other
half being that we try to do that to a path outside the cone of
interest.

So, perhaps get rid of this comment between the step 2 and 3 of the
preparation, and rewrite the comment before the step 1 (i.e. "mkdir
-p") of the preparation to explain the whole thing, perhaps like:

	# The directory "folder1" is outside the cone of interest
	# and may not exist in the sparse checkout repositories.
        # Create it as needed, add file "folder1/a" there with
	# contents that is different from the staged version.

to explain what scenario these three run_on_all commands are trying
to create?

> +	test_all_match git diff-files &&
> +	test_all_match git diff-files folder1/a &&
> +	test_all_match git diff-files "folder*/a"

I think Victoria suggested to use the double-dash disambiguators for
these tests, and it may not be a bad idea to do so, i.e.

	test_all_match git diff-files &&
	test_all_match git diff-files -- folder1/a &&
	test_all_match git diff-files -- folder\*/a

> +'
> +
>  test_done

Thanks.

[PATCH v11 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

Changes since v10:

* Rewrite the comment before the "mkdir -p"

* Add " -- " in modified test to prevent regressions.


Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  4 ++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 66 +++++++++++++++++++++++-
 3 files changed, 70 insertions(+), 2 deletions(-)

Range-diff against v10:
1:  3b284bdf3b ! 1:  3e96a0c136 t1092: add tests for `git diff-files`
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
     +	echo text >>"$1"
     +	EOF
     +
    -+	# Add file to the index but outside of cone for sparse-checkout cases.
    -+	# Add file to the index without sparse-checkout cases to ensure all have
    -+	# same output.
    ++	# The directory "folder1" is outside the cone of interest
    ++	# and may not exist in the sparse checkout repositories.
    ++	# Create it as needed, add file "folder1/a" there with
    ++	# contents that is different from the staged version.
     +	run_on_all mkdir -p folder1 &&
     +	run_on_all cp a folder1/a &&
     +
    -+	# file present on-disk with modifications
     +	run_on_all ../edit-contents folder1/a &&
     +	test_all_match git diff-files &&
    -+	test_all_match git diff-files folder1/a &&
    -+	test_all_match git diff-files "folder*/a"
    ++	test_all_match git diff-files -- folder1/a &&
    ++	test_all_match git diff-files -- "folder*/a"
     +'
     +
      test_done
2:  15472db302 ! 2:  2c53fedf08 diff-files: integrate with sparse index
    @@ t/t1092-sparse-checkout-compatibility.sh: ensure_not_expanded () {
      }
      
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with pathspec outside sparse definition' '
    - 	test_all_match git diff-files "folder*/a"
    + 	test_all_match git diff-files -- "folder*/a"
      '
      
     +test_expect_success 'sparse index is not expanded: diff-files' '
-- 
2.39.0

[PATCH v11 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin with the sparse index
feature, add tests to t1092-sparse-checkout-compatibility.sh to ensure
it currently works with sparse-checkout and will still work with sparse
index after that integration.

When adding tests against a sparse-checkout definition, we test two
modes: all changes are within the sparse-checkout cone and some changes
are outside the sparse-checkout cone.

In order to have staged changes outside of the sparse-checkout cone,
make a directory called 'folder1' and copy `a` into 'folder1/a'.
'folder1/a' is identical to `a` in the base commit. These make
'folder1/a' in the index, while leaving it outside of the
sparse-checkout definition. Change content inside 'folder1/a' in order
to test 'folder1/a' being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 39 ++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 0c784813f1..efc709afa5 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2108,4 +2108,43 @@ test_expect_success 'sparse-index is not expanded: write-tree' '
 	ensure_not_expanded write-tree
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files &&
+
+	test_all_match git diff-files deep/a &&
+
+	# test wildcard
+	test_all_match git diff-files "deep/*"
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	test_sparse_match git diff-files -- folder2/a &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# The directory "folder1" is outside the cone of interest
+	# and may not exist in the sparse checkout repositories.
+	# Create it as needed, add file "folder1/a" there with
+	# contents that is different from the staged version.
+	run_on_all mkdir -p folder1 &&
+	run_on_all cp a folder1/a &&
+
+	run_on_all ../edit-contents folder1/a &&
+	test_all_match git diff-files &&
+	test_all_match git diff-files -- folder1/a &&
+	test_all_match git diff-files -- "folder*/a"
+'
+
 test_done
-- 
2.39.0

[PATCH v11 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Remove full index requirement for `git diff-files`. Refactor the
ensure_expanded and ensure_not_expanded functions by introducing a
common helper function, ensure_index_state. Add test to ensure the index
is no expanded in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  4 ++++
 t/perf/p2000-sparse-operations.sh        |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++++++++++++++++--
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..360464e6ef 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 60d1de0662..29165b3493 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -129,5 +129,7 @@ test_perf_on_all git grep --cached bogus -- "f2/f1/f1/*"
 test_perf_on_all git write-tree
 test_perf_on_all git describe --dirty
 test_perf_on_all 'echo >>new && git describe --dirty'
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index efc709afa5..176153738a 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1377,7 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '
 	! test_region index ensure_full_index trace2.txt
 '
 
-ensure_not_expanded () {
+run_sparse_index_trace2 () {
 	rm -f trace2.txt &&
 	if test -z "$WITHOUT_UNTRACKED_TXT"
 	then
@@ -1397,7 +1397,16 @@ ensure_not_expanded () {
 			git -C sparse-index "$@" \
 			>sparse-index-out \
 			2>sparse-index-error || return 1
-	fi &&
+	fi
+}
+
+ensure_expanded () {
+	run_sparse_index_trace2 "$@" &&
+	test_region index ensure_full_index trace2.txt
+}
+
+ensure_not_expanded () {
+	run_sparse_index_trace2 "$@" &&
 	test_region ! index ensure_full_index trace2.txt
 }
 
@@ -2147,4 +2156,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
 	test_all_match git diff-files -- "folder*/a"
 '
 
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files &&
+	ensure_not_expanded diff-files deep/a &&
+	ensure_not_expanded diff-files "deep/*"
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v11 1/2] t1092: add tests for `git diff-files`

[Victoria Dye]

Shuqi Liang wrote:
> +test_expect_success 'diff-files with pathspec inside sparse definition' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	test_all_match git diff-files &&
> +
> +	test_all_match git diff-files deep/a &&
> +
> +	# test wildcard
> +	test_all_match git diff-files "deep/*"

You added the '--' separator below, but not here. Was that intentional, or
should these have it as well? It doesn't make much of a practical difference
in this case, but it would be nice to remain consistent across all tests of
'diff-files' that you're adding. 

The same goes for some of the tests in patch 2 [1] ('sparse index is not
expanded: diff-files' and the perf tests).

[1] https://lore.kernel.org/git/20230508184652.4283-3-cheskaqiqi@gmail.com/

> +'
> +
> +test_expect_success 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&
> +
> +	test_sparse_match git diff-files -- folder2/a &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	# The directory "folder1" is outside the cone of interest
> +	# and may not exist in the sparse checkout repositories.
> +	# Create it as needed, add file "folder1/a" there with
> +	# contents that is different from the staged version.

nit: 'folder1/' *definitely* won't be present in the sparse-checkout
repositories, so "will not" would be more accurate than "may not".
Otherwise, this comment is clearer than before & better explains what's
going on here.

> +	run_on_all mkdir -p folder1 &&
> +	run_on_all cp a folder1/a &&
> +
> +	run_on_all ../edit-contents folder1/a &&
> +	test_all_match git diff-files &&
> +	test_all_match git diff-files -- folder1/a &&
> +	test_all_match git diff-files -- "folder*/a"
> +'
> +
>  test_done

[PATCH v12 0/2] diff-files: integrate with sparse index

[Shuqi Liang]

* Add the '--' in all test to remain consistent.

* Change 'may not' to 'will not'.


Shuqi Liang (2):
  t1092: add tests for `git diff-files`
  diff-files: integrate with sparse index

 builtin/diff-files.c                     |  4 ++
 t/perf/p2000-sparse-operations.sh        |  2 +
 t/t1092-sparse-checkout-compatibility.sh | 66 +++++++++++++++++++++++-
 3 files changed, 70 insertions(+), 2 deletions(-)

Range-diff against v11:
1:  3e96a0c136 ! 1:  eb74730813 t1092: add tests for `git diff-files`
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
     +
     +	test_all_match git diff-files &&
     +
    -+	test_all_match git diff-files deep/a &&
    ++	test_all_match git diff-files -- deep/a &&
     +
     +	# test wildcard
    -+	test_all_match git diff-files "deep/*"
    ++	test_all_match git diff-files -- "deep/*"
     +'
     +
     +test_expect_success 'diff-files with pathspec outside sparse definition' '
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
     +	EOF
     +
     +	# The directory "folder1" is outside the cone of interest
    -+	# and may not exist in the sparse checkout repositories.
    ++	# and will not exist in the sparse checkout repositories.
     +	# Create it as needed, add file "folder1/a" there with
     +	# contents that is different from the staged version.
     +	run_on_all mkdir -p folder1 &&
2:  2c53fedf08 ! 2:  11affce5b7 diff-files: integrate with sparse index
    @@ Commit message
         diff-files' and a ~97% execution time reduction for 'git diff-files'
         for a file using a sparse index:
     
    -    Test                                           before  after
    -    -----------------------------------------------------------------
    -    2000.78: git diff-files (full-v3)              0.09    0.08 -11.1%
    -    2000.79: git diff-files (full-v4)              0.09    0.09 +0.0%
    -    2000.80: git diff-files (sparse-v3)            0.52    0.02 -96.2%
    -    2000.81: git diff-files (sparse-v4)            0.51    0.02 -96.1%
    -    2000.82: git diff-files f2/f4/a (full-v3)      0.06    0.07 +16.7%
    -    2000.83: git diff-files f2/f4/a (full-v4)      0.08    0.08 +0.0%
    -    2000.84: git diff-files f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
    -    2000.85: git diff-files f2/f4/a (sparse-v4)    0.51    0.02 -96.1%
    +    Test                                               before  after
    +    -----------------------------------------------------------------------
    +    2000.94: git diff-files (full-v3)                  0.09    0.08 -11.1%
    +    2000.95: git diff-files (full-v4)                  0.09    0.09 +0.0%
    +    2000.96: git diff-files (sparse-v3)                0.52    0.02 -96.2%
    +    2000.97: git diff-files (sparse-v4)                0.51    0.02 -96.1%
    +    2000.98: git diff-files -- f2/f4/a (full-v3)       0.06    0.07 +16.7%
    +    2000.99: git diff-files -- f2/f4/a (full-v4)       0.08    0.08 +0.0%
    +    2000.100: git diff-files -- f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
    +    2000.101: git diff-files -- f2/f4/a (sparse-v4)    0.51    0.02 -96.1%
     
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
    @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git grep --cached bogus -- "
      test_perf_on_all git describe --dirty
      test_perf_on_all 'echo >>new && git describe --dirty'
     +test_perf_on_all git diff-files
    -+test_perf_on_all git diff-files $SPARSE_CONE/a
    ++test_perf_on_all git diff-files -- $SPARSE_CONE/a
      
      test_done
     
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with p
     +	run_on_all ../edit-contents deep/a &&
     +
     +	ensure_not_expanded diff-files &&
    -+	ensure_not_expanded diff-files deep/a &&
    -+	ensure_not_expanded diff-files "deep/*"
    ++	ensure_not_expanded diff-files -- deep/a &&
    ++	ensure_not_expanded diff-files -- "deep/*"
     +'
     +
      test_done
-- 
2.39.0

[PATCH v12 1/2] t1092: add tests for `git diff-files`

[Shuqi Liang]

Before integrating the 'git diff-files' builtin with the sparse index
feature, add tests to t1092-sparse-checkout-compatibility.sh to ensure
it currently works with sparse-checkout and will still work with sparse
index after that integration.

When adding tests against a sparse-checkout definition, we test two
modes: all changes are within the sparse-checkout cone and some changes
are outside the sparse-checkout cone.

In order to have staged changes outside of the sparse-checkout cone,
make a directory called 'folder1' and copy `a` into 'folder1/a'.
'folder1/a' is identical to `a` in the base commit. These make
'folder1/a' in the index, while leaving it outside of the
sparse-checkout definition. Change content inside 'folder1/a' in order
to test 'folder1/a' being present on-disk with modifications.

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 39 ++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 0c784813f1..b06b522030 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2108,4 +2108,43 @@ test_expect_success 'sparse-index is not expanded: write-tree' '
 	ensure_not_expanded write-tree
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files &&
+
+	test_all_match git diff-files -- deep/a &&
+
+	# test wildcard
+	test_all_match git diff-files -- "deep/*"
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	test_sparse_match git diff-files -- folder2/a &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# The directory "folder1" is outside the cone of interest
+	# and will not exist in the sparse checkout repositories.
+	# Create it as needed, add file "folder1/a" there with
+	# contents that is different from the staged version.
+	run_on_all mkdir -p folder1 &&
+	run_on_all cp a folder1/a &&
+
+	run_on_all ../edit-contents folder1/a &&
+	test_all_match git diff-files &&
+	test_all_match git diff-files -- folder1/a &&
+	test_all_match git diff-files -- "folder*/a"
+'
+
 test_done
-- 
2.39.0

[PATCH v12 2/2] diff-files: integrate with sparse index

[Shuqi Liang]

Remove full index requirement for `git diff-files`. Refactor the
ensure_expanded and ensure_not_expanded functions by introducing a
common helper function, ensure_index_state. Add test to ensure the index
is no expanded in `git diff-files`.

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
for a file using a sparse index:

Test                                               before  after
-----------------------------------------------------------------------
2000.94: git diff-files (full-v3)                  0.09    0.08 -11.1%
2000.95: git diff-files (full-v4)                  0.09    0.09 +0.0%
2000.96: git diff-files (sparse-v3)                0.52    0.02 -96.2%
2000.97: git diff-files (sparse-v4)                0.51    0.02 -96.1%
2000.98: git diff-files -- f2/f4/a (full-v3)       0.06    0.07 +16.7%
2000.99: git diff-files -- f2/f4/a (full-v4)       0.08    0.08 +0.0%
2000.100: git diff-files -- f2/f4/a (sparse-v3)    0.46    0.01 -97.8%
2000.101: git diff-files -- f2/f4/a (sparse-v4)    0.51    0.02 -96.1%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/diff-files.c                     |  4 ++++
 t/perf/p2000-sparse-operations.sh        |  2 ++
 t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++++++++++++++++--
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index dc991f753b..360464e6ef 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 		usage(diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
+
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.abbrev = 0;
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 60d1de0662..901cc493ef 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -129,5 +129,7 @@ test_perf_on_all git grep --cached bogus -- "f2/f1/f1/*"
 test_perf_on_all git write-tree
 test_perf_on_all git describe --dirty
 test_perf_on_all 'echo >>new && git describe --dirty'
+test_perf_on_all git diff-files
+test_perf_on_all git diff-files -- $SPARSE_CONE/a
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index b06b522030..e58bfbfcb4 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1377,7 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '
 	! test_region index ensure_full_index trace2.txt
 '
 
-ensure_not_expanded () {
+run_sparse_index_trace2 () {
 	rm -f trace2.txt &&
 	if test -z "$WITHOUT_UNTRACKED_TXT"
 	then
@@ -1397,7 +1397,16 @@ ensure_not_expanded () {
 			git -C sparse-index "$@" \
 			>sparse-index-out \
 			2>sparse-index-error || return 1
-	fi &&
+	fi
+}
+
+ensure_expanded () {
+	run_sparse_index_trace2 "$@" &&
+	test_region index ensure_full_index trace2.txt
+}
+
+ensure_not_expanded () {
+	run_sparse_index_trace2 "$@" &&
 	test_region ! index ensure_full_index trace2.txt
 }
 
@@ -2147,4 +2156,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '
 	test_all_match git diff-files -- "folder*/a"
 '
 
+test_expect_success 'sparse index is not expanded: diff-files' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	ensure_not_expanded diff-files &&
+	ensure_not_expanded diff-files -- deep/a &&
+	ensure_not_expanded diff-files -- "deep/*"
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v12 0/2] diff-files: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> * Add the '--' in all test to remain consistent.
> 
> * Change 'may not' to 'will not'.
> 
> 
> Shuqi Liang (2):
>   t1092: add tests for `git diff-files`
>   diff-files: integrate with sparse index
> 
>  builtin/diff-files.c                     |  4 ++
>  t/perf/p2000-sparse-operations.sh        |  2 +
>  t/t1092-sparse-checkout-compatibility.sh | 66 +++++++++++++++++++++++-
>  3 files changed, 70 insertions(+), 2 deletions(-)
> 

This iteration looks good to me. Thanks for keeping up with the reviews and
getting this to a polished state!

Re: [PATCH v12 0/2] diff-files: integrate with sparse index

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

> This iteration looks good to me. Thanks for keeping up with the reviews and
> getting this to a polished state!

Thanks, both.  Let's merge the topic to 'next'.