git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Victoria Dye <vdye@github.com>
To: Shuqi Liang <cheskaqiqi@gmail.com>, git@vger.kernel.org
Cc: gitster@pobox.com, derrickstolee@github.com
Subject: Re: [PATCH v5 1/2] t1092: add tests for `git diff-files`
Date: Fri, 10 Mar 2023 10:23:51 -0800	[thread overview]
Message-ID: <b537d855-edb7-4f67-de08-d651868247a5@github.com> (raw)
In-Reply-To: <20230310050021.123769-2-cheskaqiqi@gmail.com>

Shuqi Liang wrote:

Hi Shuqi! 

Apologies for taking so long to review; thanks for working on this.

> Before integrating the 'git diff-files' builtin
> with the sparse index feature, add tests to
> t1092-sparse-checkout-compatibility.sh to ensure it currently works
> with sparse-checkout and will still work with sparse index
> after that integration.
> 
> When adding tests against a sparse-checkout
> definition, we test two modes: all changes are
> within the sparse-checkout cone and some changes are outside
> the sparse-checkout cone.
> 
> 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."

> being present on-disk without modifications, then change content inside
> 'newdirectory/testfile' in order to test 'newdirectory/testfile'
> being present on-disk with modifications.

Generally, you don't need to create a new file to get to this state, and I'd
personally advise against it. Personally, I find that adding a new file can
make the test more cumbersome than is necessary. I'll see if I can suggest
an alternative later on.

> 
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>  t/t1092-sparse-checkout-compatibility.sh | 48 ++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 801919009e..9b71d7f5f9 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2055,4 +2055,52 @@ test_expect_success 'grep sparse directory within submodules' '
>  	test_cmp actual expect
>  '
>  
> +test_expect_success 'diff-files with pathspec inside sparse definition' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	test_all_match git diff-files &&
> +
> +	test_all_match git diff-files deep/a 
> +

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.

Otherwise, this test looks good.

[1] https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefpathspecapathspec
[2] https://lore.kernel.org/git/822d7344587f698e73abba1ca726c3a905f7b403.1638201164.git.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/20220807041335.1790658-3-shaoxuan.yuan02@gmail.com/

> +'
> +
> +test_expect_success 'diff-files with pathspec outside sparse definition' '
> +	init_repos &&

Before messing with modified files on disk, it'd be nice to show a
"baseline" of correct behavior when a pathspec points to out-of-cone files,
e.g. 'test_all_match git diff-files folder2/a'.

> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	# add file to the index but outside of cone
> +	run_on_sparse mkdir newdirectory &&
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git add --sparse newdirectory/testfile &&
> +
> +	# file present on-disk without modifications

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!

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).

If you'd like any help investigating this or get stuck, please let me know -
I'd be happy to assist!

> +
> +	# file present on-disk with modifications
> +	FN=newdirectory/testfile &&
> +	OID=$(git -C sparse-checkout hash-object $FN) &&
> +	ZERO=$(test_oid zero) &&
> +	echo ":100644 100644 $OID $ZERO M	$FN" >expect &&
> +
> +	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"'.

> +'
> +
>  test_done


  reply	other threads:[~2023-03-10 18:24 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 [this message]
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
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=b537d855-edb7-4f67-de08-d651868247a5@github.com \
    --to=vdye@github.com \
    --cc=cheskaqiqi@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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 NNTP newsgroup(s).