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

From: Elijah Newren <newren@gmail.com>
Date: Nov 10, 2021, 20:06 UTC
Message-ID: <CABPp-BFqZj4qYXbPGLyX=4RM4OdLNL=VbYyhbLakU-RrvU+wfw@mail.gmail.com>
In-Reply-To: <CAPNYHkm5Qp8HQGU5_6DXc7xpCrdGgEin0WYAxbRNOkKW0j2ERQ@mail.gmail.com>

On Tue, Nov 9, 2021 at 1:15 PM Başar Uğur <basarugur@gmail.com> wrote:

Show 9 quoted lines

>
> 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

Show 5 quoted lines

> 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?