| Message ID | pull.1480.v2.git.git.1680107154078.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] describe: enable sparse index for describe | expand |
"Raghul Nanth A via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Raghul Nanth A <nanth.raghul@gmail.com> > > Add usage and performance tests for describe > > Performance metrics > ... The description is a bit skimpy. At least it should explain why blindly flipping the "requires-full-index" bit off is all that is necessary. I think in the review discussion on v1, Derrick gave some explanation you can regurgitate and reuse. > Signed-off-by: Raghul Nanth A <nanth.raghul@gmail.com> > ... > diff --git a/t/t6121-describe-sparse.sh b/t/t6121-describe-sparse.sh > new file mode 100755 > index 00000000000..ce53603c387 > --- /dev/null > +++ b/t/t6121-describe-sparse.sh > @@ -0,0 +1,67 @@ > +#!/bin/sh > + > +test_description='git describe in sparse checked out trees' > + > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > + > +. ./test-lib.sh > + > +check_describe () { > + indir= && > +... > + ' > +} Having this almost identical helper copied from a near-by test script means maintenance nightmare. People will forget to side port to this copy any fixes they make to the other one. I was hoping there would be a cleaner approach to reuse t6120, by doing something similar to either how t8001-annotate shares blame tests, or how t5559 takes advantage of t5551. One way might be ... * prepare a prerequisite like so near the beginning of t6120 test_lazy_prereq WITH_SPARSE_INDEX ' test "$TEST_NAME" = t6121-describe ' * add tests to be run with sparse-index enabled, but guarded with some variable, e.g. test_expect_success TESTING_SPARSE_INDEX 'a new test' ' ... do sparse-index testing specific code ... ' Such "sparse-index testing specific" code may include turning the working tree the previous test already prepared into sparse (i.e. additional "setup"), or running commands under "ensure_not_expanded" (i.e. new tests). * create t6121 that works similar to how t5559 takes advantage of t5551, something like #/bin/sh . ./t6120-describe.sh Ideally we should be able to do this without adding a new t6121, by adding new tests that are specific to sparse-index at the end of t6120, and avoid duplicated code. Another possibility that may take the least amount of effort (but may give us a lot less satisfactory outcome) may be to add a lib-describe.sh library that is sourced from t6120 and t6121, and move check_describe there. If check_describe has to behave slightly differently, have a new conditional in the implementation so that the caller can make a choice.
Raghul Nanth A via GitGitGadget wrote: > From: Raghul Nanth A <nanth.raghul@gmail.com> Hello! Thanks for working on this patch, it's always nice to get more sparse index support. Since this is your first contribution to the mailing list, though, it's unclear to me whether you're working on this as an independent contributor or if you're interested in the Google Summer of Code project "More Sparse Index Integrations" [1]. If you're doing this for GSoC, could you please prefix the title of this patch with "[GSoC]" (as noted in the application details [2])? On the topic of GSoC applications - if you are submitting this for GSoC, I'm a bit curious as to why you jumped right into sparse index rather than first submitting a microproject [3]. This is a good first pass at a sparse index integration, but the microproject is a better way to get acquainted with the conventions and requirements of contributing to Git, hence the strong recommendation to complete one first. [1] https://git.github.io/SoC-2023-Ideas/ [2] https://git.github.io/General-Application-Information/ [3] https://git.github.io/General-Microproject-Information/ > > Add usage and performance tests for describe > > Performance metrics > > Test HEAD~1 HEAD > ------------------------------------------------------------------------------------------------- > 2000.2: git describe --dirty (full-v3) 0.08(0.09+0.01) 0.08(0.06+0.03) +0.0% > 2000.3: git describe --dirty (full-v4) 0.09(0.07+0.03) 0.08(0.05+0.04) -11.1% > 2000.4: git describe --dirty (sparse-v3) 0.88(0.82+0.06) 0.02(0.01+0.05) -97.7% > 2000.5: git describe --dirty (sparse-v4) 0.68(0.60+0.08) 0.02(0.02+0.04) -97.1% > 2000.6: echo >>new && git describe --dirty (full-v3) 0.08(0.04+0.05) 0.08(0.05+0.04) +0.0% > 2000.7: echo >>new && git describe --dirty (full-v4) 0.08(0.07+0.03) 0.08(0.05+0.04) +0.0% > 2000.8: echo >>new && git describe --dirty (sparse-v3) 0.75(0.69+0.07) 0.02(0.03+0.03) -97.3% > 2000.9: echo >>new && git describe --dirty (sparse-v4) 0.81(0.73+0.09) 0.02(0.01+0.05) -97.5% As Junio noted [4], this description doesn't provide much information to a reader. Commit messages should (at a minimum) explain what the code change in a commit does, as well as why it's necessary. [4] https://lore.kernel.org/git/xmqq355nbii5.fsf@gitster.g/ > > Signed-off-by: Raghul Nanth A <nanth.raghul@gmail.com> > --- > describe: enable sparse index for describe > > * Removed describe tests not concerned with sparse index > > * Added performance metric to commit message > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1480%2FNanthR%2Fdescribe-sparse-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1480/NanthR/describe-sparse-v2 > Pull-Request: https://github.com/git/git/pull/1480 > > builtin/describe.c | 2 + > t/perf/p2000-sparse-operations.sh | 3 ++ > t/t1092-sparse-checkout-compatibility.sh | 11 ++++ > t/t6121-describe-sparse.sh | 67 ++++++++++++++++++++++++ > 4 files changed, 83 insertions(+) > create mode 100755 t/t6121-describe-sparse.sh > > diff --git a/builtin/describe.c b/builtin/describe.c > index 5b5930f5c8c..7ff9b5e4b20 100644 > --- a/builtin/describe.c > +++ b/builtin/describe.c > @@ -654,6 +654,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix) > int fd, result; > > setup_work_tree(); > + prepare_repo_settings(the_repository); > + the_repository->settings.command_requires_full_index = 0; Looks good. > repo_read_index(the_repository); > refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, > NULL, NULL, NULL); > diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh > index 3242cfe91a0..db7887470f9 100755 > --- a/t/perf/p2000-sparse-operations.sh > +++ b/t/perf/p2000-sparse-operations.sh > @@ -43,6 +43,7 @@ test_expect_success 'setup repo and indexes' ' > done && > > git sparse-checkout init --cone && > + git tag -a v1.0 -m "Final" && This isn't disruptive to the existing performance tests, but allows you to easily test 'git describe'. Nice! > git sparse-checkout set $SPARSE_CONE && > git checkout -b wide $OLD_COMMIT && > > @@ -125,5 +126,7 @@ test_perf_on_all git checkout-index -f --all > test_perf_on_all git update-index --add --remove $SPARSE_CONE/a > test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a" > test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*" > +test_perf_on_all git describe --dirty > +test_perf_on_all 'echo >>new && git describe --dirty' > > test_done > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index 801919009e1..9a4db09178f 100755 > --- a/t/t1092-sparse-checkout-compatibility.sh > +++ b/t/t1092-sparse-checkout-compatibility.sh > @@ -1514,6 +1514,17 @@ test_expect_success 'sparse-index is not expanded: stash' ' > ensure_not_expanded stash pop > ' > > +test_expect_success 'sparse-index is not expanded: describe' ' > + init_repos && > + # Add tag to be read by describe > + ensure_not_expanded tag -a v1.0 -m "Version 1" && The test you've added isn't verifying the sparse index compatibility of 'git tag' (which doesn't use the index at all, IIRC), so 'ensure_not_expanded' isn't needed on this line. You should use 'git -C sparse-index tag ...' to perform the action instead. > + ensure_not_expanded describe --dirty && > + ensure_not_expanded describe && > + echo "test" >>sparse-index/extra.txt && > + ensure_not_expanded describe --dirty && > + ensure_not_expanded describe > +' > + > test_expect_success 'sparse index is not expanded: diff' ' > init_repos && > > diff --git a/t/t6121-describe-sparse.sh b/t/t6121-describe-sparse.sh > new file mode 100755 > index 00000000000..ce53603c387 > --- /dev/null > +++ b/t/t6121-describe-sparse.sh Is there a specific reason you've created a new test file instead of adding the tests to 't1092-sparse-checkout-compatibility.sh'? Historically, we've used 't1092' for validating both the functional correctness of a command with sparse index enable and the preservation of a sparse index with 'ensure_not_expanded'; you've done the latter in 't1092', but put the former in this new 't6121'. 't1092' also uses a more "interesting" test repo & includes comparison functions for full checkout/sparse-checkout/sparse index, so the tests added for 'git describe' can be more thorough. > @@ -0,0 +1,67 @@ > +#!/bin/sh > + > +test_description='git describe in sparse checked out trees' > + > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > + > +. ./test-lib.sh > + > +check_describe () { > + indir= && > + while test $# != 0 > + do > + case "$1" in > + -C) > + indir="$2" > + shift > + ;; > + *) > + break > + ;; > + esac > + shift > + done && > + indir=${indir:+"$indir"/} && > + expect="$1" > + shift > + describe_opts="$@" > + test_expect_success "describe $describe_opts" ' > + git ${indir:+ -C "$indir"} describe $describe_opts >actual && > + echo "$expect" >expect && > + test_cmp expect actual > + ' > +} > + > +test_expect_success setup ' > + test_commit initial file one && > + test_commit --annotate A file A && > + > + test_tick && > + > + git sparse-checkout init --cone > +' > + > +check_describe A HEAD > + > +test_expect_success 'describe --dirty with --work-tree' ' > + ( > + cd "$TEST_DIRECTORY" && > + git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out" > + ) && > + grep "A" out > +' > + > +test_expect_success 'set-up dirty work tree' ' > + echo >>file > +' > + > +test_expect_success 'describe --dirty with --work-tree (dirty)' ' > + git describe --dirty >expected && > + ( > + cd "$TEST_DIRECTORY" && > + git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out" > + ) && > + test_cmp expected out > +' > +test_done > > base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e
Victoria Dye <vdye@github.com> writes: >> diff --git a/t/t6121-describe-sparse.sh b/t/t6121-describe-sparse.sh >> new file mode 100755 >> index 00000000000..ce53603c387 >> --- /dev/null >> +++ b/t/t6121-describe-sparse.sh > > Is there a specific reason you've created a new test file instead of adding > the tests to 't1092-sparse-checkout-compatibility.sh'? Historically, ... > ... 't1092' also uses a more "interesting" test repo & > includes comparison functions for full checkout/sparse-checkout/sparse > index, so the tests added for 'git describe' can be more thorough. Ahh... I mentioned t6120 in my response, but t1092 does sound like a lot more appropriate place for adding tests for these. Thanks for pointing it out.
On 3/29/23 23:19, Victoria Dye wrote: > Raghul Nanth A via GitGitGadget wrote: >> From: Raghul Nanth A <nanth.raghul@gmail.com> > > Hello! Thanks for working on this patch, it's always nice to get more sparse > index support. Since this is your first contribution to the mailing list, > though, it's unclear to me whether you're working on this as an independent > contributor or if you're interested in the Google Summer of Code project > "More Sparse Index Integrations" [1]. If you're doing this for GSoC, could > you please prefix the title of this patch with "[GSoC]" (as noted in the > application details [2])? > > On the topic of GSoC applications - if you are submitting this for GSoC, I'm > a bit curious as to why you jumped right into sparse index rather than first > submitting a microproject [3]. This is a good first pass at a sparse index > integration, but the microproject is a better way to get acquainted with the > conventions and requirements of contributing to Git, hence the strong > recommendation to complete one first. > > [1] https://git.github.io/SoC-2023-Ideas/ > [2] https://git.github.io/General-Application-Information/ > [3] https://git.github.io/General-Microproject-Information/ > Hello. So, yes, I am interested in GSOC. As for the reason for choosing to do sparse-index, I had just missed the part about the micro-projects. Sorry about that. So, do I submit a micro-project patch? And as for the changes mentioned, I will make the changes in the title
Raghul Nanth wrote: > Hello. So, yes, I am interested in GSOC. As for the reason for choosing to > do sparse-index, I had just missed the part about the micro-projects. > Sorry about that. So, do I submit a micro-project patch? No, you do not need to submit an additional microproject. The 'git describe' integration covers the intended purpose of a microproject (namely, becoming familiar with Git contribution best practices). > > And as for the changes mentioned, I will make the changes in the title
diff --git a/builtin/describe.c b/builtin/describe.c index 5b5930f5c8c..7ff9b5e4b20 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -654,6 +654,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix) int fd, result; setup_work_tree(); + prepare_repo_settings(the_repository); + the_repository->settings.command_requires_full_index = 0; repo_read_index(the_repository); refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL); diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh index 3242cfe91a0..db7887470f9 100755 --- a/t/perf/p2000-sparse-operations.sh +++ b/t/perf/p2000-sparse-operations.sh @@ -43,6 +43,7 @@ test_expect_success 'setup repo and indexes' ' done && git sparse-checkout init --cone && + git tag -a v1.0 -m "Final" && git sparse-checkout set $SPARSE_CONE && git checkout -b wide $OLD_COMMIT && @@ -125,5 +126,7 @@ test_perf_on_all git checkout-index -f --all test_perf_on_all git update-index --add --remove $SPARSE_CONE/a test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a" test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*" +test_perf_on_all git describe --dirty +test_perf_on_all 'echo >>new && git describe --dirty' test_done diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 801919009e1..9a4db09178f 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -1514,6 +1514,17 @@ test_expect_success 'sparse-index is not expanded: stash' ' ensure_not_expanded stash pop ' +test_expect_success 'sparse-index is not expanded: describe' ' + init_repos && + # Add tag to be read by describe + ensure_not_expanded tag -a v1.0 -m "Version 1" && + ensure_not_expanded describe --dirty && + ensure_not_expanded describe && + echo "test" >>sparse-index/extra.txt && + ensure_not_expanded describe --dirty && + ensure_not_expanded describe +' + test_expect_success 'sparse index is not expanded: diff' ' init_repos && diff --git a/t/t6121-describe-sparse.sh b/t/t6121-describe-sparse.sh new file mode 100755 index 00000000000..ce53603c387 --- /dev/null +++ b/t/t6121-describe-sparse.sh @@ -0,0 +1,67 @@ +#!/bin/sh + +test_description='git describe in sparse checked out trees' + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh + +check_describe () { + indir= && + while test $# != 0 + do + case "$1" in + -C) + indir="$2" + shift + ;; + *) + break + ;; + esac + shift + done && + indir=${indir:+"$indir"/} && + expect="$1" + shift + describe_opts="$@" + test_expect_success "describe $describe_opts" ' + git ${indir:+ -C "$indir"} describe $describe_opts >actual && + echo "$expect" >expect && + test_cmp expect actual + ' +} + +test_expect_success setup ' + test_commit initial file one && + test_commit --annotate A file A && + + test_tick && + + git sparse-checkout init --cone +' + +check_describe A HEAD + +test_expect_success 'describe --dirty with --work-tree' ' + ( + cd "$TEST_DIRECTORY" && + git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out" + ) && + grep "A" out +' + +test_expect_success 'set-up dirty work tree' ' + echo >>file +' + +test_expect_success 'describe --dirty with --work-tree (dirty)' ' + git describe --dirty >expected && + ( + cd "$TEST_DIRECTORY" && + git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out" + ) && + test_cmp expected out +' +test_done