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

From: William Baker <williamtbakeremail@gmail.com>
Date: Oct 7, 2019, 18:10 UTC
Message-ID: <0de8e44f-53b8-e530-6e75-f696ffa889ce@gmail.com>
In-Reply-To: <xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com>

On 10/3/19 4:36 PM, Junio C Hamano wrote:

Show 12 quoted lines

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

Thanks for catching this. I will update these BUGs in the next patch to avoid casting.

Show 8 quoted lines

>> +# 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?
>

You're understanding is correct, and I agree this comment could be clearer. I will fix this up in v2.

Thanks for the feedback! William