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: [RFC PATCH v6 0/2] diff-files: integrate with sparse index
Date: Tue, 21 Mar 2023 11:38:05 -0700 [thread overview]
Message-ID: <af4cd38c-bd57-6e32-867d-a205ff0bb93b@github.com> (raw)
In-Reply-To: <20230320205241.105476-1-cheskaqiqi@gmail.com>
Shuqi Liang wrote:
> Did not fix the logic of spare-checkout yet. Leave the spare diff-files
> with pathspec outside sparse definition as 'test_expect_failure' now.
> but will fix soon.
>
> Changes since v5:
>
> 1. Add space after "definition."
>
> 2. Add test case for a pathspec with wildcards or other "magic"
>
> 3. Before messing with modified files on disk, add a "baseline"
> of correct behavior when a pathspec points to out-of-cone files.
>
> 4. Write the contents of an
> existing file inside that sparse directory to disk manually
>
> 5. Use 'test_all_match' rather than 'test_sparse_match'.
> wouldn't need the additional 'test_must_be_empty' checks.
>
>
> Shuqi Liang (2):
> t1092: add tests for `git diff-files`
> diff-files: integrate with sparse index
>
> builtin/diff-files.c | 8 +++
> t/perf/p2000-sparse-operations.sh | 2 +
> t/t1092-sparse-checkout-compatibility.sh | 73 ++++++++++++++++++++++++
> 3 files changed, 83 insertions(+)
[forgot to Reply-All - Shuqi, sorry for the duplicate email!]
In future iterations, please also include a range-diff in your version
iterations. The description of changes is useful, but the range-diff
provides a much more detailed and comprehensive summary of the changes
(making it exceptionally helpful for reviewers). I *think* you can just add
the '--range-diff <previous iteration>' option to 'git format-patch' (see
MyFirstContribution [1] for more detailed instructions).
[1] https://git-scm.com/docs/MyFirstContribution#v2-git-send-email
For anyone that's interested, here's the range-diff vs. v5:
1: fb9ec0901c ! 1: 14bbcf41e0 t1092: add tests for `git diff-files`
@@ Commit message
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'
- being present on-disk without modifications, then change content inside
- 'newdirectory/testfile' in order to test 'newdirectory/testfile'
- being present on-disk with modifications.
+ the sparse-checkout cone, make a directory called 'folder1' and
+ copy `a` into 'folder1/a'. 'folder1/a' is identical to `a` in the base
+ commit. These make 'folder1/a' in the index, while leaving it outside of
+ the sparse-checkout definition. Test 'folder1/a'being present on-disk
+ without modifications, then change content inside 'folder1/a' in order
+ to test 'folder1/a' being present on-disk with modifications.
Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc
+
+ test_all_match git diff-files &&
+
-+ test_all_match git diff-files deep/a
++ test_all_match git diff-files deep/a &&
+
++ # test wildcard
++ test_all_match git diff-files deep/*
+'
+
-+test_expect_success 'diff-files with pathspec outside sparse definition' '
++test_expect_failure 'diff-files with pathspec outside sparse definition' '
+ init_repos &&
+
++ test_sparse_match test_must_fail 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 &&
++ # Add file to the index but outside of cone for sparse-checkout cases.
++ # Add file to the index without sparse-checkout cases to ensure all have
++ # same output.
++ run_on_all mkdir folder1 &&
++ run_on_all cp a folder1/a &&
+
+ # file present on-disk without modifications
-+ 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 &&
++ test_all_match git diff-files &&
++ test_all_match git diff-files folder1/a &&
+
+ # 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
++ run_on_all ../edit-contents folder1/a &&
++ test_all_match git diff-files &&
++ test_all_match git diff-files folder1/a
+'
+
test_done
2: 04e24f7db5 ! 2: 734cd24f0c diff-files: integrate with sparse index
@@ Metadata
## Commit message ##
diff-files: integrate with sparse index
+ Originally, diff-files a pathspec that is out-of-cone in a sparse-index
+ environment, Git dies with "pathspec '<x>' did not match any files",
+ mainly because it does not expand the index so nothing is matched.
+ Expand the index when the <pathspec> needs an expanded index, i.e. the
+ <pathspec> contains wildcard that may need a full-index or the
+ <pathspec> is simply outside of sparse-checkout definition.
+
Remove full index requirement for `git diff-files`
- and test to ensure the index is not expanded in `git diff-files`.
+ and add test to ensure the index only expanded when necessary
+ in `git diff-files`.
The `p2000` tests demonstrate a ~96% execution time reduction for 'git
diff-files' and a ~97% execution time reduction for 'git diff-files'
@@ builtin/diff-files.c: int cmd_diff_files(int argc, const char **argv, const char
repo_init_revisions(the_repository, &rev, prefix);
rev.abbrev = 0;
+@@ builtin/diff-files.c: 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);
++
+ result = run_diff_files(&rev, options);
+ result = diff_result_code(&rev.diffopt, result);
+ cleanup:
## t/perf/p2000-sparse-operations.sh ##
@@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all
@@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all
test_done
## t/t1092-sparse-checkout-compatibility.sh ##
-@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with pathspec outside sparse definition' '
- test_cmp expect sparse-checkout-out
+@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'diff-files with pathspec outside sparse definition' '
+ test_all_match git diff-files folder1/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_not_expanded diff-files "*/a" &&
++ test_must_be_empty sparse-index-err &&
++
++ ! ensure_not_expanded diff-files "**a" &&
++ test_must_be_empty sparse-index-err
++'
++
+test_expect_success 'sparse index is not expanded: diff-files' '
+ init_repos &&
+
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with p
+
+ run_on_all ../edit-contents deep/a &&
+
-+ ensure_not_expanded diff-files &&
-+ ensure_not_expanded diff-files deep/a
++ ensure_not_expanded diff-files &&
++ ensure_not_expanded diff-files deep/a &&
++ ensure_not_expanded diff-files deep/*
+'
+
test_done
>
>
> base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
next prev parent reply other threads:[~2023-03-21 18:38 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 ` Victoria Dye [this message]
2023-03-22 16:18 ` [PATCH v7 0/2] " 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=af4cd38c-bd57-6e32-867d-a205ff0bb93b@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).