Breaking change with "git log -n" since 2.43

Breaking change with "git log -n" since 2.43

[Maarten Ackermans]

Hi all,

I would like to report a breaking change with "git log -n" introduced
in 2.43 that's causing some trouble:
https://github.com/git/git/commit/71a1e94821666909b7b2bd62a36244c601f8430e#diff-380c4eac267b5af349ace88c78a2b004a16ed20c2b605c76827981063924bbf9R2222

To reproduce, the command `git log -n 9007199254740991` fails on
2.43.2, whereas it didn't on 2.42.0. This specific number corresponds
to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).
The max value that is supported now is a signed 32-bit integer (2^31 -
1).

I suppose git simply ignored the extra digits of the number, as the
commit message describes.

See https://github.com/intuit/auto/issues/2425#issuecomment-1956557071
for the impact.

Best regards,

Maarten Ackermans

Re: Breaking change with "git log -n" since 2.43

[Kristoffer Haugsbakk]

On Wed, Feb 21, 2024, at 14:32, Maarten Ackermans wrote:
> Hi all,
>
> I would like to report a breaking change with "git log -n" introduced
> in 2.43 that's causing some trouble:
> https://github.com/git/git/commit/71a1e94821666909b7b2bd62a36244c601f8430e#diff-380c4eac267b5af349ace88c78a2b004a16ed20c2b605c76827981063924bbf9R2222
>
> To reproduce, the command `git log -n 9007199254740991` fails on
> 2.43.2, whereas it didn't on 2.42.0. This specific number corresponds
> to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).
> The max value that is supported now is a signed 32-bit integer (2^31 -
> 1).
>
> I suppose git simply ignored the extra digits of the number, as the
> commit message describes.
>
> See https://github.com/intuit/auto/issues/2425#issuecomment-1956557071
> for the impact.
>
> Best regards,
>
> Maarten Ackermans

I don’t see how this is a breaking change considering the range is not
documented.

-- 
Kristoffer Haugsbakk

Re: Breaking change with "git log -n" since 2.43

[Maarten Ackermans]

Whether or not that is Git's definition of a breaking change, the
message of the commit in question acknowledges that the commands in
the "log" family are the oldest in the system:

> The "rev-list" and other commands in the "log" family, being the oldest part of the system, use their own custom argument parsers, and integer values of some options are parsed with atoi(), which allows a non-digit after the number (e.g., "1q") to be silently ignored. As a natural consequence, an argument that does not begin with a digit (e.g., "q") silently becomes zero, too.

Applications that have been relying on undocumented features and
limits since they were introduced, now face a hard crash: "fatal:
'9007199254740991': not an integer". Regardless of whether this is an
improvement for future implementations, a crash in existing ones is a
suboptimal experience at the least.

On Wed, Feb 21, 2024 at 8:55 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
>
> On Wed, Feb 21, 2024, at 14:32, Maarten Ackermans wrote:
> > Hi all,
> >
> > I would like to report a breaking change with "git log -n" introduced
> > in 2.43 that's causing some trouble:
> > https://github.com/git/git/commit/71a1e94821666909b7b2bd62a36244c601f8430e#diff-380c4eac267b5af349ace88c78a2b004a16ed20c2b605c76827981063924bbf9R2222
> >
> > To reproduce, the command `git log -n 9007199254740991` fails on
> > 2.43.2, whereas it didn't on 2.42.0. This specific number corresponds
> > to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:
> > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).
> > The max value that is supported now is a signed 32-bit integer (2^31 -
> > 1).
> >
> > I suppose git simply ignored the extra digits of the number, as the
> > commit message describes.
> >
> > See https://github.com/intuit/auto/issues/2425#issuecomment-1956557071
> > for the impact.
> >
> > Best regards,
> >
> > Maarten Ackermans
>
> I don’t see how this is a breaking change considering the range is not
> documented.
>
> --
> Kristoffer Haugsbakk
>

Re: Breaking change with "git log -n" since 2.43

[Sean Allred]

Maarten Ackermans <maarten.ackermans@gmail.com> writes:
> Applications that have been relying on undocumented features and
> limits since they were introduced, now face a hard crash: "fatal:
> '9007199254740991': not an integer". Regardless of whether this is an
> improvement for future implementations, a crash in existing ones is a
> suboptimal experience at the least.

What behavior would you propose instead?

--
Sean Allred

Re: Breaking change with "git log -n" since 2.43

[Maarten Ackermans]

I would suggest displaying a warning in case of invalid input (such as
this out of range error), and to fall back to output all as if the
"-n" flag was unspecified. If more strict handling is still desired,
it could instead be a deprecation warning with a grace period, giving
applications some time to update their git usage.

