From: Victoria Dye <vdye@github.com>
To: Junio C Hamano <gitster@pobox.com>, Shuqi Liang <cheskaqiqi@gmail.com>
Cc: git@vger.kernel.org, derrickstolee@github.com
Subject: Re: [PATCH v9 1/2] t1092: add tests for `git diff-files`
Date: Wed, 3 May 2023 09:37:37 -0700 [thread overview]
Message-ID: <2fa835b8-1c9b-67d3-aa4a-70a978b5f20d@github.com> (raw)
In-Reply-To: <xmqqjzxqzgbi.fsf@gitster.g>
Junio C Hamano wrote:
> Shuqi Liang <cheskaqiqi@gmail.com> writes:
>
>> +test_expect_success 'diff-files with pathspec outside sparse definition' '
>> + init_repos &&
>> +
>> + test_sparse_match test_must_fail git diff-files folder2/a &&
>
> In "sparse" directories at this point of test, "folder2" is outside
> the cone(s) of interest and is not instantiated. The reason why
> the command fails is because the command line parsing that is
> generic to all users of the revision machinery requires you to have
> a disambiguating double-dash before such a pathspec that tries to
> match a path that does not exist in the working tree and is not
> specific to "diff-files".
>
> I wonder how interesting and useful this test is. Without
> accompanying test that uses disambiguating double-dash properly,
> e.g. "git diff-files -- folder2", I doubt it is very much useful.
I agree, this test isn't helpful as-is. With the '--', 'diff-files'
shouldn't fail, so in the interest of avoiding unexpected regressions in the
future (masked by 'test_must_fail'), I think this test should be updated as
you described.
>> + # file present on-disk without modifications
>> + # use `--stat` to ignore file creation time differences in
>> + # unrefreshed index
>> + test_all_match git diff-files --stat &&
>> + test_all_match git diff-files --stat folder1/a &&
>> + test_all_match git diff-files --stat "folder*/a" &&
>
> Because in all three repositories, "folder1/a" exists in the working
> tree, the "you need to disambiguate" error like the first test
> (whose utility I questioned) would not trigger.
>
> What does this demonstrate, though? That instantiating a file on
> the working tree, even outside the cone(s) of interest in a sparsely
> checked out working tree, makes it part of the interesting set
> automatically? As there is no difference between the indexed
> contents and what is in the working tree, we cannot tell from this
> test if that is the case (not a complaint, just an observation).
This was meant [1] to check whether there are any issues expanding the index
(specifically, the 'folder1/' sparse directory) before comparing to the
now-on-disk 'folder1/a'.
However...
[1] https://lore.kernel.org/git/b537d855-edb7-4f67-de08-d651868247a5@github.com/
>
> But ...
>
>> + # file present on-disk with modifications
>> + run_on_all ../edit-contents folder1/a &&
>> + test_all_match git diff-files &&
>> + test_all_match git diff-files folder1/a &&
>> + test_all_match git diff-files "folder*/a"
>
> ... it is shown by doing the same test with modified contents?
>
> For consistency with the earlier "the same contents" test, we should
> use "--stat" here, too. Or even "--stat -p".
>
> Alternatively, we could refresh the index before running diff-files
> (here and also before the earlier "the same contents" test), I
> guess.
...to your point, we probably don't need both the "unmodified folder1/a
diff-files" *and* "modified folder1/a diff-files" tests. In fact, the empty
output of "unmodified folder1/a" could be caused by either "this file is
unmodified" or "this file isn't in the index", so the test might pass even
if there's an issue with index expansion. That isn't a problem in the
"modified folder1/a" case, since we're expecting to see - and comparing the
contents of - a diff.
I think we can drop the 'diff-files --stat' tests and go straight to the
'run_on_all ../edit-contents folder1/a'. Adding '--' here to disambiguate
the pathspecs might be nice as well.
>
>> +'
>> +
>> test_done
>
> Thanks.
Thanks for the detailed review, apologies for missing these issues earlier.
next prev parent reply other threads:[~2023-05-03 16:37 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
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 [this message]
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=2fa835b8-1c9b-67d3-aa4a-70a978b5f20d@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 read-only IMAP folder(s) and NNTP newsgroup(s).