Re: [PATCH v2] write-tree: integrate with sparse index
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 5, 2023, 19:48 UTC
- Message-ID
- <xmqqedoyun4e.fsf@gitster.g>
- In-Reply-To
- <9d0309bd-943c-dd51-97cf-59721eda78f7@github.com>
Victoria Dye <vdye@github.com> writes:
Show 28 quoted lines
> Shuqi Liang wrote: >> Update 'git write-tree' to allow using the sparse-index in memory >> without expanding to a full one. >> >> The recursive algorithm for update_one() was already updated in 2de37c5 >> (cache-tree: integrate with sparse directory entries, 2021-03-03) to >> handle sparse directory entries in the index. Hence we can just set the >> requires-full-index to false for "write-tree". >> >> The `p2000` tests demonstrate a ~96% execution time reduction for 'git >> write-tree' using a sparse index: >> >> Test before after >> ----------------------------------------------------------------- >> 2000.78: git write-tree (full-v3) 0.34 0.33 -2.9% >> 2000.79: git write-tree (full-v4) 0.32 0.30 -6.3% >> 2000.80: git write-tree (sparse-v3) 0.47 0.02 -95.8% >> 2000.81: git write-tree (sparse-v4) 0.45 0.02 -95.6% >> >> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com> >> --- >> >> * change the position of "settings.command_requires_full_index = 0" > > Could you describe why you made this change? You don't need to re-roll, but > in the future please make sure to describe the reasoning for changes like > this in these version notes if the context can't be gathered from other > discussions in the thread.
The reason, I think, is because previous iteration hit a BUG() when the command "git write-tree -h" is run outside a repository. That form of the help request is handled in the parse_options() machinery without any need to have a repository or a working tree.
But prepare_repo_settings() does need to be run inside a repository, so calling it without first checking if we are even in a repository is asking for trouble.
I guess an alternative fix could have been to see if we are indeed in a repository, by doing something like
if (the_repository->gitdir) {
prepare_repo_settings(the_repository);
the_repository->settings.command_requires_full_index = 0;
}like implementations of some subcommands do. And being explicit that way, instead of relying on an implicit safety given by ordering of calls, would be more maintainable in the longer haul.