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

From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 3/9] fsmonitor: de-duplicate BUG()s around dirty bits
Date: Wed, 20 Jan 2021 09:26:38 -0800	[thread overview]
Message-ID: <CABPp-BEL=tjCx_x1z0NpNTmt3q9_CHtufwWg9FtqKBWWub00UQ@mail.gmail.com> (raw)
In-Reply-To: <838922de2e9756c00f4a159b2b8722ae4d28b011.1611161639.git.gitgitgadget@gmail.com>

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.
>
> The end result is that the code is simpler to read while also preserving
> these assertions for developers in the FSMonitor space.

Indeed, looking through the patch, the end result is simpler to read.
Nice cleanup.

>
> 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);
> --
> gitgitgadget
>