{"thread":{"id":"60614","subject":"[PATCH] tests: drop dependency on `git diff` in check-chainlint","startedAt":"2023-12-14T03:25:05Z","lastAt":"2023-12-15T05:36:11Z","messageCount":5,"participants":["Eric Sunshine","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485628","messageId":"20231214032248.1615-1-ericsunshine@charter.net","threadId":"60614","inReplyTo":null,"subject":"[PATCH] tests: drop dependency on `git diff` in check-chainlint","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2023-12-14T03:22:48Z","receivedAt":"2023-12-14T03:25:05Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"From: Eric Sunshine \n\nThe \"check-chainlint\" target runs automatically when running tests and\nperforms self-checks to verify that the chainlinter itself produces the\nexpected output. Originally, the chainlinter was implemented via sed,\nbut the infrastructure has been rewritten in fb41727b7e (t: retire\nunused chainlint.sed, 2022-09-01) to use a Perl script instead.\n\nThe rewrite caused some slight whitespace changes in the output that are\nultimately not of much importance. In order to be able to assert that\nthe actual chainlinter errors match our expectations we thus have to\nignore whitespace characters when diffing them. As the `-w` flag is not\nin POSIX we try to use `git diff -w --no-index` before we fall back to\nnon-standard `diff -w -u`.\n\nTo accommodate for cases where the host system has no Git installation\nwe use the locally-compiled version of Git. This can result in problems\nthough when the Git project's repository is using extensions that the\nlocally-compiled version of Git doesn't understand, in which case `git`\nmay refuse to run and thus cause the checks to fail.\n\nWork around this issue by normalizing whitespace via sed before invoking\ndiff, which allows any platform diff implementation to be used, thus\neliminating the dependency upon `git diff` and the non-POSIX `-w` flag.\n\nReported-by: Patrick Steinhardt \nSigned-off-by: Eric Sunshine \n---\n\nThis is an alternative solution to the issue Patrick's patch[1]\naddresses. Hopefully, this approach should avoid the sort of push-back\nPatrick's patch received[2].\n\nI shamelessly stole most of Patrick's commit message.\n\nThe sed expressions for normalizing whitespace prior to `diff` may look\na bit hairy, but they are simple enough in concept:\n\n* collapse runs of whitespace to a single SP\n* drop blank lines (this step is not new)\n* fold out possible SP at beginning and end of each line\n* fold out SP surrounding common punctuation characters used in shell\n scripts, such as `>`, `|`, `;`, etc.\n\nBy the way, I'm somewhat surprised that this issue crops up at all\nconsidering that --no-index is being used with git-diff. As such, I\nwould have thought that the local repository's format would not have\nbeen interrogated at all. If that's a bug in `git diff --no-index`, then\nfixing that could be considered yet another alternative solution to the\nissue raised here.\n\n[1]: https://lore.kernel.org/git/4112adbe467c14a8f22a87ea41aa4705f8760cf6.1702380646.git.ps@pks.im/\n[2]: https://lore.kernel.org/git/xmqqr0jqnnmn.fsf@gitster.g/\n\n t/Makefile | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 225aaf78ed..656ff10afa 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -103,20 +103,12 @@ check-chainlint:\n \t\techo \"# chainlint: $(CHAINLINTTMP_SQ)/tests\" && \\\n \t\tfor i in $(CHAINLINTTESTS); do \\\n \t\t\techo \"# chainlint: $$i\" && \\\n-\t\t\tsed -e '/^[ \t]*$$/d' chainlint/$$i.expect; \\\n+\t\t\tsed -e 's/[ \t][ \t]*/ /g;/^ *$$/d;s/^ //;s/ $$//;s/\\([<>|();&]\\) /\\1/g;s/ \\([<>|();&]\\)/\\1/g' chainlint/$$i.expect; \\\n \t\tdone \\\n \t} >'$(CHAINLINTTMP_SQ)'/expect && \\\n \t$(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \\\n-\t\tsed -e 's/^[1-9][0-9]* //;/^[ \t]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \\\n-\tif test -f ../GIT-BUILD-OPTIONS; then \\\n-\t\t. ../GIT-BUILD-OPTIONS; \\\n-\tfi && \\\n-\tif test -x ../git$$X; then \\\n-\t\tDIFFW=\"../git$$X --no-pager diff -w --no-index\"; \\\n-\telse \\\n-\t\tDIFFW=\"diff -w -u\"; \\\n-\tfi && \\\n-\t$$DIFFW '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual\n+\t\tsed -e 's/^[1-9][0-9]* //;s/[ \t][ \t]*/ /g;/^ *$$/d;s/^ //;s/ $$//;s/\\([<>|();&]\\) /\\1/g;s/ \\([<>|();&]\\)/\\1/g' >'$(CHAINLINTTMP_SQ)'/actual && \\\n+\tdiff -u '$(CHAINLINTTMP_SQ)'/expect '$(CHAINLINTTMP_SQ)'/actual\n \n test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \\\n \ttest-lint-filenames\n-- \n2.43.0\n\n"},{"id":"485631","messageId":"ZXq3YdK2RSKF3npE@tanuki","threadId":"60614","inReplyTo":"20231214032248.1615-1-ericsunshine@charter.net","subject":"Re: [PATCH] tests: drop dependency on `git diff` in check-chainlint","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-14T08:05:53Z","receivedAt":"2023-12-14T08:05:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 13, 2023 at 10:22:48PM -0500, Eric Sunshine wrote:\n> From: Eric Sunshine \n> \n> The \"check-chainlint\" target runs automatically when running tests and\n> performs self-checks to verify that the chainlinter itself produces the\n> expected output. Originally, the chainlinter was implemented via sed,\n> but the infrastructure has been rewritten in fb41727b7e (t: retire\n> unused chainlint.sed, 2022-09-01) to use a Perl script instead.\n> \n> The rewrite caused some slight whitespace changes in the output that are\n> ultimately not of much importance. In order to be able to assert that\n> the actual chainlinter errors match our expectations we thus have to\n> ignore whitespace characters when diffing them. As the `-w` flag is not\n> in POSIX we try to use `git diff -w --no-index` before we fall back to\n> non-standard `diff -w -u`.\n> \n> To accommodate for cases where the host system has no Git installation\n> we use the locally-compiled version of Git. This can result in problems\n> though when the Git project's repository is using extensions that the\n> locally-compiled version of Git doesn't understand, in which case `git`\n> may refuse to run and thus cause the checks to fail.\n> \n> Work around this issue by normalizing whitespace via sed before invoking\n> diff, which allows any platform diff implementation to be used, thus\n> eliminating the dependency upon `git diff` and the non-POSIX `-w` flag.\n> \n> Reported-by: Patrick Steinhardt \n> Signed-off-by: Eric Sunshine \n> ---\n> \n> This is an alternative solution to the issue Patrick's patch[1]\n> addresses. Hopefully, this approach should avoid the sort of push-back\n> Patrick's patch received[2].\n\nThanks for chiming in!\n\n> I shamelessly stole most of Patrick's commit message.\n> \n> The sed expressions for normalizing whitespace prior to `diff` may look\n> a bit hairy, but they are simple enough in concept:\n> \n> * collapse runs of whitespace to a single SP\n> * drop blank lines (this step is not new)\n> * fold out possible SP at beginning and end of each line\n> * fold out SP surrounding common punctuation characters used in shell\n> scripts, such as `>`, `|`, `;`, etc.\n> \n> By the way, I'm somewhat surprised that this issue crops up at all\n> considering that --no-index is being used with git-diff. As such, I\n> would have thought that the local repository's format would not have\n> been interrogated at all. If that's a bug in `git diff --no-index`, then\n> fixing that could be considered yet another alternative solution to the\n> issue raised here.\n\nThis strongly reminds me of the thread at [1], where a similar issue was\ndiscussed for git-grep(1). Quoting Junio: \n\n> I actually do not think these \"we are allowing Git tools to be used\n> on random garbage\" is a good idea to begin with X-<. If we invented\n> something nice for our variant in \"git grep\" and wish we can use it\n> outside the repository, contributing the feature to implementations\n> of \"grep\" would have been the right way to move forward, instead of\n> contaminating the codebase with things that are not related to Git.\n\nSo this might not be the best way to go.\n\n> [1]: https://lore.kernel.org/git/4112adbe467c14a8f22a87ea41aa4705f8760cf6.1702380646.git.ps@pks.im/\n> [2]: https://lore.kernel.org/git/xmqqr0jqnnmn.fsf@gitster.g/\n> \n> t/Makefile | 14 +++-----------\n> 1 file changed, 3 insertions(+), 11 deletions(-)\n> \n> diff --git a/t/Makefile b/t/Makefile\n> index 225aaf78ed..656ff10afa 100644\n> --- a/t/Makefile\n> +++ b/t/Makefile\n> @@ -103,20 +103,12 @@ check-chainlint:\n> \t\techo \"# chainlint: $(CHAINLINTTMP_SQ)/tests\" && \\\n> \t\tfor i in $(CHAINLINTTESTS); do \\\n> \t\t\techo \"# chainlint: $$i\" && \\\n> -\t\t\tsed -e '/^[ \t]*$$/d' chainlint/$$i.expect; \\\n> +\t\t\tsed -e 's/[ \t][ \t]*/ /g;/^ *$$/d;s/^ //;s/ $$//;s/\\([<>|();&]\\) /\\1/g;s/ \\([<>|();&]\\)/\\1/g' chainlint/$$i.expect; \\\n\nThese sed expressions do look hairy indeed. I have to wonder: all that\nwe're doing here is to munge the expected files we already have in our\ntree. Can't we fix those to look exactly like the actual results instead\nand then avoid any kind of post processing altogether? If I understand\ncorrectly the only reason we do this post processing is because the\noriginal implementation of the chainlinter produced slightly different\nwhitespace.\n\nPatrick\n\n[1]: https://lore.kernel.org/git/xmqq7cnnpy3z.fsf@gitster.g/\n"},{"id":"485634","messageId":"CAPig+cSkuRfkR2D3JqYcbaJqj485nfD9Nq6pM=vXWB5DJenWpA@mail.gmail.com","threadId":"60614","inReplyTo":"ZXq3YdK2RSKF3npE@tanuki","subject":"Re: [PATCH] tests: drop dependency on `git diff` in check-chainlint","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-12-14T08:31:54Z","receivedAt":"2023-12-14T08:32:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 14, 2023 at 3:05 AM Patrick Steinhardt wrote:\n> On Wed, Dec 13, 2023 at 10:22:48PM -0500, Eric Sunshine wrote:\n> > This is an alternative solution to the issue Patrick's patch[1]\n> > addresses. Hopefully, this approach should avoid the sort of push-back\n> > Patrick's patch received[2].\n> >\n> > By the way, I'm somewhat surprised that this issue crops up at all\n> > considering that --no-index is being used with git-diff. As such, I\n> > would have thought that the local repository's format would not have\n> > been interrogated at all. If that's a bug in `git diff --no-index`, then\n> > fixing that could be considered yet another alternative solution to the\n> > issue raised here.\n>\n> This strongly reminds me of the thread at [1], where a similar issue was\n> discussed for git-grep(1). Quoting Junio:\n>\n> > I actually do not think these \"we are allowing Git tools to be used\n> > on random garbage\" is a good idea to begin with X-<. If we invented\n> > something nice for our variant in \"git grep\" and wish we can use it\n> > outside the repository, contributing the feature to implementations\n> > of \"grep\" would have been the right way to move forward, instead of\n> > contaminating the codebase with things that are not related to Git.\n>\n> So this might not be the best way to go.\n\nI recall Junio mentioning that, and I'm fine with the conclusion that\n\"fixing\" --no-index is counter to the project's goals.\n\n> > - sed -e '/^[ ]*$$/d' chainlint/$$i.expect; \\\n> > + sed -e 's/[ ][ ]*/ /g;/^ *$$/d;s/^ //;s/ $$//;s/\\([<>|();&]\\) /\\1/g;s/ \\([<>|();&]\\)/\\1/g' chainlint/$$i.expect; \\\n> >\n> > The sed expressions for normalizing whitespace prior to `diff` may look\n> > a bit hairy, but they are simple enough in concept:\n> >\n> > * collapse runs of whitespace to a single SP\n> > * drop blank lines (this step is not new)\n> > * fold out possible SP at beginning and end of each line\n> > * fold out SP surrounding common punctuation characters used in shell\n> > scripts, such as `>`, `|`, `;`, etc.\n>\n> These sed expressions do look hairy indeed. I have to wonder: all that\n> we're doing here is to munge the expected files we already have in our\n> tree. Can't we fix those to look exactly like the actual results instead\n> and then avoid any kind of post processing altogether? If I understand\n> correctly the only reason we do this post processing is because the\n> original implementation of the chainlinter produced slightly different\n> whitespace.\n\nYes and no. It's not just whitespace.\n\nI did strongly consider submitting patches to fix all the whitespace\ndifferences in the \"expect\" files when chainlint.pl replaced\nchainlint.sed, but I particularly didn't want to plague the mailing\nlist with such noise. It's really just unnecessary churn since it's so\neasy to work around it with minor sed magic.\n\nAnd time tells me that that was probably the correct decision since\nthe output of chainlint.pl has changed multiple times. Even the output\nof chainlint.sed wasn't necessarily stable[1]. Then, of course\nchainlint.pl replaced[2] chainlint.sed. The original implementation or\nchainlint.pl just dumped out the parsed token stream, but [3] improved\nit to preserve the original formatting of the test snippet, and [4]\nannotated the output with line numbers of the original test snippet.\nHad those changes been accompanied by extra patches to \"fix\" the\n\"expect\" files to suit, it would have been just that much more noise\nboth in terms of patches to review and in terms of churn in the actual\nhistory.\n\nAnd, who knows, the output of chainlint.pl might change/improve again\nsome day. So, I still favor using sed to smooth over these minor\ndifferences rather than \"fixing\" the \"expect\" file repeatedly to\nadjust them for changes which are not significant to what is actually\nbeing tested.\n\n[1]: d73f5cfa89 (chainlint.sed: stop splitting \"(...\" into separate\nlines \"(\" and \"...\", 2021-12-13)\n[2]: d00113ec34 (t/Makefile: apply chainlint.pl to existing\nself-tests, 2022-09-01)\n[3]: 73c768dae9 (chainlint: annotate original test definition rather\nthan token stream, 2022-11-08)\n[4]: 48d69d8f2f (chainlint: prefix annotated test definition with line\nnumbers, 2022-11-11)\n"},{"id":"485659","messageId":"xmqqo7esohjm.fsf@gitster.g","threadId":"60614","inReplyTo":"ZXq3YdK2RSKF3npE@tanuki","subject":"Re: [PATCH] tests: drop dependency on `git diff` in check-chainlint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-14T16:49:49Z","receivedAt":"2023-12-14T16:49:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> This strongly reminds me of the thread at [1], where a similar issue was\n> discussed for git-grep(1). Quoting Junio: \n>\n>> I actually do not think these \"we are allowing Git tools to be used\n>> on random garbage\" is a good idea to begin with X-<. If we invented\n>> something nice for our variant in \"git grep\" and wish we can use it\n>> outside the repository, contributing the feature to implementations\n>> of \"grep\" would have been the right way to move forward, instead of\n>> contaminating the codebase with things that are not related to Git.\n>\n> So this might not be the best way to go.\n\nThat is not a conclusion I want people to draw.\n\nLike it or not, \"git diff --no-index\" will be with us to stay, and\n\"--no-index\" being \"we have abused the rest of Git code to implement\n'diff' that works _outside_ a Git repository---now go and do your\nthing\", we would eventually want to correct it, if it is misbehaving\nwhen a repository it finds is in a shape it does not like, no?\n\nWe should have what you quoted in mind as a general principle, and\nthink twice when we are tempted to hoard useful features for another\ntool we initially wrote for Git and allow them to be used with the\n\"--no-index\" option, instead of contributing them to the tool that\ndoes not know or care \"git\" repositories (like \"diff\" and \"grep\").\n"},{"id":"485704","messageId":"ZXvlxqsCV9GJ7580@tanuki","threadId":"60614","inReplyTo":"xmqqo7esohjm.fsf@gitster.g","subject":"Re: [PATCH] tests: drop dependency on `git diff` in check-chainlint","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-15T05:36:06Z","receivedAt":"2023-12-15T05:36:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 14, 2023 at 08:49:49AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> > This strongly reminds me of the thread at [1], where a similar issue was\n> > discussed for git-grep(1). Quoting Junio: \n> >\n> >> I actually do not think these \"we are allowing Git tools to be used\n> >> on random garbage\" is a good idea to begin with X-<. If we invented\n> >> something nice for our variant in \"git grep\" and wish we can use it\n> >> outside the repository, contributing the feature to implementations\n> >> of \"grep\" would have been the right way to move forward, instead of\n> >> contaminating the codebase with things that are not related to Git.\n> >\n> > So this might not be the best way to go.\n> \n> That is not a conclusion I want people to draw.\n> \n> Like it or not, \"git diff --no-index\" will be with us to stay, and\n> \"--no-index\" being \"we have abused the rest of Git code to implement\n> 'diff' that works _outside_ a Git repository---now go and do your\n> thing\", we would eventually want to correct it, if it is misbehaving\n> when a repository it finds is in a shape it does not like, no?\n> \n> We should have what you quoted in mind as a general principle, and\n> think twice when we are tempted to hoard useful features for another\n> tool we initially wrote for Git and allow them to be used with the\n> \"--no-index\" option, instead of contributing them to the tool that\n> does not know or care \"git\" repositories (like \"diff\" and \"grep\").\n\nOkay, thanks for clarifying!\n\nPatrick\n"}]}