Breaking change with "git log -n" since 2.43

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

On Wed, Feb 21, 2024, at 14:32, Maarten Ackermans wrote:

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

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:

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:

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

What behavior would you propose instead?

-- Sean Allred

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:

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

On Wed, Feb 21, 2024, at 16:07, Maarten Ackermans wrote:

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`

On Wed, Feb 21, 2024, at 15:21, Maarten Ackermans wrote:

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.

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

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

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

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