{"thread":{"id":"62530","subject":"Extending whitespace checks","startedAt":"2024-11-24T02:25:23Z","lastAt":"2024-12-01T22:31:48Z","messageCount":9,"participants":["Junio C Hamano","Bence Ferdinandy","Kristoffer Haugsbakk","Jacob Keller","Jeff King","A bughunter"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"507976","messageId":"xmqqbjy5bc6m.fsf@gitster.g","threadId":"62530","inReplyTo":null,"subject":"Extending whitespace checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-24T02:25:21Z","receivedAt":"2024-11-24T02:25:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We have, via the attributes subsystem, a way to choose from a set of\npredefined whitespace rules so that \"git diff\" can notice that you\nare adding trailing whitespaces to your newly written lines, or you\nare indenting a newly introduced line in a Python script with a HT.\nThis can be used, for example, in pre-commit hook to reject an\nattempt to introduce whitespace-damaging changes to the codebase.\n\nWhich is great.\n\nI am wondering what we can do to add a different kind of checks to\nhelp file types with fixed format by extending the same mechanism,\nor the checks I have in mind are too different from the whitespace\nchecks and shoehorning it into the existing mechanism does not make\nsense. The particular check I have an immediate need for is for a\nfiletype with lines, each has exactly 4 fields separated with HT in\nbetween, so the check would ask \"does each line have exactly 3 HT on\nit?\" It would be extended to verify CSV files with fixed number of\nfields (but the validator needs to be aware of the quoting rules for\ncomma in a value in fields).\n\nI guess the best I could do (outside Git) is\n\n - write such a validator that can take one line of input and say\n \"this line comforms to the rule\".\n\n - add, via .gitattribute, my own attribute to allow me to mark\n the files that these rules apply. Git does not do anything\n special for this attribute (remember, I said \"outside Git\").\n\n - in pre-commit hook, run \"git diff ':(attr:myattr)'\" to grab\n changes in these files with special formats, and have the\n line-by-line validator (above) check the new lines.\n\nto make sure bad lines would not slip into the history, but it would\nbe really nice if I can trigger the check as part of \"git diff --check\",\nwhich means it would be more ideal if we can do this \"inside\" Git.\n\nPerhaps we could introduce a mechansim that allows me to do the\nfollowing:\n\n - An attribute, like whitespace=..., specifies what line-validation\n function to use to vet each new line introduced to a file with\n the attribute.\n\n - A line-validation function can be dynamically loaded/linked\n (here, we'd need \".gitattribute specifies the logical meaning,\n while .git/config and friends maps the 'logical meaning' to a\n specific implementation suitable for the platform\" separation,\n similar to what we use for smudge/clean filters). Perhaps this\n would be a good testbed for use of dll, written even in a foreign\n language like Rust?\n\n - In the diff machinery, where a '+' line is checked for whitespace\n anomalies in the existing code, add code to call the dynamically\n loaded line-validation function when applicable.\n\n - Profit?\n\nHmm?\n"},{"id":"507991","messageId":"D5UQHS9IV5N1.3IO1848Q1730B@ferdinandy.com","threadId":"62530","inReplyTo":"xmqqbjy5bc6m.fsf@gitster.g","subject":"Re: Extending whitespace checks","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-11-24T21:41:23Z","receivedAt":"2024-11-24T21:41:53Z","isPatch":false,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nOn Sun Nov 24, 2024 at 03:25, Junio C Hamano wrote:\n> We have, via the attributes subsystem, a way to choose from a set of\n> predefined whitespace rules so that \"git diff\" can notice that you\n> are adding trailing whitespaces to your newly written lines, or you\n> are indenting a newly introduced line in a Python script with a HT.\n> This can be used, for example, in pre-commit hook to reject an\n> attempt to introduce whitespace-damaging changes to the codebase.\n>\n> Which is great.\n>\n> I am wondering what we can do to add a different kind of checks to\n> help file types with fixed format by extending the same mechanism,\n> or the checks I have in mind are too different from the whitespace\n> checks and shoehorning it into the existing mechanism does not make\n> sense. The particular check I have an immediate need for is for a\n> filetype with lines, each has exactly 4 fields separated with HT in\n> between, so the check would ask \"does each line have exactly 3 HT on\n> it?\" It would be extended to verify CSV files with fixed number of\n> fields (but the validator needs to be aware of the quoting rules for\n> comma in a value in fields).\n>\n> I guess the best I could do (outside Git) is\n>\n> - write such a validator that can take one line of input and say\n> \"this line comforms to the rule\".\n>\n> - add, via .gitattribute, my own attribute to allow me to mark\n> the files that these rules apply. Git does not do anything\n> special for this attribute (remember, I said \"outside Git\").\n>\n> - in pre-commit hook, run \"git diff ':(attr:myattr)'\" to grab\n> changes in these files with special formats, and have the\n> line-by-line validator (above) check the new lines.\n>\n> to make sure bad lines would not slip into the history, but it would\n> be really nice if I can trigger the check as part of \"git diff --check\",\n> which means it would be more ideal if we can do this \"inside\" Git.\n>\n> Perhaps we could introduce a mechansim that allows me to do the\n> following:\n>\n> - An attribute, like whitespace=..., specifies what line-validation\n> function to use to vet each new line introduced to a file with\n> the attribute.\n>\n> - A line-validation function can be dynamically loaded/linked\n> (here, we'd need \".gitattribute specifies the logical meaning,\n> while .git/config and friends maps the 'logical meaning' to a\n> specific implementation suitable for the platform\" separation,\n> similar to what we use for smudge/clean filters). Perhaps this\n> would be a good testbed for use of dll, written even in a foreign\n> language like Rust?\n>\n> - In the diff machinery, where a '+' line is checked for whitespace\n> anomalies in the existing code, add code to call the dynamically\n> loaded line-validation function when applicable.\n>\n> - Profit?\n>\n> Hmm?\n\nThis might be a tangent, but since enhancing whitespace checking was mentioned,\nI'd thought I note here: `git log --check` running in the CI did not catch the\nwhite space errors in this patch (see the last hunk):\n\nhttps://lore.kernel.org/git/20241121225757.3877852-4-bence@ferdinandy.com/\n\nalthough it would have been certainly nice. I'm not sure if --check could\nalready catch this actually, or if it would be easy/possible to have something\ngeneral enough that does catch it.\n\n\nBest,\nBence\n"},{"id":"507992","messageId":"89b1b39c-a6d2-4f63-9cc5-722772bddd8a@app.fastmail.com","threadId":"62530","inReplyTo":"D5UQHS9IV5N1.3IO1848Q1730B@ferdinandy.com","subject":"Re: Extending whitespace checks","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-11-24T21:58:11Z","receivedAt":"2024-11-24T21:58:32Z","isPatch":false,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Sun, Nov 24, 2024, at 22:41, Bence Ferdinandy wrote:\n> This might be a tangent, but since enhancing whitespace checking was mentioned,\n> I'd thought I note here: `git log --check` running in the CI did not catch the\n> white space errors in this patch (see the last hunk):\n>\n> https://lore.kernel.org/git/20241121225757.3877852-4-bence@ferdinandy.com/\n>\n> although it would have been certainly nice. I'm not sure if --check could\n> already catch this actually, or if it would be easy/possible to have something\n> general enough that does catch it.\n\nIt looks like you indented some lines with spaces instead of tabs. It\ndoesn’t look like a “whitespace error” in the `--check` sense as I\nunderstand it.\n"},{"id":"508006","messageId":"xmqqserg6mlv.fsf@gitster.g","threadId":"62530","inReplyTo":"D5UQHS9IV5N1.3IO1848Q1730B@ferdinandy.com","subject":"Re: Extending whitespace checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-25T03:03:40Z","receivedAt":"2024-11-25T03:03:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Bence Ferdinandy\" writes:\n\n> This might be a tangent, but since enhancing whitespace checking was mentioned,\n\nSince Git was mentioned, let me talk about something that is related\nto Git, even though it is completely unrelated to the discussion you\nare trying to start. Which may distract and bury whatever you\nwanted to discuss in the list traffic noise, but I do not care ;-)\n\nDon't do that, please.\n\n\n"},{"id":"508116","messageId":"CA+P7+xoGSHzibDC0-+r-xTNkuWtuUaEqn2wgrty45fomhGDT5A@mail.gmail.com","threadId":"62530","inReplyTo":"xmqqbjy5bc6m.fsf@gitster.g","subject":"Re: Extending whitespace checks","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2024-11-25T22:04:59Z","receivedAt":"2024-11-25T22:05:12Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sat, Nov 23, 2024 at 6:25 PM Junio C Hamano wrote:\n>\n> We have, via the attributes subsystem, a way to choose from a set of\n> predefined whitespace rules so that \"git diff\" can notice that you\n> are adding trailing whitespaces to your newly written lines, or you\n> are indenting a newly introduced line in a Python script with a HT.\n> This can be used, for example, in pre-commit hook to reject an\n> attempt to introduce whitespace-damaging changes to the codebase.\n>\n> Which is great.\n>\n> I am wondering what we can do to add a different kind of checks to\n> help file types with fixed format by extending the same mechanism,\n> or the checks I have in mind are too different from the whitespace\n> checks and shoehorning it into the existing mechanism does not make\n> sense. The particular check I have an immediate need for is for a\n> filetype with lines, each has exactly 4 fields separated with HT in\n> between, so the check would ask \"does each line have exactly 3 HT on\n> it?\" It would be extended to verify CSV files with fixed number of\n> fields (but the validator needs to be aware of the quoting rules for\n> comma in a value in fields).\n>\n> I guess the best I could do (outside Git) is\n>\n> - write such a validator that can take one line of input and say\n> \"this line comforms to the rule\".\n>\n> - add, via .gitattribute, my own attribute to allow me to mark\n> the files that these rules apply. Git does not do anything\n> special for this attribute (remember, I said \"outside Git\").\n>\n> - in pre-commit hook, run \"git diff ':(attr:myattr)'\" to grab\n> changes in these files with special formats, and have the\n> line-by-line validator (above) check the new lines.\n>\n> to make sure bad lines would not slip into the history, but it would\n> be really nice if I can trigger the check as part of \"git diff --check\",\n> which means it would be more ideal if we can do this \"inside\" Git.\n>\n> Perhaps we could introduce a mechansim that allows me to do the\n> following:\n>\n> - An attribute, like whitespace=..., specifies what line-validation\n> function to use to vet each new line introduced to a file with\n> the attribute.\n>\n> - A line-validation function can be dynamically loaded/linked\n> (here, we'd need \".gitattribute specifies the logical meaning,\n> while .git/config and friends maps the 'logical meaning' to a\n> specific implementation suitable for the platform\" separation,\n> similar to what we use for smudge/clean filters). Perhaps this\n> would be a good testbed for use of dll, written even in a foreign\n> language like Rust?\n>\n> - In the diff machinery, where a '+' line is checked for whitespace\n> anomalies in the existing code, add code to call the dynamically\n> loaded line-validation function when applicable.\n>\n> - Profit?\n>\n\nI like the idea of an extensible check mechanism with an API. I can\nthink of a couple of other places where such a check could be useful\nto ensure formatting. I do think this is slightly more general than\nwhitespace checking.. The concept seems reasonable to me tho.\n\n> Hmm?\n>\n"},{"id":"508231","messageId":"20241127150429.GD2554@coredump.intra.peff.net","threadId":"62530","inReplyTo":"xmqqbjy5bc6m.fsf@gitster.g","subject":"Re: Extending whitespace checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-27T15:04:29Z","receivedAt":"2024-11-27T15:04:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 24, 2024 at 11:25:21AM +0900, Junio C Hamano wrote:\n\n> I am wondering what we can do to add a different kind of checks to\n> help file types with fixed format by extending the same mechanism,\n> or the checks I have in mind are too different from the whitespace\n> checks and shoehorning it into the existing mechanism does not make\n> sense. The particular check I have an immediate need for is for a\n> filetype with lines, each has exactly 4 fields separated with HT in\n> between, so the check would ask \"does each line have exactly 3 HT on\n> it?\" It would be extended to verify CSV files with fixed number of\n> fields (but the validator needs to be aware of the quoting rules for\n> comma in a value in fields).\n\nComing from a devil's advocate position: what makes these CSV format\nchecks any different than syntax checks we get from a compiler? Or for\nthat matter, the result of running \"make test\"?\n\nI.e., why implement a complex system for single-line verification\nplugins when you'd be left with the much larger problem of evaluating\nwhole-tree states. And once you have solutions for that (like using\nbranches to separate unverified work and then merging it once it has\npassed checks), then simple things like line syntax are easy to call\nthere.\n\nNow you could argue that the existing whitespace checks are similarly\nredundant. Rather than having \"apply\" complain about whitespace errors,\nyou could just check them as part of \"make test\".\n\nThe reasons I can think of for doing something like this are:\n\n - catching problems earlier is almost always less work for the user\n\n - for things that _are_ line-oriented, looking at individual diff\n lines lets you focus on problems being added, without worrying about\n existing violations in the final state. OTOH, that's not foolproof;\n if you modify a line with an existing whitespace problem without\n fixing it, \"diff --check\" will still complain.\n\nSo I'm not necessarily against it. But it seems like a very deep rabbit\nhole to start adding in shared-library line validators, because I think\nit ends in \"now compile this before I agree to apply the patch\". And I\nthink Git's model has mostly been the opposite: make it cheap and\nprivate to branch and make changes (including applying patches) so that\nyou can inspect the state before deciding whether and how to publish.\n\n-Peff\n"},{"id":"508253","messageId":"xmqq7c8os07x.fsf@gitster.g","threadId":"62530","inReplyTo":"20241127150429.GD2554@coredump.intra.peff.net","subject":"Re: Extending whitespace checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-27T23:53:06Z","receivedAt":"2024-11-27T23:53:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But it seems like a very deep rabbit hole to start adding in\n> shared-library line validators, because I think it ends in \"now\n> compile this before I agree to apply the patch\".\n\nI am not sure I understand your conclusion. Who is telling that to\nwhom? Somebody sends a patch that creates a file that requires a\nspecial validator and the maintainer gives the validator and tells\nthe contributor to go use it to make sure their addition passses\nbefore resubmitting?\n\nI was hoping that the ability to add extra validators is more of an\nenabler (than requirement and hindrance) for those who choose to be\nextra careful. It is similar to CFLAGS in our Makefile that allows\nyou to use options to enable more strict compiler warnings than what\nother developers usually use, to notice certain class of problems\nothers may miss.\n\nShared-libraries and plug-ins remain to be solution in search of\nproblem at least for this project. I do not really need CSV comma\ncounter, but I thought it may give a good excuse for those who want\nto play with Rust and other stuff ;-)\n"},{"id":"508383","messageId":"cEDEN8jk6KzFgDQ32ejB3TGL8nHkAwS84330i3oroO-rwjWmor_NGAwnAjOt_25zEbYgsNBoE9QMLXAsLW_R_IQVUM14kDzleSStPpilyK8=@proton.me","threadId":"62530","inReplyTo":"89b1b39c-a6d2-4f63-9cc5-722772bddd8a@app.fastmail.com","subject":"Re: Extending whitespace checks","fromName":"A bughunter","fromEmail":"a_bughunter@proton.me","sentAt":"2024-12-01T02:51:49Z","receivedAt":"2024-12-01T02:52:00Z","isPatch":false,"sender":{"key":"a_bughunter@proton.me","avatar":null},"body":"-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA512\n\n \" This might be a tangent, but since enhancing whitespace checking was mentioned, \"shoehorning\": If it ain't broke don't fix it. Prudence indeed. Especially if you are messing with something having what you called \" landmines\". \n-----BEGIN PGP SIGNATURE-----\nVersion: ProtonMail\n\nwnUEARYKACcFgmdLz0MJkKkWZTlQrvKZFiEEZlQIBcAycZ2lO9z2qRZlOVCu\n8pkAAIROAP46l2AgKrh/FKvSwUHxC6XdqMYODOi7zTMYy7Annv72nQEAn6sQ\nrtlEGS/xcb1BMKaMUWS5zYxrdi3hrwFsbasCQwU=\n=Fwc6\n-----END PGP SIGNATURE-----\n"},{"id":"508400","messageId":"20241201223146.GI145938@coredump.intra.peff.net","threadId":"62530","inReplyTo":"xmqq7c8os07x.fsf@gitster.g","subject":"Re: Extending whitespace checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-01T22:31:46Z","receivedAt":"2024-12-01T22:31:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 28, 2024 at 08:53:06AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But it seems like a very deep rabbit hole to start adding in\n> > shared-library line validators, because I think it ends in \"now\n> > compile this before I agree to apply the patch\".\n> \n> I am not sure I understand your conclusion. Who is telling that to\n> whom? Somebody sends a patch that creates a file that requires a\n> special validator and the maintainer gives the validator and tells\n> the contributor to go use it to make sure their addition passses\n> before resubmitting?\n> \n> I was hoping that the ability to add extra validators is more of an\n> enabler (than requirement and hindrance) for those who choose to be\n> extra careful. It is similar to CFLAGS in our Makefile that allows\n> you to use options to enable more strict compiler warnings than what\n> other developers usually use, to notice certain class of problems\n> others may miss.\n\nYes, we who introduce the mechanism to create plug-ins do not have to\nworry about writing those plug-ins ourselves. But we do have to maintain\nthe plug-in interface, and respond to complaints when it is not rich\nenough to do what people want to do. So I was merely pessimistically\nforeseeing where this may end up. ;)\n\nOf course...\n\n> Shared-libraries and plug-ins remain to be solution in search of\n> problem at least for this project. I do not really need CSV comma\n> counter, but I thought it may give a good excuse for those who want\n> to play with Rust and other stuff ;-)\n\n...if playing with the plug-in interface is the point, none of that may\nmatter. :)\n\n-Peff\n"}]}