| Message ID | 20230320205241.105476-3-cheskaqiqi@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | diff-files: integrate with sparse index | expand |
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
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
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(+)