{"thread":{"id":"57848","subject":"Crashes in t/t4058-diff-duplicates.sh","startedAt":"2022-05-05T08:53:56Z","lastAt":"2022-05-10T03:50:25Z","messageCount":7,"participants":["Alex Riesen","Junio C Hamano","Elijah Newren","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"454848","messageId":"YnOQmVFVRuqnanMi@pflmari","threadId":"57848","inReplyTo":null,"subject":"Crashes in t/t4058-diff-duplicates.sh","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2022-05-05T08:53:45Z","receivedAt":"2022-05-05T08:53:56Z","isPatch":false,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Hi,\n\nthe test t4058-diff-duplicates reliably dumps core here:\n\nCORE-OF: /home/xxx/yyyyyyy/git/git\n\nDUMPCORE_ARGS:\n 9975\n 9975\n 11\n !home!xxx!yyyyyyy!git!git\nDUMPCORE_ARGS_END\n\nPROC-9975:\n root -> /\n cwd -> /home/xxx/yyyyyyy/git/t/trash directory.t4058-diff-duplicates\n fd/0 -> /dev/null\n fd/1 -> /dev/pts/7\n fd/2 -> /dev/pts/7\n fd/3 -> /dev/pts/7\n fd/4 -> /dev/pts/7\n fd/5 -> /dev/pts/7\n fd/6 -> /dev/pts/7\n fd/7 -> /dev/pts/7\n fd/8 -> /home/xxx/yyyyyyy/git/t/trash directory.t4058-diff-duplicates/.git/index.lock\nPROC-9975_END\n\nENVIRONMENT:\nGIT_COMMITTER_NAME=C O Mitter\nUSER=xxx\nGIT_AUTHOR_EMAIL=author@example.com\nGIT_TEMPLATE_DIR=/home/xxx/yyyyyyy/git/templates/blt\nXDG_SEAT=seat0\nTAR_OPTIONS=--atime-preserve\nGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n_x05=[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]\nGIT_EXEC_PATH=/home/xxx/yyyyyyy/git\nSSH_AGENT_PID=3796\nXDG_SESSION_TYPE=x11\nGIT_CEILING_DIRECTORIES=/home/xxx/yyyyyyy/git/t/trash directory.t4058-diff-duplicates/..\nUSER_HOME=/home/xxx\nSHLVL=1\nLESS=RSX\nHOME=/home/xxx/yyyyyyy/git/t/trash directory.t4058-diff-duplicates\nOLDPWD=/home/xxx/yyyyyyy/git/t\nGIT_AUTHOR_DATE=1112354055 +0200\n_x35=[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]\nDESKTOP_SESSION=lightdm-xsession\nZERO_OID=0000000000000000000000000000000000000000\nOID_REGEX=[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]\nXDG_SEAT_PATH=/org/freedesktop/DisplayManager/Seat0\nPAGER=cat\nGIT_AUTHOR_NAME=A U Thor\nDBUS_SESSION_BUS_ADDRESS=unix:abstract=/tmp/dbus-qjoVRmyD3o,guid=8cdc53ee0598990a001ad838627364a4\nu200c=‌\nCOLORTERM=rxvt-xpm\ntest_prereq=\nGNOME_KEYRING_CONTROL=/run/user/1000/keyring\nGIT_TEST_MERGE_ALGORITHM=ort\nEMPTY_BLOB=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\nGITPERLLIB=/home/xxx/yyyyyyy/git/perl/build/lib:/home/xxx/yyyyyyy/git/perl/build/lib\nLOGNAME=xxx\nGIT_ATTR_NOSYSTEM=1\nWINDOWID=56623113\n_=./t4058-diff-duplicates.sh\nGIT_TEST_CHECK_CACHE_TREE=false\nXDG_SESSION_CLASS=user\nCOLORFGBG=15;default\nGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\nTERM=dumb\nXDG_SESSION_ID=2\nCOLUMNS=80\nGIT_TRACE_BARE=1\nUSER_TERM=rxvt-unicode-256color\nGIT_MERGE_VERBOSITY=5\nPATH=/home/xxx/yyyyyyy/git/bin-wrappers:/home/xxx/yyyyyyy/git/bin-wrappers:/home/xxx/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games\nLESSCHARSET=latin1\nGDM_LANG=en_US.utf8\nGIT_CONFIG_NOSYSTEM=1\nXDG_SESSION_PATH=/org/freedesktop/DisplayManager/Session0\nXDG_RUNTIME_DIR=/run/user/1000\nDISPLAY=:0\nGIT_DEFAULT_HASH=sha1\nLANG=C\nLSAN_OPTIONS=fast_unwind_on_malloc=0:strip_path_prefix=/home/xxx/yyyyyyy/git/:abort_on_error=1\nGIT_TRACE2_EVENT_NESTING=100\nGNUPGHOME=/home/xxx/yyyyyyy/git/t/trash directory.t4058-diff-duplicates/gnupg-home-not-used\nSHELL=/bin/bash\nMALLOC_CHECK_=3\nGIT_TEXTDOMAINDIR=/home/xxx/yyyyyyy/git/po/build/locale\nEMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904\nMALLOC_PERTURB_=165\nOSTYPE=linux-gnu\nASAN_OPTIONS=detect_leaks=0:strip_path_prefix=/home/xxx/yyyyyyy/git/:abort_on_error=1\nGIT_COMMITTER_EMAIL=committer@example.com\nPWD=/home/xxx/yyyyyyy/git/t/trash directory.t4058-diff-duplicates\nSHELL_PATH=/bin/sh\nPERL_PATH=/usr/bin/perl\nLC_ALL=C\nGIT_MERGE_AUTOEDIT=no\nLC_NUMERIC=C\nTZ=UTC\nGIT_COMMITTER_DATE=1112354055 +0200\nLF=\\n\nMANPATH=:/home/xxx/share/man\nEDITOR=:\nGIT_TEST_FSYNC=0\nENVIRONMENT_END\n\nPID_TRACE:\n9975 (git) S /home/xxx/yyyyyyy/git/git merge update \n cwd: /home/xxx/yyyyyyy/git/t/trash directory.t4058-diff-duplicates\n9653 (t4058-diff-dupl) S /bin/sh ./t4058-diff-duplicates.sh -d -v -i \n cwd: /home/xxx/yyyyyyy/git/t/trash directory.t4058-diff-duplicates\n4932 (bash) S bash \n cwd: /home/xxx/yyyyyyy/git/t\n4924 (urxvt) S urxvt \n cwd: /home/xxx\n1 (init) S init [2] \n cwd: /\nPID_TRACE_END\n\nGDB:\nReading symbols from /home/xxx/yyyyyyy/git/git...\n[New LWP 9975]\n[Thread debugging using libthread_db enabled]\nUsing host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\nCore was generated by `/home/xxx/yyyyyyy/git/git merge update'.\nProgram terminated with signal SIGSEGV, Segmentation fault.\n#0 0x00005629e52f4a00 in traverse_by_cache_tree (info=0x7fff27e7dba8, \n info=0x7fff27e7dba8, nr_names=2, nr_entries=4, pos=0)\n at unpack-trees.c:807\n807\t\t\tlen = ce_namelen(src[0]);\nThreads:\n Id Target Id Frame \n* 1 Thread 0x7f408feb8740 (LWP 9975) 0x00005629e52f4a00 in traverse_by_cache_tree (info=0x7fff27e7dba8, info=0x7fff27e7dba8, nr_names=2, nr_entries=4, pos=0) at unpack-trees.c:807\nStack:\nnew_ce_len = \nlen = \nrc = \no = 0x7fff27e7e930\ntree_ce = 0x5629e596e7d0\nce_len = 240\ni = 1\nsrc = {0x5629e594a518, 0x5629e596e7d0, 0x5629e596e7d0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}\nd = \nsrc = {, , , , , , , , }\no = \ntree_ce = \nce_len = \ni = \nd = \nnew_ce_len = \nlen = \nrc = \n#0 0x00005629e52f4a00 in traverse_by_cache_tree (info=0x7fff27e7dba8, info=0x7fff27e7dba8, nr_names=2, nr_entries=4, pos=0) at unpack-trees.c:807\n#1 traverse_trees_recursive (n=n@entry=2, dirmask=dirmask@entry=3, df_conflicts=df_conflicts@entry=0, names=names@entry=0x7fff27e7df80, info=info@entry=0x7fff27e7e420) at unpack-trees.c:872\n#2 0x00005629e52f5668 in unpack_callback (n=, mask=3, dirmask=3, names=0x7fff27e7df80, info=) at unpack-trees.c:1479\n#3 0x00005629e52f3162 in traverse_trees (istate=0x5629e541c980 , n=n@entry=2, t=t@entry=0x7fff27e7e6f0, info=info@entry=0x7fff27e7e420) at tree-walk.c:532\n#4 0x00005629e52f82fa in unpack_trees (len=len@entry=2, t=t@entry=0x7fff27e7e6f0, o=o@entry=0x7fff27e7e930) at unpack-trees.c:1882\n#5 0x00005629e523d2ae in checkout_fast_forward (r=0x5629e541caa0 , head=head@entry=0x5629e594be24, remote=remote@entry=0x5629e594be6c, overwrite_ignore=1) at merge.c:94\n#6 0x00005629e5135742 in cmd_merge (argc=, argv=, prefix=) at builtin/merge.c:1578\n#7 0x00005629e50c921b in run_builtin (argv=0x7fff27e7f9c0, argc=2, p=0x5629e53ead48 ) at git.c:465\n#8 handle_builtin (argc=2, argv=0x7fff27e7f9c0) at git.c:719\n#9 0x00005629e50ca53d in run_argv (argv=0x7fff27e7f700, argcp=0x7fff27e7f70c) at git.c:786\n#10 cmd_main (argc=, argc@entry=3, argv=, argv@entry=0x7fff27e7f9b8) at git.c:917\n#11 0x00005629e50c8f03 in main (argc=3, argv=0x7fff27e7f9b8) at common-main.c:56\nGDB_END\n\nend\n\nP.S. dumpcore (the tool which produced this trace) is this: https://github.com/raalkml/dumpcore\n"},{"id":"454909","messageId":"YnT19KB2XkBrJOLQ@pflmari","threadId":"57848","inReplyTo":"YnSWgDdxgm+XWiLt@nand.local","subject":"Re: Crashes in t/t4058-diff-duplicates.sh","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2022-05-06T10:18:28Z","receivedAt":"2022-05-06T10:18:54Z","isPatch":false,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Taylor Blau, Fri, May 06, 2022 05:31:12 +0200:\n> On Thu, May 05, 2022 at 10:53:45AM +0200, Alex Riesen wrote:\n> > Hi,\n> >\n> > the test t4058-diff-duplicates reliably dumps core here:\n> \n> It was a little tricky to find out what part of t4058 you were referring\n> to, but...\n\nVery sorry! I forgot to include the output of the test itself!\n\n> > Core was generated by `/home/xxx/yyyyyyy/git/git merge update'.\n> \n> ...helps us out ;-). The only match for \"git merge update\" is in\n> t4058.16, which blames back to ac14de13b2 (t4058: explore duplicate tree\n> entry handling in a bit more detail, 2020-12-11), which helpfully\n> explains that this segfault is known (and furthermore they are\n> long-lived and likely not even worth fixing, per ac14de13b2).\n\nThanks for finding the commit! Makes absolutely sense, but...\n\nI have a little problem with the approach to have it crashing though.\nIt crashes for every run of the tests: I have a crash core collecting program\non the machine I use to build binaries of the tools I use. While it is not\nhard to isolate builds of Git (as a whole) with coredump collecting switched\noff I'd prefer to not do it: it's a special case (which gets forgotten) and\nwith it I'll miss new crashes in Git (which I might have authored).\nIt is inconvenient to crash regularly.\n\nIs it reasonable to ask to replace the crash in case of this known breakage\nwith an error()+exit(130)? (`exit(130)` because the test_expect_failure seems\nto require an exit code greater than 129, and I failed to find where it is).\n\nOr, since the test-lib already has a notion of \"expected failure\" provide\nthe *tests* with a way to reduce collateral effects of that failure?\nLike below with the GDB.\n\nRegards,\nAlex\n\ndiff --git a/t/t4058-diff-duplicates.sh b/t/t4058-diff-duplicates.sh\nindex 54614b814d..b2f9ab07d1 100755\n--- a/t/t4058-diff-duplicates.sh\n+++ b/t/t4058-diff-duplicates.sh\n@@ -132,22 +132,38 @@ test_expect_success 'create a few commits' '\n \trm commit_id up final\n '\n \n+may_crash() {\n+\tlocal ret\n+\tif test -n \"$GIT_DEBUGGER\"\n+\tthen\n+\t\t\"$@\"\n+\t\tret=$?\n+\telse\n+\t\tGIT_DEBUGGER=\"gdb --batch --return-child-result --nh -ex run --args\"\n+\t\texport GIT_DEBUGGER\n+\t\t\"$@\"\n+\t\tret=$?\n+\t\tunset GIT_DEBUGGER\n+\tfi\n+\treturn $ret\n+}\n+\n test_expect_failure 'git read-tree does not segfault' '\n \ttest_when_finished rm .git/index.lock &&\n-\ttest_might_fail git read-tree --reset base\n+\ttest_might_fail may_crash git read-tree --reset base\n '\n \n test_expect_failure 'reset --hard does not segfault' '\n \ttest_when_finished rm .git/index.lock &&\n \tgit checkout base &&\n-\ttest_might_fail git reset --hard\n+\ttest_might_fail may_crash git reset --hard\n '\n \n test_expect_failure 'git diff HEAD does not segfault' '\n \tgit checkout base &&\n \tGIT_TEST_CHECK_CACHE_TREE=false &&\n \tgit reset --hard &&\n-\ttest_might_fail git diff HEAD\n+\ttest_might_fail may_crash git diff HEAD\n '\n \n test_expect_failure 'can switch to another branch when status is empty' '\n@@ -183,7 +199,7 @@ test_expect_success 'switch to base branch and force status to be clean' '\n '\n \n test_expect_failure 'fast-forward from duplicate entries to non-duplicate' '\n-\tgit merge update\n+\tmay_crash git merge update\n '\n \n test_done\n"},{"id":"454918","messageId":"xmqqv8uioc7p.fsf@gitster.g","threadId":"57848","inReplyTo":"YnT19KB2XkBrJOLQ@pflmari","subject":"Re: Crashes in t/t4058-diff-duplicates.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T16:30:34Z","receivedAt":"2022-05-06T16:30:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Taylor Blau, Fri, May 06, 2022 05:31:12 +0200:\n\n>> t4058.16, which blames back to ac14de13b2 (t4058: explore duplicate tree\n\nThat commit talks about \"trees with duplicate entries\". Does it\nmean a bad history where a tree object has two or more entries under\nthe same name? We should of course be catching these things at fsck\ntime and rejecting at network transfer time, but I agree it is not a\ngood excuse for us to segfault. We should diagnose it as a broken\ntree object and actively refuse to proceed by calling die().\n"},{"id":"454964","messageId":"CABPp-BEb8saqS0awK77y+-3oB1LAOPwOw-2dZU=67wJOKLBS1Q@mail.gmail.com","threadId":"57848","inReplyTo":"xmqqv8uioc7p.fsf@gitster.g","subject":"Re: Crashes in t/t4058-diff-duplicates.sh","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-07T04:14:07Z","receivedAt":"2022-05-07T04:14:23Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, May 6, 2022 at 9:30 AM Junio C Hamano wrote:\n>\n> Alex Riesen writes:\n>\n> > Taylor Blau, Fri, May 06, 2022 05:31:12 +0200:\n>\n> >> t4058.16, which blames back to ac14de13b2 (t4058: explore duplicate tree\n>\n> That commit talks about \"trees with duplicate entries\". Does it\n> mean a bad history where a tree object has two or more entries under\n> the same name?\n\nYes.\n\n> We should of course be catching these things at fsck\n> time and rejecting at network transfer time, but I agree it is not a\n> good excuse for us to segfault. We should diagnose it as a broken\n> tree object and actively refuse to proceed by calling die().\n"},{"id":"455000","messageId":"YnkOYyYkfC1C8c/+@pflmari","threadId":"57848","inReplyTo":"xmqqv8uioc7p.fsf@gitster.g","subject":"Re: Crashes in t/t4058-diff-duplicates.sh","fromName":"Alex Riesen","fromEmail":"alexander.riesen@cetitec.com","sentAt":"2022-05-09T12:51:47Z","receivedAt":"2022-05-09T12:52:24Z","isPatch":false,"sender":{"key":"alexander.riesen@cetitec.com","avatar":"https://avatars.githubusercontent.com/u/24452597?v=4"},"body":"Junio C Hamano, Fri, May 06, 2022 18:30:34 +0200:\n> Alex Riesen writes:\n> \n> > Taylor Blau, Fri, May 06, 2022 05:31:12 +0200:\n> \n> >> t4058.16, which blames back to ac14de13b2 (t4058: explore duplicate tree\n> \n> That commit talks about \"trees with duplicate entries\". Does it\n> mean a bad history where a tree object has two or more entries under\n> the same name? We should of course be catching these things at fsck\n> time and rejecting at network transfer time, but I agree it is not a\n> good excuse for us to segfault. We should diagnose it as a broken\n> tree object and actively refuse to proceed by calling die().\n\nThere seem to be multiple places (according to the the commit above, and these\ntests on my machine find two) where something crashes, and while one is easy to\nplug with a simple if-NULL check:\n\nProgram terminated with signal SIGSEGV, Segmentation fault.\n#0 0x000055786ef58a00 in traverse_by_cache_tree (info=0x7fff87d1f400,\n info=0x7fff87d1f400, nr_names=1, nr_entries=4, pos=0)\n at unpack-trees.c:807\n807 len = ce_namelen(src[0]); <--- src[0] is NULL\n\n\nthe other case seems to be more involved:\n\n#0 verify_one (r=r@entry=0x5555e70aeaa0 ,\n istate=istate@entry=0x5555e70ae980 , it=0x5555e839ab90,\n path=path@entry=0x7ffedea66570) at cache-tree.c:929\n929 if (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE))\n(ce cannot be resolved) ----^\n\nThreads:\n Id Target Id Frame\n* 1 Thread 0x7f26de550740 (LWP 19565) verify_one (r=r@entry=0x5555e70aeaa0 , istate=istate@entry=0x5555e70ae980 , it=0x5555e839ab90, path=path@entry=0x7ffedea66570) at cache-tree.c:929\nStack:\nce = 0x5a5a5a5a5a5a5a5a <--- Poisoned pointer?\nsub = 0x0\ni = 1\npos = 0\nlen = 6\ntree_buf = {\n alloc = 65,\n len = 33,\n buf = 0x5555e839b530 \"100644 inner\"\n}\nnew_oid = {\n hash = '\\000' ,\n algo = 0\n}\n#0 verify_one (r=r@entry=0x5555e70aeaa0 , istate=istate@entry=0x5555e70ae980 , it=0x5555e839ab90, path=path@entry=0x7ffedea66570) at cache-tree.c:929\n#1 0x00005555e6e43720 in verify_one (r=r@entry=0x5555e70aeaa0 , istate=istate@entry=0x5555e70ae980 , it=0x5555e83777b0, path=path@entry=0x7ffedea66570) at cache-tree.c:888\n#2 0x00005555e6e44398 in cache_tree_verify (r=0x5555e70aeaa0 , istate=istate@entry=0x5555e70ae980 ) at cache-tree.c:968\n#3 0x00005555e6f10807 in write_locked_index (istate=0x5555e70ae980 , lock=lock@entry=0x7ffedea66740, flags=flags@entry=1) at read-cache.c:3332\n#4 0x00005555e6df7456 in cmd_reset (argc=, argv=, prefix=) at builtin/reset.c:551\n#5 0x00005555e6d5b21b in run_builtin (argv=0x7ffedea67260, argc=2, p=0x5555e707d0a8 ) at git.c:465\n...\n\nIdeas?\n"},{"id":"455006","messageId":"Ynkx/nI67uOUDhL9@nand.local","threadId":"57848","inReplyTo":"CABPp-BEb8saqS0awK77y+-3oB1LAOPwOw-2dZU=67wJOKLBS1Q@mail.gmail.com","subject":"Re: Crashes in t/t4058-diff-duplicates.sh","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-09T15:23:42Z","receivedAt":"2022-05-09T15:23:48Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 06, 2022 at 09:14:07PM -0700, Elijah Newren wrote:\n> > That commit talks about \"trees with duplicate entries\". Does it\n> > mean a bad history where a tree object has two or more entries under\n> > the same name?\n>\n> Yes.\n>\n> > We should of course be catching these things at fsck\n> > time and rejecting at network transfer time, but I agree it is not a\n> > good excuse for us to segfault. We should diagnose it as a broken\n> > tree object and actively refuse to proceed by calling die().\n\nElijah would be able to comment more authoritatively than I could about\nwhether or not these are easily detect-able. If they are, then I think\nit'd be worth doing so and calling die(). But they may be tricker, I\ndon't know.\n\nThanks,\nTaylor\n"},{"id":"455073","messageId":"CABPp-BHyNw0dj=mh65pb9HirGFFTxF1+Kky_T4NUxFKANPE2yQ@mail.gmail.com","threadId":"57848","inReplyTo":"Ynkx/nI67uOUDhL9@nand.local","subject":"Re: Crashes in t/t4058-diff-duplicates.sh","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-10T03:50:06Z","receivedAt":"2022-05-10T03:50:25Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, May 9, 2022 at 8:23 AM Taylor Blau wrote:\n>\n> On Fri, May 06, 2022 at 09:14:07PM -0700, Elijah Newren wrote:\n> > > That commit talks about \"trees with duplicate entries\". Does it\n> > > mean a bad history where a tree object has two or more entries under\n> > > the same name?\n> >\n> > Yes.\n> >\n> > > We should of course be catching these things at fsck\n> > > time and rejecting at network transfer time, but I agree it is not a\n> > > good excuse for us to segfault. We should diagnose it as a broken\n> > > tree object and actively refuse to proceed by calling die().\n>\n> Elijah would be able to comment more authoritatively than I could about\n> whether or not these are easily detect-able. If they are, then I think\n> it'd be worth doing so and calling die(). But they may be tricker, I\n> don't know.\n\nIt's been a couple years, so I don't remember much. I think the way I\ndiscovered these issues was that in order to make sure some other code\nchanges of mine didn't regress on some issues, I was attempting to\nrecreate problematic cases that had been covered by the code I was\nrestructuring. The existing tests related to that code had some\nproblems, so I was modifying/creating my own testcases, and I\nmisunderstood the setup of those tests and the checks behind them and\nended up creating trees broken in a *different* way and which was not\ncovered by the existing code anywhere. I was already a few tangents\nfrom the focus of my work at the time (the new merge backend), so I\ndon't think I investigated whether these were easily detectable. I do\nremember being concerned that the necessary checks might be expensive,\nand feeling that it'd be unfortunate to add expensive checks for\nissues that no one had ever triggered in 15.5 years, and which I only\ndiscovered due to intentionally trying to create a broken tree but\naccidentally creating the wrong type of broken tree.\n\nAs it was, the new merge backend took a few years of work, and I\nprobably followed too many tangents along the way. This particular\nissue was a case where it clearly didn't touch code I was modifying\n(the merge or diff machinery) and instead triggered in unpack-trees.c\nand cache-tree.c. So, I decided to simply document it in case others\nwanted to investigate.\n\nLong story short, I can't comment about the difficulty of detecting\nand working around these. If you've read this email and the commit\nmessage I wrote at the time, then you know everything I remember about\nthe issue.\n"}]}