Re: [PATCH 1/1] fsmonitor: don't fill bitmap with entries to be removed

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

"William Baker via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  create mode 100755 t/t7519/fsmonitor-env
> ...
> +	if (pos >= istate->cache_nr)
> +		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %"PRIuMAX")",
> +		    (uintmax_t)pos, (uintmax_t)istate->cache_nr);

This is how we show size_t values without using "%z" that we avoid,
but are "pos" and 'cache_nr" size_t or ssize_t?  I thought they are
plain boring unsigned, so shouldn't we use the plain boring "%u"
without casting?

The same comment applies to other uses of uintmax_t cast in this
patch.

>  void fill_fsmonitor_bitmap(struct index_state *istate)
>  {
> -	unsigned int i;
> +	unsigned int i, skipped = 0;
>  	istate->fsmonitor_dirty = ewah_new();
> -	for (i = 0; i < istate->cache_nr; i++)
> -		if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
> -			ewah_set(istate->fsmonitor_dirty, i);
> +	for (i = 0; i < istate->cache_nr; i++) {
> +		if (istate->cache[i]->ce_flags & CE_REMOVE)
> +			skipped++;
> +		else if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
> +			ewah_set(istate->fsmonitor_dirty, i - skipped);
> +	}
>  }

Matches the explanation in the proposed log message pretty well.
Good job.

> @@ -354,4 +354,16 @@ test_expect_success 'discard_index() also discards fsmonitor info' '
>  	test_cmp expect actual
>  '
>  
> +# Use test files that start with 'z' so that the entries being added
> +# and removed appear at the end of the index.

In other words, future developers are warned against adding entries
to and leaving them in the index that sort later than z100 in new
tests they add before this point.  Is the above wording clear enough
to tell them that, I wonder?

> +test_expect_success 'status succeeds after staging/unstaging ' '
> +	test_commit initial &&
> +	removed=$(test_seq 1 100 | sed "s/^/z/") &&

Thanks.