{"thread":{"id":"65840","subject":"[PATCH] t4216: fix no-op test that breaks TAP output","startedAt":"2026-06-19T07:20:31Z","lastAt":"2026-06-25T20:17:11Z","messageCount":6,"participants":["Patrick Steinhardt","Taylor Blau","Junio C Hamano","Todd Zullinger"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"545920","messageId":"20260619-pks-t4216-drop-unused-prereq-v1-1-2ce0d7bea088@pks.im","threadId":"65840","inReplyTo":null,"subject":"[PATCH] t4216: fix no-op test that breaks TAP output","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-19T07:20:20Z","receivedAt":"2026-06-19T07:20:31Z","isPatch":true,"body":"In t4216 we have have a prerequisite that is active in case the system's\n`char` type is signed by default. This prerequisite isn't really used by\nanything though: while it is used to guard one of our tests, that\nspecific test is essentially a no-op. So all this infrastructure does is\nto provide some debugging hint to a reader that pays a lot of attention.\n\nBesides that, the way we set up the prerequisite also results in broken\nTAP output on systems where `char` is unsigned by default: we use\n`test_cmp()` to diff two files outside of of any test body, and if the\nfiles differ we enable the prerequisite. If so, the call to `test_cmp()`\nwould also print output, and that output is of course not valid TAP\noutput.\n\nThat wasn't a problem before 389c83025d (t: let prove fail when parsing\ninvalid TAP output, 2026-06-04), because our TAP parser was configured\nto be lenient. But starting with that commit, t4216 is now failing on\nsystems with unsigned chars.\n\nDrop the whole infrastructure. The prerequisite is not used anywhere\nelse, and the only location where it's used doesn't really provide much\nvalue.\n\nReported-by: Todd Zullinger \nSigned-off-by: Patrick Steinhardt \n---\nHi,\n\nas reported in [1]. Thanks!\n\nPatrick\n\n<20260617220330.n6byiFQr@teonanacatl.net>\n---\n t/t4216-log-bloom.sh | 21 ---------------------\n 1 file changed, 21 deletions(-)\n\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex 1064990de3..16bc39c359 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -569,27 +569,6 @@ test_expect_success 'set up repo with high bit path, version 1 changed-path' '\n \tgit -C highbit1 commit-graph write --reachable --changed-paths\n '\n \n-test_expect_success 'setup check value of version 1 changed-path' '\n-\t(\n-\t\tcd highbit1 &&\n-\t\techo \"52a9\" >expect &&\n-\t\tget_first_changed_path_filter >actual\n-\t)\n-'\n-\n-# expect will not match actual if char is unsigned by default. Write the test\n-# in this way, so that a user running this test script can still see if the two\n-# files match. (It will appear as an ordinary success if they match, and a skip\n-# if not.)\n-if test_cmp highbit1/expect highbit1/actual\n-then\n-\ttest_set_prereq SIGNED_CHAR_BY_DEFAULT\n-fi\n-test_expect_success SIGNED_CHAR_BY_DEFAULT 'check value of version 1 changed-path' '\n-\t# Only the prereq matters for this test.\n-\ttrue\n-'\n-\n test_expect_success 'setup make another commit' '\n \t# \"git log\" does not use Bloom filters for root commits - see how, in\n \t# revision.c, rev_compare_tree() (the only code path that eventually calls\n\n---\nbase-commit: 95e20213faefeb95df29277c58ac1980ab68f701\nchange-id: 20260619-pks-t4216-drop-unused-prereq-5793107a0249\n\n"},{"id":"545953","messageId":"ajVMZpjTKiXc7TRe@nand.local","threadId":"65840","inReplyTo":"20260619-pks-t4216-drop-unused-prereq-v1-1-2ce0d7bea088@pks.im","subject":"Re: [PATCH] t4216: fix no-op test that breaks TAP output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-06-19T14:04:22Z","receivedAt":"2026-06-19T14:04:26Z","isPatch":true,"body":"Hi Patrick,\n\nA couple of thanks are owed: one to Todd for reporting this issue in the\nfirst place, another to Peff for analyizing why it didn't appear broken\nbefore, and a third for you for proposing a patch to fix it.\n\nIf you choose to delete this piece of test infrastructure entirely (I\nthink that there is an alternative direction that I would prefer, but\nsee below for more on why), I think the patch you wrote below is OK.\n\nBut...\n\nOn Fri, Jun 19, 2026 at 09:20:20AM +0200, Patrick Steinhardt wrote:\n> In t4216 we have have a prerequisite that is active in case the system's\n> `char` type is signed by default. This prerequisite isn't really used by\n> anything though: while it is used to guard one of our tests, that\n> specific test is essentially a no-op. So all this infrastructure does is\n> to provide some debugging hint to a reader that pays a lot of attention.\n\nI don't think that this is guarding nothing, but I agree that the test\nas written is strange. As I recall, this was to sanity check the v1\nBloom values, but allow failures on platforms where the `char` type is\nunsigned by default.\n\nI don't feel that strongly about whether or not we check the exact\nvalue of the filter, but I think there are a couple of arguments in\nfavor of doing so. Most compelling would be that we know that our\nmurmur3 implementation is correct (in at least one case) and that we\ndon't regress that case in the future. We do have these checks for v2\nchanged-path Bloom filters where the signed-ness of `char` is\nirrelevant.\n\n> Besides that, the way we set up the prerequisite also results in broken\n> TAP output on systems where `char` is unsigned by default: we use\n> `test_cmp()` to diff two files outside of of any test body, and if the\n> files differ we enable the prerequisite. If so, the call to `test_cmp()`\n> would also print output, and that output is of course not valid TAP\n> output.\n\nGiven this and the above, I would probably err on the side of\ndesignating this as 'test_lazy_prereq' or otherwise silencing the output\nof 'test_cmp' so that this does not taint the TAP output.\n\nThanks,\nTaylor\n"},{"id":"545995","messageId":"xmqqa4sqlchz.fsf@gitster.g","threadId":"65840","inReplyTo":"ajVMZpjTKiXc7TRe@nand.local","subject":"Re: [PATCH] t4216: fix no-op test that breaks TAP output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-19T16:29:44Z","receivedAt":"2026-06-19T16:29:47Z","isPatch":true,"body":"Taylor Blau writes:\n\n> Given this and the above, I would probably err on the side of\n> designating this as 'test_lazy_prereq' or otherwise silencing the output\n> of 'test_cmp' so that this does not taint the TAP output.\n\nWe can argue the merit and demerit with a good log message. The\ncentral issue at hand is how precious 52a9 in the script lost by\nthis patch is (in other words, are we checking more than \"is our\nchar signed or unsigned?\").\n\nBy the way, I do not quite get the _BY_DEFAULT in the name\nSIGNED_CHAR_BY_DEFAULT. The builder may have configured to use\nsigned char on a platform that can handle both and their char is by\ndefault unsigned, and under such a condition, we would set this\nprerequisite, even though the default on such a platform is\nunsigned, no?\n"},{"id":"546104","messageId":"ajjBmi39IFJW5p5V@pks.im","threadId":"65840","inReplyTo":"xmqqa4sqlchz.fsf@gitster.g","subject":"Re: [PATCH] t4216: fix no-op test that breaks TAP output","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-22T05:01:14Z","receivedAt":"2026-06-22T05:01:21Z","isPatch":true,"body":"On Fri, Jun 19, 2026 at 09:29:44AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n> \n> > Given this and the above, I would probably err on the side of\n> > designating this as 'test_lazy_prereq' or otherwise silencing the output\n> > of 'test_cmp' so that this does not taint the TAP output.\n> \n> We can argue the merit and demerit with a good log message. The\n> central issue at hand is how precious 52a9 in the script lost by\n> this patch is (in other words, are we checking more than \"is our\n> char signed or unsigned?\").\n\nUltimately, I don't mind much which way we go. But if we want to retain\nthis, would you mind sending a rewritten v2, Taylor? I feel like you're\nin a better position to argue why we should retain it.\n\nThanks!\n\nPatrick\n"},{"id":"546426","messageId":"20260625185112.jjH0K9LI@teonanacatl.net","threadId":"65840","inReplyTo":"ajjBmi39IFJW5p5V@pks.im","subject":"Re: [PATCH] t4216: fix no-op test that breaks TAP output","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2026-06-25T18:51:12Z","receivedAt":"2026-06-25T18:51:16Z","isPatch":true,"body":"Patrick Steinhardt wrote:\n> On Fri, Jun 19, 2026 at 09:29:44AM -0700, Junio C Hamano wrote:\n>> Taylor Blau writes:\n>> \n>>> Given this and the above, I would probably err on the side of\n>>> designating this as 'test_lazy_prereq' or otherwise silencing the output\n>>> of 'test_cmp' so that this does not taint the TAP output.\n>> \n>> We can argue the merit and demerit with a good log message. The\n>> central issue at hand is how precious 52a9 in the script lost by\n>> this patch is (in other words, are we checking more than \"is our\n>> char signed or unsigned?\").\n> \n> Ultimately, I don't mind much which way we go. But if we want to retain\n> this, would you mind sending a rewritten v2, Taylor? I feel like you're\n> in a better position to argue why we should retain it.\n\nIs this something which can be merged before 2.55.0 final?\nIt's certainly not a grave issue, but it is a new test\nfailure for anyone who diligently runs the test suite on\nmany (most?) non-x86 architectures. It seems a shame to\npunish those folks. :)\n\nFWIW, Tested-by: Todd Zullinger \n\nI tested the earlier test_lazy_prereq version as well.\n\n-- \nTodd\n"},{"id":"546430","messageId":"xmqq8q82fk8r.fsf@gitster.g","threadId":"65840","inReplyTo":"20260625185112.jjH0K9LI@teonanacatl.net","subject":"Re: [PATCH] t4216: fix no-op test that breaks TAP output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-25T20:17:08Z","receivedAt":"2026-06-25T20:17:11Z","isPatch":true,"body":"Todd Zullinger writes:\n\n> Patrick Steinhardt wrote:\n>> On Fri, Jun 19, 2026 at 09:29:44AM -0700, Junio C Hamano wrote:\n>>> Taylor Blau writes:\n>>> \n>>>> Given this and the above, I would probably err on the side of\n>>>> designating this as 'test_lazy_prereq' or otherwise silencing the output\n>>>> of 'test_cmp' so that this does not taint the TAP output.\n>>> \n>>> We can argue the merit and demerit with a good log message. The\n>>> central issue at hand is how precious 52a9 in the script lost by\n>>> this patch is (in other words, are we checking more than \"is our\n>>> char signed or unsigned?\").\n>> \n>> Ultimately, I don't mind much which way we go. But if we want to retain\n>> this, would you mind sending a rewritten v2, Taylor? I feel like you're\n>> in a better position to argue why we should retain it.\n>\n> Is this something which can be merged before 2.55.0 final?\n> It's certainly not a grave issue, but it is a new test\n> failure for anyone who diligently runs the test suite on\n> many (most?) non-x86 architectures. It seems a shame to\n> punish those folks. :)\n>\n> FWIW, Tested-by: Todd Zullinger \n>\n> I tested the earlier test_lazy_prereq version as well.\n\nGood that you pinged, as I forgot that we haven't seen Taylor for\nsome time. Let's merge down Patrick's one you have already tested\nwhile leaving it as an option to resurrect the \"52a9 is still\nprecious\" tweak from Taylor perhaps after the release.\n\nThanks.\n"}]}