{"thread":{"id":"62734","subject":"[BUGREPORT] git diff-tree --cc SEGFAUTs","startedAt":"2025-01-03T19:28:59Z","lastAt":"2025-01-18T00:33:55Z","messageCount":38,"participants":["Wink Saville","Jeff King","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"509885","messageId":"CAKk8isqpAXLoiXxOP3uAc00M+OM0FaU3Uhnt5R1FnFMD=xGARg@mail.gmail.com","threadId":"62734","inReplyTo":null,"subject":"[BUGREPORT] git diff-tree --cc SEGFAUTs","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2025-01-03T19:28:47Z","receivedAt":"2025-01-03T19:28:59Z","isPatch":false,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"`git diff-tree --cc` SEGFAUTs after adding trace_printf to diff_tree_combined.\n\nDetails in attached git-bugreport.\n\n\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\nI'm learning some inner workings of git so I've added `trace_printf` statements\nto the `diff_tree_combined` function and others. While running ./git I ran into a\nproblem where `git diff-tree --cc` fails with a SEGFAULT.\n\nThis bug report is a \"minimal\" change to `diff_tree_combined` of the `git@github.com:git/git`\nrepo that reproduces the problem. The change adds an additional for loop inside the loop that counts\nthe number of \"surving paths\" and it prints the `struct combined_diff_path` fields so I can see\nthe list of paths that make up merge commits with changes.\n\nBelow is the diff which is applied to the `next` branch, it is also available on my fork on github:\n https://github.com/winksaville/git/tree/wink-segfault-with-minimal-changes\n```\nwink@fwlaptop 25-01-03T18:43:04.330Z:~/prgs/forks/git (wink-segfault-with-minimal-changes)\n$ git --no-pager diff next\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 641bc92dbd..455bc19087 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -20,6 +20,8 @@\n #include \"oid-array.h\"\n #include \"revision.h\"\n \n+#include \"trace.h\"\n+\n static int compare_paths(const struct combine_diff_path *one,\n \t\t\t const struct diff_filespec *two)\n {\n@@ -1595,8 +1597,16 @@ void diff_tree_combined(const struct object_id *oid,\n \t}\n \n \t/* find out number of surviving paths */\n-\tfor (num_paths = 0, p = paths; p; p = p->next)\n+\ttrace_printf(\"Wink diff_tree_combined: find number of surviving paths num_parent=%d\\n\", num_parent);\n+\tfor (num_paths = 0, p = paths; p; p = p->next) {\n+\t\ttrace_printf(\"Wink diff_tree_combined: num_paths=%d &p=%p mode=%0x, oid=%s path=%s\\n\", num_paths, p, p->mode, oid_to_hex(&p->oid), p->path);\n+\t\tfor (i = 0; i < num_parent; i++) {\n+\t\t\ttrace_printf(\"Wink diff_tree_combined: &p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents path.buf=%s\\n\",\n+\t\t\t\t i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), p->parent[i].path.buf, p->parent[i].path.buf);\n+\t\t}\n \t\tnum_paths++;\n+\t}\n+\ttrace_printf(\"Wink diff_tree_combined: found %d surviving paths\\n\", num_paths);\n \n \t/* order paths according to diffcore_order */\n \tif (opt->orderfile && num_paths) {\nwink@fwlaptop 25-01-03T18:43:18.695Z:~/prgs/forks/git (wink-segfault-with-minimal-changes)\n```\n\nI made those changes on the `next` branch of git/git repo as of yesterday, 1/2/25,\nhere are the the relavant logs:\n```\nwink@fwlaptop 25-01-03T18:43:18.695Z:~/prgs/forks/git (wink-segfault-with-minimal-changes)\n$ git log -2 --oneline\nedf34e3ab4 (HEAD -> wink-segfault-with-minimal-changes, origin/wink-segfault-with-minimal-changes) bug: SEGFAULT with minimal changes:\n6c04ab211c (upstream/next, origin/next, next) Sync with 'master'\nwink@fwlaptop 25-01-03T18:44:13.976Z:~/prgs/forks/git (wink-segfault-with-minimal-changes)\n```\n\nWhat did you expect to happen? (Expected behavior)\n\nWhen I use `git diff-tree --cc` on a merge commit with changes I expect to see\nmy trace_printf statements print the `struct combined_diff_path` members and\nalso the output diff-tree -cc results with the \"combined changes\" as can be\nseen below:\n```\nwink@fwlaptop 25-01-03T18:46:58.472Z:~/prgs/forks/git (wink-segfault-with-minimal-changes)\n$ GIT_TRACE=1 ./git diff-tree --cc 6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a\n10:47:04.259504 git.c:476 trace: built-in: git diff-tree --cc 6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a\n6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a\n10:47:04.278220 combine-diff.c:1600 Wink diff_tree_combined: find number of surviving paths num_parent=2\n10:47:04.278245 combine-diff.c:1602 Wink diff_tree_combined: num_paths=0 &p=0x5dd590883f60 mode=81a4, oid=0f41b2fd4a6b679a1cfcaa9a584c382068146212 path=refs.c\n10:47:04.278253 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[0]=0x5dd590883f98 status=M mode=81a4 oid=7dd5e9fa3323111f06303674b213ae24ed2d04b6 path.buf=(nil) contents path.buf=(null)\n10:47:04.278260 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[1]=0x5dd590883fe0 status=M mode=81a4 oid=c55583986940d8ef1e1c839364c03cd92d4f7114 path.buf=(nil) contents path.buf=(null)\n10:47:04.278265 combine-diff.c:1602 Wink diff_tree_combined: num_paths=1 &p=0x5dd59088b450 mode=81a4, oid=a0cdd99250e8286b55808b697b0a94afac5d8319 path=refs.h\n10:47:04.278270 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[0]=0x5dd59088b488 status=M mode=81a4 oid=09be47afbee51e99f4ae49588cd65596ccfcb07e path.buf=(nil) contents path.buf=(null)\n10:47:04.278274 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[1]=0x5dd59088b4d0 status=M mode=81a4 oid=b0dfc65ed2e59c4b66967840339f81e7746a96d3 path.buf=(nil) contents path.buf=(null)\n10:47:04.278278 combine-diff.c:1602 Wink diff_tree_combined: num_paths=2 &p=0x5dd59088b530 mode=81a4, oid=5cfb8b7ca8678e171b8e8a7ad6daf1af74a81b59 path=refs/files-backend.c\n10:47:04.278283 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[0]=0x5dd59088b568 status=M mode=81a4 oid=467fe347fa7e7d82ed7a2836e43ea749bb90ad7d path.buf=(nil) contents path.buf=(null)\n10:47:04.278287 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[1]=0x5dd59088b5b0 status=M mode=81a4 oid=8953d1c6d37b13b0db701888b3db92fd87a68aaa path.buf=(nil) contents path.buf=(null)\n10:47:04.278291 combine-diff.c:1602 Wink diff_tree_combined: num_paths=3 &p=0x5dd59088b620 mode=81a4, oid=16550862d3ebe3b357c52254088b143c7ba000d6 path=refs/refs-internal.h\n10:47:04.278296 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[0]=0x5dd59088b658 status=M mode=81a4 oid=66e66e0fc1e812ebebd1d4b0119899c84bf1c0ae path.buf=(nil) contents path.buf=(null)\n10:47:04.278300 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[1]=0x5dd59088b6a0 status=M mode=81a4 oid=79b287c5ec5c7d8f759869cf93cda405640186dc path.buf=(nil) contents path.buf=(null)\n10:47:04.278305 combine-diff.c:1602 Wink diff_tree_combined: num_paths=4 &p=0x5dd59088b710 mode=81a4, oid=00d95a9a2f42ce74c5cb4a42175b0953287851a6 path=refs/reftable-backend.c\n10:47:04.278309 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[0]=0x5dd59088b748 status=M mode=81a4 oid=8a2a5b847c3d86332e319da69bfb5c8a56a10e86 path.buf=(nil) contents path.buf=(null)\n10:47:04.278314 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[1]=0x5dd59088b790 status=M mode=81a4 oid=bec5962debea7b62572d08f6fa8fd38ab4cd8af6 path.buf=(nil) contents path.buf=(null)\n10:47:04.278318 combine-diff.c:1609 Wink diff_tree_combined: found 5 surviving paths\ndiff --cc refs/files-backend.c\nindex 467fe347fa,8953d1c6d3..5cfb8b7ca8\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@@ -2533,9 -2539,15 +2543,15 @@@ static int check_old_oid(struct ref_upd\n \t\t\t oid_to_hex(oid),\n \t\t\t oid_to_hex(&update->old_oid));\n \n -\treturn -1;\n +\treturn ret;\n }\n \n+ struct files_transaction_backend_data {\n+ \tstruct ref_transaction *packed_transaction;\n+ \tint packed_refs_locked;\n+ \tstruct strmap ref_locks;\n+ };\n+ \n /*\n * Prepare for carrying out update:\n * - Lock the reference referred to by update.\nwink@fwlaptop 25-01-03T18:47:04.295Z:~/prgs/forks/git (wink-segfault-with-minimal-changes)\n```\n\nThe above works because it has a hack fix I introduced and thus it works. The hack is to always\nuse `find_paths_generic` rather than `find_paths_multitree`. To do this I added\n`need_generic_pathscan = true;` on top of the above change to have it run properly:\n```\nwink@fwlaptop 25-01-03T18:47:04.295Z:~/prgs/forks/git (wink-segfault-with-minimal-changes)\n$ git diff\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 455bc19087..f03ff6f820 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1563,7 +1563,7 @@ void diff_tree_combined(const struct object_id *oid,\n (opt->pickaxe_opts &\n (DIFF_PICKAXE_KINDS_MASK & ~DIFF_PICKAXE_KIND_OBJFIND)) ||\n opt->filter;\n-\n+ need_generic_pathscan = true;\n if (need_generic_pathscan) {\n /*\n * NOTE generic case also handles --stat, as it computes\nwink@fwlaptop 25-01-03T18:49:56.900Z:~/prgs/forks/git (wink-segfault-with-minimal-changes)\n```\n\n\nWhat happened instead? (Actual behavior)\n\nIf I don't use my hack a SEGFAULT occurs:\n```\nwink@fwlaptop 25-01-03T18:51:32.242Z:~/prgs/forks/git (wink-segfault-with-minimal-changes)\n$ GIT_TRACE=1 ./git diff-tree --cc 6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a\n10:51:37.211992 git.c:476 trace: built-in: git diff-tree --cc 6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a\n6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a\n10:51:37.216500 combine-diff.c:1600 Wink diff_tree_combined: find number of surviving paths num_parent=2\n10:51:37.216516 combine-diff.c:1602 Wink diff_tree_combined: num_paths=0 &p=0x6325add91f60 mode=81a4, oid=0f41b2fd4a6b679a1cfcaa9a584c382068146212 path=refs.c\n10:51:37.216524 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[0]=0x6325add91f98 status=M mode=81a4 oid=7dd5e9fa3323111f06303674b213ae24ed2d04b6 path.buf=(nil) contents path.buf=(null)\n10:51:37.216530 combine-diff.c:1604 Wink diff_tree_combined: &p->parent[1]=0x6325add91fe0 status=M mode=81a4 oid=c55583986940d8ef1e1c839364c03cd92d4f7114 path.buf=0x632588e43a00 contents path.buf=�C׭%c\n10:51:37.216536 combine-diff.c:1602 Wink diff_tree_combined: num_paths=1 &p=0x6325add990c0 mode=81a4, oid=a0cdd99250e8286b55808b697b0a94afac5d8319 path=refs.h\nSegmentation fault (core dumped)\nwink@fwlaptop 25-01-03T18:51:37.350Z:~/prgs/forks/git (wink-segfault-with-minimal-changes)\n```\n\nWhat's different between what you expected and what actually happened?\n\nThere should be no SEGFAULT\n\nAnything else you want to add:\n\nI have a guess on what the problem might be; that `find_paths_multitree` is not properly\ninitializing path.buf. I determined this because, in my limited testing, if I always use\n`find_paths_generic` we see that all the pointers are NULL and we don't SEGFAULT.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.48.0.rc1.242.gedf34e3ab4 \ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nlibcurl: 8.11.0\nOpenSSL: OpenSSL 3.4.0 22 Oct 2024\nzlib: 1.3.1\nuname: Linux 6.12.6-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 19 Dec 2024 21:29:01 +0000 x86_64\ncompiler info: gnuc: 14.2\nlibc info: glibc: 2.40\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n"},{"id":"509893","messageId":"20250103204624.GE3212696@coredump.intra.peff.net","threadId":"62734","inReplyTo":"CAKk8isqpAXLoiXxOP3uAc00M+OM0FaU3Uhnt5R1FnFMD=xGARg@mail.gmail.com","subject":"Re: [BUGREPORT] git diff-tree --cc SEGFAUTs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-03T20:46:24Z","receivedAt":"2025-01-03T20:46:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 03, 2025 at 11:28:47AM -0800, Wink Saville wrote:\n\n> `git diff-tree --cc` SEGFAUTs after adding trace_printf to diff_tree_combined.\n\nHmm, is it really a bug in Git if you had to add new code which contains\nthe bug? :)\n\n> @@ -1595,8 +1597,16 @@ void diff_tree_combined(const struct object_id *oid,\n> \t}\n> \n> \t/* find out number of surviving paths */\n> -\tfor (num_paths = 0, p = paths; p; p = p->next)\n> +\ttrace_printf(\"Wink diff_tree_combined: find number of surviving paths num_parent=%d\\n\", num_parent);\n> +\tfor (num_paths = 0, p = paths; p; p = p->next) {\n> +\t\ttrace_printf(\"Wink diff_tree_combined: num_paths=%d &p=%p mode=%0x, oid=%s path=%s\\n\", num_paths, p, p->mode, oid_to_hex(&p->oid), p->path);\n> +\t\tfor (i = 0; i < num_parent; i++) {\n> +\t\t\ttrace_printf(\"Wink diff_tree_combined: &p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents path.buf=%s\\n\",\n> +\t\t\t\t i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), p->parent[i].path.buf, p->parent[i].path.buf);\n> +\t\t}\n\nThe parent \"path\" strbufs are only initialized in intersect_paths() if\ncombined_all_paths is set, and if there was an actual path change (a\ncopy or rename).\n\nSo you'd probably need something like this:\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 455bc19087..1e58809c4e 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1601,8 +1601,11 @@ void diff_tree_combined(const struct object_id *oid,\n \tfor (num_paths = 0, p = paths; p; p = p->next) {\n \t\ttrace_printf(\"Wink diff_tree_combined: num_paths=%d &p=%p mode=%0x, oid=%s path=%s\\n\", num_paths, p, p->mode, oid_to_hex(&p->oid), p->path);\n \t\tfor (i = 0; i < num_parent; i++) {\n+\t\t\tconst char *path = rev->combine_all_paths &&\n+\t\t\t\t\t filename_changed(p->parent[i].status) ?\n+\t\t\t\t\t p->parent[i].path.buf : NULL;\n \t\t\ttrace_printf(\"Wink diff_tree_combined: &p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents path.buf=%s\\n\",\n-\t\t\t\t i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), p->parent[i].path.buf, p->parent[i].path.buf);\n+\t\t\t\t i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), path, path);\n \t\t}\n \t\tnum_paths++;\n \t}\n\n-Peff\n"},{"id":"509897","messageId":"CAKk8isrz1NQ=3=2aZ3tANymo0eSsCy=r6W5yKgn6gxmOom54CA@mail.gmail.com","threadId":"62734","inReplyTo":"20250103204624.GE3212696@coredump.intra.peff.net","subject":"Re: [BUGREPORT] git diff-tree --cc SEGFAUTs","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2025-01-03T23:34:58Z","receivedAt":"2025-01-03T23:35:10Z","isPatch":false,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"On Fri, Jan 3, 2025 at 12:46 PM Jeff King wrote:\n>\n> On Fri, Jan 03, 2025 at 11:28:47AM -0800, Wink Saville wrote:\n>\n> > `git diff-tree --cc` SEGFAUTs after adding trace_printf to diff_tree_combined.\n>\n> Hmm, is it really a bug in Git if you had to add new code which contains\n> the bug? :)\n>\n> > @@ -1595,8 +1597,16 @@ void diff_tree_combined(const struct object_id *oid,\n> > }\n> >\n> > /* find out number of surviving paths */\n> > - for (num_paths = 0, p = paths; p; p = p->next)\n> > + trace_printf(\"Wink diff_tree_combined: find number of surviving paths num_parent=%d\\n\", num_parent);\n> > + for (num_paths = 0, p = paths; p; p = p->next) {\n> > + trace_printf(\"Wink diff_tree_combined: num_paths=%d &p=%p mode=%0x, oid=%s path=%s\\n\", num_paths, p, p->mode, oid_to_hex(&p->oid), p->path);\n> > + for (i = 0; i < num_parent; i++) {\n> > + trace_printf(\"Wink diff_tree_combined: &p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents path.buf=%s\\n\",\n> > + i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), p->parent[i].path.buf, p->parent[i].path.buf);\n> > + }\n>\n> The parent \"path\" strbufs are only initialized in intersect_paths() if\n> combined_all_paths is set, and if there was an actual path change (a\n> copy or rename).\n>\n> So you'd probably need something like this:\n>\n> diff --git a/combine-diff.c b/combine-diff.c\n> index 455bc19087..1e58809c4e 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -1601,8 +1601,11 @@ void diff_tree_combined(const struct object_id *oid,\n> for (num_paths = 0, p = paths; p; p = p->next) {\n> trace_printf(\"Wink diff_tree_combined: num_paths=%d &p=%p mode=%0x, oid=%s path=%s\\n\", num_paths, p, p->mode, oid_to_hex(&p->oid), p->path);\n> for (i = 0; i < num_parent; i++) {\n> + const char *path = rev->combine_all_paths &&\n> + filename_changed(p->parent[i].status) ?\n> + p->parent[i].path.buf : NULL;\n> trace_printf(\"Wink diff_tree_combined: &p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents path.buf=%s\\n\",\n> - i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), p->parent[i].path.buf, p->parent[i].path.buf);\n> + i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), path, path);\n> }\n> num_paths++;\n> }\n>\n> -Peff\n\nTYVM!\n\nThat worked but changed the name and fixed a typo in `combined_all_paths`:\n```\nwink@3900x 25-01-03T23:06:08.344Z:~/data/prgs/forks/git\n(wink-segfault-with-minimal-changes)\n$ git diff\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 455bc19087..70394c3350 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1601,8 +1601,9 @@ void diff_tree_combined(const struct object_id *oid,\n for (num_paths = 0, p = paths; p; p = p->next) {\n trace_printf(\"Wink diff_tree_combined: num_paths=%d\n&p=%p mode=%0x, oid=%s path=%s\\n\", num_paths, p, p->mode,\noid_to_hex(&p->oid), p->path);\n for (i = 0; i < num_parent; i++) {\n+ const char *parent_path =\nrev->combined_all_paths && filename_changed(p->parent[i].status) ?\np->parent[i].path.buf : NULL;\n trace_printf(\"Wink diff_tree_combined:\n&p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents\npath.buf=%s\\n\",\n- i, &p->parent[i],\np->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid),\np->parent[i].path.buf, p->parent[i].path.buf);\n+ i, &p->parent[i],\np->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid),\nparent_path, parent_path);\n }\n num_paths++;\n }\n```\n\nBut having to protect yourself is unobvious and especially if it isn't necessary\nwhen using the `fetch_paths_generic`.\n\nIn addition, from strbuf.h `buf` is never NULL:\n\n\"\n* strbufs have some invariants that are very important to keep in mind:\n *\n * - The `buf` member is never NULL, so it can be used in any usual C\n * string operations safely. strbufs _have_ to be initialized either by\n * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n *\n\"\n\nSo I'd say this could be considered a bug in git at least in how\ncombine_diff_path\nis being managed. I assume you agree that neither find_paths_generic or\nfind_paths_multitree are adhering to at least that strbuf invariant and I wonder\nif the other strbuf invariants are being upheld.\n\nSo, should this bug be \"closed\" and a new one \"created\"?\n\nActually, using the mailing list to identify bugs and initially discuss\nthem, seems fine. But is there a place where there is a list of current bugs and\ntheir state?\n\n-- wink\n"},{"id":"509902","messageId":"20250104003154.GB3244554@coredump.intra.peff.net","threadId":"62734","inReplyTo":"CAKk8isrz1NQ=3=2aZ3tANymo0eSsCy=r6W5yKgn6gxmOom54CA@mail.gmail.com","subject":"Re: [BUGREPORT] git diff-tree --cc SEGFAUTs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-04T00:31:54Z","receivedAt":"2025-01-04T00:31:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 03, 2025 at 03:34:58PM -0800, Wink Saville wrote:\n\n> But having to protect yourself is unobvious and especially if it isn't necessary\n> when using the `fetch_paths_generic`.\n> \n> In addition, from strbuf.h `buf` is never NULL:\n> \n> \"\n> * strbufs have some invariants that are very important to keep in mind:\n> *\n> * - The `buf` member is never NULL, so it can be used in any usual C\n> * string operations safely. strbufs _have_ to be initialized either by\n> * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n> *\n> \"\n> \n> So I'd say this could be considered a bug in git at least in how\n> combine_diff_path\n> is being managed. I assume you agree that neither find_paths_generic or\n> find_paths_multitree are adhering to at least that strbuf invariant and I wonder\n> if the other strbuf invariants are being upheld.\n\nThe strbuf invariant can only be held on strbufs which have been\ninitialized, and this one has not. I don't think it's wrong to have\nvariables which not (yet) been initialized. It can make for a fragile\ninterface, though, if uninitialized struct members are exposed widely.\n\nI'm not sure how wide this case is. It's mostly an internal combine-diff\ndata structure, though it looks like it gets exposed to other code in a\nfew spots (though nobody outside of combine-diff.c currently looks at\nthe parent paths at all).\n\nSo I wouldn't call it a bug, as the internals of Git are not part of the\npublic interface and there is no user-visible behavior problem without\npatching. But I doubt anybody would object to a patch making the API\nless fragile if it can be done cheaply and easily. And strbufs are\ndesigned to be cheap to initialize. So something like (completely\nuntested):\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 641bc92dbd..452b5f5beb 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -67,9 +67,9 @@ static struct combine_diff_path *intersect_paths(\n \t\t\tp->parent[n].mode = q->queue[i]->one->mode;\n \t\t\tp->parent[n].status = q->queue[i]->status;\n \n+\t\t\tstrbuf_init(&p->parent[n].path, 0);\n \t\t\tif (combined_all_paths &&\n \t\t\t filename_changed(p->parent[n].status)) {\n-\t\t\t\tstrbuf_init(&p->parent[n].path, 0);\n \t\t\t\tstrbuf_addstr(&p->parent[n].path,\n \t\t\t\t\t q->queue[i]->one->path);\n \t\t\t}\n@@ -92,9 +92,7 @@ static struct combine_diff_path *intersect_paths(\n \t\t\t/* p->path not in q->queue[]; drop it */\n \t\t\t*tail = p->next;\n \t\t\tfor (j = 0; j < num_parent; j++)\n-\t\t\t\tif (combined_all_paths &&\n-\t\t\t\t filename_changed(p->parent[j].status))\n-\t\t\t\t\tstrbuf_release(&p->parent[j].path);\n+\t\t\t\tstrbuf_release(&p->parent[j].path);\n \t\t\tfree(p);\n \t\t\tcontinue;\n \t\t}\n@@ -1645,9 +1643,7 @@ void diff_tree_combined(const struct object_id *oid,\n \t\tstruct combine_diff_path *tmp = paths;\n \t\tpaths = paths->next;\n \t\tfor (i = 0; i < num_parent; i++)\n-\t\t\tif (rev->combined_all_paths &&\n-\t\t\t filename_changed(tmp->parent[i].status))\n-\t\t\t\tstrbuf_release(&tmp->parent[i].path);\n+\t\t\tstrbuf_release(&tmp->parent[i].path);\n \t\tfree(tmp);\n \t}\n \n\nmight help the uninitialized-pointer issue. OTOH it is not really\nsolving the more fundamental problem, which is that p->parent[i].path is\nonly sometimes useful (we do not fill it in if it would just be the same\nas p->path, so the patch only changes it from uninitialized memory into\nan empty strbuf).\n\nAnd that is probably not something we want to change, as allocating\nduplicates of each path may be expensive. Probably we'd be better to\nencapsulate it in a function which falls back to p->path automatically.\nBut then, AFAICT there are only two sites (both inside combine-diff.c)\nwhich look at it, so it would mostly be hypothetical future-proofing. I\ndunno.\n\n> So, should this bug be \"closed\" and a new one \"created\"?\n> \n> Actually, using the mailing list to identify bugs and initially discuss\n> them, seems fine. But is there a place where there is a list of current bugs and\n> their state?\n\nNo, there's no bug tracker for the project[1]. Discussion may lead to a\npatch or not, which may be applied or not, but there is no formal\nclassification of \"open\" or \"fixed\" or \"won't fix\".\n\n-Peff\n\n[1] Some folks seem to be using:\n\n https://git.issues.gerritcodereview.com/issues?q=status:open\n\n but I don't know how active it is. I never use it and had to dig out\n the link to https://crbug.com, which now redirects there, from a\n message from 2017. There's some recent activity, but I wouldn't\n count on opening something there to get wide attention.\n"},{"id":"509904","messageId":"xmqq4j2fnv8p.fsf@gitster.g","threadId":"62734","inReplyTo":"20250104003154.GB3244554@coredump.intra.peff.net","subject":"Re: [BUGREPORT] git diff-tree --cc SEGFAUTs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-04T02:55:18Z","receivedAt":"2025-01-04T02:55:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... OTOH it is not really\n> solving the more fundamental problem, which is that p->parent[i].path is\n> only sometimes useful (we do not fill it in if it would just be the same\n> as p->path, so the patch only changes it from uninitialized memory into\n> an empty strbuf).\n>\n> And that is probably not something we want to change, as allocating\n> duplicates of each path may be expensive.\n\nNicely said. I reached the same conclusion after looking at the\nexisting code, even though I have to admit that I am not a huge fan\nof the more recent part of combine-diff.c and its data structures.\n\nThanks.\n"},{"id":"509905","messageId":"20250104033210.GA892381@coredump.intra.peff.net","threadId":"62734","inReplyTo":"xmqq4j2fnv8p.fsf@gitster.g","subject":"Re: [BUGREPORT] git diff-tree --cc SEGFAUTs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-04T03:32:10Z","receivedAt":"2025-01-04T03:32:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 03, 2025 at 06:55:18PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... OTOH it is not really\n> > solving the more fundamental problem, which is that p->parent[i].path is\n> > only sometimes useful (we do not fill it in if it would just be the same\n> > as p->path, so the patch only changes it from uninitialized memory into\n> > an empty strbuf).\n> >\n> > And that is probably not something we want to change, as allocating\n> > duplicates of each path may be expensive.\n> \n> Nicely said. I reached the same conclusion after looking at the\n> existing code, even though I have to admit that I am not a huge fan\n> of the more recent part of combine-diff.c and its data structures.\n\nI poked at this a little bit more, so here are a few tidbits:\n\n - the patch I showed earlier is not sufficient! There are lots of\n other spots that create combine_diff_path structs but don't bother\n to put anything in the parent paths at all. It works now because\n they also don't set a status that triggers filename_changed(). But\n what I showed earlier was wrong, because it was assuming in the\n cleanup functions that the strbufs were always initialized.\n\n - there's really no need for a strbuf at all here. It is always\n uninitialized/empty, or contains a direct copy of a path string. So\n a raw pointer with xstrdup() is plenty. And then we can use NULL to\n mean \"it was not set\".\n\n Which would Just Work for all those other spots if they bothered to\n zero the memory they allocated, but they don't. So we have to update\n them to set it to NULL anyway. That patch is below.\n\n - it is not at all clear to me that we need to be allocating at all.\n We always copy a string from the diff_queue. Do our\n combine_diff_path structs persist beyond then? I'm not sure. It is\n probably asking for trouble to just point to them directly without\n copying, as it creates a dependency (that even if it is not needed\n now, is a trap for somebody later). But it would drop some\n allocation/cleanup code, and we could just have p->parent[i].path\n fall back to p->path naturally.\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 641bc92dbd..0d9d344c4e 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -66,13 +66,9 @@ static struct combine_diff_path *intersect_paths(\n \t\t\toidcpy(&p->parent[n].oid, &q->queue[i]->one->oid);\n \t\t\tp->parent[n].mode = q->queue[i]->one->mode;\n \t\t\tp->parent[n].status = q->queue[i]->status;\n-\n-\t\t\tif (combined_all_paths &&\n-\t\t\t filename_changed(p->parent[n].status)) {\n-\t\t\t\tstrbuf_init(&p->parent[n].path, 0);\n-\t\t\t\tstrbuf_addstr(&p->parent[n].path,\n-\t\t\t\t\t q->queue[i]->one->path);\n-\t\t\t}\n+\t\t\tp->parent[n].path = combined_all_paths &&\n+\t\t\t\t\t filename_changed(p->parent[n].status) ?\n+\t\t\t\t\t xstrdup(q->queue[i]->one->path) : NULL;\n \t\t\t*tail = p;\n \t\t\ttail = &p->next;\n \t\t}\n@@ -92,9 +88,7 @@ static struct combine_diff_path *intersect_paths(\n \t\t\t/* p->path not in q->queue[]; drop it */\n \t\t\t*tail = p->next;\n \t\t\tfor (j = 0; j < num_parent; j++)\n-\t\t\t\tif (combined_all_paths &&\n-\t\t\t\t filename_changed(p->parent[j].status))\n-\t\t\t\t\tstrbuf_release(&p->parent[j].path);\n+\t\t\t\tfree(p->parent[j].path);\n \t\t\tfree(p);\n \t\t\tcontinue;\n \t\t}\n@@ -108,10 +102,9 @@ static struct combine_diff_path *intersect_paths(\n \t\toidcpy(&p->parent[n].oid, &q->queue[i]->one->oid);\n \t\tp->parent[n].mode = q->queue[i]->one->mode;\n \t\tp->parent[n].status = q->queue[i]->status;\n-\t\tif (combined_all_paths &&\n-\t\t filename_changed(p->parent[n].status))\n-\t\t\tstrbuf_addstr(&p->parent[n].path,\n-\t\t\t\t q->queue[i]->one->path);\n+\t\tp->parent[n].path = combined_all_paths &&\n+\t\t\t\t filename_changed(p->parent[n].status) ?\n+\t\t\t\t xstrdup(q->queue[i]->one->path) : NULL;\n \n \t\ttail = &p->next;\n \t\ti++;\n@@ -996,8 +989,9 @@ static void show_combined_header(struct combine_diff_path *elem,\n \n \tif (rev->combined_all_paths) {\n \t\tfor (i = 0; i < num_parent; i++) {\n-\t\t\tchar *path = filename_changed(elem->parent[i].status)\n-\t\t\t\t? elem->parent[i].path.buf : elem->path;\n+\t\t\tconst char *path = elem->parent[i].path ?\n+\t\t\t\t\t elem->parent[i].path :\n+\t\t\t\t\t elem->path;\n \t\t\tif (elem->parent[i].status == DIFF_STATUS_ADDED)\n \t\t\t\tdump_quoted_path(\"--- \", \"\", \"/dev/null\",\n \t\t\t\t\t\t line_prefix, c_meta, c_reset);\n@@ -1278,12 +1272,10 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \n \tfor (i = 0; i < num_parent; i++)\n \t\tif (rev->combined_all_paths) {\n-\t\t\tif (filename_changed(p->parent[i].status))\n-\t\t\t\twrite_name_quoted(p->parent[i].path.buf, stdout,\n-\t\t\t\t\t\t inter_name_termination);\n-\t\t\telse\n-\t\t\t\twrite_name_quoted(p->path, stdout,\n-\t\t\t\t\t\t inter_name_termination);\n+\t\t\tconst char *path = p->parent[i].path ?\n+\t\t\t\t\t p->parent[i].path :\n+\t\t\t\t\t p->path;\n+\t\t\twrite_name_quoted(path, stdout, inter_name_termination);\n \t\t}\n \twrite_name_quoted(p->path, stdout, line_termination);\n }\n@@ -1645,9 +1637,7 @@ void diff_tree_combined(const struct object_id *oid,\n \t\tstruct combine_diff_path *tmp = paths;\n \t\tpaths = paths->next;\n \t\tfor (i = 0; i < num_parent; i++)\n-\t\t\tif (rev->combined_all_paths &&\n-\t\t\t filename_changed(tmp->parent[i].status))\n-\t\t\t\tstrbuf_release(&tmp->parent[i].path);\n+\t\t\tfree(tmp->parent[i].path);\n \t\tfree(tmp);\n \t}\n \ndiff --git a/diff-lib.c b/diff-lib.c\nindex c6d3bc4d37..88a5aed736 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -417,9 +417,11 @@ static int show_modified(struct rev_info *revs,\n \t\tmemset(p->parent, 0, 2 * sizeof(struct combine_diff_parent));\n \t\tp->parent[0].status = DIFF_STATUS_MODIFIED;\n \t\tp->parent[0].mode = new_entry->ce_mode;\n+\t\tp->parent[0].path = NULL;\n \t\toidcpy(&p->parent[0].oid, &new_entry->oid);\n \t\tp->parent[1].status = DIFF_STATUS_MODIFIED;\n \t\tp->parent[1].mode = old_entry->ce_mode;\n+\t\tp->parent[1].path = NULL;\n \t\toidcpy(&p->parent[1].oid, &old_entry->oid);\n \t\tshow_combined_diff(p, 2, revs);\n \t\tfree(p);\ndiff --git a/diff.h b/diff.h\nindex 6e6007c17b..3157faeabb 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -480,7 +480,7 @@ struct combine_diff_path {\n \t\tchar status;\n \t\tunsigned int mode;\n \t\tstruct object_id oid;\n-\t\tstruct strbuf path;\n+\t\tchar *path;\n \t} parent[FLEX_ARRAY];\n };\n #define combine_diff_path_size(n, l) \\\ndiff --git a/tree-diff.c b/tree-diff.c\nindex d9237ffd9b..57af377c2b 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -272,6 +272,7 @@ static struct combine_diff_path *emit_path(struct combine_diff_path *p,\n \t\t\t}\n \n \t\t\tp->parent[i].mode = mode_i;\n+\t\t\tp->parent[i].path = NULL;\n \t\t\toidcpy(&p->parent[i].oid, oid_i);\n \t\t}\n \n"},{"id":"509917","messageId":"CAKk8isrRCZsrt=2YB+L3EjK3ZAYbSk0e+1YZexqZUhB78L36dg@mail.gmail.com","threadId":"62734","inReplyTo":"20250104033210.GA892381@coredump.intra.peff.net","subject":"Re: [BUGREPORT] git diff-tree --cc SEGFAUTs","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2025-01-04T18:09:51Z","receivedAt":"2025-01-04T18:10:03Z","isPatch":false,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"On Fri, Jan 3, 2025 at 7:32 PM Jeff King wrote:\n>\n> On Fri, Jan 03, 2025 at 06:55:18PM -0800, Junio C Hamano wrote:\n>\n> > Jeff King writes:\n> >\n> > > ... OTOH it is not really\n> > > solving the more fundamental problem, which is that p->parent[i].path is\n> > > only sometimes useful (we do not fill it in if it would just be the same\n> > > as p->path, so the patch only changes it from uninitialized memory into\n> > > an empty strbuf).\n> > >\n> > > And that is probably not something we want to change, as allocating\n> > > duplicates of each path may be expensive.\n> >\n> > Nicely said. I reached the same conclusion after looking at the\n> > existing code, even though I have to admit that I am not a huge fan\n> > of the more recent part of combine-diff.c and its data structures.\n>\n> I poked at this a little bit more, so here are a few tidbits:\n>\n> - the patch I showed earlier is not sufficient! There are lots of\n> other spots that create combine_diff_path structs but don't bother\n> to put anything in the parent paths at all. It works now because\n> they also don't set a status that triggers filename_changed(). But\n> what I showed earlier was wrong, because it was assuming in the\n> cleanup functions that the strbufs were always initialized.\n>\n> - there's really no need for a strbuf at all here. It is always\n> uninitialized/empty, or contains a direct copy of a path string. So\n> a raw pointer with xstrdup() is plenty. And then we can use NULL to\n> mean \"it was not set\".\n>\n> Which would Just Work for all those other spots if they bothered to\n> zero the memory they allocated, but they don't. So we have to update\n> them to set it to NULL anyway. That patch is below.\n>\n> - it is not at all clear to me that we need to be allocating at all.\n> We always copy a string from the diff_queue. Do our\n> combine_diff_path structs persist beyond then? I'm not sure. It is\n> probably asking for trouble to just point to them directly without\n> copying, as it creates a dependency (that even if it is not needed\n> now, is a trap for somebody later). But it would drop some\n> allocation/cleanup code, and we could just have p->parent[i].path\n> fall back to p->path naturally.\n>\n> diff --git a/combine-diff.c b/combine-diff.c\n> index 641bc92dbd..0d9d344c4e 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -66,13 +66,9 @@ static struct combine_diff_path *intersect_paths(\n> oidcpy(&p->parent[n].oid, &q->queue[i]->one->oid);\n> p->parent[n].mode = q->queue[i]->one->mode;\n> p->parent[n].status = q->queue[i]->status;\n> -\n> - if (combined_all_paths &&\n> - filename_changed(p->parent[n].status)) {\n> - strbuf_init(&p->parent[n].path, 0);\n> - strbuf_addstr(&p->parent[n].path,\n> - q->queue[i]->one->path);\n> - }\n> + p->parent[n].path = combined_all_paths &&\n> + filename_changed(p->parent[n].status) ?\n> + xstrdup(q->queue[i]->one->path) : NULL;\n> *tail = p;\n> tail = &p->next;\n> }\n> @@ -92,9 +88,7 @@ static struct combine_diff_path *intersect_paths(\n> /* p->path not in q->queue[]; drop it */\n> *tail = p->next;\n> for (j = 0; j < num_parent; j++)\n> - if (combined_all_paths &&\n> - filename_changed(p->parent[j].status))\n> - strbuf_release(&p->parent[j].path);\n> + free(p->parent[j].path);\n> free(p);\n> continue;\n> }\n> @@ -108,10 +102,9 @@ static struct combine_diff_path *intersect_paths(\n> oidcpy(&p->parent[n].oid, &q->queue[i]->one->oid);\n> p->parent[n].mode = q->queue[i]->one->mode;\n> p->parent[n].status = q->queue[i]->status;\n> - if (combined_all_paths &&\n> - filename_changed(p->parent[n].status))\n> - strbuf_addstr(&p->parent[n].path,\n> - q->queue[i]->one->path);\n> + p->parent[n].path = combined_all_paths &&\n> + filename_changed(p->parent[n].status) ?\n> + xstrdup(q->queue[i]->one->path) : NULL;\n>\n> tail = &p->next;\n> i++;\n> @@ -996,8 +989,9 @@ static void show_combined_header(struct combine_diff_path *elem,\n>\n> if (rev->combined_all_paths) {\n> for (i = 0; i < num_parent; i++) {\n> - char *path = filename_changed(elem->parent[i].status)\n> - ? elem->parent[i].path.buf : elem->path;\n> + const char *path = elem->parent[i].path ?\n> + elem->parent[i].path :\n> + elem->path;\n> if (elem->parent[i].status == DIFF_STATUS_ADDED)\n> dump_quoted_path(\"--- \", \"\", \"/dev/null\",\n> line_prefix, c_meta, c_reset);\n> @@ -1278,12 +1272,10 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n>\n> for (i = 0; i < num_parent; i++)\n> if (rev->combined_all_paths) {\n> - if (filename_changed(p->parent[i].status))\n> - write_name_quoted(p->parent[i].path.buf, stdout,\n> - inter_name_termination);\n> - else\n> - write_name_quoted(p->path, stdout,\n> - inter_name_termination);\n> + const char *path = p->parent[i].path ?\n> + p->parent[i].path :\n> + p->path;\n> + write_name_quoted(path, stdout, inter_name_termination);\n> }\n> write_name_quoted(p->path, stdout, line_termination);\n> }\n> @@ -1645,9 +1637,7 @@ void diff_tree_combined(const struct object_id *oid,\n> struct combine_diff_path *tmp = paths;\n> paths = paths->next;\n> for (i = 0; i < num_parent; i++)\n> - if (rev->combined_all_paths &&\n> - filename_changed(tmp->parent[i].status))\n> - strbuf_release(&tmp->parent[i].path);\n> + free(tmp->parent[i].path);\n> free(tmp);\n> }\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index c6d3bc4d37..88a5aed736 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -417,9 +417,11 @@ static int show_modified(struct rev_info *revs,\n> memset(p->parent, 0, 2 * sizeof(struct combine_diff_parent));\n> p->parent[0].status = DIFF_STATUS_MODIFIED;\n> p->parent[0].mode = new_entry->ce_mode;\n> + p->parent[0].path = NULL;\n> oidcpy(&p->parent[0].oid, &new_entry->oid);\n> p->parent[1].status = DIFF_STATUS_MODIFIED;\n> p->parent[1].mode = old_entry->ce_mode;\n> + p->parent[1].path = NULL;\n> oidcpy(&p->parent[1].oid, &old_entry->oid);\n> show_combined_diff(p, 2, revs);\n> free(p);\n> diff --git a/diff.h b/diff.h\n> index 6e6007c17b..3157faeabb 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -480,7 +480,7 @@ struct combine_diff_path {\n> char status;\n> unsigned int mode;\n> struct object_id oid;\n> - struct strbuf path;\n> + char *path;\n> } parent[FLEX_ARRAY];\n> };\n> #define combine_diff_path_size(n, l) \\\n> diff --git a/tree-diff.c b/tree-diff.c\n> index d9237ffd9b..57af377c2b 100644\n> --- a/tree-diff.c\n> +++ b/tree-diff.c\n> @@ -272,6 +272,7 @@ static struct combine_diff_path *emit_path(struct combine_diff_path *p,\n> }\n>\n> p->parent[i].mode = mode_i;\n> + p->parent[i].path = NULL;\n> oidcpy(&p->parent[i].oid, oid_i);\n> }\n\nThe above LGTM and hopefully it can be accepted.\n\nWith that change I can revert my trace_printfs of combine_diff_path\nback to something simple:\n```\n$ git diff HEAD^\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 5e0b7919bc..4764383f20 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1593,8 +1593,8 @@ void diff_tree_combined(const struct object_id *oid,\n for (num_paths = 0, p = paths; p; p = p->next) {\n trace_printf(\"Wink diff_tree_combined: num_paths=%d\n&p=%p mode=%0x, oid=%s path=%s\\n\", num_paths, p, p->mode,\noid_to_hex(&p->oid), p->path);\n for (i = 0; i < num_parent; i++) {\n- trace_printf(\"Wink diff_tree_combined:\n&p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents\npath.buf=%s\\n\",\n- i, &p->parent[i],\np->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid),\np->parent[i].path.buf, p->parent[i].path.buf);\n+ trace_printf(\"Wink diff_tree_combined:\n&p->parent[%d]=%p status=%c mode=%x oid=%s path=%s\\n\",\n+ i, &p->parent[i],\np->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid),\np->parent[i].path);\n }\n num_paths++;\n }\n```\n\nAnd the output doesn't SEGFAULT :)\n```\n$ GIT_TRACE=1 ./git diff-tree --cc 6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a\n10:06:11.716284 git.c:476 trace: built-in: git diff-tree\n--cc 6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a\n6f8ae955bda8ad246cc1f5f7a15f1c3b1c04696a\n10:06:11.718102 combine-diff.c:1592 Wink diff_tree_combined: find\nnumber of surviving paths num_parent=2\n10:06:11.718108 combine-diff.c:1594 Wink diff_tree_combined:\nnum_paths=0 &p=0x643ac70f7ef0 mode=81a4,\noid=0f41b2fd4a6b679a1cfcaa9a584c382068146212 path=refs.c\n10:06:11.718112 combine-diff.c:1596 Wink diff_tree_combined:\n&p->parent[0]=0x643ac70f7f28 status=M mode=81a4\noid=7dd5e9fa3323111f06303674b213ae24ed2d04b6 path=(null)\n10:06:11.718116 combine-diff.c:1596 Wink diff_tree_combined:\n&p->parent[1]=0x643ac70f7f60 status=M mode=81a4\noid=c55583986940d8ef1e1c839364c03cd92d4f7114 path=(null)\n10:06:11.718120 combine-diff.c:1594 Wink diff_tree_combined:\nnum_paths=1 &p=0x643ac70f7fb0 mode=81a4,\noid=a0cdd99250e8286b55808b697b0a94afac5d8319 path=refs.h\n10:06:11.718123 combine-diff.c:1596 Wink diff_tree_combined:\n&p->parent[0]=0x643ac70f7fe8 status=M mode=81a4\noid=09be47afbee51e99f4ae49588cd65596ccfcb07e path=(null)\n10:06:11.718126 combine-diff.c:1596 Wink diff_tree_combined:\n&p->parent[1]=0x643ac70f8020 status=M mode=81a4\noid=b0dfc65ed2e59c4b66967840339f81e7746a96d3 path=(null)\n10:06:11.718129 combine-diff.c:1594 Wink diff_tree_combined:\nnum_paths=2 &p=0x643ac70f8900 mode=81a4,\noid=5cfb8b7ca8678e171b8e8a7ad6daf1af74a81b59 path=refs/files-backend.c\n10:06:11.718132 combine-diff.c:1596 Wink diff_tree_combined:\n&p->parent[0]=0x643ac70f8938 status=M mode=81a4\noid=467fe347fa7e7d82ed7a2836e43ea749bb90ad7d path=(null)\n10:06:11.718135 combine-diff.c:1596 Wink diff_tree_combined:\n&p->parent[1]=0x643ac70f8970 status=M mode=81a4\noid=8953d1c6d37b13b0db701888b3db92fd87a68aaa path=(null)\n10:06:11.718138 combine-diff.c:1594 Wink diff_tree_combined:\nnum_paths=3 &p=0x643ac70f89d0 mode=81a4,\noid=16550862d3ebe3b357c52254088b143c7ba000d6 path=refs/refs-internal.h\n10:06:11.718142 combine-diff.c:1596 Wink diff_tree_combined:\n&p->parent[0]=0x643ac70f8a08 status=M mode=81a4\noid=66e66e0fc1e812ebebd1d4b0119899c84bf1c0ae path=(null)\n10:06:11.718162 combine-diff.c:1596 Wink diff_tree_combined:\n&p->parent[1]=0x643ac70f8a40 status=M mode=81a4\noid=79b287c5ec5c7d8f759869cf93cda405640186dc path=(null)\n10:06:11.718181 combine-diff.c:1594 Wink diff_tree_combined:\nnum_paths=4 &p=0x643ac70f8aa0 mode=81a4,\noid=00d95a9a2f42ce74c5cb4a42175b0953287851a6\npath=refs/reftable-backend.c\n10:06:11.718184 combine-diff.c:1596 Wink diff_tree_combined:\n&p->parent[0]=0x643ac70f8ad8 status=M mode=81a4\noid=8a2a5b847c3d86332e319da69bfb5c8a56a10e86 path=(null)\n10:06:11.718188 combine-diff.c:1596 Wink diff_tree_combined:\n&p->parent[1]=0x643ac70f8b10 status=M mode=81a4\noid=bec5962debea7b62572d08f6fa8fd38ab4cd8af6 path=(null)\n10:06:11.718192 combine-diff.c:1601 Wink diff_tree_combined: found\n5 surviving paths\ndiff --cc refs/files-backend.c\nindex 467fe347fa,8953d1c6d3..5cfb8b7ca8\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@@ -2533,9 -2539,15 +2543,15 @@@ static int check_old_oid(struct ref_upd\n oid_to_hex(oid),\n oid_to_hex(&update->old_oid));\n\n - return -1;\n + return ret;\n }\n\n+ struct files_transaction_backend_data {\n+ struct ref_transaction *packed_transaction;\n+ int packed_refs_locked;\n+ struct strmap ref_locks;\n+ };\n+\n /*\n * Prepare for carrying out update:\n * - Lock the reference referred to by update.\n```\n"},{"id":"509943","messageId":"CAKk8isoPDcHJXm6HL1x4knNATWsy9mhPXTN5-P-rgFyUfZruDw@mail.gmail.com","threadId":"62734","inReplyTo":"CAKk8isrRCZsrt=2YB+L3EjK3ZAYbSk0e+1YZexqZUhB78L36dg@mail.gmail.com","subject":"Re: [BUGREPORT] git diff-tree --cc SEGFAUTs","fromName":"Wink Saville","fromEmail":"wink@saville.com","sentAt":"2025-01-05T22:13:18Z","receivedAt":"2025-01-05T22:13:30Z","isPatch":false,"sender":{"key":"wink@saville.com","avatar":"https://avatars.githubusercontent.com/u/1024284?v=4"},"body":"I'd like to suggest one minor tweak to Peff's change. Rather than just\nchanging the type of combine_diff_parent::path to `char *` I like to suggest\nchanging the name and adding a comment. My inclination is to use\n`changed_path` as the field name.\n\nThe resulting diff against next is attached.\n\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 641bc92dbd..be5df18d75 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -66,13 +66,9 @@ static struct combine_diff_path *intersect_paths(\n \t\t\toidcpy(&p->parent[n].oid, &q->queue[i]->one->oid);\n \t\t\tp->parent[n].mode = q->queue[i]->one->mode;\n \t\t\tp->parent[n].status = q->queue[i]->status;\n-\n-\t\t\tif (combined_all_paths &&\n-\t\t\t filename_changed(p->parent[n].status)) {\n-\t\t\t\tstrbuf_init(&p->parent[n].path, 0);\n-\t\t\t\tstrbuf_addstr(&p->parent[n].path,\n-\t\t\t\t\t q->queue[i]->one->path);\n-\t\t\t}\n+\t\t\tp->parent[n].changed_path = combined_all_paths &&\n+\t\t\t\t\t filename_changed(p->parent[n].status) ?\n+\t\t\t\t\t xstrdup(q->queue[i]->one->path) : NULL;\n \t\t\t*tail = p;\n \t\t\ttail = &p->next;\n \t\t}\n@@ -92,9 +88,7 @@ static struct combine_diff_path *intersect_paths(\n \t\t\t/* p->path not in q->queue[]; drop it */\n \t\t\t*tail = p->next;\n \t\t\tfor (j = 0; j < num_parent; j++)\n-\t\t\t\tif (combined_all_paths &&\n-\t\t\t\t filename_changed(p->parent[j].status))\n-\t\t\t\t\tstrbuf_release(&p->parent[j].path);\n+\t\t\t\tfree(p->parent[j].changed_path);\n \t\t\tfree(p);\n \t\t\tcontinue;\n \t\t}\n@@ -108,10 +102,9 @@ static struct combine_diff_path *intersect_paths(\n \t\toidcpy(&p->parent[n].oid, &q->queue[i]->one->oid);\n \t\tp->parent[n].mode = q->queue[i]->one->mode;\n \t\tp->parent[n].status = q->queue[i]->status;\n-\t\tif (combined_all_paths &&\n-\t\t filename_changed(p->parent[n].status))\n-\t\t\tstrbuf_addstr(&p->parent[n].path,\n-\t\t\t\t q->queue[i]->one->path);\n+\t\tp->parent[n].changed_path = combined_all_paths &&\n+\t\t\t\t filename_changed(p->parent[n].status) ?\n+\t\t\t\t xstrdup(q->queue[i]->one->path) : NULL;\n \n \t\ttail = &p->next;\n \t\ti++;\n@@ -996,8 +989,9 @@ static void show_combined_header(struct combine_diff_path *elem,\n \n \tif (rev->combined_all_paths) {\n \t\tfor (i = 0; i < num_parent; i++) {\n-\t\t\tchar *path = filename_changed(elem->parent[i].status)\n-\t\t\t\t? elem->parent[i].path.buf : elem->path;\n+\t\t\tconst char *path = elem->parent[i].changed_path ?\n+\t\t\t\t\t elem->parent[i].changed_path :\n+\t\t\t\t\t elem->path;\n \t\t\tif (elem->parent[i].status == DIFF_STATUS_ADDED)\n \t\t\t\tdump_quoted_path(\"--- \", \"\", \"/dev/null\",\n \t\t\t\t\t\t line_prefix, c_meta, c_reset);\n@@ -1278,12 +1272,10 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \n \tfor (i = 0; i < num_parent; i++)\n \t\tif (rev->combined_all_paths) {\n-\t\t\tif (filename_changed(p->parent[i].status))\n-\t\t\t\twrite_name_quoted(p->parent[i].path.buf, stdout,\n-\t\t\t\t\t\t inter_name_termination);\n-\t\t\telse\n-\t\t\t\twrite_name_quoted(p->path, stdout,\n-\t\t\t\t\t\t inter_name_termination);\n+\t\t\tconst char *path = p->parent[i].changed_path ?\n+\t\t\t\t\t p->parent[i].changed_path :\n+\t\t\t\t\t p->path;\n+\t\t\twrite_name_quoted(path, stdout, inter_name_termination);\n \t\t}\n \twrite_name_quoted(p->path, stdout, line_termination);\n }\n@@ -1645,9 +1637,7 @@ void diff_tree_combined(const struct object_id *oid,\n \t\tstruct combine_diff_path *tmp = paths;\n \t\tpaths = paths->next;\n \t\tfor (i = 0; i < num_parent; i++)\n-\t\t\tif (rev->combined_all_paths &&\n-\t\t\t filename_changed(tmp->parent[i].status))\n-\t\t\t\tstrbuf_release(&tmp->parent[i].path);\n+\t\t\tfree(tmp->parent[i].changed_path);\n \t\tfree(tmp);\n \t}\n \ndiff --git a/diff-lib.c b/diff-lib.c\nindex c6d3bc4d37..602ae0c84b 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -417,9 +417,11 @@ static int show_modified(struct rev_info *revs,\n \t\tmemset(p->parent, 0, 2 * sizeof(struct combine_diff_parent));\n \t\tp->parent[0].status = DIFF_STATUS_MODIFIED;\n \t\tp->parent[0].mode = new_entry->ce_mode;\n+\t\tp->parent[0].changed_path = NULL;\n \t\toidcpy(&p->parent[0].oid, &new_entry->oid);\n \t\tp->parent[1].status = DIFF_STATUS_MODIFIED;\n \t\tp->parent[1].mode = old_entry->ce_mode;\n+\t\tp->parent[1].changed_path = NULL;\n \t\toidcpy(&p->parent[1].oid, &old_entry->oid);\n \t\tshow_combined_diff(p, 2, revs);\n \t\tfree(p);\ndiff --git a/diff.h b/diff.h\nindex 6e6007c17b..d13be142dd 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -480,7 +480,7 @@ struct combine_diff_path {\n \t\tchar status;\n \t\tunsigned int mode;\n \t\tstruct object_id oid;\n-\t\tstruct strbuf path;\n+\t\tchar *changed_path; // NULL unless status == 'R' or 'C', see filename_changed()\n \t} parent[FLEX_ARRAY];\n };\n #define combine_diff_path_size(n, l) \\\ndiff --git a/tree-diff.c b/tree-diff.c\nindex d9237ffd9b..85f1d2a4a6 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -272,6 +272,7 @@ static struct combine_diff_path *emit_path(struct combine_diff_path *p,\n \t\t\t}\n \n \t\t\tp->parent[i].mode = mode_i;\n+\t\t\tp->parent[i].changed_path = NULL;\n \t\t\toidcpy(&p->parent[i].oid, oid_i);\n \t\t}\n \n"},{"id":"510223","messageId":"20250109082723.GA2748497@coredump.intra.peff.net","threadId":"62734","inReplyTo":"20250104033210.GA892381@coredump.intra.peff.net","subject":"[PATCH 0/14] combine-diff cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-09T08:27:23Z","receivedAt":"2025-01-09T08:27:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since Wink successfully nerd-sniped me into digging into the\ncombine-diff code, and since I had such a hard time figuring out some of\nits logic, I spent a little time trying to put that puzzling to good use\nto make it more readable.\n\nAside from a minor leak fix in the first patch, I didn't find any bugs.\nSo arguably this whole thing could be discarded as churn. But I hope at\nleast some of it is worthwhile, and I tried to order it to keep the less\ncontroversial bits near the top.\n\nThe series can be split into a few sections:\n\n [01/14]: run_diff_files(): delay allocation of combine_diff_path\n [02/14]: combine-diff: add combine_diff_path_new()\n [03/14]: tree-diff: clear parent array in path_appendnew()\n [04/14]: combine-diff: use pointer for parent paths\n [05/14]: diff: add a comment about combine_diff_path.parent.path\n [06/14]: run_diff_files(): de-mystify the size of combine_diff_path struct\n\n These first six clean up most of the allocation and initialization\n confusion that started this thread. They can't go all the way\n because of the scariness in path_appendnew().\n\n [07/14]: tree-diff: drop path_appendnew() alloc optimization\n [08/14]: tree-diff: pass whole path string to path_appendnew()\n [09/14]: tree-diff: inline path_appendnew()\n [10/14]: combine-diff: drop public declaration of combine_diff_path_size()\n\n And these ones take it further, but at the cost of losing an\n optimization in patch 07. I don't think it was doing much (and I\n gave some timings there). But it's a judgement call on whether the\n cleaner code is worthwhile.\n\n [11/14]: tree-diff: drop list-tail argument to diff_tree_paths()\n [12/14]: tree-diff: use the name \"tail\" to refer to list tail\n [13/14]: tree-diff: simplify emit_path() list management\n [14/14]: tree-diff: make list tail-passing more explicit\n\n And these last four fix some confusion I had while reading the\n functions. I think they _could_ be done independent of 7-14,\n but there'd be some kinks to work out in emit_path().\n\n The final one is probably a matter of taste, and I'm not sure if\n people find it easier to understand than the original or not. If\n not, it can easily be dropped.\n\n combine-diff.c | 80 +++++++++++++-------------\n diff-lib.c | 36 ++++--------\n diff.h | 18 ++++--\n tree-diff.c | 152 ++++++++++++-------------------------------------\n 4 files changed, 102 insertions(+), 184 deletions(-)\n\n-Peff\n"},{"id":"510224","messageId":"20250109082818.GA2748836@coredump.intra.peff.net","threadId":"62734","inReplyTo":"20250109082723.GA2748497@coredump.intra.peff.net","subject":"[PATCH 01/14] run_diff_files(): delay allocation of combine_diff_path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-09T08:28:18Z","receivedAt":"2025-01-09T08:28:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While looping over the index entries, when we see a higher level stage\nthe first thing we do is allocate a combine_diff_path struct for it. But\nthis can leak; if check_removed() returns an error, we'll continue to\nthe next iteration of the loop without cleaning up.\n\nWe can fix this by just delaying the allocation by a few lines.\n\nI don't think this leak is triggered in the test suite, but it's pretty\neasy to see by inspection. My ulterior motive here is that the delayed\nallocation means we have all of the data needed to initialize \"dpath\" at\nthe time of malloc, making it easier to factor out a constructor\nfunction.\n\nSigned-off-by: Jeff King \n---\n diff-lib.c | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex c6d3bc4d37..85b8f1fa59 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -156,18 +156,6 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tsize_t path_len;\n \t\t\tstruct stat st;\n \n-\t\t\tpath_len = ce_namelen(ce);\n-\n-\t\t\tdpath = xmalloc(combine_diff_path_size(5, path_len));\n-\t\t\tdpath->path = (char *) &(dpath->parent[5]);\n-\n-\t\t\tdpath->next = NULL;\n-\t\t\tmemcpy(dpath->path, ce->name, path_len);\n-\t\t\tdpath->path[path_len] = '\\0';\n-\t\t\toidclr(&dpath->oid, the_repository->hash_algo);\n-\t\t\tmemset(&(dpath->parent[0]), 0,\n-\t\t\t sizeof(struct combine_diff_parent)*5);\n-\n \t\t\tchanged = check_removed(ce, &st);\n \t\t\tif (!changed)\n \t\t\t\twt_mode = ce_mode_from_stat(ce, st.st_mode);\n@@ -178,7 +166,19 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t}\n \t\t\t\twt_mode = 0;\n \t\t\t}\n+\n+\t\t\tpath_len = ce_namelen(ce);\n+\n+\t\t\tdpath = xmalloc(combine_diff_path_size(5, path_len));\n+\t\t\tdpath->path = (char *) &(dpath->parent[5]);\n+\n+\t\t\tdpath->next = NULL;\n+\t\t\tmemcpy(dpath->path, ce->name, path_len);\n+\t\t\tdpath->path[path_len] = '\\0';\n+\t\t\toidclr(&dpath->oid, the_repository->hash_algo);\n \t\t\tdpath->mode = wt_mode;\n+\t\t\tmemset(&(dpath->parent[0]), 0,\n+\t\t\t sizeof(struct combine_diff_parent)*5);\n \n \t\t\twhile (i < entries) {\n \t\t\t\tstruct cache_entry *nce = istate->cache[i];\n-- \n2.48.0.rc2.413.gc1c80375a3\n\n"},{"id":"510225","messageId":"20250109083236.GB2748836@coredump.intra.peff.net","threadId":"62734","inReplyTo":"20250109082723.GA2748497@coredump.intra.peff.net","subject":"[PATCH 02/14] combine-diff: add combine_diff_path_new()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-09T08:32:36Z","receivedAt":"2025-01-09T08:32:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The combine_diff_path struct has variable size, since it embeds both the\nmemory allocation for the path field as well as a variable-sized parent\narray. This makes allocating one a bit tricky.\n\nWe have a helper to compute the required size, but it's up to individual\nsites to actually initialize all of the fields. Let's provide a\nconstructor function to make that a little nicer. Besides being shorter,\nit also hides away tricky bits like the computation of the \"path\"\npointer (which is right after the \"parent\" flex array).\n\nAs a bonus, using the same constructor everywhere means that we'll\nconsistently initialize all parts of the struct. A few code paths left\nthe parent array unitialized. This didn't cause any bugs, but we'll be\nable to simplify some code in the next few patches knowing that the\nparent fields have all been zero'd.\n\nThis also gets rid of some questionable uses of \"int\" to store buffer\nlengths. Though we do use them to allocate, I don't think there are any\ninteger overflow vulnerabilities here (the allocation helper promotes\nthem to size_t and checks arithmetic for overflow, and the actual memcpy\nof the bytes is done using the possibly-truncated \"int\" value).\n\nSadly we can't use the FLEX_* macros to simplify the allocation here,\nbecause there are two variable-sized parts to the struct (and those\nmacros only handle one).\n\nNor can we get stop publicly declaring combine_diff_path_size(). This\npatch does not touch the code in path_appendnew() at all, which is not\nready to be moved to our new constructor for a few reasons:\n\n - path_appendnew() has a memory-reuse optimization where it tries to\n reuse combine_diff_path structs rather than freeing and\n reallocating.\n\n - path_appendnew() does not create the struct from a single path\n string, but rather allocates and copies into the buffer from\n multiple sources.\n\nThese can be addressed by some refactoring, but let's leave it as-is for\nnow.\n\nSigned-off-by: Jeff King \n---\n combine-diff.c | 40 ++++++++++++++++++++++++++--------------\n diff-lib.c | 29 ++++++-----------------------\n diff.h | 5 +++++\n 3 files changed, 37 insertions(+), 37 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 641bc92dbd..45548fd438 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -47,22 +47,13 @@ static struct combine_diff_path *intersect_paths(\n \n \tif (!n) {\n \t\tfor (i = 0; i < q->nr; i++) {\n-\t\t\tint len;\n-\t\t\tconst char *path;\n \t\t\tif (diff_unmodified_pair(q->queue[i]))\n \t\t\t\tcontinue;\n-\t\t\tpath = q->queue[i]->two->path;\n-\t\t\tlen = strlen(path);\n-\t\t\tp = xmalloc(combine_diff_path_size(num_parent, len));\n-\t\t\tp->path = (char *) &(p->parent[num_parent]);\n-\t\t\tmemcpy(p->path, path, len);\n-\t\t\tp->path[len] = 0;\n-\t\t\tp->next = NULL;\n-\t\t\tmemset(p->parent, 0,\n-\t\t\t sizeof(p->parent[0]) * num_parent);\n-\n-\t\t\toidcpy(&p->oid, &q->queue[i]->two->oid);\n-\t\t\tp->mode = q->queue[i]->two->mode;\n+\t\t\tp = combine_diff_path_new(q->queue[i]->two->path,\n+\t\t\t\t\t\t strlen(q->queue[i]->two->path),\n+\t\t\t\t\t\t q->queue[i]->two->mode,\n+\t\t\t\t\t\t &q->queue[i]->two->oid,\n+\t\t\t\t\t\t num_parent);\n \t\t\toidcpy(&p->parent[n].oid, &q->queue[i]->one->oid);\n \t\t\tp->parent[n].mode = q->queue[i]->one->mode;\n \t\t\tp->parent[n].status = q->queue[i]->status;\n@@ -1667,3 +1658,24 @@ void diff_tree_combined_merge(const struct commit *commit,\n \tdiff_tree_combined(&commit->object.oid, &parents, rev);\n \toid_array_clear(&parents);\n }\n+\n+struct combine_diff_path *combine_diff_path_new(const char *path,\n+\t\t\t\t\t\tsize_t path_len,\n+\t\t\t\t\t\tunsigned int mode,\n+\t\t\t\t\t\tconst struct object_id *oid,\n+\t\t\t\t\t\tsize_t num_parents)\n+{\n+\tstruct combine_diff_path *p;\n+\n+\tp = xmalloc(combine_diff_path_size(num_parents, path_len));\n+\tp->path = (char *)&(p->parent[num_parents]);\n+\tmemcpy(p->path, path, path_len);\n+\tp->path[path_len] = 0;\n+\tp->next = NULL;\n+\tp->mode = mode;\n+\toidcpy(&p->oid, oid);\n+\n+\tmemset(p->parent, 0, sizeof(p->parent[0]) * num_parents);\n+\n+\treturn p;\n+}\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 85b8f1fa59..471ef99614 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -153,7 +153,6 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tstruct diff_filepair *pair;\n \t\t\tunsigned int wt_mode = 0;\n \t\t\tint num_compare_stages = 0;\n-\t\t\tsize_t path_len;\n \t\t\tstruct stat st;\n \n \t\t\tchanged = check_removed(ce, &st);\n@@ -167,18 +166,8 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\twt_mode = 0;\n \t\t\t}\n \n-\t\t\tpath_len = ce_namelen(ce);\n-\n-\t\t\tdpath = xmalloc(combine_diff_path_size(5, path_len));\n-\t\t\tdpath->path = (char *) &(dpath->parent[5]);\n-\n-\t\t\tdpath->next = NULL;\n-\t\t\tmemcpy(dpath->path, ce->name, path_len);\n-\t\t\tdpath->path[path_len] = '\\0';\n-\t\t\toidclr(&dpath->oid, the_repository->hash_algo);\n-\t\t\tdpath->mode = wt_mode;\n-\t\t\tmemset(&(dpath->parent[0]), 0,\n-\t\t\t sizeof(struct combine_diff_parent)*5);\n+\t\t\tdpath = combine_diff_path_new(ce->name, ce_namelen(ce),\n+\t\t\t\t\t\t wt_mode, null_oid(), 5);\n \n \t\t\twhile (i < entries) {\n \t\t\t\tstruct cache_entry *nce = istate->cache[i];\n@@ -405,16 +394,10 @@ static int show_modified(struct rev_info *revs,\n \tif (revs->combine_merges && !cached &&\n \t (!oideq(oid, &old_entry->oid) || !oideq(&old_entry->oid, &new_entry->oid))) {\n \t\tstruct combine_diff_path *p;\n-\t\tint pathlen = ce_namelen(new_entry);\n-\n-\t\tp = xmalloc(combine_diff_path_size(2, pathlen));\n-\t\tp->path = (char *) &p->parent[2];\n-\t\tp->next = NULL;\n-\t\tmemcpy(p->path, new_entry->name, pathlen);\n-\t\tp->path[pathlen] = 0;\n-\t\tp->mode = mode;\n-\t\toidclr(&p->oid, the_repository->hash_algo);\n-\t\tmemset(p->parent, 0, 2 * sizeof(struct combine_diff_parent));\n+\n+\t\tp = combine_diff_path_new(new_entry->name,\n+\t\t\t\t\t ce_namelen(new_entry),\n+\t\t\t\t\t mode, null_oid(), 2);\n \t\tp->parent[0].status = DIFF_STATUS_MODIFIED;\n \t\tp->parent[0].mode = new_entry->ce_mode;\n \t\toidcpy(&p->parent[0].oid, &new_entry->oid);\ndiff --git a/diff.h b/diff.h\nindex 6e6007c17b..5cddd5a870 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -486,6 +486,11 @@ struct combine_diff_path {\n #define combine_diff_path_size(n, l) \\\n \tst_add4(sizeof(struct combine_diff_path), (l), 1, \\\n \t\tst_mult(sizeof(struct combine_diff_parent), (n)))\n+struct combine_diff_path *combine_diff_path_new(const char *path,\n+\t\t\t\t\t\tsize_t path_len,\n+\t\t\t\t\t\tunsigned int mode,\n+\t\t\t\t\t\tconst struct object_id *oid,\n+\t\t\t\t\t\tsize_t num_parents);\n \n void show_combined_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tstruct rev_info *);\n-- \n2.48.0.rc2.413.gc1c80375a3\n\n"},{"id":"510226","messageId":"20250109083310.GC2748836@coredump.intra.peff.net","threadId":"62734","inReplyTo":"20250109082723.GA2748497@coredump.intra.peff.net","subject":"[PATCH 03/14] tree-diff: clear parent array in path_appendnew()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-09T08:33:10Z","receivedAt":"2025-01-09T08:33:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of the other functions which allocate a combine_diff_path struct\nzero out the parent array, but this code path does not. There's no bug,\nsince our caller will fill in most of the fields. But leaving the unused\nfields (like combine_diff_parent.path) uninitialized makes working with\nthe struct more error-prone than it needs to be.\n\nLet's just zero the parent field to be consistent with the\ncombine_diff_path_new() allocator.\n\nSigned-off-by: Jeff King \n---\n tree-diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex d9237ffd9b..24f7b5912c 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -151,8 +151,6 @@ static int emit_diff_first_parent_only(struct diff_options *opt, struct combine_\n *\tprocess(p);\n *\tp = pprev;\n *\t; don't forget to free tail->next in the end\n- *\n- * p->parent[] remains uninitialized.\n */\n static struct combine_diff_path *path_appendnew(struct combine_diff_path *last,\n \tint nparent, const struct strbuf *base, const char *path, int pathlen,\n@@ -187,6 +185,8 @@ static struct combine_diff_path *path_appendnew(struct combine_diff_path *last,\n \tp->mode = mode;\n \toidcpy(&p->oid, oid ? oid : null_oid());\n \n+\tmemset(p->parent, 0, sizeof(p->parent[0]) * nparent);\n+\n \treturn p;\n }\n \n-- \n2.48.0.rc2.413.gc1c80375a3\n\n"},{"id":"510227","messageId":"20250109084229.GD2748836@coredump.intra.peff.net","threadId":"62734","inReplyTo":"20250109082723.GA2748497@coredump.intra.peff.net","subject":"[PATCH 04/14] combine-diff: use pointer for parent paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-09T08:42:29Z","receivedAt":"2025-01-09T08:42:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit d76ce4f734 (log,diff-tree: add --combined-all-paths option,\n2019-02-07) added a \"path\" field to each combine_diff_parent struct.\nIt's defined as a strbuf, but this is overkill. We never manipulate the\nbuffer beyond inserting a single string into it.\n\nAnd in fact there's a small bug: we zero the parent structs, including\nthe path strbufs. For the 0th parent, we strbuf_init() the strbuf before\nadding to it. But for subsequent parents, we never do the init. This is\ntechnically violating the strbuf API, though the code there is resilient\nenough to handle this zero'd state.\n\nThis patch switches us to just store an allocated string pointer.\nZeroing it is enough to properly initialize it there (modulo the usual\nassumption we make that a NULL pointer is all-zeroes).\n\nAnd as a bonus, we can just check for a non-NULL value to see if it is\npresent, rather than repeating the combined_all_paths logic at each\nsite.\n\nSigned-off-by: Jeff King \n---\n combine-diff.c | 30 +++++++++++-------------------\n diff.h | 2 +-\n 2 files changed, 12 insertions(+), 20 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 45548fd438..ae3cbfc699 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -60,9 +60,7 @@ static struct combine_diff_path *intersect_paths(\n \n \t\t\tif (combined_all_paths &&\n \t\t\t filename_changed(p->parent[n].status)) {\n-\t\t\t\tstrbuf_init(&p->parent[n].path, 0);\n-\t\t\t\tstrbuf_addstr(&p->parent[n].path,\n-\t\t\t\t\t q->queue[i]->one->path);\n+\t\t\t\tp->parent[n].path = xstrdup(q->queue[i]->one->path);\n \t\t\t}\n \t\t\t*tail = p;\n \t\t\ttail = &p->next;\n@@ -83,9 +81,7 @@ static struct combine_diff_path *intersect_paths(\n \t\t\t/* p->path not in q->queue[]; drop it */\n \t\t\t*tail = p->next;\n \t\t\tfor (j = 0; j < num_parent; j++)\n-\t\t\t\tif (combined_all_paths &&\n-\t\t\t\t filename_changed(p->parent[j].status))\n-\t\t\t\t\tstrbuf_release(&p->parent[j].path);\n+\t\t\t\tfree(p->parent[j].path);\n \t\t\tfree(p);\n \t\t\tcontinue;\n \t\t}\n@@ -101,8 +97,7 @@ static struct combine_diff_path *intersect_paths(\n \t\tp->parent[n].status = q->queue[i]->status;\n \t\tif (combined_all_paths &&\n \t\t filename_changed(p->parent[n].status))\n-\t\t\tstrbuf_addstr(&p->parent[n].path,\n-\t\t\t\t q->queue[i]->one->path);\n+\t\t\tp->parent[n].path = xstrdup(q->queue[i]->one->path);\n \n \t\ttail = &p->next;\n \t\ti++;\n@@ -987,8 +982,9 @@ static void show_combined_header(struct combine_diff_path *elem,\n \n \tif (rev->combined_all_paths) {\n \t\tfor (i = 0; i < num_parent; i++) {\n-\t\t\tchar *path = filename_changed(elem->parent[i].status)\n-\t\t\t\t? elem->parent[i].path.buf : elem->path;\n+\t\t\tconst char *path = elem->parent[i].path ?\n+\t\t\t\t\t elem->parent[i].path :\n+\t\t\t\t\t elem->path;\n \t\t\tif (elem->parent[i].status == DIFF_STATUS_ADDED)\n \t\t\t\tdump_quoted_path(\"--- \", \"\", \"/dev/null\",\n \t\t\t\t\t\t line_prefix, c_meta, c_reset);\n@@ -1269,12 +1265,10 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \n \tfor (i = 0; i < num_parent; i++)\n \t\tif (rev->combined_all_paths) {\n-\t\t\tif (filename_changed(p->parent[i].status))\n-\t\t\t\twrite_name_quoted(p->parent[i].path.buf, stdout,\n-\t\t\t\t\t\t inter_name_termination);\n-\t\t\telse\n-\t\t\t\twrite_name_quoted(p->path, stdout,\n-\t\t\t\t\t\t inter_name_termination);\n+\t\t\tconst char *path = p->parent[i].path ?\n+\t\t\t\t\t p->parent[i].path :\n+\t\t\t\t\t p->path;\n+\t\t\twrite_name_quoted(path, stdout, inter_name_termination);\n \t\t}\n \twrite_name_quoted(p->path, stdout, line_termination);\n }\n@@ -1636,9 +1630,7 @@ void diff_tree_combined(const struct object_id *oid,\n \t\tstruct combine_diff_path *tmp = paths;\n \t\tpaths = paths->next;\n \t\tfor (i = 0; i < num_parent; i++)\n-\t\t\tif (rev->combined_all_paths &&\n-\t\t\t filename_changed(tmp->parent[i].status))\n-\t\t\t\tstrbuf_release(&tmp->parent[i].path);\n+\t\t\tfree(tmp->parent[i].path);\n \t\tfree(tmp);\n \t}\n \ndiff --git a/diff.h b/diff.h\nindex 5cddd5a870..f5f6ea00fb 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -480,7 +480,7 @@ struct combine_diff_path {\n \t\tchar status;\n \t\tunsigned int mode;\n \t\tstruct object_id oid;\n-\t\tstruct strbuf path;\n+\t\tchar *path;\n \t} parent[FLEX_ARRAY];\n };\n #define combine_diff_path_size(n, l) \\\n-- \n2.48.0.rc2.413.gc1c80375a3\n\n"},{"id":"510228","messageId":"20250109084248.GE2748836@coredump.intra.peff.net","threadId":"62734","inReplyTo":"20250109082723.GA2748497@coredump.intra.peff.net","subject":"[PATCH 05/14] diff: add a comment about combine_diff_path.parent.path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-09T08:42:48Z","receivedAt":"2025-01-09T08:42:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We only fill in the per-parent \"path\" field when it differs from what's\nin combine_diff_path.path (and even then only when the option is\nappropriate). Let's document that.\n\nSuggested-by: Wink Saville \nSigned-off-by: Jeff King \n---\n diff.h | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/diff.h b/diff.h\nindex f5f6ea00fb..60e7db4ad6 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -480,6 +480,12 @@ struct combine_diff_path {\n \t\tchar status;\n \t\tunsigned int mode;\n \t\tstruct object_id oid;\n+\t\t/*\n+\t\t * This per-parent path is filled only when doing a combined\n+\t\t * diff with revs.combined_all_paths set, and only if the path\n+\t\t * differs from the post-image (e.g., a rename or copy).\n+\t\t * Otherwise it is left NULL.\n+\t\t */\n \t\tchar *path;\n \t} parent[FLEX_ARRAY];\n };\n-- \n2.48.0.rc2.413.gc1c80375a3\n\n"},{"id":"510229","messageId":"20250109084421.GF2748836@coredump.intra.peff.net","threadId":"62734","inReplyTo":"20250109082723.GA2748497@coredump.intra.peff.net","subject":"[PATCH 06/14] run_diff_files(): de-mystify the size of combine_diff_path struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-09T08:44:21Z","receivedAt":"2025-01-09T08:44:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We allocate a combine_diff_path struct with space for 5 parents. Why 5?\n\nThe history is not particularly enlightening. The allocation comes from\nb4b1550315 (Don't instantiate structures with FAMs., 2006-06-18), which\njust switched to xmalloc from a stack struct with 5 elements. That\nstruct changed to 5 from 4 in 2454c962fb (combine-diff: show mode\nchanges as well., 2006-02-06), when we also moved from storing raw sha1\nbytes to the combine_diff_parent struct. But no explanation is given.\nThat 4 comes from the earliest code in ea726d02e9 (diff-files: -c and\n--cc options., 2006-01-28).\n\nOne might guess it is for the 4 stages we can store in the index. But\nthis code path only ever diffs the current state against stages 2 and 3.\nSo we only need two slots.\n\nAnd it's easy to see this is still the case. We fill the parent slots by\nsubtracting 2 from the ce_stage() values, ignoring values below 2. And\nsince ce_stage() is only 2 bits, there are 4 values, and thus we need 2\nslots.\n\nLet's use the correct value (saving a tiny bit of memory) and add a\ncomment explaining what's going on (saving a tiny bit of programmer\nbrain power).\n\nArguably we could use:\n\n 1 + (STAGEMASK >> STAGESHIFT) - 2\n\nwhich lets the compiler enforce that we will not go out-of-bounds if we\nsee an unexpected value from ce_stage(). But that is more confusing to\nexplain, and the constant \"2\" is baked into other parts of the function.\nIt is a fundamental constant, not something where somebody might bump a\nmacro and forget to update this code.\n\nSigned-off-by: Jeff King \n---\n diff-lib.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 471ef99614..353b473ed5 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -166,8 +166,13 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\twt_mode = 0;\n \t\t\t}\n \n+\t\t\t/*\n+\t\t\t * Allocate space for two parents, which will come from\n+\t\t\t * index stages #2 and #3, if present. Below we'll fill\n+\t\t\t * these from (stage - 2).\n+\t\t\t */\n \t\t\tdpath = combine_diff_path_new(ce->name, ce_namelen(ce),\n-\t\t\t\t\t\t wt_mode, null_oid(), 5);\n+\t\t\t\t\t\t wt_mode, null_oid(), 2);\n \n \t\t\twhile (i < entries) {\n \t\t\t\tstruct cache_entry *nce = istate->cache[i];\n-- \n2.48.0.rc2.413.gc1c80375a3\n\n"},{"id":"510230","messageId":"20250109084649.GG2748836@coredump.intra.peff.net","threadId":"62734","inReplyTo":"20250109082723.GA2748497@coredump.intra.peff.net","subject":"[PATCH 07/14] tree-diff: drop path_appendnew() alloc optimization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-09T08:46:49Z","receivedAt":"2025-01-09T08:46:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're diffing trees, we create a list of combine_diff_path structs\nthat represent changed paths. We allocate each struct and add it to the\nlist with path_appendnew(), which we then feed to opt->pathchange().\nThat function tells us whether the path is of interest or not; if not,\nthen we can throw away the struct we allocated.\n\nSo there's an optimization to avoid extra allocations: instead of\nthrowing away the new entry, we try to reuse it. If it was large enough\nto store the next path we care about, we can do so. And if not, we fall\nback to freeing and re-allocating a new struct.\n\nThis comes from 72441af7c4 (tree-diff: rework diff_tree() to generate\ndiffs for multiparent cases as well, 2014-04-07), where the goal was to\nhave even the 2-parent diff code use the combine-diff infrastructure,\nbut without taking a performance hit.\n\nThe implementation causes some complexities in the interface (as we\nstore the allocation length inside the \"next\" pointer), and prevents us\nfrom using the regular combine_diff_path_new() constructor. The\ncomplexity is mostly contained inside two functions, but it's worth\nre-evaluating how much it's helping.\n\nThat commit claims it helps ~1% on generating two-parent diffs in\nlinux.git. Here are the timings I get on the same command today (\"old\"\nis the current tip of master, and \"new\" has this patch applied):\n\n Benchmark 1: ./git.old log --raw --no-abbrev --no-renames v3.10..v3.11\n Time (mean ± σ): 532.9 ms ± 5.8 ms [User: 472.7 ms, System: 59.6 ms]\n Range (min … max): 525.9 ms … 543.3 ms 10 runs\n\n Benchmark 2: ./git.new log --raw --no-abbrev --no-renames v3.10..v3.11\n Time (mean ± σ): 538.3 ms ± 5.7 ms [User: 478.0 ms, System: 59.7 ms]\n Range (min … max): 528.5 ms … 545.3 ms 10 runs\n\n Summary\n ./git.old log --raw --no-abbrev --no-renames v3.10..v3.11 ran\n 1.01 ± 0.02 times faster than ./git.new log --raw --no-abbrev --no-renames v3.10..v3.11\n\nSo we do end up on average 1% faster, but with 2% of noise. I tried to\nfocus more on diff performance by running the commit traversal\nseparately, like:\n\n git rev-list v3.10..v3.11 >in\n\nand then timing just the diffs:\n\n Benchmark 1: ./git.old diff-tree --stdin -r