{"thread":{"id":"60973","subject":"Breaking change with \"git log -n\" since 2.43","startedAt":"2024-02-21T13:32:58Z","lastAt":"2024-02-21T17:51:54Z","messageCount":10,"participants":["Maarten Ackermans","Kristoffer Haugsbakk","Sean Allred","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"489078","messageId":"CAB=tB2vB0LbP=DznSqTFYHCRxDxd6U=Q+P33yeBzGssq2eK1vA@mail.gmail.com","threadId":"60973","inReplyTo":null,"subject":"Breaking change with \"git log -n\" since 2.43","fromName":"Maarten Ackermans","fromEmail":"maarten.ackermans@gmail.com","sentAt":"2024-02-21T13:32:46Z","receivedAt":"2024-02-21T13:32:58Z","isPatch":false,"sender":{"key":"maarten.ackermans@gmail.com","avatar":"https://gravatar.com/avatar/5b96124c22967f046cfa68ad16a880c2722672bd22c49665e283b66965c25b8e?d=mp&s=160"},"body":"Hi all,\n\nI would like to report a breaking change with \"git log -n\" introduced\nin 2.43 that's causing some trouble:\nhttps://github.com/git/git/commit/71a1e94821666909b7b2bd62a36244c601f8430e#diff-380c4eac267b5af349ace88c78a2b004a16ed20c2b605c76827981063924bbf9R2222\n\nTo reproduce, the command `git log -n 9007199254740991` fails on\n2.43.2, whereas it didn't on 2.42.0. This specific number corresponds\nto the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:\nhttps://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).\nThe max value that is supported now is a signed 32-bit integer (2^31 -\n1).\n\nI suppose git simply ignored the extra digits of the number, as the\ncommit message describes.\n\nSee https://github.com/intuit/auto/issues/2425#issuecomment-1956557071\nfor the impact.\n\nBest regards,\n\nMaarten Ackermans\n"},{"id":"489079","messageId":"9c52ea4e-f84e-4c64-977d-14a468236c80@app.fastmail.com","threadId":"60973","inReplyTo":"CAB=tB2vB0LbP=DznSqTFYHCRxDxd6U=Q+P33yeBzGssq2eK1vA@mail.gmail.com","subject":"Re: Breaking change with \"git log -n\" since 2.43","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-21T13:55:18Z","receivedAt":"2024-02-21T13:55:40Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Feb 21, 2024, at 14:32, Maarten Ackermans wrote:\n> Hi all,\n>\n> I would like to report a breaking change with \"git log -n\" introduced\n> in 2.43 that's causing some trouble:\n> https://github.com/git/git/commit/71a1e94821666909b7b2bd62a36244c601f8430e#diff-380c4eac267b5af349ace88c78a2b004a16ed20c2b605c76827981063924bbf9R2222\n>\n> To reproduce, the command `git log -n 9007199254740991` fails on\n> 2.43.2, whereas it didn't on 2.42.0. This specific number corresponds\n> to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:\n> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).\n> The max value that is supported now is a signed 32-bit integer (2^31 -\n> 1).\n>\n> I suppose git simply ignored the extra digits of the number, as the\n> commit message describes.\n>\n> See https://github.com/intuit/auto/issues/2425#issuecomment-1956557071\n> for the impact.\n>\n> Best regards,\n>\n> Maarten Ackermans\n\nI don’t see how this is a breaking change considering the range is not\ndocumented.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"489080","messageId":"CAB=tB2tgbLjBPvgBQDoNJi7e8+LMzxHSbg6D2jKUSJXPmQFrxA@mail.gmail.com","threadId":"60973","inReplyTo":"9c52ea4e-f84e-4c64-977d-14a468236c80@app.fastmail.com","subject":"Re: Breaking change with \"git log -n\" since 2.43","fromName":"Maarten Ackermans","fromEmail":"maarten.ackermans@gmail.com","sentAt":"2024-02-21T14:21:31Z","receivedAt":"2024-02-21T14:21:43Z","isPatch":false,"sender":{"key":"maarten.ackermans@gmail.com","avatar":"https://gravatar.com/avatar/5b96124c22967f046cfa68ad16a880c2722672bd22c49665e283b66965c25b8e?d=mp&s=160"},"body":"Whether or not that is Git's definition of a breaking change, the\nmessage of the commit in question acknowledges that the commands in\nthe \"log\" family are the oldest in the system:\n\n> 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.\n\nApplications that have been relying on undocumented features and\nlimits since they were introduced, now face a hard crash: \"fatal:\n'9007199254740991': not an integer\". Regardless of whether this is an\nimprovement for future implementations, a crash in existing ones is a\nsuboptimal experience at the least.\n\nOn Wed, Feb 21, 2024 at 8:55 PM Kristoffer Haugsbakk\n wrote:\n>\n> On Wed, Feb 21, 2024, at 14:32, Maarten Ackermans wrote:\n> > Hi all,\n> >\n> > I would like to report a breaking change with \"git log -n\" introduced\n> > in 2.43 that's causing some trouble:\n> > https://github.com/git/git/commit/71a1e94821666909b7b2bd62a36244c601f8430e#diff-380c4eac267b5af349ace88c78a2b004a16ed20c2b605c76827981063924bbf9R2222\n> >\n> > To reproduce, the command `git log -n 9007199254740991` fails on\n> > 2.43.2, whereas it didn't on 2.42.0. This specific number corresponds\n> > to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:\n> > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).\n> > The max value that is supported now is a signed 32-bit integer (2^31 -\n> > 1).\n> >\n> > I suppose git simply ignored the extra digits of the number, as the\n> > commit message describes.\n> >\n> > See https://github.com/intuit/auto/issues/2425#issuecomment-1956557071\n> > for the impact.\n> >\n> > Best regards,\n> >\n> > Maarten Ackermans\n>\n> I don’t see how this is a breaking change considering the range is not\n> documented.\n>\n> --\n> Kristoffer Haugsbakk\n>\n"},{"id":"489081","messageId":"m04je1dhdx.fsf@epic96565.epic.com","threadId":"60973","inReplyTo":"CAB=tB2tgbLjBPvgBQDoNJi7e8+LMzxHSbg6D2jKUSJXPmQFrxA@mail.gmail.com","subject":"Re: Breaking change with \"git log -n\" since 2.43","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2024-02-21T14:24:26Z","receivedAt":"2024-02-21T14:25:00Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"\nMaarten Ackermans writes:\n> Applications that have been relying on undocumented features and\n> limits since they were introduced, now face a hard crash: \"fatal:\n> '9007199254740991': not an integer\". Regardless of whether this is an\n> improvement for future implementations, a crash in existing ones is a\n> suboptimal experience at the least.\n\nWhat behavior would you propose instead?\n\n--\nSean Allred\n"},{"id":"489082","messageId":"CAB=tB2uZb+8QLmrk_tK5PKJtDE=RmBr=eBBb7U7ygSmkFoXvWg@mail.gmail.com","threadId":"60973","inReplyTo":"m04je1dhdx.fsf@epic96565.epic.com","subject":"Re: Breaking change with \"git log -n\" since 2.43","fromName":"Maarten Ackermans","fromEmail":"maarten.ackermans@gmail.com","sentAt":"2024-02-21T15:07:35Z","receivedAt":"2024-02-21T15:07:48Z","isPatch":false,"sender":{"key":"maarten.ackermans@gmail.com","avatar":"https://gravatar.com/avatar/5b96124c22967f046cfa68ad16a880c2722672bd22c49665e283b66965c25b8e?d=mp&s=160"},"body":"I would suggest displaying a warning in case of invalid input (such as\nthis out of range error), and to fall back to output all as if the\n\"-n\" flag was unspecified. If more strict handling is still desired,\nit could instead be a deprecation warning with a grace period, giving\napplications some time to update their git usage.\n\nOn Wed, Feb 21, 2024 at 9:25 PM Sean Allred wrote:\n>\n>\n> Maarten Ackermans writes:\n> > Applications that have been relying on undocumented features and\n> > limits since they were introduced, now face a hard crash: \"fatal:\n> > '9007199254740991': not an integer\". Regardless of whether this is an\n> > improvement for future implementations, a crash in existing ones is a\n> > suboptimal experience at the least.\n>\n> What behavior would you propose instead?\n>\n> --\n> Sean Allred\n"},{"id":"489083","messageId":"m0zfvtc0a4.fsf@epic96565.epic.com","threadId":"60973","inReplyTo":"CAB=tB2uZb+8QLmrk_tK5PKJtDE=RmBr=eBBb7U7ygSmkFoXvWg@mail.gmail.com","subject":"Re: Breaking change with \"git log -n\" since 2.43","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2024-02-21T15:17:24Z","receivedAt":"2024-02-21T15:19:50Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"\nMaarten Ackermans writes:\n\n> I would suggest displaying a warning in case of invalid input (such as\n> this out of range error), and to fall back to output all as if the\n> \"-n\" flag was unspecified.\n\nWas this the prior behavior? It sounds like from the commit you\nreferenced, atoi() simply would've stopped parsing after a point and\nyou'd end up with a (large, but finite) value for `-n`. I'm definitely\nreading between the lines here, though, and I must admit I've never\nprovided such bogus input to git-log myself.\n\n--\nSean Allred\n"},{"id":"489084","messageId":"310b2dcf-df69-4984-9a92-b8485e0f715b@app.fastmail.com","threadId":"60973","inReplyTo":"CAB=tB2uZb+8QLmrk_tK5PKJtDE=RmBr=eBBb7U7ygSmkFoXvWg@mail.gmail.com","subject":"Re: Breaking change with \"git log -n\" since 2.43","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-21T15:32:13Z","receivedAt":"2024-02-21T15:32:44Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Feb 21, 2024, at 16:07, Maarten Ackermans wrote:\n> I would suggest displaying a warning in case of invalid input (such as\n> this out of range error), and to fall back to output all as if the\n> \"-n\" flag was unspecified. If more strict handling is still desired,\n> it could instead be a deprecation warning with a grace period, giving\n> applications some time to update their git usage.\n\nFrom 71a1e9482:\n\n “ As a natural consequence, an argument that does not begin with a\n digit (e.g., \"q\") silently becomes zero, too.\n\nIt sounds like the non-breaking behavior for non-number input like `q`\nis to silently become `0`.[1] But then that too-large number input would\nalso become `0`, which doesn’t help for that JavaScript\napplication/library. Unless `strtol_i` is able to differentiate between\ndifferent errors by returning different negative numbers?\n\n† 1: Or else you risk breaking usages where they rely on bad input\n becoming `0`\n-- \nKristoffer Haugsbakk\n"},{"id":"489085","messageId":"89622ba9-10b3-4f03-8088-881a7e18a07a@app.fastmail.com","threadId":"60973","inReplyTo":"CAB=tB2tgbLjBPvgBQDoNJi7e8+LMzxHSbg6D2jKUSJXPmQFrxA@mail.gmail.com","subject":"Re: Breaking change with \"git log -n\" since 2.43","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-21T15:38:21Z","receivedAt":"2024-02-21T15:38:42Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Feb 21, 2024, at 15:21, Maarten Ackermans wrote:\n> Whether or not that is Git's definition of a breaking change, the\n> message of the commit in question acknowledges that the commands in\n> the \"log\" family are the oldest in the system:\n\nI was assuming Semantic Versioning (SV). Not because that’s the policy\nin Git (?) but because this problem happened in a JS app./library. And\nSV as far as I understand it define _breaking changes_ relative to the\n“public API”. And for git-log(1) the public API is the documentation,\nright?\n\n(But maybe I’ve just never managed to understand SV.)\n\nI also see now that this is some SV app./library.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"489086","messageId":"m0v86hbze6.fsf@epic96565.epic.com","threadId":"60973","inReplyTo":"CAB=tB2vKj45yr3amMbhv_dYBdqYOtoiMS7Ecx4WO1TE2STHEsA@mail.gmail.com","subject":"Re: Breaking change with \"git log -n\" since 2.43","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2024-02-21T15:34:28Z","receivedAt":"2024-02-21T15:38:59Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"\nMaarten Ackermans writes:\n\n> I think you’re right, so reinstating the original behavior with a\n> deprecation warning would be more prudent.\n>\n> After the grace period, if invalid input is given, fall back to output\n> all with a warning. Or you can go the strict route again and crash\n> with a fatal error (hard to imagine an actual use case for such a\n> large, specific number, anyway).\n\nTo be clear, I'm not advocating we go back to the prior behavior. I'm\njust clarifying what your proposal would mean.\n\nI don't pretend to have any nuanced understanding of how the Git project\nhandles situations like this, but IMHO, I would have to fall back to the\nprinciple of least surprise. Rejecting invalid input is not a crash --\nit is a useful guardrail to ensure the user and the computer agree on\nwhat's been requested / what's going to happen.\n\nAgain, I don't have any experience with how similar situations are\nhandled here, but I would contend that the behavior you describe as the\nbreaking behavior is the behavior that should exist long-term.\n\n--\nSean Allred\n"},{"id":"489098","messageId":"20240221175153.GD634809@coredump.intra.peff.net","threadId":"60973","inReplyTo":"CAB=tB2vB0LbP=DznSqTFYHCRxDxd6U=Q+P33yeBzGssq2eK1vA@mail.gmail.com","subject":"Re: Breaking change with \"git log -n\" since 2.43","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-21T17:51:53Z","receivedAt":"2024-02-21T17:51:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2024 at 08:32:46PM +0700, Maarten Ackermans wrote:\n\n> To reproduce, the command `git log -n 9007199254740991` fails on\n> 2.43.2, whereas it didn't on 2.42.0. This specific number corresponds\n> to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:\n> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).\n> The max value that is supported now is a signed 32-bit integer (2^31 -\n> 1).\n> \n> I suppose git simply ignored the extra digits of the number, as the\n> commit message describes.\n\nThe max value was always a signed 32-bit integer. The extra digits\nweren't ignored, but rather there was integer truncation at the C level.\nI believe that is technically implementation defined by the compiler,\nthough in practice your value would generally become -1.\n\nBut passing, say, 9007199254740993 would give quite unexpected results\n(the truncated value is \"1\" and we'd show only a single commit).\n\nSo I'm sympathetic that your specific number used to work and now\ndoesn't, but it feels like going back to the truncating behavior is a\nstep in the wrong direction.\n\nIf the goal is to have no limit at all, then passing an explicit \"-1\"\nworks, though I don't think that's a documented outcome. I do suspect\nthat we _would_ try to keep that historical behavior, as there is no\nother way to cancel a previous \"-n\" or otherwise say \"no limit\". It\nmight be worth formalizing that with documentation and a test.\n\n-Peff\n"}]}