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