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

From: Junio C Hamano <gitster@pobox.com>
Date: May 19, 2022, 20:05 UTC
Message-ID: <xmqqleuxmgof.fsf@gitster.g>
In-Reply-To: <269c206c331bb43006678beaa20832a75754c3df.1652982758.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

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

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

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.

Show 8 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?

Thanks.