* [PATCH v6 1/2] t1092: add tests for `git diff-files`
2023-03-20 20:52 ` [RFC PATCH v6 0/2] " Shuqi Liang
@ 2023-03-20 20:52 ` Shuqi Liang
2023-03-21 21:21 ` Victoria Dye
2023-03-20 20:52 ` [PATCH v6 2/2] diff-files: integrate with sparse index Shuqi Liang
` (2 subsequent siblings)
3 siblings, 1 reply; 73+ messages in thread
From: Shuqi Liang @ 2023-03-20 20:52 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread* Re: [PATCH v6 1/2] t1092: add tests for `git diff-files`
2023-03-20 20:52 ` [PATCH v6 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-03-21 21:21 ` Victoria Dye
2023-03-21 21:25 ` Junio C Hamano
0 siblings, 1 reply; 73+ messages in thread
From: Victoria Dye @ 2023-03-21 21:21 UTC (permalink / raw)
To: Shuqi Liang, git; +Cc: gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH v6 1/2] t1092: add tests for `git diff-files`
2023-03-21 21:21 ` Victoria Dye
@ 2023-03-21 21:25 ` Junio C Hamano
2023-03-21 22:19 ` Victoria Dye
0 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2023-03-21 21:25 UTC (permalink / raw)
To: Victoria Dye; +Cc: Shuqi Liang, git, derrickstolee
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"?
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v6 1/2] t1092: add tests for `git diff-files`
2023-03-21 21:25 ` Junio C Hamano
@ 2023-03-21 22:19 ` Victoria Dye
0 siblings, 0 replies; 73+ messages in thread
From: Victoria Dye @ 2023-03-21 22:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shuqi Liang, git, derrickstolee
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.
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v6 2/2] diff-files: integrate with sparse index
2023-03-20 20:52 ` [RFC PATCH v6 0/2] " Shuqi Liang
2023-03-20 20:52 ` [PATCH v6 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-03-20 20:52 ` Shuqi Liang
2023-03-21 22:34 ` Victoria Dye
2023-03-21 18:38 ` [RFC PATCH v6 0/2] " Victoria Dye
2023-03-22 16:18 ` [PATCH v7 " Shuqi Liang
3 siblings, 1 reply; 73+ messages in thread
From: Shuqi Liang @ 2023-03-20 20:52 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH v6 2/2] diff-files: integrate with sparse index
2023-03-20 20:52 ` [PATCH v6 2/2] diff-files: integrate with sparse index Shuqi Liang
@ 2023-03-21 22:34 ` Victoria Dye
0 siblings, 0 replies; 73+ messages in thread
From: Victoria Dye @ 2023-03-21 22:34 UTC (permalink / raw)
To: Shuqi Liang, git; +Cc: gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [RFC PATCH v6 0/2] diff-files: integrate with sparse index
2023-03-20 20:52 ` [RFC PATCH v6 0/2] " Shuqi Liang
2023-03-20 20:52 ` [PATCH v6 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-03-20 20:52 ` [PATCH v6 2/2] diff-files: integrate with sparse index Shuqi Liang
@ 2023-03-21 18:38 ` Victoria Dye
2023-03-22 16:18 ` [PATCH v7 " Shuqi Liang
3 siblings, 0 replies; 73+ messages in thread
From: Victoria Dye @ 2023-03-21 18:38 UTC (permalink / raw)
To: Shuqi Liang, git; +Cc: gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH v7 0/2] diff-files: integrate with sparse index
2023-03-20 20:52 ` [RFC PATCH v6 0/2] " Shuqi Liang
` (2 preceding siblings ...)
2023-03-21 18:38 ` [RFC PATCH v6 0/2] " Victoria Dye
@ 2023-03-22 16:18 ` Shuqi Liang
2023-03-22 16:18 ` [PATCH v7 1/2] t1092: add tests for `git diff-files` Shuqi Liang
` (4 more replies)
3 siblings, 5 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-03-22 16:18 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH v7 1/2] t1092: add tests for `git diff-files`
2023-03-22 16:18 ` [PATCH v7 " Shuqi Liang
@ 2023-03-22 16:18 ` Shuqi Liang
2023-04-13 21:56 ` Victoria Dye
2023-03-22 16:18 ` [PATCH v7 2/2] diff-files: integrate with sparse index Shuqi Liang
` (3 subsequent siblings)
4 siblings, 1 reply; 73+ messages in thread
From: Shuqi Liang @ 2023-03-22 16:18 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread* Re: [PATCH v7 1/2] t1092: add tests for `git diff-files`
2023-03-22 16:18 ` [PATCH v7 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-04-13 21:56 ` Victoria Dye
0 siblings, 0 replies; 73+ messages in thread
From: Victoria Dye @ 2023-04-13 21:56 UTC (permalink / raw)
To: Shuqi Liang, git; +Cc: gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v7 2/2] diff-files: integrate with sparse index
2023-03-22 16:18 ` [PATCH v7 " Shuqi Liang
2023-03-22 16:18 ` [PATCH v7 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-03-22 16:18 ` Shuqi Liang
2023-04-13 21:54 ` Victoria Dye
2023-03-22 23:36 ` [PATCH v7 0/2] " Junio C Hamano
` (2 subsequent siblings)
4 siblings, 1 reply; 73+ messages in thread
From: Shuqi Liang @ 2023-03-22 16:18 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread* Re: [PATCH v7 2/2] diff-files: integrate with sparse index
2023-03-22 16:18 ` [PATCH v7 2/2] diff-files: integrate with sparse index Shuqi Liang
@ 2023-04-13 21:54 ` Victoria Dye
2023-04-20 4:50 ` Shuqi Liang
0 siblings, 1 reply; 73+ messages in thread
From: Victoria Dye @ 2023-04-13 21:54 UTC (permalink / raw)
To: Shuqi Liang, git; +Cc: gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH v7 2/2] diff-files: integrate with sparse index
2023-04-13 21:54 ` Victoria Dye
@ 2023-04-20 4:50 ` Shuqi Liang
2023-04-20 15:26 ` Victoria Dye
0 siblings, 1 reply; 73+ messages in thread
From: Shuqi Liang @ 2023-04-20 4:50 UTC (permalink / raw)
To: Victoria Dye, git, Junio C Hamano, Derrick Stolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH v7 2/2] diff-files: integrate with sparse index
2023-04-20 4:50 ` Shuqi Liang
@ 2023-04-20 15:26 ` Victoria Dye
2023-04-21 1:10 ` Shuqi Liang
0 siblings, 1 reply; 73+ messages in thread
From: Victoria Dye @ 2023-04-20 15:26 UTC (permalink / raw)
To: Shuqi Liang, git, Junio C Hamano, Derrick Stolee
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/
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v7 2/2] diff-files: integrate with sparse index
2023-04-20 15:26 ` Victoria Dye
@ 2023-04-21 1:10 ` Shuqi Liang
2023-04-21 21:26 ` Victoria Dye
0 siblings, 1 reply; 73+ messages in thread
From: Shuqi Liang @ 2023-04-21 1:10 UTC (permalink / raw)
To: Victoria Dye, Junio C Hamano, git, Derrick Stolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v7 2/2] diff-files: integrate with sparse index
2023-04-21 1:10 ` Shuqi Liang
@ 2023-04-21 21:26 ` Victoria Dye
2023-04-22 21:25 ` Shuqi Liang
0 siblings, 1 reply; 73+ messages in thread
From: Victoria Dye @ 2023-04-21 21:26 UTC (permalink / raw)
To: Shuqi Liang, Junio C Hamano, git, Derrick Stolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v7 2/2] diff-files: integrate with sparse index
2023-04-21 21:26 ` Victoria Dye
@ 2023-04-22 21:25 ` Shuqi Liang
0 siblings, 0 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-04-22 21:25 UTC (permalink / raw)
To: Victoria Dye, Junio C Hamano, git, Derrick Stolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v7 0/2] diff-files: integrate with sparse index
2023-03-22 16:18 ` [PATCH v7 " Shuqi Liang
2023-03-22 16:18 ` [PATCH v7 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-03-22 16:18 ` [PATCH v7 2/2] diff-files: integrate with sparse index Shuqi Liang
@ 2023-03-22 23:36 ` Junio C Hamano
2023-03-23 7:42 ` Shuqi Liang
2023-04-13 21:36 ` Junio C Hamano
2023-04-23 1:07 ` [PATCH v8 " Shuqi Liang
4 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2023-03-22 23:36 UTC (permalink / raw)
To: Shuqi Liang; +Cc: git, vdye, derrickstolee
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.
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v7 0/2] diff-files: integrate with sparse index
2023-03-22 23:36 ` [PATCH v7 0/2] " Junio C Hamano
@ 2023-03-23 7:42 ` Shuqi Liang
2023-03-23 16:03 ` Junio C Hamano
2023-03-23 17:25 ` Victoria Dye
0 siblings, 2 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-03-23 7:42 UTC (permalink / raw)
To: Junio C Hamano, git, Victoria Dye, Derrick Stolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v7 0/2] diff-files: integrate with sparse index
2023-03-23 7:42 ` Shuqi Liang
@ 2023-03-23 16:03 ` Junio C Hamano
2023-03-23 23:59 ` Shuqi Liang
2023-03-23 17:25 ` Victoria Dye
1 sibling, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2023-03-23 16:03 UTC (permalink / raw)
To: Shuqi Liang; +Cc: git, Victoria Dye, Derrick Stolee
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.
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v7 0/2] diff-files: integrate with sparse index
2023-03-23 16:03 ` Junio C Hamano
@ 2023-03-23 23:59 ` Shuqi Liang
0 siblings, 0 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-03-23 23:59 UTC (permalink / raw)
To: Junio C Hamano, Victoria Dye, git, Derrick Stolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v7 0/2] diff-files: integrate with sparse index
2023-03-23 7:42 ` Shuqi Liang
2023-03-23 16:03 ` Junio C Hamano
@ 2023-03-23 17:25 ` Victoria Dye
1 sibling, 0 replies; 73+ messages in thread
From: Victoria Dye @ 2023-03-23 17:25 UTC (permalink / raw)
To: Shuqi Liang, Junio C Hamano, git, Derrick Stolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v7 0/2] diff-files: integrate with sparse index
2023-03-22 16:18 ` [PATCH v7 " Shuqi Liang
` (2 preceding siblings ...)
2023-03-22 23:36 ` [PATCH v7 0/2] " Junio C Hamano
@ 2023-04-13 21:36 ` Junio C Hamano
2023-04-13 21:38 ` Victoria Dye
2023-04-23 1:07 ` [PATCH v8 " Shuqi Liang
4 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2023-04-13 21:36 UTC (permalink / raw)
To: vdye, derrickstolee; +Cc: git, Shuqi Liang
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.
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH v7 0/2] diff-files: integrate with sparse index
2023-04-13 21:36 ` Junio C Hamano
@ 2023-04-13 21:38 ` Victoria Dye
0 siblings, 0 replies; 73+ messages in thread
From: Victoria Dye @ 2023-04-13 21:38 UTC (permalink / raw)
To: Junio C Hamano, derrickstolee; +Cc: git, Shuqi Liang
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.
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v8 0/2] diff-files: integrate with sparse index
2023-03-22 16:18 ` [PATCH v7 " Shuqi Liang
` (3 preceding siblings ...)
2023-04-13 21:36 ` Junio C Hamano
@ 2023-04-23 1:07 ` Shuqi Liang
2023-04-23 1:07 ` [PATCH v8 1/2] t1092: add tests for `git diff-files` Shuqi Liang
` (4 more replies)
4 siblings, 5 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-04-23 1:07 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH v8 1/2] t1092: add tests for `git diff-files`
2023-04-23 1:07 ` [PATCH v8 " Shuqi Liang
@ 2023-04-23 1:07 ` Shuqi Liang
2023-04-23 1:07 ` [PATCH v8 2/2] diff-files: integrate with sparse index Shuqi Liang
` (3 subsequent siblings)
4 siblings, 0 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-04-23 1:07 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread* [PATCH v8 2/2] diff-files: integrate with sparse index
2023-04-23 1:07 ` [PATCH v8 " Shuqi Liang
2023-04-23 1:07 ` [PATCH v8 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-04-23 1:07 ` Shuqi Liang
2023-05-01 22:26 ` Victoria Dye
2023-04-25 16:57 ` [PATCH v8 0/2] " Junio C Hamano
` (2 subsequent siblings)
4 siblings, 1 reply; 73+ messages in thread
From: Shuqi Liang @ 2023-04-23 1:07 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread* Re: [PATCH v8 2/2] diff-files: integrate with sparse index
2023-04-23 1:07 ` [PATCH v8 2/2] diff-files: integrate with sparse index Shuqi Liang
@ 2023-05-01 22:26 ` Victoria Dye
0 siblings, 0 replies; 73+ messages in thread
From: Victoria Dye @ 2023-05-01 22:26 UTC (permalink / raw)
To: Shuqi Liang, git; +Cc: gitster, derrickstolee
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'.
^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH v8 0/2] diff-files: integrate with sparse index
2023-04-23 1:07 ` [PATCH v8 " Shuqi Liang
2023-04-23 1:07 ` [PATCH v8 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-04-23 1:07 ` [PATCH v8 2/2] diff-files: integrate with sparse index Shuqi Liang
@ 2023-04-25 16:57 ` Junio C Hamano
2023-05-01 22:04 ` Junio C Hamano
2023-05-02 17:23 ` [PATCH v9 " Shuqi Liang
4 siblings, 0 replies; 73+ messages in thread
From: Junio C Hamano @ 2023-04-25 16:57 UTC (permalink / raw)
To: Shuqi Liang; +Cc: git, vdye, derrickstolee
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.
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v8 0/2] diff-files: integrate with sparse index
2023-04-23 1:07 ` [PATCH v8 " Shuqi Liang
` (2 preceding siblings ...)
2023-04-25 16:57 ` [PATCH v8 0/2] " Junio C Hamano
@ 2023-05-01 22:04 ` Junio C Hamano
2023-05-02 17:23 ` [PATCH v9 " Shuqi Liang
4 siblings, 0 replies; 73+ messages in thread
From: Junio C Hamano @ 2023-05-01 22:04 UTC (permalink / raw)
To: Shuqi Liang; +Cc: git, vdye, derrickstolee
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.
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH v9 0/2] diff-files: integrate with sparse index
2023-04-23 1:07 ` [PATCH v8 " Shuqi Liang
` (3 preceding siblings ...)
2023-05-01 22:04 ` Junio C Hamano
@ 2023-05-02 17:23 ` Shuqi Liang
2023-05-02 17:23 ` [PATCH v9 1/2] t1092: add tests for `git diff-files` Shuqi Liang
` (2 more replies)
4 siblings, 3 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-05-02 17:23 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH v9 1/2] t1092: add tests for `git diff-files`
2023-05-02 17:23 ` [PATCH v9 " Shuqi Liang
@ 2023-05-02 17:23 ` Shuqi Liang
2023-05-02 19:25 ` Junio C Hamano
2023-05-02 17:23 ` [PATCH v9 2/2] diff-files: integrate with sparse index Shuqi Liang
2023-05-03 21:55 ` [PATCH v10 0/2] " Shuqi Liang
2 siblings, 1 reply; 73+ messages in thread
From: Shuqi Liang @ 2023-05-02 17:23 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH v9 1/2] t1092: add tests for `git diff-files`
2023-05-02 17:23 ` [PATCH v9 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-05-02 19:25 ` Junio C Hamano
2023-05-03 16:37 ` Victoria Dye
0 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2023-05-02 19:25 UTC (permalink / raw)
To: Shuqi Liang; +Cc: git, vdye, derrickstolee
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.
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v9 1/2] t1092: add tests for `git diff-files`
2023-05-02 19:25 ` Junio C Hamano
@ 2023-05-03 16:37 ` Victoria Dye
0 siblings, 0 replies; 73+ messages in thread
From: Victoria Dye @ 2023-05-03 16:37 UTC (permalink / raw)
To: Junio C Hamano, Shuqi Liang; +Cc: git, derrickstolee
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.
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v9 2/2] diff-files: integrate with sparse index
2023-05-02 17:23 ` [PATCH v9 " Shuqi Liang
2023-05-02 17:23 ` [PATCH v9 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-05-02 17:23 ` Shuqi Liang
2023-05-03 21:55 ` [PATCH v10 0/2] " Shuqi Liang
2 siblings, 0 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-05-02 17:23 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread* [PATCH v10 0/2] diff-files: integrate with sparse index
2023-05-02 17:23 ` [PATCH v9 " Shuqi Liang
2023-05-02 17:23 ` [PATCH v9 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-05-02 17:23 ` [PATCH v9 2/2] diff-files: integrate with sparse index Shuqi Liang
@ 2023-05-03 21:55 ` Shuqi Liang
2023-05-03 21:55 ` [PATCH v10 1/2] t1092: add tests for `git diff-files` Shuqi Liang
` (2 more replies)
2 siblings, 3 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-05-03 21:55 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH v10 1/2] t1092: add tests for `git diff-files`
2023-05-03 21:55 ` [PATCH v10 0/2] " Shuqi Liang
@ 2023-05-03 21:55 ` Shuqi Liang
2023-05-03 23:25 ` Junio C Hamano
2023-05-03 21:55 ` [PATCH v10 2/2] diff-files: integrate with sparse index Shuqi Liang
2023-05-08 18:46 ` [PATCH v11 0/2] " Shuqi Liang
2 siblings, 1 reply; 73+ messages in thread
From: Shuqi Liang @ 2023-05-03 21:55 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH v10 1/2] t1092: add tests for `git diff-files`
2023-05-03 21:55 ` [PATCH v10 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-05-03 23:25 ` Junio C Hamano
0 siblings, 0 replies; 73+ messages in thread
From: Junio C Hamano @ 2023-05-03 23:25 UTC (permalink / raw)
To: Shuqi Liang; +Cc: git, vdye, derrickstolee
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.
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v10 2/2] diff-files: integrate with sparse index
2023-05-03 21:55 ` [PATCH v10 0/2] " Shuqi Liang
2023-05-03 21:55 ` [PATCH v10 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-05-03 21:55 ` Shuqi Liang
2023-05-08 18:46 ` [PATCH v11 0/2] " Shuqi Liang
2 siblings, 0 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-05-03 21:55 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread* [PATCH v11 0/2] diff-files: integrate with sparse index
2023-05-03 21:55 ` [PATCH v10 0/2] " Shuqi Liang
2023-05-03 21:55 ` [PATCH v10 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-05-03 21:55 ` [PATCH v10 2/2] diff-files: integrate with sparse index Shuqi Liang
@ 2023-05-08 18:46 ` Shuqi Liang
2023-05-08 18:46 ` [PATCH v11 1/2] t1092: add tests for `git diff-files` Shuqi Liang
` (2 more replies)
2 siblings, 3 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-05-08 18:46 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH v11 1/2] t1092: add tests for `git diff-files`
2023-05-08 18:46 ` [PATCH v11 0/2] " Shuqi Liang
@ 2023-05-08 18:46 ` Shuqi Liang
2023-05-08 22:25 ` Victoria Dye
2023-05-08 18:46 ` [PATCH v11 2/2] diff-files: integrate with sparse index Shuqi Liang
2023-05-09 19:42 ` [PATCH v12 0/2] " Shuqi Liang
2 siblings, 1 reply; 73+ messages in thread
From: Shuqi Liang @ 2023-05-08 18:46 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH v11 1/2] t1092: add tests for `git diff-files`
2023-05-08 18:46 ` [PATCH v11 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-05-08 22:25 ` Victoria Dye
0 siblings, 0 replies; 73+ messages in thread
From: Victoria Dye @ 2023-05-08 22:25 UTC (permalink / raw)
To: Shuqi Liang, git; +Cc: gitster, derrickstolee
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
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v11 2/2] diff-files: integrate with sparse index
2023-05-08 18:46 ` [PATCH v11 0/2] " Shuqi Liang
2023-05-08 18:46 ` [PATCH v11 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-05-08 18:46 ` Shuqi Liang
2023-05-09 19:42 ` [PATCH v12 0/2] " Shuqi Liang
2 siblings, 0 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-05-08 18:46 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread* [PATCH v12 0/2] diff-files: integrate with sparse index
2023-05-08 18:46 ` [PATCH v11 0/2] " Shuqi Liang
2023-05-08 18:46 ` [PATCH v11 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-05-08 18:46 ` [PATCH v11 2/2] diff-files: integrate with sparse index Shuqi Liang
@ 2023-05-09 19:42 ` Shuqi Liang
2023-05-09 19:42 ` [PATCH v12 1/2] t1092: add tests for `git diff-files` Shuqi Liang
` (2 more replies)
2 siblings, 3 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-05-09 19:42 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
* 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
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH v12 1/2] t1092: add tests for `git diff-files`
2023-05-09 19:42 ` [PATCH v12 0/2] " Shuqi Liang
@ 2023-05-09 19:42 ` Shuqi Liang
2023-05-09 19:42 ` [PATCH v12 2/2] diff-files: integrate with sparse index Shuqi Liang
2023-05-11 3:41 ` [PATCH v12 0/2] " Victoria Dye
2 siblings, 0 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-05-09 19:42 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH v12 2/2] diff-files: integrate with sparse index
2023-05-09 19:42 ` [PATCH v12 0/2] " Shuqi Liang
2023-05-09 19:42 ` [PATCH v12 1/2] t1092: add tests for `git diff-files` Shuqi Liang
@ 2023-05-09 19:42 ` Shuqi Liang
2023-05-11 3:41 ` [PATCH v12 0/2] " Victoria Dye
2 siblings, 0 replies; 73+ messages in thread
From: Shuqi Liang @ 2023-05-09 19:42 UTC (permalink / raw)
To: git; +Cc: Shuqi Liang, vdye, gitster, derrickstolee
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
^ permalink raw reply related [flat|nested] 73+ messages in thread* Re: [PATCH v12 0/2] diff-files: integrate with sparse index
2023-05-09 19:42 ` [PATCH v12 0/2] " Shuqi Liang
2023-05-09 19:42 ` [PATCH v12 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-05-09 19:42 ` [PATCH v12 2/2] diff-files: integrate with sparse index Shuqi Liang
@ 2023-05-11 3:41 ` Victoria Dye
2023-05-11 5:04 ` Junio C Hamano
2 siblings, 1 reply; 73+ messages in thread
From: Victoria Dye @ 2023-05-11 3:41 UTC (permalink / raw)
To: Shuqi Liang, git; +Cc: gitster, derrickstolee
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!
^ permalink raw reply [flat|nested] 73+ messages in thread