Raghul Nanth A via GitGitGadget wrote: > From: Raghul Nanth A <nanth.raghul@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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
