Re: [PATCH v7 2/2] diff-files: integrate with sparse index
- From
Victoria Dye <vdye@github.com>
- Date
- Apr 13, 2023, 21:54 UTC
- Message-ID
- <c382017a-8c65-24ba-5092-6b46428d8b9b@github.com>
- In-Reply-To
- <20230322161820.3609-3-cheskaqiqi@gmail.com>
Shuqi Liang wrote:
Show 22 quoted lines
> 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/
Show 31 quoted lines
> 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?
Show 20 quoted lines
> + > 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/
Show 17 quoted lines
> +' > + > +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