From: Jeff Hostetler <git@jeffhostetler.com>
Date: Thu, 23 Mar 2023 13:59:15 GMT
Subject: Re: [PATCH 2/4] split-index; stop abusing the `base_oid` to strip the "link" extension
Message-ID: <ba23850b-0fc5-4a82-49df-88550ae8b4ed@jeffhostetler.com>
In-Reply-To: <xmqqjzz8cwdp.fsf@gitster.g>



On 3/22/23 5:24 PM, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> When a split-index is in effect, the `$GIT_DIR/index` file needs to
>> contain a "link" extension that contains all the information about the
>> split-index, including the information about the shared index.
>> ...
>> Let's stop zeroing out the `base_oid` to indicate that the "link"
>> extension should not be written.
> 
> Nicely explained.
> 
>> One might be tempted to simply call `discard_split_index()` instead,
>> under the assumption that Git decided to write a non-split index and
>> therefore the the `split_index` structure might no longer be wanted.
> 
> "the the".
> 
>> +enum strip_extensions {
>> +	WRITE_ALL_EXTENSIONS = 0,
>> +	STRIP_ALL_EXTENSIONS = 1,
>> +	STRIP_LINK_EXTENSION_ONLY = 2
>> +};
> 
> We do not need to spell out the specific values for this enum; the
> users' (i.e. the callers of do_write_index()) sole requirement is
> for these symbols to have different values.

There are several calls to do_write_locked_index() that pass 0 or 1
as the new final arg.  If we update them to use these enum values,
then we don't need integer values here.

> 
> Also do we envision that (1) we would need to keep STRIP_LINK_ONLY
> to be with the largest value among the enum values, or (2) we would
> never add new value to the set?  Otherwise let's end the last one
> with a trailing comma.
> 
> Looking at the way strip_extensions variable is used in
> do_write_index(), an alternative design might be to make it a set of
> bits (e.g. unsigned write_extension) and give one bit to each
> extension.  But such a clean-up is better left outside the topic, I
> would imagine, as we do not have any need to skip an arbitrary set
> of extensions right now.

Agreed, I thought about suggesting a set of bits too, but right now
we only need to strip all of them or just this one.

> 
>> +/*
>> + * Write the Git index into a `.lock` file
>> + *
>> + * If `strip_link_extension` is non-zero, avoid writing any "link" extension
>> + * (used by the split-index feature).
>> + */
> 
> Not exposing "enum strip_extensions" to the caller of this function,
> like this patch does, is probably a very safe and sensible thing to
> do.  We do not have a reason to allow its callers to (perhaps
> mistakenly) pass STRIP_ALL_EXTENSIONS to it.
> 
>>   static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,
>> -				 unsigned flags)
>> +				 unsigned flags, int strip_link_extension)
>>   {
>>   	int ret;
>>   	int was_full = istate->sparse_index == INDEX_EXPANDED;
>> @@ -3185,7 +3197,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
>>   	 */
>>   	trace2_region_enter_printf("index", "do_write_index", the_repository,
>>   				   "%s", get_lock_file_path(lock));
>> -	ret = do_write_index(istate, lock->tempfile, 0, flags);
>> +	ret = do_write_index(istate, lock->tempfile, strip_link_extension ? STRIP_LINK_EXTENSION_ONLY : 0, flags);

In the else of the ?: operator, could we use the WRITE_ALL_EXTENSIONS
instead of 0?

>>   	trace2_region_leave_printf("index", "do_write_index", the_repository,
>>   				   "%s", get_lock_file_path(lock));
>>   
> 
> OK.
> 
> Very nicely done.