From: Shuqi Liang <cheskaqiqi@gmail.com>
To: Victoria Dye <vdye@github.com>, git@vger.kernel.org
Subject: Re: [PATCH v5 1/2] t1092: add tests for `git diff-files`
Date: Mon, 20 Mar 2023 16:55:13 -0400 [thread overview]
Message-ID: <CAMO4yUFsGQbeu=wbqG8EuptYFDv9c1tB8Y0RAU_UJ-GdbMAfVg@mail.gmail.com> (raw)
In-Reply-To: <b537d855-edb7-4f67-de08-d651868247a5@github.com>
On Fri, Mar 10, 2023 at 1:23 PM Victoria Dye <vdye@github.com> wrote:
Hi Victoria!
Sorry for the late reply! I was having midterm exams.
Thank you for the feedback on the patch .
I appreciate your time giving me the advice for improvement.
> > In order to have staged changes outside of
> > the sparse-checkout cone, create a 'newdirectory/testfile' and
> > add it to the index, while leaving it outside of
> > the sparse-checkout definition.Test 'newdirectory/testfile'
> nit: missing space after "definition."
Will do!
> I'd be interested in seeing an additional test case for a pathspec with
> wildcards or other "magic" [1], e.g. 'git diff-files "deep/*"'. In past
> sparse index integrations, there has occasionally been a need for special
> handling of those types of pathspecs [2][3], so it would be good for the
> test to cover cases like that.
Will do!
> From the comment you've added here, it looks like the state you want to test
> is "file outside sparse checkout definition exists on disk". However, since
> one of the goals of this test is to verify sparse index behavior once its
> integrated with the 'diff-files' command, whether the "outside of sparse-
> checkout definition" file belongs to a sparse directory entry is an
> important (and fairly nuanced) factor to consider.
>
> The main difference between a "regular" sparse-checkout and a sparse
> index-enabled sparse-checkout is the existence of "sparse directory"
> entries: index entries with 'SKIP_WORKTREE' that represent directories and
> their contents. In the context of these tests, the thing we really want to
> verify is that the sparse index-enabled case produces the same results as
> the full index when an operation needs to get some information out of a
> sparse directory.
>
> Coming back to this test, the 'newdirectory/testfile' you create, while
> outside the sparse-checkout definition, never actually belongs to a sparse
> directory because 'newdirectory' is never collapsed - in fact, I don't
> think it even gets the SKIP_WORKTREE bit in the index. To ensure you have a
> sparse directory & SKIP_WORKTREE, you'd need to run 'git sparse-checkout
> reapply' after removing 'newdirectory/testfile' from disk - which doesn't
> help if you want to test what happens when the file exists on disk!
Thanks for the explanation here! I learn a lot.
> In fact, because of built-in safeguards around on-disk files and
> sparse-checkout, there isn't really a way in Git to materialize a file
> without also expanding its sparse directory and removing SKIP_WORKTREE. If
> you want to preserve a sparse directory, you should write the contents of an
> existing file inside that sparse directory to disk manually:
>
> run_on_sparse mkdir folder1 &&
> run_on_sparse cp a folder1/a && # `folder1/a` is identical to `a` in the base commit
>
> Git's index will not be touched by doing this, meaning the next command you
> invoke (in this case, 'diff-files') will need to reconcile the file on disk
> with what it sees in the index.
>
> > + test_sparse_match git diff-files &&
> > + test_must_be_empty sparse-checkout-out &&
> > + test_must_be_empty sparse-checkout-err &&
> > + test_sparse_match git diff-files newdirectory/testfile &&
> > + test_must_be_empty sparse-checkout-out &&
> > + test_must_be_empty sparse-checkout-err &&
>
> These checks should be 'test_all_match' rather than 'test_sparse_match'.
> Since (through other tests) we're confident that 'git diff-files' on an
> unmodified, non-sparse-checkout repository will give us an empty result, you
> wouldn't need the additional 'test_must_be_empty' checks.
>
> A bit of a "spoiler": when I tested this out locally, I found that the diff
> was *not* empty for the sparse-checkout cases, until I ran 'git status'
> (which strips the 'SKIP_WORKTREE' bit from the file and writes it to the
> index). That's not the desired behavior, so there's a bug in the
> sparse-checkout logic used in 'diff-files' that needs to be fixed (my first
> guess would be that 'clear_skip_worktree_from_present_files()' is not being
> applied when the index is read).
Yeah. After I use
run_on_sparse mkdir folder1 &&
run_on_sparse cp a folder1/a && # `folder1/a` is identical to `a` in
the base commit
diff was *not* empty for the sparse-checkout cases
when I look into builtin/diff-files.c
'repo_read_index_preload' call 'repo_read_index' which call
'clear_skip_worktree_from_present_files()' to apply when the index is read.
I got stuck in here and do not have the idea to investigate it. Any
tips would be helpful!
> > + run_on_sparse ../edit-contents newdirectory/testfile &&
> > + test_sparse_match git diff-files &&
> > + test_cmp expect sparse-checkout-out &&
> > + test_sparse_match git diff-files newdirectory/testfile &&
> > + test_cmp expect sparse-checkout-out
>
> Similarly, you should use 'run_on_all' to modify the file & 'test_all_match'
> to verify that they all match here. It would demonstrate that we don't
> expect any "special" behavior from sparse-checkout, meaning you can probably
> avoid checking the result verbatim.
> Finally, as with the earlier test, it'd be nice to show that the result is
> the same with a wildcard pathspec, e.g. 'git diff-files "folder*/a"'.
Will do!
----------------------------------------------------------------------------
Thanks
Shuqi
next prev parent reply other threads:[~2023-03-20 20:55 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 [this message]
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
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='CAMO4yUFsGQbeu=wbqG8EuptYFDv9c1tB8Y0RAU_UJ-GdbMAfVg@mail.gmail.com' \
--to=cheskaqiqi@gmail.com \
--cc=git@vger.kernel.org \
--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).