On 5/19/2022 4:05 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
> * 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.
> 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?
>> - /*
>> - * 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
