Re: [PATCH 3/9] midx: mark bad packed objects

From: Stefan Beller <sbeller@google.com>
Date: Aug 20, 2018, 21:23 UTC
Message-ID: <CAGZ79kY8i3y6_r=bQAx7V5=ckSmb_RgyByjJwtS+7Qr2y8x8LQ@mail.gmail.com>
In-Reply-To: <20180820165124.152146-4-dstolee@microsoft.com>

On Mon, Aug 20, 2018 at 9:52 AM Derrick Stolee <dstolee@microsoft.com> wrote:

Show 8 quoted lines

>
> When an object fails to decompress from a pack-file, we mark the object
> as 'bad' so we can retry with a different copy of the object (if such a
> copy exists).
>
> Before now, the multi-pack-index did not update the bad objects list for
> the pack-files it contains, and we did not check the bad objects list
> when reading an object. Now, do both.

This sounds like a bug fix unlike patches 1 & 2 that sound like feature work(2) or making code more readable(1). (After studying the code, this doesn't sound like a bug fix any more, but a safety thing)

Is it worth having this on a separate track coming in faster than the rest of this series?

Show 16 quoted lines

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  midx.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/midx.c b/midx.c
> index 6824acf5f8..7fa75a37a3 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -280,6 +280,16 @@ static int nth_midxed_pack_entry(struct multi_pack_index *m, struct pack_entry *
>         if (!is_pack_valid(p))
>                 return 0;
>
> +       if (p->num_bad_objects) {
> +               uint32_t i;

Is there a reason that i needs to be if 32 bits? Would size_t (for iterating) or 'int' (as a default like in many for loops) be ok, too?