Re: [PATCH v3 3/9] diffcore-rename: simplify limit check

[Elijah Newren <newren@gmail.com>]

On Tue, Nov 9, 2021 at 1:15 PM Başar Uğur <basarugur@gmail.com> wrote:
>
> Hi all,
>
> First post on Git mailing list, to provide a comment on a patch. Hope
> this works.
>
> In cases where the `rename_limit` is already greater than 65535, the
> `st_mult(rename_limit, rename_limit)` multiplication overflows and
> process halts.

Out of curiosity, what system are you on?  And how many renames do you
actually have?

We used to clamp to 32767, but one specific repository needed values
larger than that, in the range of ~50000.  However, due to a number of
rename detection related optimizations added since then, the git of
today can handle that same particular repository and support full
rename detection with a rename limit under 1000 for merge/cherry-pick
(sorry, forgot the exact numbers), and probably less than 10000 for
diff (just guestimating; I don't want to go and check).

Anyway, all that aside, I don't see any such overflow for rename_limit
being 2**16; we can push it much farther:

    size_t a = 4294967295;   /*  2**32 -1  */
    size_t b = a;
    size_t c = st_mult(a, b);
    printf("%"PRIuMAX" = %"PRIuMAX" * %"PRIuMAX"\n", c, a, b);

Output:

    18446744065119617025 = 4294967295 * 4294967295

Adding one to the value of a results in:

    fatal: size_t overflow: 4294967296 * 4294967296

> But I don't think an 'overflow error' is very helpful
> for the users in understanding what is wrong with their configuration;
> i.e. `diff.renameLimit` documentation says nothing about a 'maximum
> allowed value'. I would either clamp it to a reasonable range, or
> inform the users about the limits, or maybe both.

That sounds reasonable, but I'm not sure it's worth the effort in
practice.  2**32 - 1 is so impractically large for a rename_limit that
I don't see why anyone would need a value even remotely close to that
level (and I wouldn't at all be surprised if other things in git
scaling broke before we even got to that point).

Perhaps you're dealing with a 32-bit system?  Even then, the
repository that hit this was about 6.5GB packed .git history; and you
might need to be a lot larger than that given the optimization since
then in order to hit this.  Can 32-bit systems even handle that size
of repository without dying in several other ways?