{"thread":{"id":"50691","subject":"One failed self test on Fedora 29","startedAt":"2019-03-08T10:49:19Z","lastAt":"2019-03-13T16:00:27Z","messageCount":25,"participants":["Jeffrey Walton","Todd Zullinger","Junio C Hamano","Jeff King","Duy Nguyen","Thomas Braun","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"370984","messageId":"CAH8yC8k_Zyi89uxTWTrjN65UAAc1L+jLho+P7O7UyvE-LvZuzA@mail.gmail.com","threadId":"50691","inReplyTo":null,"subject":"One failed self test on Fedora 29","fromName":"Jeffrey Walton","fromEmail":"noloader@gmail.com","sentAt":"2019-03-08T10:48:55Z","receivedAt":"2019-03-08T10:49:19Z","isPatch":false,"sender":{"key":"noloader@gmail.com","avatar":null},"body":"Fedora 29, x86_64. One failed self test:\n\n*** t0021-conversion.sh ***\nok 1 - setup\nok 2 - check\nok 3 - expanded_in_repo\nok 4 - filter shell-escaped filenames\nok 5 - required filter should filter data\nok 6 - required filter smudge failure\nok 7 - required filter clean failure\nok 8 - filtering large input to small output should use little memory\nok 9 - filter that does not read is fine\nok 10 # skip filter large file (missing EXPENSIVE)\nok 11 - filter: clean empty file\nok 12 - filter: smudge empty file\nnot ok 13 - disable filter with empty override\n#\n#               test_config_global filter.disable.smudge false &&\n#               test_config_global filter.disable.clean false &&\n#               test_config filter.disable.smudge false &&\n#               test_config filter.disable.clean false &&\n#\n#               echo \"*.disable filter=disable\" >.gitattributes &&\n#\n#               echo test >test.disable &&\n#               git -c filter.disable.clean= add test.disable 2>err &&\n#               test_must_be_empty err &&\n#               rm -f test.disable &&\n#               git -c filter.disable.smudge= checkout -- test.disable 2>err &&\n#               test_must_be_empty err\n#\nok 14 - diff does not reuse worktree files that need cleaning\nok 15 - required process filter should filter data\nok 16 - required process filter takes precedence\nok 17 - required process filter should be used only for \"clean\" operation only\nok 18 - required process filter should process multiple packets\nok 19 - required process filter with clean error should fail\nok 20 - process filter should restart after unexpected write failure\nok 21 - process filter should not be restarted if it signals an error\nok 22 - process filter abort stops processing of all further files\nok 23 - invalid process filter must fail (and not hang!)\nok 24 - delayed checkout in process filter\nok 25 - missing file in delayed checkout\nok 26 - invalid file in delayed checkout\n# failed 1 among 26 test(s)\n1..26\ngmake[2]: *** [Makefile:56: t0021-conversion.sh] Error 1\n\nDoes anyone need a config.log or other test data?\n"},{"id":"371002","messageId":"20190308174343.GX31362@zaya.teonanacatl.net","threadId":"50691","inReplyTo":"CAH8yC8k_Zyi89uxTWTrjN65UAAc1L+jLho+P7O7UyvE-LvZuzA@mail.gmail.com","subject":"Re: One failed self test on Fedora 29","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-03-08T17:43:43Z","receivedAt":"2019-03-08T17:43:54Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nJeffrey Walton wrote:\n> Fedora 29, x86_64. One failed self test:\n> \n> *** t0021-conversion.sh ***\n[...]\n> not ok 13 - disable filter with empty override\n> #\n> #               test_config_global filter.disable.smudge false &&\n> #               test_config_global filter.disable.clean false &&\n> #               test_config filter.disable.smudge false &&\n> #               test_config filter.disable.clean false &&\n> #\n> #               echo \"*.disable filter=disable\" >.gitattributes &&\n> #\n> #               echo test >test.disable &&\n> #               git -c filter.disable.clean= add test.disable 2>err &&\n> #               test_must_be_empty err &&\n> #               rm -f test.disable &&\n> #               git -c filter.disable.smudge= checkout -- test.disable 2>err &&\n> #               test_must_be_empty err\n> #\n[...]\n> # failed 1 among 26 test(s)\n> 1..26\n> gmake[2]: *** [Makefile:56: t0021-conversion.sh] Error 1\n> \n> Does anyone need a config.log or other test data?\n\nIt would probably help to know what commit you're building.\nThe verbose test output would also be useful, e.g.:\n\n    cd t && ./t0021-conversion.sh -v -i\n\nIf it's not reliably reproducible, the --stress* options\nmight help catch a failing run.\n\nFWIW, I just built and ran the tests on a Fedora 29\ncontainer for master, next, and pu a few times (some with\nvarious --stress options) without any test failures.\n\nI did this with and without a config.mak from the fedora git\npackages.  I've never used the configure script, it seems\nlike unnecessary overhead.\n\n    $ git branch -v\n      master 6e0cc67761 Start 2.22 cycle\n      next   541d9dca55 Merge branch 'yb/utf-16le-bom-spellfix' into next\n    * pu     7eadd8ba98 Merge branch 'js/remote-curl-i18n' into pu\n\n-- \nTodd\n"},{"id":"371026","messageId":"CAH8yC8mg3vjPoof5SDemQ_YiL+7e1ak535U2nFnPbaWJ8xSWOA@mail.gmail.com","threadId":"50691","inReplyTo":"20190308174343.GX31362@zaya.teonanacatl.net","subject":"Re: One failed self test on Fedora 29","fromName":"Jeffrey Walton","fromEmail":"noloader@gmail.com","sentAt":"2019-03-09T12:34:15Z","receivedAt":"2019-03-09T12:34:42Z","isPatch":false,"sender":{"key":"noloader@gmail.com","avatar":null},"body":"On Fri, Mar 8, 2019 at 12:43 PM Todd Zullinger <tmz@pobox.com> wrote:\n>\n> Jeffrey Walton wrote:\n> > Fedora 29, x86_64. One failed self test:\n> >\n> > *** t0021-conversion.sh ***\n> [...]\n> > not ok 13 - disable filter with empty override\n> > #\n> > #               test_config_global filter.disable.smudge false &&\n> > #               test_config_global filter.disable.clean false &&\n> > #               test_config filter.disable.smudge false &&\n> > #               test_config filter.disable.clean false &&\n> > #\n> > #               echo \"*.disable filter=disable\" >.gitattributes &&\n> > #\n> > #               echo test >test.disable &&\n> > #               git -c filter.disable.clean= add test.disable 2>err &&\n> > #               test_must_be_empty err &&\n> > #               rm -f test.disable &&\n> > #               git -c filter.disable.smudge= checkout -- test.disable 2>err &&\n> > #               test_must_be_empty err\n> > #\n> [...]\n> > # failed 1 among 26 test(s)\n> > 1..26\n> > gmake[2]: *** [Makefile:56: t0021-conversion.sh] Error 1\n> >\n> > Does anyone need a config.log or other test data?\n>\n> It would probably help to know what commit you're building.\n> The verbose test output would also be useful, e.g.:\n\nI built with CFLAGS += -fsanitize=undefined. It looks like the\nmisaligned accesses generate UBsan findings, which is causing\nt0021-conversion to fail.\n\ngit-2.21.0$ grep -IR 'runtime error'\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:392:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:397:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:402:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:407:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:412:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:417:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:422:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:427:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:432:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:437:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:442:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:447:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:452:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:457:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:462:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/err:sha1dc/sha1.c:467:2: runtime\nerror: load of misaligned address 0x0000024fc245 for type 'const\nuint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:392:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:397:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:402:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:407:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:412:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:417:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:422:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:427:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:432:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:437:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:442:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:447:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:452:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:457:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:462:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\nt/trash directory.t0021-conversion/git-stderr.log:sha1dc/sha1.c:467:2:\nruntime error: load of misaligned address 0x000001a39cf5 for type\n'const uint32_t', which requires 4 byte alignment\n"},{"id":"371029","messageId":"CAH8yC8kn=EmEm_UPrnpwaofv97S42Se6FC+hWcm0EHCX-4rewQ@mail.gmail.com","threadId":"50691","inReplyTo":"CAH8yC8mg3vjPoof5SDemQ_YiL+7e1ak535U2nFnPbaWJ8xSWOA@mail.gmail.com","subject":"Re: One failed self test on Fedora 29","fromName":"Jeffrey Walton","fromEmail":"noloader@gmail.com","sentAt":"2019-03-09T13:12:05Z","receivedAt":"2019-03-09T13:12:37Z","isPatch":false,"sender":{"key":"noloader@gmail.com","avatar":null},"body":"On Sat, Mar 9, 2019 at 7:34 AM Jeffrey Walton <noloader@gmail.com> wrote:\n>\n> On Fri, Mar 8, 2019 at 12:43 PM Todd Zullinger <tmz@pobox.com> wrote:\n> >\n> > Jeffrey Walton wrote:\n> > > Fedora 29, x86_64. One failed self test:\n> > >\n> > > *** t0021-conversion.sh ***\n> > [...]\n> > > not ok 13 - disable filter with empty override\n> > > #\n> > > #               test_config_global filter.disable.smudge false &&\n> > > #               test_config_global filter.disable.clean false &&\n> > > #               test_config filter.disable.smudge false &&\n> > > #               test_config filter.disable.clean false &&\n> > > #\n> > > #               echo \"*.disable filter=disable\" >.gitattributes &&\n> > > #\n> > > #               echo test >test.disable &&\n> > > #               git -c filter.disable.clean= add test.disable 2>err &&\n> > > #               test_must_be_empty err &&\n> > > #               rm -f test.disable &&\n> > > #               git -c filter.disable.smudge= checkout -- test.disable 2>err &&\n> > > #               test_must_be_empty err\n> > > #\n> > [...]\n> > > # failed 1 among 26 test(s)\n> > > 1..26\n> > > gmake[2]: *** [Makefile:56: t0021-conversion.sh] Error 1\n> > >\n> > > Does anyone need a config.log or other test data?\n> >\n> > It would probably help to know what commit you're building.\n> > The verbose test output would also be useful, e.g.:\n>\n> I built with CFLAGS += -fsanitize=undefined. It looks like the\n> misaligned accesses generate UBsan findings, which is causing\n> t0021-conversion to fail.\n>\n> git-2.21.0$ grep -IR 'runtime error'\n> t/trash directory.t0021-conversion/err:sha1dc/sha1.c:392:2: runtime\n> error: load of misaligned address 0x0000024fc245 for type 'const\n> uint32_t', which requires 4 byte alignment\n> t/trash directory.t0021-conversion/err:sha1dc/sha1.c:397:2: runtime\n> error: load of misaligned address 0x0000024fc245 for type 'const\n> uint32_t', which requires 4 byte alignment\n> t/trash directory.t0021-conversion/err:sha1dc/sha1.c:402:2: runtime\n> error: load of misaligned address 0x0000024fc245 for type 'const\n> uint32_t', which requires 4 byte alignment\n\nI think this is the patch for sha1dc/sha1.c . It stops using unaligned\naccesses by default, but still honors SHA1DC_FORCE_UNALIGNED_ACCESS\nfor those who want it. Folks who want the undefined behavior have to\ndo something special.\n\nJeff\n\n\n--- sha1dc/sha1.c\n+++ sha1dc/sha1.c\n@@ -26,14 +26,6 @@\n #include \"sha1.h\"\n #include \"ubc_check.h\"\n \n-#if (defined(__amd64__) || defined(__amd64) || defined(__x86_64__) || defined(__x86_64) || \\\n-     defined(i386) || defined(__i386) || defined(__i386__) || defined(__i486__)  || \\\n-     defined(__i586__) || defined(__i686__) || defined(_M_IX86) || defined(__X86__) || \\\n-     defined(_X86_) || defined(__THW_INTEL__) || defined(__I86__) || defined(__INTEL__) || \\\n-     defined(__386) || defined(_M_X64) || defined(_M_AMD64))\n-#define SHA1DC_ON_INTEL_LIKE_PROCESSOR\n-#endif\n-\n /*\n    Because Little-Endian architectures are most common,\n    we only set SHA1DC_BIGENDIAN if one of these conditions is met.\n@@ -124,7 +116,7 @@\n #endif\n /*ENDIANNESS SELECTION*/\n \n-#if defined(SHA1DC_FORCE_UNALIGNED_ACCESS) || defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)\n+#if defined(SHA1DC_FORCE_UNALIGNED_ACCESS)\n #define SHA1DC_ALLOW_UNALIGNED_ACCESS\n #endif /*UNALIGNMENT DETECTION*/\n"},{"id":"371097","messageId":"xmqq1s3emapy.fsf@gitster-ct.c.googlers.com","threadId":"50691","inReplyTo":"CAH8yC8kn=EmEm_UPrnpwaofv97S42Se6FC+hWcm0EHCX-4rewQ@mail.gmail.com","subject":"Re: One failed self test on Fedora 29","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-11T02:00:25Z","receivedAt":"2019-03-11T02:00:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeffrey Walton <noloader@gmail.com> writes:\n\n> I think this is the patch for sha1dc/sha1.c . It stops using unaligned\n> accesses by default, but still honors SHA1DC_FORCE_UNALIGNED_ACCESS\n> for those who want it. Folks who want the undefined behavior have to\n> do something special.\n\nHmph, I somehow thought that folks who want to stick to the\nstandard printed on paper penalizing what practicaly works well in\nthe real world would be the one doing extra things.\n"},{"id":"371098","messageId":"CAH8yC8krMuLkTnLsEAOxW67=aUhB2B9nGkAfsotuO95dd6+qLg@mail.gmail.com","threadId":"50691","inReplyTo":"xmqq1s3emapy.fsf@gitster-ct.c.googlers.com","subject":"Re: One failed self test on Fedora 29","fromName":"Jeffrey Walton","fromEmail":"noloader@gmail.com","sentAt":"2019-03-11T02:16:01Z","receivedAt":"2019-03-11T02:16:30Z","isPatch":false,"sender":{"key":"noloader@gmail.com","avatar":null},"body":"On Sun, Mar 10, 2019 at 10:00 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jeffrey Walton <noloader@gmail.com> writes:\n>\n> > I think this is the patch for sha1dc/sha1.c . It stops using unaligned\n> > accesses by default, but still honors SHA1DC_FORCE_UNALIGNED_ACCESS\n> > for those who want it. Folks who want the undefined behavior have to\n> > do something special.\n>\n> Hmph, I somehow thought that folks who want to stick to the\n> standard printed on paper penalizing what practicaly works well in\n> the real world would be the one doing extra things.\n\nIn the paper example, we do. I prefer printed books. Just last month I\nwent to Office Depot and had portions of an ARM manual distributed as\nPDF printed and spiral bound. I don't mind doing extra work as the\nspecial case.\n\nJeff\n"},{"id":"371104","messageId":"20190311032923.GA7087@sigill.intra.peff.net","threadId":"50691","inReplyTo":"CAH8yC8mg3vjPoof5SDemQ_YiL+7e1ak535U2nFnPbaWJ8xSWOA@mail.gmail.com","subject":"Re: One failed self test on Fedora 29","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-11T03:29:24Z","receivedAt":"2019-03-11T03:32:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 09, 2019 at 07:34:15AM -0500, Jeffrey Walton wrote:\n\n> > It would probably help to know what commit you're building.\n> > The verbose test output would also be useful, e.g.:\n> \n> I built with CFLAGS += -fsanitize=undefined. It looks like the\n> misaligned accesses generate UBsan findings, which is causing\n> t0021-conversion to fail.\n\nYou probably should use SANITIZE=undefined instead. The Makefile has\nsome smarts to tweak build parameters based on your sanitize flag (e.g.,\ndefining NO_UNALIGNED_LOADS).\n\nThat said, I do not think sha1dc works with UBsan at this point at all.\nI usually do error-checking builds with:\n\n  make SANITIZE=address,undefined BLK_SHA1=Yes\n\nWhat puzzles me is not that t0021 failed, but that everything else\n_didn't_. Almost every script fails for me when Git is built with UBSan\nand sha1dc.\n\nIt would be nice to make sha1dc respect NO_UNALIGNED_LOADS. But barring\nthat, we should probably default to BLK_SHA1 when we see\nSANITIZE=undefined.\n\n-Peff\n"},{"id":"371105","messageId":"20190311033755.GB7087@sigill.intra.peff.net","threadId":"50691","inReplyTo":"xmqq1s3emapy.fsf@gitster-ct.c.googlers.com","subject":"disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-11T03:37:55Z","receivedAt":"2019-03-11T03:37:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 11, 2019 at 11:00:25AM +0900, Junio C Hamano wrote:\n\n> Jeffrey Walton <noloader@gmail.com> writes:\n> \n> > I think this is the patch for sha1dc/sha1.c . It stops using unaligned\n> > accesses by default, but still honors SHA1DC_FORCE_UNALIGNED_ACCESS\n> > for those who want it. Folks who want the undefined behavior have to\n> > do something special.\n> \n> Hmph, I somehow thought that folks who want to stick to the\n> standard printed on paper penalizing what practicaly works well in\n> the real world would be the one doing extra things.\n\nUnfortunately, I don't think sha1dc currently supports #defines in that\ndirection. The only logic is \"if we are on intel, do unaligned loads\"\nand \"even if we are not on intel, do it anyway\". There is no \"even if we\nare on intel, do not do unaligned loads\".\n\nI think you'd need something like this:\n\ndiff --git a/Makefile b/Makefile\nindex 148668368b..705c54dcd8 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1194,6 +1194,7 @@ BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n BASIC_CFLAGS += -fno-omit-frame-pointer\n ifneq ($(filter undefined,$(SANITIZERS)),)\n BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n+BASIC_CFLAGS += -DSHA1DC_DISALLOW_UNALIGNED_ACCESS\n endif\n ifneq ($(filter leak,$(SANITIZERS)),)\n BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS\ndiff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\nindex df0630bc6d..0bdf80d778 100644\n--- a/sha1dc/sha1.c\n+++ b/sha1dc/sha1.c\n@@ -124,9 +124,11 @@\n #endif\n /*ENDIANNESS SELECTION*/\n \n+#ifndef SHA1DC_DISALLOW_UNALIGNED_ACCESS\n #if defined(SHA1DC_FORCE_UNALIGNED_ACCESS) || defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)\n #define SHA1DC_ALLOW_UNALIGNED_ACCESS\n #endif /*UNALIGNMENT DETECTION*/\n+#endif\n \n \n #define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n))))\n\nbut of course we cannot touch sha1dc/*, because we might actually be\nusing the submodule copy instead. And AFAIK there is no good way to\nmodify the submodule-provided content as part of the build. Why do we\neven have the submodule again? ;P\n\nI guess the same would be true for DC_SHA1_EXTERNAL, too, though.\n\nSo anyway, I think this needs a patch to the upstream sha1dc project.\n\n-Peff\n"},{"id":"371139","messageId":"CAH8yC8=9HOAmDE93aXd300kOJSOa0sA8Qv2cNn2M-EO-gqiktw@mail.gmail.com","threadId":"50691","inReplyTo":"20190311033755.GB7087@sigill.intra.peff.net","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Jeffrey Walton","fromEmail":"noloader@gmail.com","sentAt":"2019-03-11T10:40:21Z","receivedAt":"2019-03-11T10:40:52Z","isPatch":false,"sender":{"key":"noloader@gmail.com","avatar":null},"body":"On Sun, Mar 10, 2019 at 11:37 PM Jeff King <peff@peff.net> wrote:\n>\n> On Mon, Mar 11, 2019 at 11:00:25AM +0900, Junio C Hamano wrote:\n>\n> > Jeffrey Walton <noloader@gmail.com> writes:\n> >\n> > > I think this is the patch for sha1dc/sha1.c . It stops using unaligned\n> > > accesses by default, but still honors SHA1DC_FORCE_UNALIGNED_ACCESS\n> > > for those who want it. Folks who want the undefined behavior have to\n> > > do something special.\n> >\n> > Hmph, I somehow thought that folks who want to stick to the\n> > standard printed on paper penalizing what practicaly works well in\n> > the real world would be the one doing extra things.\n>\n> Unfortunately, I don't think sha1dc currently supports #defines in that\n> direction. The only logic is \"if we are on intel, do unaligned loads\"\n> and \"even if we are not on intel, do it anyway\". There is no \"even if we\n> are on intel, do not do unaligned loads\".\n>\n> I think you'd need something like this:\n>\n> diff --git a/Makefile b/Makefile\n> index 148668368b..705c54dcd8 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1194,6 +1194,7 @@ BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n>  BASIC_CFLAGS += -fno-omit-frame-pointer\n>  ifneq ($(filter undefined,$(SANITIZERS)),)\n>  BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n> +BASIC_CFLAGS += -DSHA1DC_DISALLOW_UNALIGNED_ACCESS\n>  endif\n>  ifneq ($(filter leak,$(SANITIZERS)),)\n>  BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS\n> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n> index df0630bc6d..0bdf80d778 100644\n> --- a/sha1dc/sha1.c\n> +++ b/sha1dc/sha1.c\n> @@ -124,9 +124,11 @@\n>  #endif\n>  /*ENDIANNESS SELECTION*/\n>\n> +#ifndef SHA1DC_DISALLOW_UNALIGNED_ACCESS\n>  #if defined(SHA1DC_FORCE_UNALIGNED_ACCESS) || defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)\n>  #define SHA1DC_ALLOW_UNALIGNED_ACCESS\n>  #endif /*UNALIGNMENT DETECTION*/\n> +#endif\n>\n>\n>  #define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n))))\n>\n> but of course we cannot touch sha1dc/*, because we might actually be\n> using the submodule copy instead. And AFAIK there is no good way to\n> modify the submodule-provided content as part of the build. Why do we\n> even have the submodule again? ;P\n>\n> I guess the same would be true for DC_SHA1_EXTERNAL, too, though.\n>\n> So anyway, I think this needs a patch to the upstream sha1dc project.\n\nhttps://github.com/cr-marcstevens/sha1collisiondetection/issues/47\n\nJeff\n"},{"id":"371144","messageId":"CACsJy8CdqbOKu7SHMt_Pz1EtRz08HGpwWHUHoZbUiow_pPh=+A@mail.gmail.com","threadId":"50691","inReplyTo":"20190311033755.GB7087@sigill.intra.peff.net","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-11T11:58:18Z","receivedAt":"2019-03-11T11:58:47Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 11, 2019 at 10:48 AM Jeff King <peff@peff.net> wrote:\n> And AFAIK there is no good way to\n> modify the submodule-provided content as part of the build. Why do we\n> even have the submodule again? ;P\n\nBecause of dogfooding of course. This is an interesting use case\nthough. I wonder if people often want to \"patch\" submodules like this\n(and what we could do if that's the case)\n-- \nDuy\n"},{"id":"371173","messageId":"8cf2fa6c-d742-a2a6-cde7-66cef87b04e8@virtuell-zuhause.de","threadId":"50691","inReplyTo":"CACsJy8CdqbOKu7SHMt_Pz1EtRz08HGpwWHUHoZbUiow_pPh=+A@mail.gmail.com","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2019-03-11T18:15:12Z","receivedAt":"2019-03-11T18:15:30Z","isPatch":false,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"Am 11.03.2019 um 12:58 schrieb Duy Nguyen:\n> On Mon, Mar 11, 2019 at 10:48 AM Jeff King <peff@peff.net> wrote:\n>> And AFAIK there is no good way to\n>> modify the submodule-provided content as part of the build. Why do we\n>> even have the submodule again? ;P\n> \n> Because of dogfooding of course. This is an interesting use case\n> though. I wonder if people often want to \"patch\" submodules like this\n> (and what we could do if that's the case)\n\nI usually do the following:\n\n- Fork the sub-project\n- Add a branch with my proposed patches\n- Update the URL and the commit of the submodule in the super-project\n\nThis of course requires all users to do\n\ngit submodule sync\n\nwhich is a bit incovenient, but works.\n\n\n\n\n\n"},{"id":"371175","messageId":"20190311181904.GA16865@sigill.intra.peff.net","threadId":"50691","inReplyTo":"CAH8yC8=9HOAmDE93aXd300kOJSOa0sA8Qv2cNn2M-EO-gqiktw@mail.gmail.com","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-11T18:19:04Z","receivedAt":"2019-03-11T18:19:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 11, 2019 at 06:40:21AM -0400, Jeffrey Walton wrote:\n\n> > So anyway, I think this needs a patch to the upstream sha1dc project.\n> \n> https://github.com/cr-marcstevens/sha1collisiondetection/issues/47\n\nThanks, it looks like the turnaround on that may be pretty quick. Once\nit's merged there, we'll need a local patch to bump the gitlink sha1 and\ntweak the Makefile.\n\n-Peff\n"},{"id":"371176","messageId":"20190311182328.GB16865@sigill.intra.peff.net","threadId":"50691","inReplyTo":"8cf2fa6c-d742-a2a6-cde7-66cef87b04e8@virtuell-zuhause.de","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-11T18:23:29Z","receivedAt":"2019-03-11T18:23:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 11, 2019 at 07:15:12PM +0100, Thomas Braun wrote:\n\n> Am 11.03.2019 um 12:58 schrieb Duy Nguyen:\n> > On Mon, Mar 11, 2019 at 10:48 AM Jeff King <peff@peff.net> wrote:\n> >> And AFAIK there is no good way to\n> >> modify the submodule-provided content as part of the build. Why do we\n> >> even have the submodule again? ;P\n> > \n> > Because of dogfooding of course. This is an interesting use case\n> > though. I wonder if people often want to \"patch\" submodules like this\n> > (and what we could do if that's the case)\n> \n> I usually do the following:\n> \n> - Fork the sub-project\n> - Add a branch with my proposed patches\n> - Update the URL and the commit of the submodule in the super-project\n> \n> This of course requires all users to do\n> \n> git submodule sync\n> \n> which is a bit incovenient, but works.\n\nThe problem to me is not that the steps that a developer has to do, but\nrather that we are dependent on the upstream project to make a simple\nfix (which they may not agree to do, or may take a long time to do).\n\nWhereas if we import the content into our repo as a subtree, we are free\nto hack it up as we see fit, and then occasionally pull from upstream\nand reconcile the changes. Changing upstream isn't advisable in the\ngeneral case, but I think makes a lot of sense for small changes\n(especially if you have the discipline to actually get the same or\nsimilar change pushed upstream).\n\nIn this particular case, though, the sha1dc project is pretty\nresponsive, so I don't think it's going to be a big deal. It just seems\nlike an anti-pattern in general.\n\n-Peff\n"},{"id":"371226","messageId":"xmqqa7i0h7r6.fsf@gitster-ct.c.googlers.com","threadId":"50691","inReplyTo":"20190311182328.GB16865@sigill.intra.peff.net","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-12T07:27:57Z","receivedAt":"2019-03-12T07:28:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The problem to me is not that the steps that a developer has to do, but\n> rather that we are dependent on the upstream project to make a simple\n> fix (which they may not agree to do, or may take a long time to do).\n\nYeah.  In practice, I think the recommended way to work for a\ndepending project like us is to keep a fork in a separate repository\nwe control of the submodule project, and allow our fork to be\nslightly ahead of the upstream while feeding our change to them.\n\n"},{"id":"371227","messageId":"8736nscw2y.fsf@evledraar.gmail.com","threadId":"50691","inReplyTo":"20190311182328.GB16865@sigill.intra.peff.net","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-12T08:53:41Z","receivedAt":"2019-03-12T08:53:47Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 11 2019, Jeff King wrote:\n\n> On Mon, Mar 11, 2019 at 07:15:12PM +0100, Thomas Braun wrote:\n>\n>> Am 11.03.2019 um 12:58 schrieb Duy Nguyen:\n>> > On Mon, Mar 11, 2019 at 10:48 AM Jeff King <peff@peff.net> wrote:\n>> >> And AFAIK there is no good way to\n>> >> modify the submodule-provided content as part of the build. Why do we\n>> >> even have the submodule again? ;P\n>> >\n>> > Because of dogfooding of course. This is an interesting use case\n>> > though. I wonder if people often want to \"patch\" submodules like this\n>> > (and what we could do if that's the case)\n>>\n>> I usually do the following:\n>>\n>> - Fork the sub-project\n>> - Add a branch with my proposed patches\n>> - Update the URL and the commit of the submodule in the super-project\n>>\n>> This of course requires all users to do\n>>\n>> git submodule sync\n>>\n>> which is a bit incovenient, but works.\n>\n> The problem to me is not that the steps that a developer has to do, but\n> rather that we are dependent on the upstream project to make a simple\n> fix (which they may not agree to do, or may take a long time to do).\n>\n> Whereas if we import the content into our repo as a subtree, we are free\n> to hack it up as we see fit, and then occasionally pull from upstream\n> and reconcile the changes. Changing upstream isn't advisable in the\n> general case, but I think makes a lot of sense for small changes\n> (especially if you have the discipline to actually get the same or\n> similar change pushed upstream).\n>\n> In this particular case, though, the sha1dc project is pretty\n> responsive, so I don't think it's going to be a big deal. It just seems\n> like an anti-pattern in general.\n\nThere's a at least a couple of aspects to this.\n\nOne is whether we should have the submodule in\nsha1collisiondetection/. I agree that's probably a bad idea now\nper-se. Honestly I wasn't expecting the answer when I submitted the\nfinal patch to switch to it fully to be to the effect of submodules\nbeing too immature for the git project itself to use. So now we're\neffectively mid-series, and should maybe just back out.\n\nBut the other is the developer social engineering question of how we\nstrike the right trade-off when we import upstream code.\n\nI fully agree with what you've said in theory, but if we look at what's\nhappened in practice we as a project are demonstrably not disciplined\nenough to manage upstream code like this without overtly perma-forking\nit.\n\nE.g. I gave up on updating compat/regex some time ago because of the\nvarious cross-tree patches that had ended up modifying it. Now we can't\njust upstream a new engine anymore.\n\nSomeone needs to first go through those various modifications, upstream\nthem one-by-one or prove they're not needed anymore (and many are\nportability / obscure compiler fixes, so that's hard...). The\ncompat/regex isn't unique here, e.g. compat/poll/ is another example of\nthis.\n\nAs far as I can tell none of the people changing that code went through\nthe process of submitting a parallel upstream fix or seeing if the issue\nwas fixed upstream and we could just update the code we were carrying,\nand of course that gets progressively harder for any one contributor as\nour divergence grows.\n\nSo even though the theory of the sha1collisiondetection/ submodule +\nsha1dc/ code fork is silly, perhaps we've stumbled upon some way where\nwe at least file an upstream bug for issues we find and fix. As\ndemonstrated by other such changes that's already leaps and bounds ahead\nof what we're usually doing.\n"},{"id":"371235","messageId":"20190312105159.GB2023@sigill.intra.peff.net","threadId":"50691","inReplyTo":"xmqqa7i0h7r6.fsf@gitster-ct.c.googlers.com","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-12T10:51:59Z","receivedAt":"2019-03-12T10:52:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2019 at 04:27:57PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The problem to me is not that the steps that a developer has to do, but\n> > rather that we are dependent on the upstream project to make a simple\n> > fix (which they may not agree to do, or may take a long time to do).\n> \n> Yeah.  In practice, I think the recommended way to work for a\n> depending project like us is to keep a fork in a separate repository\n> we control of the submodule project, and allow our fork to be\n> slightly ahead of the upstream while feeding our change to them.\n\nReading Thomas's email again, that might actually have been what he was\nrecommending. If so, sorry for the confusion. And I agree that's a valid\nsolution.\n\nThat said, I do wonder at some point if there's a huge value in using a\nsubmodule at that point. I think there is if the dependent project is\nlarge (and if it's optional, and some people might not need it). But in\nthis case, it is not a big deal to just carry the sha1dc code in-tree.\n\n-Peff\n"},{"id":"371237","messageId":"20190312110549.GC2023@sigill.intra.peff.net","threadId":"50691","inReplyTo":"8736nscw2y.fsf@evledraar.gmail.com","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-12T11:05:49Z","receivedAt":"2019-03-12T11:05:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2019 at 09:53:41AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> There's a at least a couple of aspects to this.\n> \n> One is whether we should have the submodule in\n> sha1collisiondetection/. I agree that's probably a bad idea now\n> per-se. Honestly I wasn't expecting the answer when I submitted the\n> final patch to switch to it fully to be to the effect of submodules\n> being too immature for the git project itself to use. So now we're\n> effectively mid-series, and should maybe just back out.\n\nI think it's especially funky because we have three different ways of\ngetting sha1dc (in-tree, submodule, or against an external library). And\nI almost blindly submitted a patch making the in-tree version work\n(since that's what's used by default, and what I use) which could have\ntotally broken things for the other use cases without anybody realizing\nuntil the change trickled down to somebody who uses those flags.\n\n(Technically in this case it wouldn't actually have _broken_ them, but\njust not helped them, so they'd be no worse off. But hopefully you get\nthe point).\n\nSpeaking of external libraries, in some ways the issue I raised is no\ndifferent than it is for any external library, where we're at the mercy\nof whatever version is on the system. The big dependency for us is\nusually libcurl, and we do have to sometimes work around old versions\nthere.\n\nBut I do think there's one thing that make the sha1dc submodule approach\nmore painful is that we don't control the content of the code, but we\n_do_ build it ourselves with our usual compiler flags. So we're weirdly\nintimate with it (and in fact, an external library would not have the\nproblem being discussed here, since it would have been built separately\nwithout UBSan).\n\n> I fully agree with what you've said in theory, but if we look at what's\n> happened in practice we as a project are demonstrably not disciplined\n> enough to manage upstream code like this without overtly perma-forking\n> it.\n\nI'm not sure I agree completely. Most of the things we've imported are\nsmall enough that we're reasonably happy to accept them as a snapshot in\ntime and take ownership. I.e., I do not recall a lot of instances of\nfixing bugs in compat/regex or compat/poll that we could have gotten\nmore easily by merging from upstream. But I admit I don't actually pay\nmuch attention to those areas, so I might be completely off-base.\n\nThe one place I really _would_ have liked to remain compatible with\nupstream is xdiff. And we were traditionally pretty hesitant to clean\nthings up there for fear of diverging. But in practice, upstream there\nhas been stagnant, and we've done most of the bug fixes and improvements\nto it (in-tree).\n\n> As far as I can tell none of the people changing that code went through\n> the process of submitting a parallel upstream fix or seeing if the issue\n> was fixed upstream and we could just update the code we were carrying,\n> and of course that gets progressively harder for any one contributor as\n> our divergence grows.\n\nTo be clear, I do sympathize with the notion that not pulling things\nin-tree keeps our relationship with upstream more disciplined, and that\nhas value. I'm just not altogether clear how much it's really hurt us\noverall to be undisciplined.\n\n-Peff\n"},{"id":"371244","messageId":"87y35kb8ft.fsf@evledraar.gmail.com","threadId":"50691","inReplyTo":"20190312110549.GC2023@sigill.intra.peff.net","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-12T12:09:42Z","receivedAt":"2019-03-12T12:09:49Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 12 2019, Jeff King wrote:\n\n> On Tue, Mar 12, 2019 at 09:53:41AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> There's a at least a couple of aspects to this.\n>>\n>> One is whether we should have the submodule in\n>> sha1collisiondetection/. I agree that's probably a bad idea now\n>> per-se. Honestly I wasn't expecting the answer when I submitted the\n>> final patch to switch to it fully to be to the effect of submodules\n>> being too immature for the git project itself to use. So now we're\n>> effectively mid-series, and should maybe just back out.\n>\n> I think it's especially funky because we have three different ways of\n> getting sha1dc (in-tree, submodule, or against an external library). And\n> I almost blindly submitted a patch making the in-tree version work\n> (since that's what's used by default, and what I use) which could have\n> totally broken things for the other use cases without anybody realizing\n> until the change trickled down to somebody who uses those flags.\n>\n> (Technically in this case it wouldn't actually have _broken_ them, but\n> just not helped them, so they'd be no worse off. But hopefully you get\n> the point).\n>\n> Speaking of external libraries, in some ways the issue I raised is no\n> different than it is for any external library, where we're at the mercy\n> of whatever version is on the system. The big dependency for us is\n> usually libcurl, and we do have to sometimes work around old versions\n> there.\n>\n> But I do think there's one thing that make the sha1dc submodule approach\n> more painful is that we don't control the content of the code, but we\n> _do_ build it ourselves with our usual compiler flags. So we're weirdly\n> intimate with it (and in fact, an external library would not have the\n> problem being discussed here, since it would have been built separately\n> without UBSan).\n>\n>> I fully agree with what you've said in theory, but if we look at what's\n>> happened in practice we as a project are demonstrably not disciplined\n>> enough to manage upstream code like this without overtly perma-forking\n>> it.\n>\n> I'm not sure I agree completely. Most of the things we've imported are\n> small enough that we're reasonably happy to accept them as a snapshot in\n> time and take ownership. I.e., I do not recall a lot of instances of\n> fixing bugs in compat/regex or compat/poll that we could have gotten\n> more easily by merging from upstream. But I admit I don't actually pay\n> much attention to those areas, so I might be completely off-base.\n>\n> The one place I really _would_ have liked to remain compatible with\n> upstream is xdiff. And we were traditionally pretty hesitant to clean\n> things up there for fear of diverging. But in practice, upstream there\n> has been stagnant, and we've done most of the bug fixes and improvements\n> to it (in-tree).\n>\n>> As far as I can tell none of the people changing that code went through\n>> the process of submitting a parallel upstream fix or seeing if the issue\n>> was fixed upstream and we could just update the code we were carrying,\n>> and of course that gets progressively harder for any one contributor as\n>> our divergence grows.\n>\n> To be clear, I do sympathize with the notion that not pulling things\n> in-tree keeps our relationship with upstream more disciplined, and that\n> has value. I'm just not altogether clear how much it's really hurt us\n> overall to be undisciplined.\n\nI agree that say the compat/regex divergence hasn't hurt us much if at\nall. Just that we have a few conflicting desires:\n\n A. Make sure you can \"make\" git by default without pulling down a bunch\n    of libraries, especially if they're not ubiquitous. Thus shipping\n    the likes of sha1dc.\n\n B. Being able to hotfix those libraries.\n\n C. Upstreaming those hotfixes when they happen.\n\n D. Updating the library we pulled in due to \"A\" from upstream.\n\nIn practice because we've wanted \"A\" we've felt the need to do \"B\", but\nthen also not \"C\" without ever having the discussion that skipping that\npart was a good idea, and as a result \"D\" is hard.\n\nDo we urgently need \"D\"? No. I'm just pointing out that in addition to\noptimzing for \"B\" being easy we should also weigh being good free\nsoftware citizens and coordinate fixes with upstream, which also makes a\nfuture \"D\" easier.\n\nAlso, while I don't know of any bugs in compat/regex. I think it's a bit\nconcerning that we're carrying 2010-era code for something like the\nregex engine that we expose e.g. over gitweb.\n"},{"id":"371297","messageId":"20190312210124.GA5025@sigill.intra.peff.net","threadId":"50691","inReplyTo":"87y35kb8ft.fsf@evledraar.gmail.com","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-12T21:01:25Z","receivedAt":"2019-03-12T21:01:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2019 at 01:09:42PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > To be clear, I do sympathize with the notion that not pulling things\n> > in-tree keeps our relationship with upstream more disciplined, and that\n> > has value. I'm just not altogether clear how much it's really hurt us\n> > overall to be undisciplined.\n> \n> I agree that say the compat/regex divergence hasn't hurt us much if at\n> all. Just that we have a few conflicting desires:\n> \n>  A. Make sure you can \"make\" git by default without pulling down a bunch\n>     of libraries, especially if they're not ubiquitous. Thus shipping\n>     the likes of sha1dc.\n> \n>  B. Being able to hotfix those libraries.\n> \n>  C. Upstreaming those hotfixes when they happen.\n> \n>  D. Updating the library we pulled in due to \"A\" from upstream.\n> \n> In practice because we've wanted \"A\" we've felt the need to do \"B\", but\n> then also not \"C\" without ever having the discussion that skipping that\n> part was a good idea, and as a result \"D\" is hard.\n> \n> Do we urgently need \"D\"? No. I'm just pointing out that in addition to\n> optimzing for \"B\" being easy we should also weigh being good free\n> software citizens and coordinate fixes with upstream, which also makes a\n> future \"D\" easier.\n\nYeah, I think your mental model there makes some sense.\n\n> Also, while I don't know of any bugs in compat/regex. I think it's a bit\n> concerning that we're carrying 2010-era code for something like the\n> regex engine that we expose e.g. over gitweb.\n\nTBH, I think it's probably a bad idea to open any version of that code\nto untrusted people, because it's easy to write a DoS regex against it.\nThere are libraries (like re2) that would be a better fit (I seem to\nrecall that there may be some DFA-only support in pcre, too, but I\nhaven't looked at it in a long time; if there is, it might be a sane\ngitweb feature to only expose that engine).\n\n-Peff\n"},{"id":"371299","messageId":"20190312210626.GA5157@sigill.intra.peff.net","threadId":"50691","inReplyTo":"20190311033755.GB7087@sigill.intra.peff.net","subject":"[PATCH] Makefile: fix unaligned loads in sha1dc with UBSan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-12T21:06:26Z","receivedAt":"2019-03-12T21:06:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 10, 2019 at 11:37:55PM -0400, Jeff King wrote:\n\n> Unfortunately, I don't think sha1dc currently supports #defines in that\n> direction. The only logic is \"if we are on intel, do unaligned loads\"\n> and \"even if we are not on intel, do it anyway\". There is no \"even if we\n> are on intel, do not do unaligned loads\".\n> \n> I think you'd need something like this:\n> [...]\n\nThe sha1dc folks gave us a very nice and quick turnaround on this.\nThanks to them, and to Jeffrey for opening an issue there.\n\nHere's a commit which updates Git to use the new feature. I've tested it\nwith both the in-tree and submodule builds like:\n\n  make DC_SHA1_SUBMODULE=Yes SANITIZE=undefined && (cd t && ./t0001-*)\n  make DC_SHA1_SUBMODULE=    SANITIZE=undefined && (cd t && ./t0001-*)\n\nboth of which fail without this patch and succeed without it.\n\n-- >8 --\nSubject: [PATCH] Makefile: fix unaligned loads in sha1dc with UBSan\n\nThe sha1dc library uses unaligned loads on platforms that support them.\nThis is normally what you'd want for performance, but it does cause\nUBSan to complain when we compile with SANITIZE=undefined. Just like we\nset -DNO_UNALIGNED_LOADS for our own code in that case, we should set\n-DSHA1DC_FORCE_ALIGNED_ACCESS.\n\nOf course that does nothing without pulling in the patches from sha1dc\nto respect that define. So let's do that, too, updating both the\nsubmodule link and our in-tree copy (from the same commit).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Makefile               | 1 +\n sha1collisiondetection | 2 +-\n sha1dc/sha1.c          | 5 +++--\n 3 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 537493822b..593c2c729a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1195,6 +1195,7 @@ BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n BASIC_CFLAGS += -fno-omit-frame-pointer\n ifneq ($(filter undefined,$(SANITIZERS)),)\n BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n+BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS\n endif\n ifneq ($(filter leak,$(SANITIZERS)),)\n BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS\ndiff --git a/sha1collisiondetection b/sha1collisiondetection\nindex 232357eb2e..16033998da 160000\n--- a/sha1collisiondetection\n+++ b/sha1collisiondetection\n@@ -1 +1 @@\n-Subproject commit 232357eb2ea0397388254a4b188333a227bf5b10\n+Subproject commit 16033998da4b273aebd92c84b1e1b12e4aaf7009\ndiff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\nindex df0630bc6d..5931cf25d5 100644\n--- a/sha1dc/sha1.c\n+++ b/sha1dc/sha1.c\n@@ -124,10 +124,11 @@\n #endif\n /*ENDIANNESS SELECTION*/\n \n+#ifndef SHA1DC_FORCE_ALIGNED_ACCESS\n #if defined(SHA1DC_FORCE_UNALIGNED_ACCESS) || defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)\n #define SHA1DC_ALLOW_UNALIGNED_ACCESS\n-#endif /*UNALIGNMENT DETECTION*/\n-\n+#endif /*UNALIGNED ACCESS DETECTION*/\n+#endif /*FORCE ALIGNED ACCESS*/\n \n #define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n))))\n #define rotate_left(x,n)  (((x)<<(n))|((x)>>(32-(n))))\n-- \n2.21.0.539.gcf54785f87\n\n"},{"id":"371300","messageId":"87va0nbxmj.fsf@evledraar.gmail.com","threadId":"50691","inReplyTo":"20190312210626.GA5157@sigill.intra.peff.net","subject":"Re: [PATCH] Makefile: fix unaligned loads in sha1dc with UBSan","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-12T21:17:56Z","receivedAt":"2019-03-12T21:18:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 12 2019, Jeff King wrote:\n\n> On Sun, Mar 10, 2019 at 11:37:55PM -0400, Jeff King wrote:\n>\n>> Unfortunately, I don't think sha1dc currently supports #defines in that\n>> direction. The only logic is \"if we are on intel, do unaligned loads\"\n>> and \"even if we are not on intel, do it anyway\". There is no \"even if we\n>> are on intel, do not do unaligned loads\".\n>>\n>> I think you'd need something like this:\n>> [...]\n>\n> The sha1dc folks gave us a very nice and quick turnaround on this.\n> Thanks to them, and to Jeffrey for opening an issue there.\n\nThanks. Good to have it resolved this way.\n\n> Here's a commit which updates Git to use the new feature. I've tested it\n> with both the in-tree and submodule builds like:\n>\n>   make DC_SHA1_SUBMODULE=Yes SANITIZE=undefined && (cd t && ./t0001-*)\n>   make DC_SHA1_SUBMODULE=    SANITIZE=undefined && (cd t && ./t0001-*)\n>\n> both of which fail without this patch and succeed without it.\n\nFWIW I've reproduced this testing and found the same thing. Looks good\nto me.\n"},{"id":"371301","messageId":"20190312211908.GA24509@sigill.intra.peff.net","threadId":"50691","inReplyTo":"87va0nbxmj.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: fix unaligned loads in sha1dc with UBSan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-12T21:19:08Z","receivedAt":"2019-03-12T21:19:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2019 at 10:17:56PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > Here's a commit which updates Git to use the new feature. I've tested it\n> > with both the in-tree and submodule builds like:\n> >\n> >   make DC_SHA1_SUBMODULE=Yes SANITIZE=undefined && (cd t && ./t0001-*)\n> >   make DC_SHA1_SUBMODULE=    SANITIZE=undefined && (cd t && ./t0001-*)\n> >\n> > both of which fail without this patch and succeed without it.\n> \n> FWIW I've reproduced this testing and found the same thing. Looks good\n> to me.\n\nEr, that second \"without\" should be \"with\", but hopefully you figured\nthat out during your testing. :)\n\n-Peff\n"},{"id":"371349","messageId":"7435b80b-6bd7-cab9-c646-5c81b639f117@virtuell-zuhause.de","threadId":"50691","inReplyTo":"20190312105159.GB2023@sigill.intra.peff.net","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2019-03-13T11:47:51Z","receivedAt":"2019-03-13T11:48:13Z","isPatch":false,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"Am 12.03.2019 um 11:51 schrieb Jeff King:\n> On Tue, Mar 12, 2019 at 04:27:57PM +0900, Junio C Hamano wrote:\n> \n>> Jeff King <peff@peff.net> writes:\n>>\n>>> The problem to me is not that the steps that a developer has to do, but\n>>> rather that we are dependent on the upstream project to make a simple\n>>> fix (which they may not agree to do, or may take a long time to do).\n>>\n>> Yeah.  In practice, I think the recommended way to work for a\n>> depending project like us is to keep a fork in a separate repository\n>> we control of the submodule project, and allow our fork to be\n>> slightly ahead of the upstream while feeding our change to them.\n> \n> Reading Thomas's email again, that might actually have been what he was\n> recommending. If so, sorry for the confusion. And I agree that's a valid\n> solution.\n\nYes that is what I tried to explain. Looks like it was lost in translation.\n\n> That said, I do wonder at some point if there's a huge value in using a\n> submodule at that point. I think there is if the dependent project is\n> large (and if it's optional, and some people might not need it). But in\n> this case, it is not a big deal to just carry the sha1dc code in-tree.\n\nA big win with submodules is that you have separate histories and can,\nquite easily, update to newer versions without manual copying.\n\nOne grievance with submodules is the URL switching if you need to go\nwith a forked repo for some time and then back to the original.\nIs it possible to have multiple remotes for a submodule?\n\nSomething like:\n\n[submodule \"libfoo\"]\n\tpath = include/foo\n\turl1 = git://foo.com/upstream/lib.git\n\turl2 = git://foo.com/myFork/lib.git\n\nWith that the error prone git submodule sync step is not required anymore.\n\nsubmodule.alternateLocation looks like it is going into the right direction.\n"},{"id":"371372","messageId":"20190313153923.GC24101@sigill.intra.peff.net","threadId":"50691","inReplyTo":"7435b80b-6bd7-cab9-c646-5c81b639f117@virtuell-zuhause.de","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-13T15:39:24Z","receivedAt":"2019-03-13T15:40:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 13, 2019 at 12:47:51PM +0100, Thomas Braun wrote:\n\n> > Reading Thomas's email again, that might actually have been what he was\n> > recommending. If so, sorry for the confusion. And I agree that's a valid\n> > solution.\n> \n> Yes that is what I tried to explain. Looks like it was lost in translation.\n\nI think the problem was on the reading end. :)\n\n> > That said, I do wonder at some point if there's a huge value in using a\n> > submodule at that point. I think there is if the dependent project is\n> > large (and if it's optional, and some people might not need it). But in\n> > this case, it is not a big deal to just carry the sha1dc code in-tree.\n> \n> A big win with submodules is that you have separate histories and can,\n> quite easily, update to newer versions without manual copying.\n\nTrue. We'd generally be picking up snapshots in our in-tree sha1dc/, so\nbisecting on it is not as fine-grained. We _could_ pull in the full\nhistory using something like git-subtree, but that comes with its own\ncomplications.\n\n> One grievance with submodules is the URL switching if you need to go\n> with a forked repo for some time and then back to the original.\n> Is it possible to have multiple remotes for a submodule?\n> \n> Something like:\n> \n> [submodule \"libfoo\"]\n> \tpath = include/foo\n> \turl1 = git://foo.com/upstream/lib.git\n> \turl2 = git://foo.com/myFork/lib.git\n> \n> With that the error prone git submodule sync step is not required anymore.\n\nI assume you'd fetch from _all_ of them during a fetch, and assume that\none of them will get you the objects you need (or I guess if you are\nlooking for a specific object, you'd try them one at a time until you\nget the object).\n\nThat makes sense, though it might be kind of annoying when fetching is\nexpensive (especially if it involves manually authenticating).\n\n> submodule.alternateLocation looks like it is going into the right direction.\n\nI think that's mostly about pointing back to the superproject for local\nstorage. Though I think there's a pretty reasonable solution to the\nproblem we're discussing there: git.git could carry a \"sha1dc\" branch\nthat points to our modified submodule history. So it's \"in-tree\" in the\nsense that that it is in our repo, and under our full control, but still\nmanaged like a submodule.\n\nAnd we'd probably not even duplicate a lot of storage in the actual\nclone of the upstream project, because it would be pointing to us as an\nalternate.\n\n-Peff\n"},{"id":"371373","messageId":"87lg1ibw8b.fsf@evledraar.gmail.com","threadId":"50691","inReplyTo":"20190313153923.GC24101@sigill.intra.peff.net","subject":"Re: disabling sha1dc unaligned access, was Re: One failed self test on Fedora 29","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-13T16:00:20Z","receivedAt":"2019-03-13T16:00:27Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 13 2019, Jeff King wrote:\n\n> On Wed, Mar 13, 2019 at 12:47:51PM +0100, Thomas Braun wrote:\n>\n>> > Reading Thomas's email again, that might actually have been what he was\n>> > recommending. If so, sorry for the confusion. And I agree that's a valid\n>> > solution.\n>>\n>> Yes that is what I tried to explain. Looks like it was lost in translation.\n>\n> I think the problem was on the reading end. :)\n>\n>> > That said, I do wonder at some point if there's a huge value in using a\n>> > submodule at that point. I think there is if the dependent project is\n>> > large (and if it's optional, and some people might not need it). But in\n>> > this case, it is not a big deal to just carry the sha1dc code in-tree.\n>>\n>> A big win with submodules is that you have separate histories and can,\n>> quite easily, update to newer versions without manual copying.\n>\n> True. We'd generally be picking up snapshots in our in-tree sha1dc/, so\n> bisecting on it is not as fine-grained. We _could_ pull in the full\n> history using something like git-subtree, but that comes with its own\n> complications.\n>\n>> One grievance with submodules is the URL switching if you need to go\n>> with a forked repo for some time and then back to the original.\n>> Is it possible to have multiple remotes for a submodule?\n>>\n>> Something like:\n>>\n>> [submodule \"libfoo\"]\n>> \tpath = include/foo\n>> \turl1 = git://foo.com/upstream/lib.git\n>> \turl2 = git://foo.com/myFork/lib.git\n>>\n>> With that the error prone git submodule sync step is not required anymore.\n>\n> I assume you'd fetch from _all_ of them during a fetch, and assume that\n> one of them will get you the objects you need (or I guess if you are\n> looking for a specific object, you'd try them one at a time until you\n> get the object).\n>\n> That makes sense, though it might be kind of annoying when fetching is\n> expensive (especially if it involves manually authenticating).\n>\n>> submodule.alternateLocation looks like it is going into the right direction.\n>\n> I think that's mostly about pointing back to the superproject for local\n> storage. Though I think there's a pretty reasonable solution to the\n> problem we're discussing there: git.git could carry a \"sha1dc\" branch\n> that points to our modified submodule history. So it's \"in-tree\" in the\n> sense that that it is in our repo, and under our full control, but still\n> managed like a submodule.\n>\n> And we'd probably not even duplicate a lot of storage in the actual\n> clone of the upstream project, because it would be pointing to us as an\n> alternate.\n\nNow if only we could think of some way to give the people best\npositioned to fix some of these UI issues for git users everywhere the\nincentive to do so[1] :)\n\n1. https://public-inbox.org/git/20171208223001.556-5-avarab@gmail.com/\n"}]}