On Wed, Feb 21, 2024 at 9:25 PM Sean Allred <allred.sean@gmail.com> wrote:
>
>
> Maarten Ackermans <maarten.ackermans@gmail.com> writes:
> > Applications that have been relying on undocumented features and
> > limits since they were introduced, now face a hard crash: "fatal:
> > '9007199254740991': not an integer". Regardless of whether this is an
> > improvement for future implementations, a crash in existing ones is a
> > suboptimal experience at the least.
>
> What behavior would you propose instead?
>
> --
> Sean Allred

Re: Breaking change with "git log -n" since 2.43

[Sean Allred]

Maarten Ackermans <maarten.ackermans@gmail.com> writes:

> I would suggest displaying a warning in case of invalid input (such as
> this out of range error), and to fall back to output all as if the
> "-n" flag was unspecified.

Was this the prior behavior? It sounds like from the commit you
referenced, atoi() simply would've stopped parsing after a point and
you'd end up with a (large, but finite) value for `-n`. I'm definitely
reading between the lines here, though, and I must admit I've never
provided such bogus input to git-log myself.

--
Sean Allred

Re: Breaking change with "git log -n" since 2.43

[Sean Allred]

Maarten Ackermans <maarten.ackermans@gmail.com> writes:

> I think you’re right, so reinstating the original behavior with a
> deprecation warning would be more prudent.
>
> After the grace period, if invalid input is given, fall back to output
> all with a warning. Or you can go the strict route again and crash
> with a fatal error (hard to imagine an actual use case for such a
> large, specific number, anyway).

To be clear, I'm not advocating we go back to the prior behavior. I'm
just clarifying what your proposal would mean.

I don't pretend to have any nuanced understanding of how the Git project
handles situations like this, but IMHO, I would have to fall back to the
principle of least surprise. Rejecting invalid input is not a crash --
it is a useful guardrail to ensure the user and the computer agree on
what's been requested / what's going to happen.

Again, I don't have any experience with how similar situations are
handled here, but I would contend that the behavior you describe as the
breaking behavior is the behavior that should exist long-term.

--
Sean Allred

Re: Breaking change with "git log -n" since 2.43

[Kristoffer Haugsbakk]

On Wed, Feb 21, 2024, at 16:07, Maarten Ackermans wrote:
> I would suggest displaying a warning in case of invalid input (such as
> this out of range error), and to fall back to output all as if the
> "-n" flag was unspecified. If more strict handling is still desired,
> it could instead be a deprecation warning with a grace period, giving
> applications some time to update their git usage.

From 71a1e9482:

  “ As a natural consequence, an argument that does not begin with a
    digit (e.g., "q") silently becomes zero, too.

It sounds like the non-breaking behavior for non-number input like `q`
is to silently become `0`.[1] But then that too-large number input would
also become `0`, which doesn’t help for that JavaScript
application/library. Unless `strtol_i` is able to differentiate between
different errors by returning different negative numbers?

† 1: Or else you risk breaking usages where they rely on bad input
    becoming `0`
-- 
Kristoffer Haugsbakk

Re: Breaking change with "git log -n" since 2.43

[Kristoffer Haugsbakk]

On Wed, Feb 21, 2024, at 15:21, Maarten Ackermans wrote:
> Whether or not that is Git's definition of a breaking change, the
> message of the commit in question acknowledges that the commands in
> the "log" family are the oldest in the system:

I was assuming Semantic Versioning (SV). Not because that’s the policy
in Git (?) but because this problem happened in a JS app./library. And
SV as far as I understand it define _breaking changes_ relative to the
“public API”. And for git-log(1) the public API is the documentation,
right?

(But maybe I’ve just never managed to understand SV.)

I also see now that this is some SV app./library.

-- 
Kristoffer Haugsbakk

Re: Breaking change with "git log -n" since 2.43

[Jeff King]

On Wed, Feb 21, 2024 at 08:32:46PM +0700, Maarten Ackermans wrote:

> To reproduce, the command `git log -n 9007199254740991` fails on
> 2.43.2, whereas it didn't on 2.42.0. This specific number corresponds
> to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).
> The max value that is supported now is a signed 32-bit integer (2^31 -
> 1).
> 
> I suppose git simply ignored the extra digits of the number, as the
> commit message describes.

The max value was always a signed 32-bit integer. The extra digits
weren't ignored, but rather there was integer truncation at the C level.
I believe that is technically implementation defined by the compiler,
though in practice your value would generally become -1.

But passing, say, 9007199254740993 would give quite unexpected results
(the truncated value is "1" and we'd show only a single commit).

So I'm sympathetic that your specific number used to work and now
doesn't, but it feels like going back to the truncating behavior is a
step in the wrong direction.

If the goal is to have no limit at all, then passing an explicit "-1"
works, though I don't think that's a documented outcome. I do suspect
that we _would_ try to keep that historical behavior, as there is no
other way to cancel a previous "-n" or otherwise say "no limit". It
might be worth formalizing that with documentation and a test.

-Peff