Re: [PATCH] describe: enable sparse index for describe
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 27, 2023, 18:26 UTC
- Message-ID
- <xmqqjzz29hkw.fsf@gitster.g>
- In-Reply-To
- <pull.1480.git.git.1679926829475.gitgitgadget@gmail.com>
"Raghul Nanth A via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> builtin/describe.c | 2 + > t/perf/p2000-sparse-operations.sh | 14 +- > t/t1092-sparse-checkout-compatibility.sh | 10 + > t/t6121-describe-sparse.sh | 675 +++++++++++++++++++++++ > 4 files changed, 697 insertions(+), 4 deletions(-) > create mode 100755 t/t6121-describe-sparse.sh
This copying of a file with 600+ lines only to touch up a handful lines (like a 20+ lines patch) is almost criminal. Imagine the effort to keep them in sync over time, when "describe" itself may learn new features and improved output, independent from the sparse-index compatibility.
Can't we do better than this with a bit of refactoring?
Show 10 quoted lines
> 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;
Offhand, the only case I know that "describe" even _needs_ a working tree or the index is when asked to do the "--dirty" thing. To figure out if the working tree files are modified, the code calls into run_diff_index(), but has that codepath been made sparse-index aware already?
Show 43 quoted lines
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh > index 3242cfe91a0..a8a9ed79441 100755 > --- a/t/perf/p2000-sparse-operations.sh > +++ b/t/perf/p2000-sparse-operations.sh > @@ -59,7 +59,8 @@ test_expect_success 'setup repo and indexes' ' > git sparse-checkout set $SPARSE_CONE && > git config index.version 3 && > git update-index --index-version=3 && > - git checkout HEAD~4 > + git checkout HEAD~4 && > + git tag -a v1.0 -m "Final" > ) && > git -c core.sparseCheckoutCone=true clone --branch=wide --sparse . full-v4 && > ( > @@ -68,7 +69,8 @@ test_expect_success 'setup repo and indexes' ' > git sparse-checkout set $SPARSE_CONE && > git config index.version 4 && > git update-index --index-version=4 && > - git checkout HEAD~4 > + git checkout HEAD~4 && > + git tag -a v1.0 -m "Final" > ) && > git -c core.sparseCheckoutCone=true clone --branch=wide --sparse . sparse-v3 && > ( > @@ -77,7 +79,8 @@ test_expect_success 'setup repo and indexes' ' > git sparse-checkout set $SPARSE_CONE && > git config index.version 3 && > git update-index --index-version=3 && > - git checkout HEAD~4 > + git checkout HEAD~4 && > + git tag -a v1.0 -m "Final" > ) && > git -c core.sparseCheckoutCone=true clone --branch=wide --sparse . sparse-v4 && > ( > @@ -86,7 +89,8 @@ test_expect_success 'setup repo and indexes' ' > git sparse-checkout set $SPARSE_CONE && > git config index.version 4 && > git update-index --index-version=4 && > - git checkout HEAD~4 > + git checkout HEAD~4 && > + git tag -a v1.0 -m "Final" > ) > '
It is unclear from the proposed commit log what the relevance of adding a step to create an annotated tag to these tests. It is not like any later step uses that tag to figure out anything. There may be good reasons to add these tags (otherwise you would not be adding them to these tests), but please explain why in the proposed log message so that future readers of the "git log -p" do not have to ask this question.
Show 8 quoted lines
> @@ -125,5 +129,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
Just like '>', '>>' is a rediraction operator and should have SP before it (you got it right) and no SP between it and its operand. I.e.
echo >>new && git describe --dirty
You have the same in t1092, I think.