{"thread":{"id":"65527","subject":"[PATCH 0/5] Duplicate entry hardening","startedAt":"2026-04-21T00:26:16Z","lastAt":"2026-06-14T15:22:41Z","messageCount":23,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Patrick Steinhardt","Christian Couder","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"541999","messageId":"pull.2096.git.1776731171.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":null,"subject":"[PATCH 0/5] Duplicate entry hardening","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-21T00:26:06Z","receivedAt":"2026-04-21T00:26:16Z","isPatch":true,"body":"We had some corrupt trees with duplicate entries in real world repositories,\nwhich triggered an assertion failure in merge-ort. Further, the corrupt tree\ncreation in the third party tool would have been avoided had verify_cache()\ncorrectly checked for D/F conflicts. Provide fixes for both issues,\nincluding 3 preparatory changes for the merge-ort fix.\n\nElijah Newren (5):\n merge-ort: propagate callback errors from traverse_trees_wrapper()\n merge-ort: drop unnecessary show_all_errors from collect_merge_info()\n merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()\n merge-ort: abort merge when trees have duplicate entries\n cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts\n\n cache-tree.c | 46 ++++++++++++++--\n merge-ort.c | 78 ++++++++++++++++------------\n t/meson.build | 1 +\n t/t0093-direct-index-write.pl | 38 ++++++++++++++\n t/t0093-verify-cache-df-gap.sh | 59 +++++++++++++++++++++\n t/t6422-merge-rename-corner-cases.sh | 54 +++++++++++++++++++\n 6 files changed, 239 insertions(+), 37 deletions(-)\n create mode 100644 t/t0093-direct-index-write.pl\n create mode 100755 t/t0093-verify-cache-df-gap.sh\n\n\nbase-commit: e8955061076952cc5eab0300424fc48b601fe12d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2096%2Fnewren%2Fduplicate-entry-hardening-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2096/newren/duplicate-entry-hardening-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2096\n-- \ngitgitgadget\n"},{"id":"542000","messageId":"282f906d1b4767d95e2a66072c280c2294a93a9f.1776731171.git.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.git.1776731171.gitgitgadget@gmail.com","subject":"[PATCH 1/5] merge-ort: propagate callback errors from traverse_trees_wrapper()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-21T00:26:07Z","receivedAt":"2026-04-21T00:26:18Z","isPatch":true,"body":"From: Elijah Newren \n\ntraverse_trees_wrapper() saves entries from a first pass through\ntraverse_trees() and then replays them through the real callback\n(collect_merge_info_callback). However, the replay loop silently\ndiscards the callback return value. This means any error reported by\nthe callback during replay -- including a future check for malformed\ntrees -- would be ignored, allowing the merge to proceed with corrupt\nstate.\n\nCapture the return value, stop the loop on negative (error) returns,\nand propagate the error to the caller. Note that the callback returns\na positive mask value on success, so we normalize non-negative returns\nto 0 for the caller.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 00923ce3cd..4b8e32209d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1008,18 +1008,20 @@ static int traverse_trees_wrapper(struct index_state *istate,\n \tinfo->traverse_path = renames->callback_data_traverse_path;\n \tinfo->fn = old_fn;\n \tfor (i = old_offset; i < renames->callback_data_nr; ++i) {\n-\t\tinfo->fn(n,\n-\t\t\t renames->callback_data[i].mask,\n-\t\t\t renames->callback_data[i].dirmask,\n-\t\t\t renames->callback_data[i].names,\n-\t\t\t info);\n+\t\tret = info->fn(n,\n+\t\t\t renames->callback_data[i].mask,\n+\t\t\t renames->callback_data[i].dirmask,\n+\t\t\t renames->callback_data[i].names,\n+\t\t\t info);\n+\t\tif (ret < 0)\n+\t\t\tbreak;\n \t}\n \n \trenames->callback_data_nr = old_offset;\n \tfree(renames->callback_data_traverse_path);\n \trenames->callback_data_traverse_path = old_callback_data_traverse_path;\n \tinfo->traverse_path = NULL;\n-\treturn 0;\n+\treturn ret < 0 ? ret : 0;\n }\n \n static void setup_path_info(struct merge_options *opt,\n-- \ngitgitgadget\n\n"},{"id":"542001","messageId":"949b5d8e3f3aefd9497a7b85d860259b9d5db418.1776731171.git.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.git.1776731171.gitgitgadget@gmail.com","subject":"[PATCH 2/5] merge-ort: drop unnecessary show_all_errors from collect_merge_info()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-21T00:26:08Z","receivedAt":"2026-04-21T00:26:21Z","isPatch":true,"body":"From: Elijah Newren \n\ncollect_merge_info() has set info.show_all_errors = 1 since\nd2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),\n2020-12-13). This setting was copied from unpack-trees.c where it\ncontrols batching of error messages for porcelain display, but\nmerge-ort has no such error-batching logic and never needed it.\n\nWith show_all_errors set, traverse_trees() captures a negative callback\nreturn but continues processing remaining entries rather than stopping\nimmediately. Removing the setting restores the default behavior where\na negative return from collect_merge_info_callback() breaks out of the\ntraversal loop right away, allowing a future commit to exit early when\na corrupt tree is detected.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 4b8e32209d..74e9636020 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1740,7 +1740,6 @@ static int collect_merge_info(struct merge_options *opt,\n \tsetup_traverse_info(&info, opt->priv->toplevel_dir);\n \tinfo.fn = collect_merge_info_callback;\n \tinfo.data = opt;\n-\tinfo.show_all_errors = 1;\n \n \tif (repo_parse_tree(opt->repo, merge_base) < 0 ||\n \t repo_parse_tree(opt->repo, side1) < 0 ||\n-- \ngitgitgadget\n\n"},{"id":"542002","messageId":"d422f73e129535ffc7b24e64f4cfae1335f25452.1776731171.git.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.git.1776731171.gitgitgadget@gmail.com","subject":"[PATCH 3/5] merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-21T00:26:09Z","receivedAt":"2026-04-21T00:26:24Z","isPatch":true,"body":"From: Elijah Newren \n\nclear_or_reinit_internal_opts() is responsible for cleaning up the\nvarious data structures in merge_options_internal. It already handles\nmany renames-related structures (dirs_removed, dir_renames,\nrelevant_sources, cached_pairs, deferred, etc.) but does not free\nrenames->pairs[].queue.\n\nIn the normal code path, resolve_and_process_renames() frees\npairs[s].queue and reinitializes it with diff_queue_init() before\nclear_or_reinit_internal_opts() runs, so the omission is harmless.\nHowever, if collect_merge_info() encounters an error and returns early\n(before resolve_and_process_renames() is ever called), any diff pairs\nalready queued by collect_rename_info()/add_pair() will have their\nbacking array leaked.\n\nFix this by freeing renames->pairs[].queue in the cleanup function.\nIn the normal path the pointer is already NULL (from the earlier\ndiff_queue_init() in resolve_and_process_renames()), so free(NULL) is\na safe no-op.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 74e9636020..8f911cb639 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -728,6 +728,8 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n \t\tstrintmap_clear_func(&renames->deferred[i].possible_trivial_merges);\n \t\tstrset_clear_func(&renames->deferred[i].target_dirs);\n \t\trenames->deferred[i].trivial_merges_okay = 1; /* 1 == maybe */\n+\t\tfree(renames->pairs[i].queue);\n+\t\tdiff_queue_init(&renames->pairs[i]);\n \t}\n \trenames->cached_pairs_valid_side = 0;\n \trenames->dir_rename_mask = 0;\n-- \ngitgitgadget\n\n"},{"id":"542003","messageId":"0d81c027aafcb386398836ffc73b058b7ea4c702.1776731171.git.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.git.1776731171.gitgitgadget@gmail.com","subject":"[PATCH 4/5] merge-ort: abort merge when trees have duplicate entries","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-21T00:26:10Z","receivedAt":"2026-04-21T00:26:26Z","isPatch":true,"body":"From: Elijah Newren \n\nTrees with duplicate entries are malformed; fsck reports \"contains\nduplicate file entries\" for them. merge-ort has from the beginning\nassumed that we would never hit such trees. It was written with the\nassumption that traverse_trees() calls collect_merge_info_callback() at\nmost once per path. The \"sanity checks\" in that callback (added in\nd2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),\n2020-12-13)) verify properties of each individual call but not that\ninvariant. The strmap_put() in setup_path_info() silently overwrites\nthe entry from any prior call for the same path, because it assumed\nthere would be no other path. Unfortunately, supplemental data\nstructures for various optimizations could still be tweaked before the\nextra paths were overwritten, and those data structures not matching\nexpected state could trip various assertions.\n\nChange the return type of setup_path_info() from void to int to allow us\nto detect this case, and abort the merge with a clear error message when\nit occurs.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 61 ++++++++++++++++------------\n t/t6422-merge-rename-corner-cases.sh | 54 ++++++++++++++++++++++++\n 2 files changed, 88 insertions(+), 27 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8f911cb639..be0829bbb7 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1026,18 +1026,18 @@ static int traverse_trees_wrapper(struct index_state *istate,\n \treturn ret < 0 ? ret : 0;\n }\n \n-static void setup_path_info(struct merge_options *opt,\n-\t\t\t struct string_list_item *result,\n-\t\t\t const char *current_dir_name,\n-\t\t\t int current_dir_name_len,\n-\t\t\t char *fullpath, /* we'll take over ownership */\n-\t\t\t struct name_entry *names,\n-\t\t\t struct name_entry *merged_version,\n-\t\t\t unsigned is_null, /* boolean */\n-\t\t\t unsigned df_conflict, /* boolean */\n-\t\t\t unsigned filemask,\n-\t\t\t unsigned dirmask,\n-\t\t\t int resolved /* boolean */)\n+static int setup_path_info(struct merge_options *opt,\n+\t\t\t struct string_list_item *result,\n+\t\t\t const char *current_dir_name,\n+\t\t\t int current_dir_name_len,\n+\t\t\t char *fullpath, /* we'll take over ownership */\n+\t\t\t struct name_entry *names,\n+\t\t\t struct name_entry *merged_version,\n+\t\t\t unsigned is_null, /* boolean */\n+\t\t\t unsigned df_conflict, /* boolean */\n+\t\t\t unsigned filemask,\n+\t\t\t unsigned dirmask,\n+\t\t\t int resolved /* boolean */)\n {\n \t/* result->util is void*, so mi is a convenience typed variable */\n \tstruct merged_info *mi;\n@@ -1081,9 +1081,11 @@ static void setup_path_info(struct merge_options *opt,\n \t\t\t */\n \t\t\tmi->is_null = 1;\n \t}\n-\tstrmap_put(&opt->priv->paths, fullpath, mi);\n+\tif (strmap_put(&opt->priv->paths, fullpath, mi))\n+\t\treturn error(_(\"tree has duplicate entries for '%s'\"), fullpath);\n \tresult->string = fullpath;\n \tresult->util = mi;\n+\treturn 0;\n }\n \n static void add_pair(struct merge_options *opt,\n@@ -1350,9 +1352,10 @@ static int collect_merge_info_callback(int n,\n \t */\n \tif (side1_matches_mbase && side2_matches_mbase) {\n \t\t/* mbase, side1, & side2 all match; use mbase as resolution */\n-\t\tsetup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n-\t\t\t\tnames, names+0, mbase_null, 0 /* df_conflict */,\n-\t\t\t\tfilemask, dirmask, 1 /* resolved */);\n+\t\tif (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n+\t\t\t\t names, names+0, mbase_null, 0 /* df_conflict */,\n+\t\t\t\t filemask, dirmask, 1 /* resolved */))\n+\t\t\treturn -1; /* Quit traversing */\n \t\treturn mask;\n \t}\n \n@@ -1364,9 +1367,10 @@ static int collect_merge_info_callback(int n,\n \t */\n \tif (sides_match && filemask == 0x07) {\n \t\t/* use side1 (== side2) version as resolution */\n-\t\tsetup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n-\t\t\t\tnames, names+1, side1_null, 0,\n-\t\t\t\tfilemask, dirmask, 1);\n+\t\tif (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n+\t\t\t\t names, names+1, side1_null, 0,\n+\t\t\t\t filemask, dirmask, 1))\n+\t\t\treturn -1; /* Quit traversing */\n \t\treturn mask;\n \t}\n \n@@ -1378,18 +1382,20 @@ static int collect_merge_info_callback(int n,\n \t */\n \tif (side1_matches_mbase && filemask == 0x07) {\n \t\t/* use side2 version as resolution */\n-\t\tsetup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n-\t\t\t\tnames, names+2, side2_null, 0,\n-\t\t\t\tfilemask, dirmask, 1);\n+\t\tif (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n+\t\t\t\t names, names+2, side2_null, 0,\n+\t\t\t\t filemask, dirmask, 1))\n+\t\t\treturn -1; /* Quit traversing */\n \t\treturn mask;\n \t}\n \n \t/* Similar to above but swapping sides 1 and 2 */\n \tif (side2_matches_mbase && filemask == 0x07) {\n \t\t/* use side1 version as resolution */\n-\t\tsetup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n-\t\t\t\tnames, names+1, side1_null, 0,\n-\t\t\t\tfilemask, dirmask, 1);\n+\t\tif (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n+\t\t\t\t names, names+1, side1_null, 0,\n+\t\t\t\t filemask, dirmask, 1))\n+\t\t\treturn -1; /* Quit traversing */\n \t\treturn mask;\n \t}\n \n@@ -1413,8 +1419,9 @@ static int collect_merge_info_callback(int n,\n \t * unconflict some more cases, but that comes later so all we can\n \t * do now is record the different non-null file hashes.)\n \t */\n-\tsetup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n-\t\t\tnames, NULL, 0, df_conflict, filemask, dirmask, 0);\n+\tif (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n+\t\t\t names, NULL, 0, df_conflict, filemask, dirmask, 0))\n+\t\treturn -1; /* Quit traversing */\n \n \tci = pi.util;\n \tVERIFY_CI(ci);\ndiff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh\nindex e18d5a227d..81b645bb3b 100755\n--- a/t/t6422-merge-rename-corner-cases.sh\n+++ b/t/t6422-merge-rename-corner-cases.sh\n@@ -1525,4 +1525,58 @@ test_expect_success 'submodule/directory preliminary conflict' '\n \t)\n '\n \n+# Testcase: submodule/directory conflict with duplicate tree entries\n+# One side has a path as a gitlink (submodule). The other side replaces\n+# the gitlink with a directory. A third-party tool creates a tree on the\n+# submodule side that has *both* a gitlink and a tree entry for the same\n+# path (adding a file inside the submodule path ignoring that there's a\n+# gitlink there). collect_merge_info_callback() should detect the\n+# duplicate and abort rather than silently corrupting its bookkeeping.\n+\n+test_expect_success 'duplicate tree entries trigger an error' '\n+\ttest_when_finished \"rm -rf duplicate-entry\" &&\n+\tgit init duplicate-entry &&\n+\t(\n+\t\tcd duplicate-entry &&\n+\n+\t\t# Base commit: \"docs\" is a gitlink (submodule)\n+\t\tempty_tree=$(git mktree file.txt &&\n+\t\tgit add file.txt &&\n+\t\tgit commit -m base &&\n+\n+\t\t# side1: remove the gitlink, replace with a directory\n+\t\tgit checkout -b side1 &&\n+\t\tgit rm --cached docs &&\n+\t\tmkdir -p docs &&\n+\t\techo hello >docs/requirements.txt &&\n+\t\tgit add docs/requirements.txt &&\n+\t\tgit commit -m \"side1: submodule to directory\" &&\n+\n+\t\t# side2: keep the gitlink but craft a tree that also\n+\t\t# contains a tree entry for \"docs\" (simulating a tool\n+\t\t# that adds files inside a submodule path without\n+\t\t# removing the gitlink first).\n+\t\tgit checkout main &&\n+\t\tgit checkout -b side2 &&\n+\t\tblob_oid=$(echo world | git hash-object -w --stdin) &&\n+\t\tdocs_tree=$(printf \"100644 blob %s\\trequirements.txt\\n\" \\\n+\t\t\t\"$blob_oid\" | git mktree) &&\n+\t\tcur_tree=$(git rev-parse HEAD^{tree}) &&\n+\t\tgit cat-file -p $cur_tree >tree-listing &&\n+\t\tprintf \"040000 tree %s\\tdocs\\n\" \"$docs_tree\" >>tree-listing &&\n+\t\tnew_tree=$(git mktree err &&\n+\t\ttest_grep \"duplicate entries\" err\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"542004","messageId":"a87bbaa84fd5dcb2a585f82c4a5dfa1572b54588.1776731171.git.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.git.1776731171.gitgitgadget@gmail.com","subject":"[PATCH 5/5] cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-21T00:26:11Z","receivedAt":"2026-04-21T00:26:29Z","isPatch":true,"body":"From: Elijah Newren \n\nverify_cache() checks that the index does not contain both \"path\" and\n\"path/file\" before writing a tree. It does this by comparing only\nadjacent entries, relying on the assumption that \"path/file\" would\nimmediately follow \"path\" in sorted order. Unfortunately, this\nassumption does not always hold. For example:\n\n docs <-- submodule entry\n docs-internal/README.md <-- intervening entry\n docs/requirements.txt <-- D/F conflict, NOT adjacent to \"docs\"\n\nWhen this happens, verify_cache() silently misses the D/F conflict and\nwrite-tree produces a corrupt tree object containing duplicate entries\n(one for the submodule \"docs\" and one for the tree \"docs\").\n\nI could not find any caller in current git that both allows the index to\nget into this state and then tries to write it out without doing other\nchecks beyond the verify_cache() call in cache_tree_update(), but\nverify_cache() is documented as a safety net for preventing corrupt\ntrees and should actually provide that guarantee. A downstream consumer\nthat relied solely on cache_tree_update()'s internal checking via\nverify_cache() to prevent duplicate tree entries was bitten by the gap.\n\nAdd a test that constructs a corrupt index directly (bypassing the D/F\nchecks in add_index_entry) and verifies that write-tree now rejects it.\n\nSigned-off-by: Elijah Newren \n---\n cache-tree.c | 46 ++++++++++++++++++++++++--\n t/meson.build | 1 +\n t/t0093-direct-index-write.pl | 38 ++++++++++++++++++++++\n t/t0093-verify-cache-df-gap.sh | 59 ++++++++++++++++++++++++++++++++++\n 4 files changed, 141 insertions(+), 3 deletions(-)\n create mode 100644 t/t0093-direct-index-write.pl\n create mode 100755 t/t0093-verify-cache-df-gap.sh\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 7881b42aa2..f11844fe72 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -192,22 +192,62 @@ static int verify_cache(struct index_state *istate, int flags)\n \tfor (i = 0; i + 1 < istate->cache_nr; i++) {\n \t\t/* path/file always comes after path because of the way\n \t\t * the cache is sorted. Also path can appear only once,\n-\t\t * which means conflicting one would immediately follow.\n+\t\t * so path/file is likely the immediately following path\n+\t\t * but might be separated if there is e.g. a\n+\t\t * path-internal/... file.\n \t\t */\n \t\tconst struct cache_entry *this_ce = istate->cache[i];\n \t\tconst struct cache_entry *next_ce = istate->cache[i + 1];\n \t\tconst char *this_name = this_ce->name;\n \t\tconst char *next_name = next_ce->name;\n \t\tint this_len = ce_namelen(this_ce);\n+\t\tconst char *conflict_name = NULL;\n+\n \t\tif (this_len < ce_namelen(next_ce) &&\n-\t\t next_name[this_len] == '/' &&\n+\t\t next_name[this_len] <= '/' &&\n \t\t strncmp(this_name, next_name, this_len) == 0) {\n+\t\t\tif (next_name[this_len] == '/') {\n+\t\t\t\tconflict_name = next_name;\n+\t\t\t} else if (next_name[this_len] < '/') {\n+\t\t\t\t/*\n+\t\t\t\t * The immediately next entry shares our\n+\t\t\t\t * prefix but sorts before \"path/\" (e.g.,\n+\t\t\t\t * \"path-internal\" between \"path\" and\n+\t\t\t\t * \"path/file\", since '-' (0x2D) < '/'\n+\t\t\t\t * (0x2F)). Binary search to find where\n+\t\t\t\t * \"path/\" would be and check for a D/F\n+\t\t\t\t * conflict there.\n+\t\t\t\t */\n+\t\t\t\tstruct cache_entry *other;\n+\t\t\t\tstruct strbuf probe = STRBUF_INIT;\n+\t\t\t\tint pos;\n+\n+\t\t\t\tstrbuf_add(&probe, this_name, this_len);\n+\t\t\t\tstrbuf_addch(&probe, '/');\n+\t\t\t\tpos = index_name_pos_sparse(istate,\n+\t\t\t\t\t\t\t probe.buf,\n+\t\t\t\t\t\t\t probe.len);\n+\t\t\t\tstrbuf_release(&probe);\n+\n+\t\t\t\tif (pos < 0)\n+\t\t\t\t\tpos = -pos - 1;\n+\t\t\t\tif (pos >= (int)istate->cache_nr)\n+\t\t\t\t\tcontinue;\n+\t\t\t\tother = istate->cache[pos];\n+\t\t\t\tif (ce_namelen(other) > this_len &&\n+\t\t\t\t other->name[this_len] == '/' &&\n+\t\t\t\t !strncmp(this_name, other->name, this_len))\n+\t\t\t\t\tconflict_name = other->name;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (conflict_name) {\n \t\t\tif (10 < ++funny) {\n \t\t\t\tfprintf(stderr, \"...\\n\");\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tfprintf(stderr, \"You have both %s and %s\\n\",\n-\t\t\t\tthis_name, next_name);\n+\t\t\t\tthis_name, conflict_name);\n \t\t}\n \t}\n \tif (funny)\ndiff --git a/t/meson.build b/t/meson.build\nindex 7528e5cda5..362177999b 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -124,6 +124,7 @@ integration_tests = [\n 't0090-cache-tree.sh',\n 't0091-bugreport.sh',\n 't0092-diagnose.sh',\n+ 't0093-verify-cache-df-gap.sh',\n 't0095-bloom.sh',\n 't0100-previous.sh',\n 't0101-at-syntax.sh',\ndiff --git a/t/t0093-direct-index-write.pl b/t/t0093-direct-index-write.pl\nnew file mode 100644\nindex 0000000000..2881a3ebb2\n--- /dev/null\n+++ b/t/t0093-direct-index-write.pl\n@@ -0,0 +1,38 @@\n+#!/usr/bin/perl\n+#\n+# Build a v2 index file from entries listed on stdin.\n+# Each line: \"octalmode hex-oid name\"\n+# Output: binary index written to stdout.\n+#\n+# This bypasses all D/F safety checks in add_index_entry(), simulating\n+# what happens when code uses ADD_CACHE_JUST_APPEND to bulk-load entries.\n+use strict;\n+use warnings;\n+use Digest::SHA qw(sha1 sha256);\n+\n+my $hash_algo = $ENV{'GIT_DEFAULT_HASH'} || 'sha1';\n+my $hash_func = $hash_algo eq 'sha256' ? \\&sha256 : \\&sha1;\n+\n+my @entries;\n+while (my $line = ) {\n+\tchomp $line;\n+\tmy ($mode, $oid_hex, $name) = split(/ /, $line, 3);\n+\tpush @entries, [$mode, $oid_hex, $name];\n+}\n+\n+my $body = \"DIRC\" . pack(\"NN\", 2, scalar @entries);\n+\n+for my $ent (@entries) {\n+\tmy ($mode, $oid_hex, $name) = @{$ent};\n+\t# 10 x 32-bit stat fields (zeroed), with mode in position 7\n+\tmy $stat = pack(\"N10\", 0, 0, 0, 0, 0, 0, oct($mode), 0, 0, 0);\n+\tmy $oid = pack(\"H*\", $oid_hex);\n+\tmy $flags = pack(\"n\", length($name) & 0xFFF);\n+\tmy $entry = $stat . $oid . $flags . $name . \"\\0\";\n+\t# Pad to 8-byte boundary\n+\twhile (length($entry) % 8) { $entry .= \"\\0\"; }\n+\t$body .= $entry;\n+}\n+\n+binmode STDOUT;\n+print $body . $hash_func->($body);\ndiff --git a/t/t0093-verify-cache-df-gap.sh b/t/t0093-verify-cache-df-gap.sh\nnew file mode 100755\nindex 0000000000..0b6829d805\n--- /dev/null\n+++ b/t/t0093-verify-cache-df-gap.sh\n@@ -0,0 +1,59 @@\n+#!/bin/sh\n+\n+test_description='verify_cache() must catch non-adjacent D/F conflicts\n+\n+Ensure that verify_cache() can complain about bad entries like:\n+\n+ docs <-- submodule\n+ docs-internal/... <-- sorts here because \"-\" < \"/\"\n+ docs/... <-- D/F conflict with \"docs\" above, not adjacent\n+\n+In order to test verify_cache, we directly construct a corrupt index\n+(bypassing the D/F safety checks in add_index_entry) and verify that\n+write-tree rejects it.\n+'\n+\n+. ./test-lib.sh\n+\n+if ! test_have_prereq PERL\n+then\n+\tskip_all='skipping verify_cache D/F tests; Perl not available'\n+\ttest_done\n+fi\n+\n+# Build a v2 index from entries on stdin, bypassing D/F checks.\n+# Each line: \"octalmode hex-oid name\" (entries must be pre-sorted).\n+build_corrupt_index () {\n+\tperl \"$TEST_DIRECTORY/t0093-direct-index-write.pl\" >\"$1\"\n+}\n+\n+test_expect_success 'setup objects' '\n+\ttest_commit base &&\n+\tBLOB=$(git rev-parse HEAD:base.t) &&\n+\tSUB_COMMIT=$(git rev-parse HEAD)\n+'\n+\n+test_expect_success 'adjacent D/F conflict is caught by verify_cache' '\n+\tcat >index-entries <<-EOF &&\n+\t0160000 $SUB_COMMIT docs\n+\t0100644 $BLOB docs/requirements.txt\n+\tEOF\n+\tbuild_corrupt_index .git/index err &&\n+\ttest_grep \"You have both docs and docs/requirements.txt\" err\n+'\n+\n+test_expect_success 'non-adjacent D/F conflict is caught by verify_cache' '\n+\tcat >index-entries <<-EOF &&\n+\t0160000 $SUB_COMMIT docs\n+\t0100644 $BLOB docs-internal/README.md\n+\t0100644 $BLOB docs/requirements.txt\n+\tEOF\n+\tbuild_corrupt_index .git/index err &&\n+\ttest_grep \"You have both docs and docs/requirements.txt\" err\n+'\n+\n+test_done\n-- \ngitgitgadget\n"},{"id":"544377","messageId":"xmqq33z65ui1.fsf@gitster.g","threadId":"65527","inReplyTo":"282f906d1b4767d95e2a66072c280c2294a93a9f.1776731171.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] merge-ort: propagate callback errors from traverse_trees_wrapper()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-01T12:13:10Z","receivedAt":"2026-06-01T12:13:13Z","isPatch":true,"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> traverse_trees_wrapper() saves entries from a first pass through\n> traverse_trees() and then replays them through the real callback\n> (collect_merge_info_callback). However, the replay loop silently\n> discards the callback return value. This means any error reported by\n> the callback during replay -- including a future check for malformed\n> trees -- would be ignored, allowing the merge to proceed with corrupt\n> state.\n>\n> Capture the return value, stop the loop on negative (error) returns,\n> and propagate the error to the caller. Note that the callback returns\n> a positive mask value on success, so we normalize non-negative returns\n> to 0 for the caller.\n\nAll makes perfect sense.\n\nHow would the externally visible behaviour change at this step?\n\nUpon an error from the callback, we used to keep going and processed\nother callback data in the renames structure. We now leave the rest\nunprocessed.\n\nThe caller of this helper would never have seen a failure, but now\nthey will. Both callers, collect_merge_info_callback() and\nhandle_deferred_entries(), are reacting to a negative \"error\" return\nwell (perhaps because they sometimes call traverse_trees() in the\nsame control flow, which does return an error already), so\npresumably there is no downside caused by aborting the innermost\nprocess upon the first error return.\n\n\n\n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 14 ++++++++------\n> 1 file changed, 8 insertions(+), 6 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 00923ce3cd..4b8e32209d 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -1008,18 +1008,20 @@ static int traverse_trees_wrapper(struct index_state *istate,\n> \tinfo->traverse_path = renames->callback_data_traverse_path;\n> \tinfo->fn = old_fn;\n> \tfor (i = old_offset; i < renames->callback_data_nr; ++i) {\n> -\t\tinfo->fn(n,\n> -\t\t\t renames->callback_data[i].mask,\n> -\t\t\t renames->callback_data[i].dirmask,\n> -\t\t\t renames->callback_data[i].names,\n> -\t\t\t info);\n> +\t\tret = info->fn(n,\n> +\t\t\t renames->callback_data[i].mask,\n> +\t\t\t renames->callback_data[i].dirmask,\n> +\t\t\t renames->callback_data[i].names,\n> +\t\t\t info);\n> +\t\tif (ret < 0)\n> +\t\t\tbreak;\n> \t}\n> \n> \trenames->callback_data_nr = old_offset;\n> \tfree(renames->callback_data_traverse_path);\n> \trenames->callback_data_traverse_path = old_callback_data_traverse_path;\n> \tinfo->traverse_path = NULL;\n> -\treturn 0;\n> +\treturn ret < 0 ? ret : 0;\n> }\n> \n> static void setup_path_info(struct merge_options *opt,\n"},{"id":"544378","messageId":"xmqqy0gy4fgx.fsf@gitster.g","threadId":"65527","inReplyTo":"949b5d8e3f3aefd9497a7b85d860259b9d5db418.1776731171.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] merge-ort: drop unnecessary show_all_errors from collect_merge_info()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-01T12:23:10Z","receivedAt":"2026-06-01T12:23:12Z","isPatch":true,"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> collect_merge_info() has set info.show_all_errors = 1 since\n> d2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),\n> 2020-12-13). This setting was copied from unpack-trees.c where it\n> controls batching of error messages for porcelain display, but\n> merge-ort has no such error-batching logic and never needed it.\n>\n> With show_all_errors set, traverse_trees() captures a negative callback\n> return but continues processing remaining entries rather than stopping\n> immediately. Removing the setting restores the default behavior where\n> a negative return from collect_merge_info_callback() breaks out of the\n> traversal loop right away, allowing a future commit to exit early when\n> a corrupt tree is detected.\n\nNice spotting. As the error handling eventually is to die without\nmaking any further damange, returning early without seeing \"more\nerrors\" is a good change.\n\n>\n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 1 -\n> 1 file changed, 1 deletion(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 4b8e32209d..74e9636020 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -1740,7 +1740,6 @@ static int collect_merge_info(struct merge_options *opt,\n> \tsetup_traverse_info(&info, opt->priv->toplevel_dir);\n> \tinfo.fn = collect_merge_info_callback;\n> \tinfo.data = opt;\n> -\tinfo.show_all_errors = 1;\n> \n> \tif (repo_parse_tree(opt->repo, merge_base) < 0 ||\n> \t repo_parse_tree(opt->repo, side1) < 0 ||\n"},{"id":"544379","messageId":"xmqqtsrm4fgv.fsf@gitster.g","threadId":"65527","inReplyTo":"0d81c027aafcb386398836ffc73b058b7ea4c702.1776731171.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] merge-ort: abort merge when trees have duplicate entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-01T12:23:12Z","receivedAt":"2026-06-01T12:23:14Z","isPatch":true,"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> Trees with duplicate entries are malformed; fsck reports \"contains\n> duplicate file entries\" for them. merge-ort has from the beginning\n> assumed that we would never hit such trees. It was written with the\n> assumption that traverse_trees() calls collect_merge_info_callback() at\n> most once per path. The \"sanity checks\" in that callback (added in\n> d2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),\n> 2020-12-13)) verify properties of each individual call but not that\n> invariant. The strmap_put() in setup_path_info() silently overwrites\n> the entry from any prior call for the same path, because it assumed\n> there would be no other path. Unfortunately, supplemental data\n> structures for various optimizations could still be tweaked before the\n> extra paths were overwritten, and those data structures not matching\n> expected state could trip various assertions.\n>\n> Change the return type of setup_path_info() from void to int to allow us\n> to detect this case, and abort the merge with a clear error message when\n> it occurs.\n\nOK.\n\n> @@ -1081,9 +1081,11 @@ static void setup_path_info(struct merge_options *opt,\n> \t\t\t */\n> \t\t\tmi->is_null = 1;\n> \t}\n> -\tstrmap_put(&opt->priv->paths, fullpath, mi);\n> +\tif (strmap_put(&opt->priv->paths, fullpath, mi))\n> +\t\treturn error(_(\"tree has duplicate entries for '%s'\"), fullpath);\n\nOK. I was wondering what _other_ kind of malformed trees would the\nupdated code by this change is prepared to handle (most notably,\ntree entries must be sorted, and one way to detect duplicate is to\nremember one single path that we saw earlier, which would work as\nlong as the entries are sorted). This \"ah, we saw that path already\"\napproach is much more robust in that it does not have to depend on a\nsorted tree.\n\nMakes sense.\n"},{"id":"544380","messageId":"xmqqpl2a4f09.fsf@gitster.g","threadId":"65527","inReplyTo":"pull.2096.git.1776731171.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] Duplicate entry hardening","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-01T12:33:10Z","receivedAt":"2026-06-01T12:33:12Z","isPatch":true,"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> We had some corrupt trees with duplicate entries in real world repositories,\n> which triggered an assertion failure in merge-ort. Further, the corrupt tree\n> creation in the third party tool would have been avoided had verify_cache()\n> correctly checked for D/F conflicts. Provide fixes for both issues,\n> including 3 preparatory changes for the merge-ort fix.\n>\n> Elijah Newren (5):\n> merge-ort: propagate callback errors from traverse_trees_wrapper()\n> merge-ort: drop unnecessary show_all_errors from collect_merge_info()\n> merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()\n> merge-ort: abort merge when trees have duplicate entries\n> cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts\n\nThis is a fix to an important corner of our system, but somehow left\nin \"Needs review\" state for much longer than I would have liked, so\neven though I am officially on vacation ;-), I took some time to\nread these through (by the way it was a pleasant read, thank you).\n\nI wonder if we create a rule like\n\n Those of you who have more than 30 commits in our project are\n expected to review one topic (or more) from other contributors\n for every three patches you send and ask for reviews by others.\n\nit would help balance the patch vs review ratio, perhaps?\n\n"},{"id":"544381","messageId":"xmqqldcy4f07.fsf@gitster.g","threadId":"65527","inReplyTo":"a87bbaa84fd5dcb2a585f82c4a5dfa1572b54588.1776731171.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-01T12:33:12Z","receivedAt":"2026-06-01T12:33:14Z","isPatch":true,"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> I could not find any caller in current git that both allows the index to\n> get into this state and then tries to write it out without doing other\n> checks beyond the verify_cache() call in cache_tree_update(), but\n> verify_cache() is documented as a safety net for preventing corrupt\n> trees and should actually provide that guarantee.\n\nOh, absolutely. This kind of tightening is very much appreciated.\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 7881b42aa2..f11844fe72 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -192,22 +192,62 @@ static int verify_cache(struct index_state *istate, int flags)\n> \tfor (i = 0; i + 1 < istate->cache_nr; i++) {\n> \t\t/* path/file always comes after path because of the way\n> \t\t * the cache is sorted. Also path can appear only once,\n> -\t\t * which means conflicting one would immediately follow.\n> +\t\t * so path/file is likely the immediately following path\n> +\t\t * but might be separated if there is e.g. a\n> +\t\t * path-internal/... file.\n> \t\t */\n> \t\tconst struct cache_entry *this_ce = istate->cache[i];\n> \t\tconst struct cache_entry *next_ce = istate->cache[i + 1];\n> \t\tconst char *this_name = this_ce->name;\n> \t\tconst char *next_name = next_ce->name;\n> \t\tint this_len = ce_namelen(this_ce);\n> +\t\tconst char *conflict_name = NULL;\n> +\n> \t\tif (this_len < ce_namelen(next_ce) &&\n> -\t\t next_name[this_len] == '/' &&\n> +\t\t next_name[this_len] <= '/' &&\n> \t\t strncmp(this_name, next_name, this_len) == 0) {\n> +\t\t\tif (next_name[this_len] == '/') {\n> +\t\t\t\tconflict_name = next_name;\n> +\t\t\t} else if (next_name[this_len] < '/') {\n> +\t\t\t\t/*\n> +\t\t\t\t * The immediately next entry shares our\n> +\t\t\t\t * prefix but sorts before \"path/\" (e.g.,\n> +\t\t\t\t * \"path-internal\" between \"path\" and\n> +\t\t\t\t * \"path/file\", since '-' (0x2D) < '/'\n> +\t\t\t\t * (0x2F)). Binary search to find where\n> +\t\t\t\t * \"path/\" would be and check for a D/F\n> +\t\t\t\t * conflict there.\n> +\t\t\t\t */\n> +\t\t\t\tstruct cache_entry *other;\n> +\t\t\t\tstruct strbuf probe = STRBUF_INIT;\n> +\t\t\t\tint pos;\n> +\n> +\t\t\t\tstrbuf_add(&probe, this_name, this_len);\n> +\t\t\t\tstrbuf_addch(&probe, '/');\n> +\t\t\t\tpos = index_name_pos_sparse(istate,\n> +\t\t\t\t\t\t\t probe.buf,\n> +\t\t\t\t\t\t\t probe.len);\n> +\t\t\t\tstrbuf_release(&probe);\n> +\n> +\t\t\t\tif (pos < 0)\n> +\t\t\t\t\tpos = -pos - 1;\n> +\t\t\t\tif (pos >= (int)istate->cache_nr)\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\tother = istate->cache[pos];\n> +\t\t\t\tif (ce_namelen(other) > this_len &&\n> +\t\t\t\t other->name[this_len] == '/' &&\n> +\t\t\t\t !strncmp(this_name, other->name, this_len))\n> +\t\t\t\t\tconflict_name = other->name;\n> +\t\t\t}\n> +\t\t}\n\nThe narrow and tall comment block is a sign that this loop is\ngetting too deeply nested. I wonder if it makes it easier to follow\nif we extract this new logic into a small helper function on its\nown?\n\nWhat the code checks and how it does so both make sense to me, though.\n\nThanks.\n"},{"id":"544390","messageId":"ah2PLBluBFy44AQI@pks.im","threadId":"65527","inReplyTo":"xmqqpl2a4f09.fsf@gitster.g","subject":"Re: [PATCH 0/5] Duplicate entry hardening","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-01T13:54:52Z","receivedAt":"2026-06-01T13:55:06Z","isPatch":true,"body":"On Mon, Jun 01, 2026 at 09:33:10PM +0900, Junio C Hamano wrote:\n> \"Elijah Newren via GitGitGadget\" writes:\n> \n> > We had some corrupt trees with duplicate entries in real world repositories,\n> > which triggered an assertion failure in merge-ort. Further, the corrupt tree\n> > creation in the third party tool would have been avoided had verify_cache()\n> > correctly checked for D/F conflicts. Provide fixes for both issues,\n> > including 3 preparatory changes for the merge-ort fix.\n> >\n> > Elijah Newren (5):\n> > merge-ort: propagate callback errors from traverse_trees_wrapper()\n> > merge-ort: drop unnecessary show_all_errors from collect_merge_info()\n> > merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()\n> > merge-ort: abort merge when trees have duplicate entries\n> > cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts\n> \n> This is a fix to an important corner of our system, but somehow left\n> in \"Needs review\" state for much longer than I would have liked, so\n> even though I am officially on vacation ;-), I took some time to\n> read these through (by the way it was a pleasant read, thank you).\n\nHonestly, I always shy away from the merge-related subsystems. It has a\nlot of subtleties that I don't have any experience with, so I never\nreally consider my input to be helpful here.\n\n> I wonder if we create a rule like\n> \n> Those of you who have more than 30 commits in our project are\n> expected to review one topic (or more) from other contributors\n> for every three patches you send and ask for reviews by others.\n\nHeh, that would make me condense patch series into fewer patches ;)\n\n> it would help balance the patch vs review ratio, perhaps?\n\nIt's a good question. I typically try to aim for reviewing series on the\nmailing list at least every second day, and I always encourage other\nfolks in my team to do the same. But recently I (well, rather we)\nhaven't really been able to due to the current situation at GitLab,\nwhich forces us to put almost all of our focus towards a different\nproject for a while.\n\nOverall I agree that everyone who is a core contributor should also make\nreviews part of their regular worflow. At least for corporate\ncontributors that might also make it easier to communicate this to their\nrespective employers. Regardless of that, my expectation is that there\nwill be times where it works well, and other times where it works less\nwell.\n\nPatrick\n"},{"id":"545376","messageId":"CAP8UFD35cLP6FcEuPr+SghKae1ew4JWLWYAoMQ-fuEOu-JmZdg@mail.gmail.com","threadId":"65527","inReplyTo":"ah2PLBluBFy44AQI@pks.im","subject":"Automated reviews by AI (was Re: [PATCH 0/5] Duplicate entry hardening)","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-06-12T13:29:24Z","receivedAt":"2026-06-12T13:29:37Z","isPatch":true,"body":"On Tue, Jun 2, 2026 at 8:16 AM Patrick Steinhardt wrote:\n>\n> On Mon, Jun 01, 2026 at 09:33:10PM +0900, Junio C Hamano wrote:\n\n> > This is a fix to an important corner of our system, but somehow left\n> > in \"Needs review\" state for much longer than I would have liked, so\n> > even though I am officially on vacation ;-), I took some time to\n> > read these through (by the way it was a pleasant read, thank you).\n>\n> Honestly, I always shy away from the merge-related subsystems. It has a\n> lot of subtleties that I don't have any experience with, so I never\n> really consider my input to be helpful here.\n>\n> > I wonder if we create a rule like\n> >\n> > Those of you who have more than 30 commits in our project are\n> > expected to review one topic (or more) from other contributors\n> > for every three patches you send and ask for reviews by others.\n>\n> Heh, that would make me condense patch series into fewer patches ;)\n>\n> > it would help balance the patch vs review ratio, perhaps?\n>\n> It's a good question. I typically try to aim for reviewing series on the\n> mailing list at least every second day, and I always encourage other\n> folks in my team to do the same. But recently I (well, rather we)\n> haven't really been able to due to the current situation at GitLab,\n> which forces us to put almost all of our focus towards a different\n> project for a while.\n>\n> Overall I agree that everyone who is a core contributor should also make\n> reviews part of their regular worflow. At least for corporate\n> contributors that might also make it easier to communicate this to their\n> respective employers. Regardless of that, my expectation is that there\n> will be times where it works well, and other times where it works less\n> well.\n\nSashiko (https://github.com/sashiko-dev/sashiko) is used these days by\nLinux kernel developers and seems to work well for them.\n\nAt GitLab and probably in other companies, some of us also use AI to\nreview our work before sending it to the mailing list. And yeah, it\nhelps find issues before our patches reach the mailing list.\n\nIn the same way as we require that patches must pass CI, do we want to\nrequire that patches \"pass\" an AI review before they get accepted?\n\nThe benefit would be that it would hopefully catch a lot of trivial\nthings like indentation, typos/grammos, etc, and a lot of things a bit\nmore difficult to spot like memory issues. Perhaps with some amount of\nprompting/configuration (for example pointing it at our\nCodingGuidelines and SubmittingPatches) it could also catch issues\nlike style issues, commits that do too many things, refactoring\nopportunities, etc.\n\nWe would likely still require at least one human review (by someone\nwho is not the maintainer) to validate architectural decisions, to\nmake sure it goes in the same direction as other efforts, and perhaps\nalso to make sure that AI suggestions were properly handled by the\npatch author.\n\nIf we decide to require it, then there are a lot of questions that we\nwill have to answer.\n\nDo we want to have our own system somehow managed by us or would we be\nhappy to use existing systems already in place in some companies as\nlong as we can still tweak them in some ways, like the current CI\nsystems we use?\n\nIf we use existing systems likely at GitLab and GitHub, it might be\nmore difficult to get coherent results as they might use different\nLLMs, but maybe it could help tighten our docs to make sure everyone\nis aligned, and we could get better reviews by using multiple systems\nbecause an LLM might find an issue that the other LLM missed.\n\nDo we want an AI review right after a patch is posted or only if there\nis no human review in the next X days?\n\nAlso what if the AI makes a long concrete suggestion to improve on the\npatches? Could that be incompatible with our AI policy to apply it?\nShould we try to prevent the AI from making such a suggestion in the\nfirst place?\n\nI haven't looked at how Sashiko is used for the kernel, but maybe\nthere will need to be some kinds of restrictions/authentications to\navoid potential abuse.\n"},{"id":"545413","messageId":"xmqqecibh7w3.fsf@gitster.g","threadId":"65527","inReplyTo":"CAP8UFD35cLP6FcEuPr+SghKae1ew4JWLWYAoMQ-fuEOu-JmZdg@mail.gmail.com","subject":"Re: Automated reviews by AI (was Re: [PATCH 0/5] Duplicate entry hardening)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-12T19:32:28Z","receivedAt":"2026-06-12T19:32:31Z","isPatch":true,"body":"Christian Couder writes:\n\n> On Tue, Jun 2, 2026 at 8:16 AM Patrick Steinhardt wrote:\n>>\n>> Overall I agree that everyone who is a core contributor should also make\n>> reviews part of their regular worflow. At least for corporate\n>> contributors that might also make it easier to communicate this to their\n>> respective employers. Regardless of that, my expectation is that there\n>> will be times where it works well, and other times where it works less\n>> well.\n>\n> Sashiko (https://github.com/sashiko-dev/sashiko) is used these days by\n> Linux kernel developers and seems to work well for them.\n>\n> At GitLab and probably in other companies, some of us also use AI to\n> review our work before sending it to the mailing list. And yeah, it\n> helps find issues before our patches reach the mailing list.\n>\n> In the same way as we require that patches must pass CI, do we want to\n> require that patches \"pass\" an AI review before they get accepted?\n\nI do not think so. You (figuratively, not limited to Christian\nCouder) are welcome to use whatever tool available to you to help\nyou polish your submission, and the higher quality your patches are\n(e.g., fewer typos and jumps in logic flow that interferes the\nthought process of human reviewers), the more helpful you are being\nto the community. The use of GitHub PR initiated CI run falls into\nthe same category, I think, in that we do not require you to have an\naccount and trigger the CI there, but you are doing a good service\nif you made sure you caught breakages on macOS you do not have\naccess to otherwise before sending your patches to the list.\n\nBut I do not think we should require you to bring your own token\nbudget to be able to contribute.\n\n> The benefit would be that it would hopefully catch a lot of trivial\n> things like indentation, typos/grammos, etc, and a lot of things a bit\n> more difficult to spot like memory issues. Perhaps with some amount of\n> prompting/configuration (for example pointing it at our\n> CodingGuidelines and SubmittingPatches) it could also catch issues\n> like style issues, commits that do too many things, refactoring\n> opportunities, etc.\n\nYes.\n\nSimilarly, you are welcome to use tools including AI tools to help\nyou review others' patches, or help sanity check your reviews of\nothers' patches before you send them out. The reason why such an\neffort is valuable to the community is the same.\n\nBut I personally consider that the use of the tools (not limited to\nAI tools) is up to each developer. What counts a lot more is the\nquality of the output. Just like PR driven CI at GitHub is offered\nto everybody who wants to participate and is willing to have an\naccount there, it may help those aspiring developers if automated\nreview services are made easily available, but it is a different\nstory to _require_ use of such service.\n\n"},{"id":"545473","messageId":"CABPp-BFW8vMcPascUsujYSjSz79zUyuryNoyH+Ej2W+f6FNGyw@mail.gmail.com","threadId":"65527","inReplyTo":"xmqqldcy4f07.fsf@gitster.g","subject":"Re: [PATCH 5/5] cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-06-14T03:16:17Z","receivedAt":"2026-06-14T03:16:30Z","isPatch":true,"body":"On Mon, Jun 1, 2026 at 5:33 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n[...]\n> The narrow and tall comment block is a sign that this loop is\n> getting too deeply nested. I wonder if it makes it easier to follow\n> if we extract this new logic into a small helper function on its\n> own?\n\nGood point; I'll break it out into a small helper in v2.\n\n> What the code checks and how it does so both make sense to me, though.\n\nAs always, thanks for the careful review.\n"},{"id":"545474","messageId":"CABPp-BEGvmes=mH=XKf0YYRLB-S2bAd_LB4hqaQOxp9xBCF3Bw@mail.gmail.com","threadId":"65527","inReplyTo":"xmqq33z65ui1.fsf@gitster.g","subject":"Re: [PATCH 1/5] merge-ort: propagate callback errors from traverse_trees_wrapper()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-06-14T03:16:23Z","receivedAt":"2026-06-14T03:16:37Z","isPatch":true,"body":"Sorry for the late reply...\n\nOn Mon, Jun 1, 2026 at 5:13 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > From: Elijah Newren \n> >\n> > traverse_trees_wrapper() saves entries from a first pass through\n> > traverse_trees() and then replays them through the real callback\n> > (collect_merge_info_callback). However, the replay loop silently\n> > discards the callback return value. This means any error reported by\n> > the callback during replay -- including a future check for malformed\n> > trees -- would be ignored, allowing the merge to proceed with corrupt\n> > state.\n> >\n> > Capture the return value, stop the loop on negative (error) returns,\n> > and propagate the error to the caller. Note that the callback returns\n> > a positive mask value on success, so we normalize non-negative returns\n> > to 0 for the caller.\n>\n> All makes perfect sense.\n>\n> How would the externally visible behaviour change at this step?\n\nThere's almost no change at this point. There is only one callpath\nthat can result in a negative return value, from near the top of\ntraverse_trees():\n if (traverse_trees_cur_depth > r->settings.max_allowed_tree_depth)\n return error(\"exceeded maximum allowed tree depth\");\nAll other paths return non-negative values currently, so this patch is\nmostly preparatory for later patches in this series.\n\n> Upon an error from the callback, we used to keep going and processed\n> other callback data in the renames structure. We now leave the rest\n> unprocessed.\n>\n> The caller of this helper would never have seen a failure, but now\n> they will. Both callers, collect_merge_info_callback() and\n> handle_deferred_entries(), are reacting to a negative \"error\" return\n> well (perhaps because they sometimes call traverse_trees() in the\n> same control flow, which does return an error already), so\n> presumably there is no downside caused by aborting the innermost\n> process upon the first error return.\n\nI'd state it a bit differently: not only is there no downwise to\naborting upon the first error, there IS a clear downside from ignoring\nthe errors and attempting to proceed anyway. This code wasn't a\ndeferred error kind of thing; it was an ignored error. For the\nmaximum allowed tree depth issue, we'd just prune the trees below that\ndepth and pretend that was the correct merge. And our lack of\ndetecting duplicate tree entries essentially means that we have a\n\"last one wins\" (are we sure that's really the correct rule?) with the\nadded wrinkle that the first one can toggle various state flags that\ncan further tweak the merge and maybe even trip some assertions.\n\nI'll add some of this info to the commit message.\n"},{"id":"545478","messageId":"pull.2096.v2.git.1781419047.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.git.1776731171.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Duplicate entry hardening","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-06-14T06:37:21Z","receivedAt":"2026-06-14T06:37:29Z","isPatch":true,"body":"We had some corrupt trees with duplicate entries in real world repositories,\nwhich triggered an assertion failure in merge-ort. Further, the corrupt tree\ncreation in the third party tool would have been avoided had verify_cache()\ncorrectly checked for D/F conflicts. Provide fixes for both issues,\nincluding 3 preparatory changes for the merge-ort fix.\n\nChanges since v1:\n\n * Add some more detail to the commit message of Patch 1\n * Split some code out in Patch 5 into a helper function.\n\nElijah Newren (5):\n merge-ort: propagate callback errors from traverse_trees_wrapper()\n merge-ort: drop unnecessary show_all_errors from collect_merge_info()\n merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()\n merge-ort: abort merge when trees have duplicate entries\n cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts\n\n cache-tree.c | 64 +++++++++++++++++++----\n merge-ort.c | 78 ++++++++++++++++------------\n t/meson.build | 1 +\n t/t0093-direct-index-write.pl | 38 ++++++++++++++\n t/t0093-verify-cache-df-gap.sh | 59 +++++++++++++++++++++\n t/t6422-merge-rename-corner-cases.sh | 54 +++++++++++++++++++\n 6 files changed, 249 insertions(+), 45 deletions(-)\n create mode 100644 t/t0093-direct-index-write.pl\n create mode 100755 t/t0093-verify-cache-df-gap.sh\n\n\nbase-commit: e8955061076952cc5eab0300424fc48b601fe12d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2096%2Fnewren%2Fduplicate-entry-hardening-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2096/newren/duplicate-entry-hardening-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/2096\n\nRange-diff vs v1:\n\n 1: 282f906d1b ! 1: dc0f596f31 merge-ort: propagate callback errors from traverse_trees_wrapper()\n @@ Commit message\n traverse_trees_wrapper() saves entries from a first pass through\n traverse_trees() and then replays them through the real callback\n (collect_merge_info_callback). However, the replay loop silently\n - discards the callback return value. This means any error reported by\n - the callback during replay -- including a future check for malformed\n - trees -- would be ignored, allowing the merge to proceed with corrupt\n - state.\n + discards the callback return value. This is not a deferred error;\n + it is an ignored error.\n \n - Capture the return value, stop the loop on negative (error) returns,\n - and propagate the error to the caller. Note that the callback returns\n - a positive mask value on success, so we normalize non-negative returns\n - to 0 for the caller.\n + Today the only originator of a negative return in this entire call\n + graph is traverse_trees()'s \"exceeded maximum allowed tree depth\"\n + check; everything else (collect_merge_info_callback,\n + traverse_trees_wrapper, the inner traverse_trees recursion) only\n + relays that. So in current Git, the visible effect of dropping the\n + replay callback's return value is narrow but bad: a tree nested past\n + core.maxTreeDepth has its -1 swallowed, the subtree below the limit\n + is silently pruned, and the merge completes as if that were the\n + correct result.\n +\n + A later patch in this series will teach collect_merge_info_callback()\n + to return -1 on an additional path -- detecting duplicate\n + entries in malformed trees -- which is similarly handled today by\n + just ignoring the problem (resulting in mostly a \"last one wins\" rule,\n + though the non-last entry can mutate various state flags).\n +\n + Capture the return value, stop the loop on negative returns, and\n + propagate the error to the caller. The callback returns a positive mask\n + value on success, so normalize non-negative returns to\n + 0 for the caller.\n \n Signed-off-by: Elijah Newren \n \n 2: 949b5d8e3f = 2: b4ff725a77 merge-ort: drop unnecessary show_all_errors from collect_merge_info()\n 3: d422f73e12 = 3: 673fbea13f merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()\n 4: 0d81c027aa = 4: b5421244d4 merge-ort: abort merge when trees have duplicate entries\n 5: a87bbaa84f ! 5: cf50f1aabc cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts\n @@ Commit message\n Signed-off-by: Elijah Newren \n \n ## cache-tree.c ##\n +@@ cache-tree.c: void cache_tree_invalidate_path(struct index_state *istate, const char *path)\n + \t\tistate->cache_changed |= CACHE_TREE_CHANGED;\n + }\n + \n ++/*\n ++ * Check whether this_ce and the next entry in the index form a D/F\n ++ * conflict (\"path\" vs \"path/file\"). Returns the conflicting \"path/...\"\n ++ * name when one is found, or NULL otherwise.\n ++ *\n ++ * The cache is sorted, so \"path/file\" sorts after \"path\" and the\n ++ * conflict is usually visible as adjacent entries. But other entries\n ++ * can sort between them -- e.g. \"path-internal\" sits between \"path\"\n ++ * and \"path/file\" because '-' (0x2D) precedes '/' (0x2F) -- so when\n ++ * the immediately following entry shares our prefix but starts with a\n ++ * character that sorts before '/', binary search for \"path/\" instead.\n ++ */\n ++static const char *find_df_conflict(struct index_state *istate,\n ++\t\t\t\t const struct cache_entry *this_ce,\n ++\t\t\t\t const struct cache_entry *next_ce)\n ++{\n ++\tconst char *this_name = this_ce->name;\n ++\tconst char *next_name = next_ce->name;\n ++\tint this_len = ce_namelen(this_ce);\n ++\tconst struct cache_entry *other;\n ++\tstruct strbuf probe = STRBUF_INIT;\n ++\tint pos;\n ++\n ++\tif (this_len >= ce_namelen(next_ce) ||\n ++\t next_name[this_len] > '/' ||\n ++\t strncmp(this_name, next_name, this_len))\n ++\t\treturn NULL;\n ++\n ++\tif (next_name[this_len] == '/')\n ++\t\treturn next_name;\n ++\n ++\tstrbuf_add(&probe, this_name, this_len);\n ++\tstrbuf_addch(&probe, '/');\n ++\tpos = index_name_pos_sparse(istate, probe.buf, probe.len);\n ++\tstrbuf_release(&probe);\n ++\n ++\tif (pos < 0)\n ++\t\tpos = -pos - 1;\n ++\tif (pos >= (int)istate->cache_nr)\n ++\t\treturn NULL;\n ++\tother = istate->cache[pos];\n ++\tif (ce_namelen(other) > this_len &&\n ++\t other->name[this_len] == '/' &&\n ++\t !strncmp(this_name, other->name, this_len))\n ++\t\treturn other->name;\n ++\treturn NULL;\n ++}\n ++\n + static int verify_cache(struct index_state *istate, int flags)\n + {\n + \tunsigned i, funny;\n @@ cache-tree.c: static int verify_cache(struct index_state *istate, int flags)\n + \t */\n + \tfunny = 0;\n \tfor (i = 0; i + 1 < istate->cache_nr; i++) {\n - \t\t/* path/file always comes after path because of the way\n - \t\t * the cache is sorted. Also path can appear only once,\n +-\t\t/* path/file always comes after path because of the way\n +-\t\t * the cache is sorted. Also path can appear only once,\n -\t\t * which means conflicting one would immediately follow.\n -+\t\t * so path/file is likely the immediately following path\n -+\t\t * but might be separated if there is e.g. a\n -+\t\t * path-internal/... file.\n - \t\t */\n +-\t\t */\n \t\tconst struct cache_entry *this_ce = istate->cache[i];\n \t\tconst struct cache_entry *next_ce = istate->cache[i + 1];\n - \t\tconst char *this_name = this_ce->name;\n - \t\tconst char *next_name = next_ce->name;\n - \t\tint this_len = ce_namelen(this_ce);\n -+\t\tconst char *conflict_name = NULL;\n -+\n - \t\tif (this_len < ce_namelen(next_ce) &&\n +-\t\tconst char *this_name = this_ce->name;\n +-\t\tconst char *next_name = next_ce->name;\n +-\t\tint this_len = ce_namelen(this_ce);\n +-\t\tif (this_len < ce_namelen(next_ce) &&\n -\t\t next_name[this_len] == '/' &&\n -+\t\t next_name[this_len] <= '/' &&\n - \t\t strncmp(this_name, next_name, this_len) == 0) {\n -+\t\t\tif (next_name[this_len] == '/') {\n -+\t\t\t\tconflict_name = next_name;\n -+\t\t\t} else if (next_name[this_len] < '/') {\n -+\t\t\t\t/*\n -+\t\t\t\t * The immediately next entry shares our\n -+\t\t\t\t * prefix but sorts before \"path/\" (e.g.,\n -+\t\t\t\t * \"path-internal\" between \"path\" and\n -+\t\t\t\t * \"path/file\", since '-' (0x2D) < '/'\n -+\t\t\t\t * (0x2F)). Binary search to find where\n -+\t\t\t\t * \"path/\" would be and check for a D/F\n -+\t\t\t\t * conflict there.\n -+\t\t\t\t */\n -+\t\t\t\tstruct cache_entry *other;\n -+\t\t\t\tstruct strbuf probe = STRBUF_INIT;\n -+\t\t\t\tint pos;\n -+\n -+\t\t\t\tstrbuf_add(&probe, this_name, this_len);\n -+\t\t\t\tstrbuf_addch(&probe, '/');\n -+\t\t\t\tpos = index_name_pos_sparse(istate,\n -+\t\t\t\t\t\t\t probe.buf,\n -+\t\t\t\t\t\t\t probe.len);\n -+\t\t\t\tstrbuf_release(&probe);\n -+\n -+\t\t\t\tif (pos < 0)\n -+\t\t\t\t\tpos = -pos - 1;\n -+\t\t\t\tif (pos >= (int)istate->cache_nr)\n -+\t\t\t\t\tcontinue;\n -+\t\t\t\tother = istate->cache[pos];\n -+\t\t\t\tif (ce_namelen(other) > this_len &&\n -+\t\t\t\t other->name[this_len] == '/' &&\n -+\t\t\t\t !strncmp(this_name, other->name, this_len))\n -+\t\t\t\t\tconflict_name = other->name;\n -+\t\t\t}\n -+\t\t}\n +-\t\t strncmp(this_name, next_name, this_len) == 0) {\n ++\t\tconst char *conflict_name;\n +\n ++\t\tconflict_name = find_df_conflict(istate, this_ce, next_ce);\n +\t\tif (conflict_name) {\n \t\t\tif (10 < ++funny) {\n \t\t\t\tfprintf(stderr, \"...\\n\");\n @@ cache-tree.c: static int verify_cache(struct index_state *istate, int flags)\n \t\t\t}\n \t\t\tfprintf(stderr, \"You have both %s and %s\\n\",\n -\t\t\t\tthis_name, next_name);\n -+\t\t\t\tthis_name, conflict_name);\n ++\t\t\t\tthis_ce->name, conflict_name);\n \t\t}\n \t}\n \tif (funny)\n\n-- \ngitgitgadget\n"},{"id":"545479","messageId":"dc0f596f31635515cc96160bbb8bdc5b773e2ba6.1781419047.git.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.v2.git.1781419047.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] merge-ort: propagate callback errors from traverse_trees_wrapper()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-06-14T06:37:22Z","receivedAt":"2026-06-14T06:37:31Z","isPatch":true,"body":"From: Elijah Newren \n\ntraverse_trees_wrapper() saves entries from a first pass through\ntraverse_trees() and then replays them through the real callback\n(collect_merge_info_callback). However, the replay loop silently\ndiscards the callback return value. This is not a deferred error;\nit is an ignored error.\n\nToday the only originator of a negative return in this entire call\ngraph is traverse_trees()'s \"exceeded maximum allowed tree depth\"\ncheck; everything else (collect_merge_info_callback,\ntraverse_trees_wrapper, the inner traverse_trees recursion) only\nrelays that. So in current Git, the visible effect of dropping the\nreplay callback's return value is narrow but bad: a tree nested past\ncore.maxTreeDepth has its -1 swallowed, the subtree below the limit\nis silently pruned, and the merge completes as if that were the\ncorrect result.\n\nA later patch in this series will teach collect_merge_info_callback()\nto return -1 on an additional path -- detecting duplicate\nentries in malformed trees -- which is similarly handled today by\njust ignoring the problem (resulting in mostly a \"last one wins\" rule,\nthough the non-last entry can mutate various state flags).\n\nCapture the return value, stop the loop on negative returns, and\npropagate the error to the caller. The callback returns a positive mask\nvalue on success, so normalize non-negative returns to\n0 for the caller.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 00923ce3cd..4b8e32209d 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1008,18 +1008,20 @@ static int traverse_trees_wrapper(struct index_state *istate,\n \tinfo->traverse_path = renames->callback_data_traverse_path;\n \tinfo->fn = old_fn;\n \tfor (i = old_offset; i < renames->callback_data_nr; ++i) {\n-\t\tinfo->fn(n,\n-\t\t\t renames->callback_data[i].mask,\n-\t\t\t renames->callback_data[i].dirmask,\n-\t\t\t renames->callback_data[i].names,\n-\t\t\t info);\n+\t\tret = info->fn(n,\n+\t\t\t renames->callback_data[i].mask,\n+\t\t\t renames->callback_data[i].dirmask,\n+\t\t\t renames->callback_data[i].names,\n+\t\t\t info);\n+\t\tif (ret < 0)\n+\t\t\tbreak;\n \t}\n \n \trenames->callback_data_nr = old_offset;\n \tfree(renames->callback_data_traverse_path);\n \trenames->callback_data_traverse_path = old_callback_data_traverse_path;\n \tinfo->traverse_path = NULL;\n-\treturn 0;\n+\treturn ret < 0 ? ret : 0;\n }\n \n static void setup_path_info(struct merge_options *opt,\n-- \ngitgitgadget\n\n"},{"id":"545480","messageId":"b4ff725a77366a1fae136c3bb72f6198f47d6ebb.1781419047.git.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.v2.git.1781419047.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] merge-ort: drop unnecessary show_all_errors from collect_merge_info()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-06-14T06:37:23Z","receivedAt":"2026-06-14T06:37:32Z","isPatch":true,"body":"From: Elijah Newren \n\ncollect_merge_info() has set info.show_all_errors = 1 since\nd2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),\n2020-12-13). This setting was copied from unpack-trees.c where it\ncontrols batching of error messages for porcelain display, but\nmerge-ort has no such error-batching logic and never needed it.\n\nWith show_all_errors set, traverse_trees() captures a negative callback\nreturn but continues processing remaining entries rather than stopping\nimmediately. Removing the setting restores the default behavior where\na negative return from collect_merge_info_callback() breaks out of the\ntraversal loop right away, allowing a future commit to exit early when\na corrupt tree is detected.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 4b8e32209d..74e9636020 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1740,7 +1740,6 @@ static int collect_merge_info(struct merge_options *opt,\n \tsetup_traverse_info(&info, opt->priv->toplevel_dir);\n \tinfo.fn = collect_merge_info_callback;\n \tinfo.data = opt;\n-\tinfo.show_all_errors = 1;\n \n \tif (repo_parse_tree(opt->repo, merge_base) < 0 ||\n \t repo_parse_tree(opt->repo, side1) < 0 ||\n-- \ngitgitgadget\n\n"},{"id":"545481","messageId":"673fbea13f7612eefec8cc2561b1a0289e4b8e71.1781419047.git.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.v2.git.1781419047.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-06-14T06:37:24Z","receivedAt":"2026-06-14T06:37:34Z","isPatch":true,"body":"From: Elijah Newren \n\nclear_or_reinit_internal_opts() is responsible for cleaning up the\nvarious data structures in merge_options_internal. It already handles\nmany renames-related structures (dirs_removed, dir_renames,\nrelevant_sources, cached_pairs, deferred, etc.) but does not free\nrenames->pairs[].queue.\n\nIn the normal code path, resolve_and_process_renames() frees\npairs[s].queue and reinitializes it with diff_queue_init() before\nclear_or_reinit_internal_opts() runs, so the omission is harmless.\nHowever, if collect_merge_info() encounters an error and returns early\n(before resolve_and_process_renames() is ever called), any diff pairs\nalready queued by collect_rename_info()/add_pair() will have their\nbacking array leaked.\n\nFix this by freeing renames->pairs[].queue in the cleanup function.\nIn the normal path the pointer is already NULL (from the earlier\ndiff_queue_init() in resolve_and_process_renames()), so free(NULL) is\na safe no-op.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 74e9636020..8f911cb639 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -728,6 +728,8 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,\n \t\tstrintmap_clear_func(&renames->deferred[i].possible_trivial_merges);\n \t\tstrset_clear_func(&renames->deferred[i].target_dirs);\n \t\trenames->deferred[i].trivial_merges_okay = 1; /* 1 == maybe */\n+\t\tfree(renames->pairs[i].queue);\n+\t\tdiff_queue_init(&renames->pairs[i]);\n \t}\n \trenames->cached_pairs_valid_side = 0;\n \trenames->dir_rename_mask = 0;\n-- \ngitgitgadget\n\n"},{"id":"545482","messageId":"b5421244d41ed90a66f64c619d18935a71475bc7.1781419047.git.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.v2.git.1781419047.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] merge-ort: abort merge when trees have duplicate entries","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-06-14T06:37:25Z","receivedAt":"2026-06-14T06:37:36Z","isPatch":true,"body":"From: Elijah Newren \n\nTrees with duplicate entries are malformed; fsck reports \"contains\nduplicate file entries\" for them. merge-ort has from the beginning\nassumed that we would never hit such trees. It was written with the\nassumption that traverse_trees() calls collect_merge_info_callback() at\nmost once per path. The \"sanity checks\" in that callback (added in\nd2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),\n2020-12-13)) verify properties of each individual call but not that\ninvariant. The strmap_put() in setup_path_info() silently overwrites\nthe entry from any prior call for the same path, because it assumed\nthere would be no other path. Unfortunately, supplemental data\nstructures for various optimizations could still be tweaked before the\nextra paths were overwritten, and those data structures not matching\nexpected state could trip various assertions.\n\nChange the return type of setup_path_info() from void to int to allow us\nto detect this case, and abort the merge with a clear error message when\nit occurs.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 61 ++++++++++++++++------------\n t/t6422-merge-rename-corner-cases.sh | 54 ++++++++++++++++++++++++\n 2 files changed, 88 insertions(+), 27 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8f911cb639..be0829bbb7 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1026,18 +1026,18 @@ static int traverse_trees_wrapper(struct index_state *istate,\n \treturn ret < 0 ? ret : 0;\n }\n \n-static void setup_path_info(struct merge_options *opt,\n-\t\t\t struct string_list_item *result,\n-\t\t\t const char *current_dir_name,\n-\t\t\t int current_dir_name_len,\n-\t\t\t char *fullpath, /* we'll take over ownership */\n-\t\t\t struct name_entry *names,\n-\t\t\t struct name_entry *merged_version,\n-\t\t\t unsigned is_null, /* boolean */\n-\t\t\t unsigned df_conflict, /* boolean */\n-\t\t\t unsigned filemask,\n-\t\t\t unsigned dirmask,\n-\t\t\t int resolved /* boolean */)\n+static int setup_path_info(struct merge_options *opt,\n+\t\t\t struct string_list_item *result,\n+\t\t\t const char *current_dir_name,\n+\t\t\t int current_dir_name_len,\n+\t\t\t char *fullpath, /* we'll take over ownership */\n+\t\t\t struct name_entry *names,\n+\t\t\t struct name_entry *merged_version,\n+\t\t\t unsigned is_null, /* boolean */\n+\t\t\t unsigned df_conflict, /* boolean */\n+\t\t\t unsigned filemask,\n+\t\t\t unsigned dirmask,\n+\t\t\t int resolved /* boolean */)\n {\n \t/* result->util is void*, so mi is a convenience typed variable */\n \tstruct merged_info *mi;\n@@ -1081,9 +1081,11 @@ static void setup_path_info(struct merge_options *opt,\n \t\t\t */\n \t\t\tmi->is_null = 1;\n \t}\n-\tstrmap_put(&opt->priv->paths, fullpath, mi);\n+\tif (strmap_put(&opt->priv->paths, fullpath, mi))\n+\t\treturn error(_(\"tree has duplicate entries for '%s'\"), fullpath);\n \tresult->string = fullpath;\n \tresult->util = mi;\n+\treturn 0;\n }\n \n static void add_pair(struct merge_options *opt,\n@@ -1350,9 +1352,10 @@ static int collect_merge_info_callback(int n,\n \t */\n \tif (side1_matches_mbase && side2_matches_mbase) {\n \t\t/* mbase, side1, & side2 all match; use mbase as resolution */\n-\t\tsetup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n-\t\t\t\tnames, names+0, mbase_null, 0 /* df_conflict */,\n-\t\t\t\tfilemask, dirmask, 1 /* resolved */);\n+\t\tif (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n+\t\t\t\t names, names+0, mbase_null, 0 /* df_conflict */,\n+\t\t\t\t filemask, dirmask, 1 /* resolved */))\n+\t\t\treturn -1; /* Quit traversing */\n \t\treturn mask;\n \t}\n \n@@ -1364,9 +1367,10 @@ static int collect_merge_info_callback(int n,\n \t */\n \tif (sides_match && filemask == 0x07) {\n \t\t/* use side1 (== side2) version as resolution */\n-\t\tsetup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n-\t\t\t\tnames, names+1, side1_null, 0,\n-\t\t\t\tfilemask, dirmask, 1);\n+\t\tif (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n+\t\t\t\t names, names+1, side1_null, 0,\n+\t\t\t\t filemask, dirmask, 1))\n+\t\t\treturn -1; /* Quit traversing */\n \t\treturn mask;\n \t}\n \n@@ -1378,18 +1382,20 @@ static int collect_merge_info_callback(int n,\n \t */\n \tif (side1_matches_mbase && filemask == 0x07) {\n \t\t/* use side2 version as resolution */\n-\t\tsetup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n-\t\t\t\tnames, names+2, side2_null, 0,\n-\t\t\t\tfilemask, dirmask, 1);\n+\t\tif (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n+\t\t\t\t names, names+2, side2_null, 0,\n+\t\t\t\t filemask, dirmask, 1))\n+\t\t\treturn -1; /* Quit traversing */\n \t\treturn mask;\n \t}\n \n \t/* Similar to above but swapping sides 1 and 2 */\n \tif (side2_matches_mbase && filemask == 0x07) {\n \t\t/* use side1 version as resolution */\n-\t\tsetup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n-\t\t\t\tnames, names+1, side1_null, 0,\n-\t\t\t\tfilemask, dirmask, 1);\n+\t\tif (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n+\t\t\t\t names, names+1, side1_null, 0,\n+\t\t\t\t filemask, dirmask, 1))\n+\t\t\treturn -1; /* Quit traversing */\n \t\treturn mask;\n \t}\n \n@@ -1413,8 +1419,9 @@ static int collect_merge_info_callback(int n,\n \t * unconflict some more cases, but that comes later so all we can\n \t * do now is record the different non-null file hashes.)\n \t */\n-\tsetup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n-\t\t\tnames, NULL, 0, df_conflict, filemask, dirmask, 0);\n+\tif (setup_path_info(opt, &pi, dirname, info->pathlen, fullpath,\n+\t\t\t names, NULL, 0, df_conflict, filemask, dirmask, 0))\n+\t\treturn -1; /* Quit traversing */\n \n \tci = pi.util;\n \tVERIFY_CI(ci);\ndiff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh\nindex e18d5a227d..81b645bb3b 100755\n--- a/t/t6422-merge-rename-corner-cases.sh\n+++ b/t/t6422-merge-rename-corner-cases.sh\n@@ -1525,4 +1525,58 @@ test_expect_success 'submodule/directory preliminary conflict' '\n \t)\n '\n \n+# Testcase: submodule/directory conflict with duplicate tree entries\n+# One side has a path as a gitlink (submodule). The other side replaces\n+# the gitlink with a directory. A third-party tool creates a tree on the\n+# submodule side that has *both* a gitlink and a tree entry for the same\n+# path (adding a file inside the submodule path ignoring that there's a\n+# gitlink there). collect_merge_info_callback() should detect the\n+# duplicate and abort rather than silently corrupting its bookkeeping.\n+\n+test_expect_success 'duplicate tree entries trigger an error' '\n+\ttest_when_finished \"rm -rf duplicate-entry\" &&\n+\tgit init duplicate-entry &&\n+\t(\n+\t\tcd duplicate-entry &&\n+\n+\t\t# Base commit: \"docs\" is a gitlink (submodule)\n+\t\tempty_tree=$(git mktree file.txt &&\n+\t\tgit add file.txt &&\n+\t\tgit commit -m base &&\n+\n+\t\t# side1: remove the gitlink, replace with a directory\n+\t\tgit checkout -b side1 &&\n+\t\tgit rm --cached docs &&\n+\t\tmkdir -p docs &&\n+\t\techo hello >docs/requirements.txt &&\n+\t\tgit add docs/requirements.txt &&\n+\t\tgit commit -m \"side1: submodule to directory\" &&\n+\n+\t\t# side2: keep the gitlink but craft a tree that also\n+\t\t# contains a tree entry for \"docs\" (simulating a tool\n+\t\t# that adds files inside a submodule path without\n+\t\t# removing the gitlink first).\n+\t\tgit checkout main &&\n+\t\tgit checkout -b side2 &&\n+\t\tblob_oid=$(echo world | git hash-object -w --stdin) &&\n+\t\tdocs_tree=$(printf \"100644 blob %s\\trequirements.txt\\n\" \\\n+\t\t\t\"$blob_oid\" | git mktree) &&\n+\t\tcur_tree=$(git rev-parse HEAD^{tree}) &&\n+\t\tgit cat-file -p $cur_tree >tree-listing &&\n+\t\tprintf \"040000 tree %s\\tdocs\\n\" \"$docs_tree\" >>tree-listing &&\n+\t\tnew_tree=$(git mktree err &&\n+\t\ttest_grep \"duplicate entries\" err\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"545483","messageId":"cf50f1aabcf84aa755808318756c233305cc008d.1781419047.git.gitgitgadget@gmail.com","threadId":"65527","inReplyTo":"pull.2096.v2.git.1781419047.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-06-14T06:37:26Z","receivedAt":"2026-06-14T06:37:38Z","isPatch":true,"body":"From: Elijah Newren \n\nverify_cache() checks that the index does not contain both \"path\" and\n\"path/file\" before writing a tree. It does this by comparing only\nadjacent entries, relying on the assumption that \"path/file\" would\nimmediately follow \"path\" in sorted order. Unfortunately, this\nassumption does not always hold. For example:\n\n docs <-- submodule entry\n docs-internal/README.md <-- intervening entry\n docs/requirements.txt <-- D/F conflict, NOT adjacent to \"docs\"\n\nWhen this happens, verify_cache() silently misses the D/F conflict and\nwrite-tree produces a corrupt tree object containing duplicate entries\n(one for the submodule \"docs\" and one for the tree \"docs\").\n\nI could not find any caller in current git that both allows the index to\nget into this state and then tries to write it out without doing other\nchecks beyond the verify_cache() call in cache_tree_update(), but\nverify_cache() is documented as a safety net for preventing corrupt\ntrees and should actually provide that guarantee. A downstream consumer\nthat relied solely on cache_tree_update()'s internal checking via\nverify_cache() to prevent duplicate tree entries was bitten by the gap.\n\nAdd a test that constructs a corrupt index directly (bypassing the D/F\nchecks in add_index_entry) and verifies that write-tree now rejects it.\n\nSigned-off-by: Elijah Newren \n---\n cache-tree.c | 64 ++++++++++++++++++++++++++++------\n t/meson.build | 1 +\n t/t0093-direct-index-write.pl | 38 ++++++++++++++++++++\n t/t0093-verify-cache-df-gap.sh | 59 +++++++++++++++++++++++++++++++\n 4 files changed, 151 insertions(+), 11 deletions(-)\n create mode 100644 t/t0093-direct-index-write.pl\n create mode 100755 t/t0093-verify-cache-df-gap.sh\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 7881b42aa2..4d2669b312 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -161,6 +161,54 @@ void cache_tree_invalidate_path(struct index_state *istate, const char *path)\n \t\tistate->cache_changed |= CACHE_TREE_CHANGED;\n }\n \n+/*\n+ * Check whether this_ce and the next entry in the index form a D/F\n+ * conflict (\"path\" vs \"path/file\"). Returns the conflicting \"path/...\"\n+ * name when one is found, or NULL otherwise.\n+ *\n+ * The cache is sorted, so \"path/file\" sorts after \"path\" and the\n+ * conflict is usually visible as adjacent entries. But other entries\n+ * can sort between them -- e.g. \"path-internal\" sits between \"path\"\n+ * and \"path/file\" because '-' (0x2D) precedes '/' (0x2F) -- so when\n+ * the immediately following entry shares our prefix but starts with a\n+ * character that sorts before '/', binary search for \"path/\" instead.\n+ */\n+static const char *find_df_conflict(struct index_state *istate,\n+\t\t\t\t const struct cache_entry *this_ce,\n+\t\t\t\t const struct cache_entry *next_ce)\n+{\n+\tconst char *this_name = this_ce->name;\n+\tconst char *next_name = next_ce->name;\n+\tint this_len = ce_namelen(this_ce);\n+\tconst struct cache_entry *other;\n+\tstruct strbuf probe = STRBUF_INIT;\n+\tint pos;\n+\n+\tif (this_len >= ce_namelen(next_ce) ||\n+\t next_name[this_len] > '/' ||\n+\t strncmp(this_name, next_name, this_len))\n+\t\treturn NULL;\n+\n+\tif (next_name[this_len] == '/')\n+\t\treturn next_name;\n+\n+\tstrbuf_add(&probe, this_name, this_len);\n+\tstrbuf_addch(&probe, '/');\n+\tpos = index_name_pos_sparse(istate, probe.buf, probe.len);\n+\tstrbuf_release(&probe);\n+\n+\tif (pos < 0)\n+\t\tpos = -pos - 1;\n+\tif (pos >= (int)istate->cache_nr)\n+\t\treturn NULL;\n+\tother = istate->cache[pos];\n+\tif (ce_namelen(other) > this_len &&\n+\t other->name[this_len] == '/' &&\n+\t !strncmp(this_name, other->name, this_len))\n+\t\treturn other->name;\n+\treturn NULL;\n+}\n+\n static int verify_cache(struct index_state *istate, int flags)\n {\n \tunsigned i, funny;\n@@ -190,24 +238,18 @@ static int verify_cache(struct index_state *istate, int flags)\n \t */\n \tfunny = 0;\n \tfor (i = 0; i + 1 < istate->cache_nr; i++) {\n-\t\t/* path/file always comes after path because of the way\n-\t\t * the cache is sorted. Also path can appear only once,\n-\t\t * which means conflicting one would immediately follow.\n-\t\t */\n \t\tconst struct cache_entry *this_ce = istate->cache[i];\n \t\tconst struct cache_entry *next_ce = istate->cache[i + 1];\n-\t\tconst char *this_name = this_ce->name;\n-\t\tconst char *next_name = next_ce->name;\n-\t\tint this_len = ce_namelen(this_ce);\n-\t\tif (this_len < ce_namelen(next_ce) &&\n-\t\t next_name[this_len] == '/' &&\n-\t\t strncmp(this_name, next_name, this_len) == 0) {\n+\t\tconst char *conflict_name;\n+\n+\t\tconflict_name = find_df_conflict(istate, this_ce, next_ce);\n+\t\tif (conflict_name) {\n \t\t\tif (10 < ++funny) {\n \t\t\t\tfprintf(stderr, \"...\\n\");\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tfprintf(stderr, \"You have both %s and %s\\n\",\n-\t\t\t\tthis_name, next_name);\n+\t\t\t\tthis_ce->name, conflict_name);\n \t\t}\n \t}\n \tif (funny)\ndiff --git a/t/meson.build b/t/meson.build\nindex 7528e5cda5..362177999b 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -124,6 +124,7 @@ integration_tests = [\n 't0090-cache-tree.sh',\n 't0091-bugreport.sh',\n 't0092-diagnose.sh',\n+ 't0093-verify-cache-df-gap.sh',\n 't0095-bloom.sh',\n 't0100-previous.sh',\n 't0101-at-syntax.sh',\ndiff --git a/t/t0093-direct-index-write.pl b/t/t0093-direct-index-write.pl\nnew file mode 100644\nindex 0000000000..2881a3ebb2\n--- /dev/null\n+++ b/t/t0093-direct-index-write.pl\n@@ -0,0 +1,38 @@\n+#!/usr/bin/perl\n+#\n+# Build a v2 index file from entries listed on stdin.\n+# Each line: \"octalmode hex-oid name\"\n+# Output: binary index written to stdout.\n+#\n+# This bypasses all D/F safety checks in add_index_entry(), simulating\n+# what happens when code uses ADD_CACHE_JUST_APPEND to bulk-load entries.\n+use strict;\n+use warnings;\n+use Digest::SHA qw(sha1 sha256);\n+\n+my $hash_algo = $ENV{'GIT_DEFAULT_HASH'} || 'sha1';\n+my $hash_func = $hash_algo eq 'sha256' ? \\&sha256 : \\&sha1;\n+\n+my @entries;\n+while (my $line = ) {\n+\tchomp $line;\n+\tmy ($mode, $oid_hex, $name) = split(/ /, $line, 3);\n+\tpush @entries, [$mode, $oid_hex, $name];\n+}\n+\n+my $body = \"DIRC\" . pack(\"NN\", 2, scalar @entries);\n+\n+for my $ent (@entries) {\n+\tmy ($mode, $oid_hex, $name) = @{$ent};\n+\t# 10 x 32-bit stat fields (zeroed), with mode in position 7\n+\tmy $stat = pack(\"N10\", 0, 0, 0, 0, 0, 0, oct($mode), 0, 0, 0);\n+\tmy $oid = pack(\"H*\", $oid_hex);\n+\tmy $flags = pack(\"n\", length($name) & 0xFFF);\n+\tmy $entry = $stat . $oid . $flags . $name . \"\\0\";\n+\t# Pad to 8-byte boundary\n+\twhile (length($entry) % 8) { $entry .= \"\\0\"; }\n+\t$body .= $entry;\n+}\n+\n+binmode STDOUT;\n+print $body . $hash_func->($body);\ndiff --git a/t/t0093-verify-cache-df-gap.sh b/t/t0093-verify-cache-df-gap.sh\nnew file mode 100755\nindex 0000000000..0b6829d805\n--- /dev/null\n+++ b/t/t0093-verify-cache-df-gap.sh\n@@ -0,0 +1,59 @@\n+#!/bin/sh\n+\n+test_description='verify_cache() must catch non-adjacent D/F conflicts\n+\n+Ensure that verify_cache() can complain about bad entries like:\n+\n+ docs <-- submodule\n+ docs-internal/... <-- sorts here because \"-\" < \"/\"\n+ docs/... <-- D/F conflict with \"docs\" above, not adjacent\n+\n+In order to test verify_cache, we directly construct a corrupt index\n+(bypassing the D/F safety checks in add_index_entry) and verify that\n+write-tree rejects it.\n+'\n+\n+. ./test-lib.sh\n+\n+if ! test_have_prereq PERL\n+then\n+\tskip_all='skipping verify_cache D/F tests; Perl not available'\n+\ttest_done\n+fi\n+\n+# Build a v2 index from entries on stdin, bypassing D/F checks.\n+# Each line: \"octalmode hex-oid name\" (entries must be pre-sorted).\n+build_corrupt_index () {\n+\tperl \"$TEST_DIRECTORY/t0093-direct-index-write.pl\" >\"$1\"\n+}\n+\n+test_expect_success 'setup objects' '\n+\ttest_commit base &&\n+\tBLOB=$(git rev-parse HEAD:base.t) &&\n+\tSUB_COMMIT=$(git rev-parse HEAD)\n+'\n+\n+test_expect_success 'adjacent D/F conflict is caught by verify_cache' '\n+\tcat >index-entries <<-EOF &&\n+\t0160000 $SUB_COMMIT docs\n+\t0100644 $BLOB docs/requirements.txt\n+\tEOF\n+\tbuild_corrupt_index .git/index err &&\n+\ttest_grep \"You have both docs and docs/requirements.txt\" err\n+'\n+\n+test_expect_success 'non-adjacent D/F conflict is caught by verify_cache' '\n+\tcat >index-entries <<-EOF &&\n+\t0160000 $SUB_COMMIT docs\n+\t0100644 $BLOB docs-internal/README.md\n+\t0100644 $BLOB docs/requirements.txt\n+\tEOF\n+\tbuild_corrupt_index .git/index err &&\n+\ttest_grep \"You have both docs and docs/requirements.txt\" err\n+'\n+\n+test_done\n-- \ngitgitgadget\n"},{"id":"545489","messageId":"xmqq5x3ldu4h.fsf@gitster.g","threadId":"65527","inReplyTo":"cf50f1aabcf84aa755808318756c233305cc008d.1781419047.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/5] cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-14T15:22:38Z","receivedAt":"2026-06-14T15:22:41Z","isPatch":true,"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 7881b42aa2..4d2669b312 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -161,6 +161,54 @@ void cache_tree_invalidate_path(struct index_state *istate, const char *path)\n> \t\tistate->cache_changed |= CACHE_TREE_CHANGED;\n> }\n> \n> +/*\n> + * Check whether this_ce and the next entry in the index form a D/F\n> + * conflict (\"path\" vs \"path/file\"). Returns the conflicting \"path/...\"\n> + * name when one is found, or NULL otherwise.\n> + *\n> + * The cache is sorted, so \"path/file\" sorts after \"path\" and the\n> + * conflict is usually visible as adjacent entries. But other entries\n> + * can sort between them -- e.g. \"path-internal\" sits between \"path\"\n> + * and \"path/file\" because '-' (0x2D) precedes '/' (0x2F) -- so when\n> + * the immediately following entry shares our prefix but starts with a\n> + * character that sorts before '/', binary search for \"path/\" instead.\n> + */\n> +static const char *find_df_conflict(struct index_state *istate,\n> +\t\t\t\t const struct cache_entry *this_ce,\n> +\t\t\t\t const struct cache_entry *next_ce)\n> +{\n> +\tconst char *this_name = this_ce->name;\n> +\tconst char *next_name = next_ce->name;\n> +\tint this_len = ce_namelen(this_ce);\n> +\tconst struct cache_entry *other;\n> +\tstruct strbuf probe = STRBUF_INIT;\n> +\tint pos;\n\n> +\tif (this_len >= ce_namelen(next_ce) ||\n> +\t next_name[this_len] > '/' ||\n> +\t strncmp(this_name, next_name, this_len))\n> +\t\treturn NULL;\n\nFirst we reject an abvious \"cannot conflict\" case. If the current\none is not strictly shorter than the next one, it cannot be an\noverlapping with one of the leading directories in the next one and\nthe next one cannot be \"hiding\" an overlapping one in the \"docs,\ndocs-i, docs/r\" relationship described in the log message (where\nnext==\"docs-i' hides the fact that this==\"docs\" conflicts with\n\"docs/r\"). Of course, the current one cannot be such a confliciting\nentry, if an earlier part of the next name does not entirely match\nthe current name. In addition, if the byte after the matching\nleading part in the next name is larger than '/', any entry that\ncomes later than the next name cannot have '/' at that byte position.\n\nMakes sense.\n\n> +\tif (next_name[this_len] == '/')\n> +\t\treturn next_name;\n\nAnd if we see '/' there, we definitely have conflict right there.\n\nSo these trivial cases after us, we need to see if an entry that\ncomes after next has a name whose leading part is identical to this\nname, plus '/'. How would we compute it?\n\n> +\tstrbuf_add(&probe, this_name, this_len);\n> +\tstrbuf_addch(&probe, '/');\n> +\tpos = index_name_pos_sparse(istate, probe.buf, probe.len);\n> +\tstrbuf_release(&probe);\n\nWe see where the first of such a name (i.e. this name followed by '/')\nmay appear. Normal cache entries in the index would never have a\ntrailing slash in their names, so I expect that this would either\nfind an directory that is sparsed out and returns a non-negative\nindex into the .cache[] array, or a negative index that indicates\nwhere a name like this + '/' + anything would appear.\n\n> +\n> +\tif (pos < 0)\n> +\t\tpos = -pos - 1;\n\nSo in order to check both cases, we do the usual index flipping. We\ndo not need to to treat \"it exists---that's sparse dir\" case and \"it\nis expanded and we are trying to find the first entry in that index\"\ncase differently below, which simplifies things a bit.\n\n> +\tif (pos >= (int)istate->cache_nr)\n> +\t\treturn NULL;\n\nOK, such an entry whose name is this followed by '/' does not exist\nso we are safe.\n\n> +\tother = istate->cache[pos];\n> +\tif (ce_namelen(other) > this_len &&\n> +\t other->name[this_len] == '/' &&\n> +\t !strncmp(this_name, other->name, this_len))\n> +\t\treturn other->name;\n> +\treturn NULL;\n\nThis looks very similar to the inverse of the earlier one but\nslightly different. It might be easier to spot where it differs, if\nwe flipped the polarity, like so, perhaps?\n\n\tother = istate->cache[pos];\n\tif (this_len >= ce_namelen(other) ||\n\t other->name[this_len] != '/' ||\n\t strncmp(this_name, other->name, this_len))\n\t\treturn NULL;\n\n\treturn other->name;\n\nOr perhaps not. I do not care very strongly, but I only spotted the\nsubtle difference while attempting to flip the polarity in my head,\nso it might help ther readers the same way. I dunno.\n\nThanks, looking very good.\n\n"}]}