Re: [PATCH 1/1] fsmonitor: don't fill bitmap with entries to be removed
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 3, 2019, 23:36 UTC
- Message-ID
- <xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <ce9bf4237e69fcaf2b3e8b50bb88ff61c3b0f710.1570132194.git.gitgitgadget@gmail.com>
"William Baker via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> 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.
Show 15 quoted lines
> 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.
Show 6 quoted lines
> @@ -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.