Re: [PATCH v2 04/10] sparse-index: introduce partially-sparse indexes

From: Derrick Stolee <derrickstolee@github.com>
Date: May 20, 2022, 18:05 UTC
Message-ID: <0b2215e1-7f12-ed0d-b7bf-03648b19e30a@github.com>
In-Reply-To: <xmqqleuxmgof.fsf@gitster.g>

On 5/19/2022 4:05 PM, Junio C Hamano wrote:

Show 21 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +enum sparse_index_mode {
>> +	/*
>> +	 * COMPLETELY_FULL: there are no sparse directories
>> +	 * in the index at all.
>> +	 */
>> +	COMPLETELY_FULL = 0,
> 
> Two things that make me wonder are:
> 
>  * Do concrete values assigned to these symbols matter?  If we do
> 
> 	if (some_function_that_returns_this_type())
> 		/* ok, we know it is full */
> 		do_full_thing();
> 
>    then the answer is yes.  If we write these values to on-disk
>    file, and other versions and reimplementations of Git need to
>    know the concrete values, then the answer is yes.  Otherwise, we
>    may not want to say "= 0" and the like here.

The = 0 case matters because that is the default when the index
struct is initialized to zero. The other values are not important.

Show 7 quoted lines

>  * Is it worth repeating the label in the comment?  IOW, I am
>    wondering if
> 
> 	/* there are no sparse directories in the index */
> 	COMPLETELY_FULL,
> 
>    is equally readable and slightly more maintainable.

Can do.

Show 5 quoted lines

> Also these names being in cache.h and visible everywhere is somewhat
> worrying.  Other CPP macros and enum constants in the file have
> names that are prepared to be truly global, but COMPLETELY_FULL and
> COLLAPSED do not hint that they are about sparse-index state as
> strongly as PARTIALLY_SPARSE does.

Would prepending with "INDEX_" be sufficient?

Show 14 quoted lines

>> -		 /*
>> -		  * sparse_index == 1 when sparse-directory
>> -		  * entries exist. Requires sparse-checkout
>> -		  * in cone mode.
>> -		  */
>> -		 sparse_index : 1;
>> +		 fsmonitor_has_run_once : 1;
>> +	enum sparse_index_mode sparse_index;
> 
> Good that we can lose the comment on a single bit.  Are some of the
> enum sparse_index_mode values only possible in cone mode, just like
> having true in this bit was only possible in cone mode?  Perhaps a
> comment before "enum sparse_index_mode {" can say "in non-cone mode,
> COMPLETELY_FULL is the only possible value" or somesuch?

I can add that comment for the COMPLETELY_FULL enum value.

Thanks, -Stolee