Re: [PATCH v2] write-tree: integrate with sparse index

From: Junio C Hamano <gitster@pobox.com>
To: Victoria Dye <vdye@github.com>
Cc: Shuqi Liang <cheskaqiqi@gmail.com>,
	git@vger.kernel.org, derrickstolee@github.com
Subject: Re: [PATCH v2] write-tree: integrate with sparse index
Date: Wed, 05 Apr 2023 12:48:01 -0700	[thread overview]
Message-ID: <xmqqedoyun4e.fsf@gitster.g> (raw)
In-Reply-To: <9d0309bd-943c-dd51-97cf-59721eda78f7@github.com> (Victoria Dye's message of "Wed, 5 Apr 2023 10:31:36 -0700")

Victoria Dye <vdye@github.com> writes:

> 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.