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

From: Başar Uğur <basarugur@gmail.com>
Date: Nov 11, 2021, 09:02 UTC
Message-ID: <CAPNYHkn5aHW1e_G5BKcaHWnrTSR+=VKOnKPdDPLJ5bH9DedKTA@mail.gmail.com>
In-Reply-To: <CABPp-BFqZj4qYXbPGLyX=4RM4OdLNL=VbYyhbLakU-RrvU+wfw@mail.gmail.com>

On Wed, Nov 10, 2021 at 9:06 PM Elijah Newren <newren@gmail.com> wrote:

Show 14 quoted lines

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

I am on a 64-bit Windows 10; but following up on your question, it became obvious that these limits have something to do with 'which git executable' I was dealing with. This problem surfaced when I was working on Visual Studio 2019, and was trying to rename not more than 10 files. My config had 999999 as the renameLimit, and VS2019 showed the 'fatal error' in its git output. However, git bash was all fine with listing the renamed files. And the difference between these scenarios turned out to be, yes, different git executables. VS2019 has its own copy of git which is 32-bit, and it hits this 999999 * 999999 overflow; whereas *my* copy of git used in bash is 64-bit which does not have that overflow problem.

Show 42 quoted lines

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

Good point, but the system aside, 2**16 - 1 = 65535 would remain to be the limit for the 32-bit git executables, wherever they are used. Therefore, maybe there is a point to curb it, or mention this somewhere, as I have said before.

-- 
Basar