Re: [PATCH v3 10/21] read-cache: regenerate shared index if necessary

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 2, 2017, 11:23 UTC
Message-ID: <CAP8UFD3_1EN=0EsD12Cew1MuW8yhtPAZw0M_g3wmvKFk-uGXxw@mail.gmail.com>
In-Reply-To: <xmqqk2al9ocv.fsf@gitster.mtv.corp.google.com>

On Tue, Dec 27, 2016 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> +     case 0:
>> +             return 1; /* 0% means always write a new shared index */
>> +     case 100:
>> +             return 0; /* 100% means never write a new shared index */
>> +     default:
>> +             ; /* do nothing: just use the configured value */
>> +     }
>
> Just like you did in 04/21, write "break" to avoid mistakes made in
> the future, i.e.
>
>         default:
>                 break; /* just use the configured value */

Ok, I will do that.

Show 14 quoted lines

>> +
>> +     /* Count not shared entries */
>> +     for (i = 0; i < istate->cache_nr; i++) {
>> +             struct cache_entry *ce = istate->cache[i];
>> +             if (!ce->index)
>> +                     not_shared++;
>> +     }
>> +
>> +     return istate->cache_nr * max_split < not_shared * 100;
>
> On a 32-bit arch with 2G int and more than 20 million paths in the
> index, multiplying by max_split that can come close to 100 can
> theoretically cause integer overflow, but in practice it probably
> does not matter.  Or does it?

From a cursory look a "struct cache_entry" takes at least 80 bytes without counting the "char name[FLEX_ARRAY]" on a 32 bit machine, so I don't think it would be a good idea to work on a repo with 20 million paths on a 32 bit machine, but maybe theoretically it could be a problem.

To be safe I think I will use:

return (int64_t)istate->cache_nr * max_split < (int64_t)not_shared * 100;