Re: [PATCH v2 3/8] fsmonitor: de-duplicate BUG()s around dirty bits

[Junio C Hamano <gitster@pobox.com>]

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

> From: Derrick Stolee <dstolee@microsoft.com>
>
> The index has an fsmonitor_dirty bitmap that records which index entries
> are "dirty" based on the response from the FSMonitor. If this bitmap
> ever grows larger than the index, then there was an error in how it was
> constructed, and it was probably a developer's bug.
>
> There are several BUG() statements that are very similar, so replace
> these uses with a simpler assert_index_minimum(). Since there is one
> caller that uses a custom 'pos' value instead of the bit_size member, we
> cannot simplify it too much. However, the error string is identical in
> each, so this simplifies things.

Also that single caller with a custom 'pos' used to allow 'pos' to
point one beyond the end of istate->cache[] array, but now it is
forbidden.  If this is a strict bugfix, it probably deserves a
mention here.

> The end result is that the code is simpler to read while also preserving
> these assertions for developers in the FSMonitor space.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  fsmonitor.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index ca031c3abb8..52a50a9545a 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -13,14 +13,19 @@
>  
>  struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
>  
> +static void assert_index_minimum(struct index_state *istate, size_t pos)
> +{
> +	if (pos > istate->cache_nr)
> +		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> +		    (uintmax_t)pos, istate->cache_nr);
> +}
> +
>  static void fsmonitor_ewah_callback(size_t pos, void *is)
>  {
>  	struct index_state *istate = (struct index_state *)is;
>  	struct cache_entry *ce;
>  
> -	if (pos >= istate->cache_nr)
> -		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
> -		    (uintmax_t)pos, istate->cache_nr);
> +	assert_index_minimum(istate, pos);
>  
>  	ce = istate->cache[pos];
>  	ce->ce_flags &= ~CE_FSMONITOR_VALID;
> @@ -82,10 +87,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
>  	}
>  	istate->fsmonitor_dirty = fsmonitor_dirty;
>  
> -	if (!istate->split_index &&
> -	    istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> -		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> -		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> +	if (!istate->split_index)
> +		assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
>  
>  	trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
>  	return 0;
> @@ -110,10 +113,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
>  	uint32_t ewah_size = 0;
>  	int fixup = 0;
>  
> -	if (!istate->split_index &&
> -	    istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> -		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> -		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> +	if (!istate->split_index)
> +		assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
>  
>  	put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);
>  	strbuf_add(sb, &hdr_version, sizeof(uint32_t));
> @@ -335,9 +336,7 @@ void tweak_fsmonitor(struct index_state *istate)
>  			}
>  
>  			/* Mark all previously saved entries as dirty */
> -			if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> -				BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> -				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> +			assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
>  			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
>  
>  			refresh_fsmonitor(istate);