From: Shuqi Liang <cheskaqiqi@gmail.com>
To: Victoria Dye <vdye@github.com>,
git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH v7 2/2] diff-files: integrate with sparse index
Date: Thu, 20 Apr 2023 00:50:00 -0400 [thread overview]
Message-ID: <CAMO4yUF1P1Sv1aVJ1aw9US-QeNYD-GfaS7ndr=bwp-dgvOyexA@mail.gmail.com> (raw)
In-Reply-To: <c382017a-8c65-24ba-5092-6b46428d8b9b@github.com>
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
next prev parent reply other threads:[~2023-04-20 4:50 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-04 2:57 [RFC][PATCH] t1092: add tests for `git diff-files` Shuqi Liang
2023-03-06 14:14 ` Derrick Stolee
2023-03-07 6:58 ` [PATCH v2 0/2] diff-files: integrate with sparse index Shuqi Liang
2023-03-07 6:58 ` [PATCH v2 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-03-07 18:53 ` Junio C Hamano
2023-03-08 22:04 ` Shuqi Liang
2023-03-08 22:40 ` Junio C Hamano
2023-03-07 6:58 ` [PATCH v2 2/2] diff-files: integrate with sparse index Shuqi Liang
2023-03-09 1:33 ` [PATCH v3 0/2] " Shuqi Liang
2023-03-09 1:33 ` [PATCH v3 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-03-09 3:00 ` Junio C Hamano
2023-03-09 1:33 ` [PATCH v3 2/2] diff-files: integrate with sparse index Shuqi Liang
2023-03-09 6:39 ` [PATCH v4 0/2] " Shuqi Liang
2023-03-09 6:39 ` [PATCH v4 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-03-09 17:20 ` Junio C Hamano
2023-03-09 23:21 ` Shuqi Liang
2023-03-09 23:40 ` Junio C Hamano
2023-03-09 6:39 ` [PATCH v4 2/2] diff-files: integrate with sparse index Shuqi Liang
2023-03-10 5:00 ` [PATCH v5 0/2] " Shuqi Liang
2023-03-10 5:00 ` [PATCH v5 1/2] t1092: add tests for `git diff-files` Shuqi Liang
2023-03-10 18:23 ` Victoria Dye
2023-03-20 20:55 ` Shuqi Liang
2023-03-10 5:00 ` [PATCH v5 2/2] diff-files: integrate with sparse index Shuqi Liang
2023-03-10 18:23 ` Victoria Dye
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-21 21:21 ` Victoria Dye
2023-03-21 21:25 ` Junio C Hamano
2023-03-21 22:19 ` Victoria Dye
2023-03-20 20:52 ` [PATCH v6 2/2] diff-files: integrate with sparse index 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
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
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 [this message]
2023-04-20 15:26 ` Victoria Dye
2023-04-21 1:10 ` Shuqi Liang
2023-04-21 21:26 ` Victoria Dye
2023-04-22 21:25 ` Shuqi Liang
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 23:59 ` Shuqi Liang
2023-03-23 17:25 ` Victoria Dye
2023-04-13 21:36 ` Junio C Hamano
2023-04-13 21:38 ` Victoria Dye
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-05-01 22:26 ` Victoria Dye
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
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
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
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
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
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
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
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 ` [PATCH v12 0/2] " Victoria Dye
2023-05-11 5:04 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMO4yUF1P1Sv1aVJ1aw9US-QeNYD-GfaS7ndr=bwp-dgvOyexA@mail.gmail.com' \
--to=cheskaqiqi@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=vdye@github.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).