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@xxxxxxxxx/ > 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@xxxxxxxxxx/ > +' > + > +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
