{"thread":{"id":"61631","subject":"[PATCH 1/7] merge-ort: extract handling of priv member into reusable function","startedAt":"2024-06-13T20:25:11Z","lastAt":"2024-07-06T06:11:06Z","messageCount":34,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Taylor Blau","Eric Sunshine","Elijah Newren","Derrick Stolee","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"497102","messageId":"d4ba1fccd9145db9b3fe1530881052315cfa16b8.1718310307.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.git.1718310307.gitgitgadget@gmail.com","subject":"[PATCH 1/7] merge-ort: extract handling of priv member into reusable function","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-13T20:25:01Z","receivedAt":"2024-06-13T20:25:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn preparation for a subsequent commit which will ensure we do not\nforget to maintain our invariants for the priv member in error\ncodepaths, extract the necessary functionality out into a separate\nfunction. This change is cosmetic at this point, and introduces no\nchanges beyond an extra assertion sanity check.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 28 +++++++++++++++++++++++-----\n 1 file changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex eaede6cead9..10f5a655f29 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -5000,6 +5000,28 @@ static void merge_check_renames_reusable(struct merge_result *result,\n \n /*** Function Grouping: merge_incore_*() and their internal variants ***/\n \n+static void move_opt_priv_to_result_priv(struct merge_options *opt,\n+\t\t\t\t\t struct merge_result *result)\n+{\n+\t/*\n+\t * opt->priv and result->priv are a bit weird. opt->priv contains\n+\t * information that we can re-use in subsequent merge operations to\n+\t * enable our cached renames optimization. The best way to provide\n+\t * that to subsequent merges is putting it in result->priv.\n+\t * However, putting it directly there would mean retrofitting lots\n+\t * of functions in this file to also take a merge_result pointer,\n+\t * which is ugly and annoying. So, we just make sure at the end of\n+\t * the merge (the outer merge if there are internal recursive ones)\n+\t * to move it.\n+\t */\n+\tassert(opt->priv && !result->priv);\n+\tif (!opt->priv->call_depth) {\n+\t\tresult->priv = opt->priv;\n+\t\tresult->_properly_initialized = RESULT_INITIALIZED;\n+\t\topt->priv = NULL;\n+\t}\n+}\n+\n /*\n * Originally from merge_trees_internal(); heavily adapted, though.\n */\n@@ -5060,11 +5082,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n \t\t/* existence of conflicted entries implies unclean */\n \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n \t}\n-\tif (!opt->priv->call_depth) {\n-\t\tresult->priv = opt->priv;\n-\t\tresult->_properly_initialized = RESULT_INITIALIZED;\n-\t\topt->priv = NULL;\n-\t}\n+\tmove_opt_priv_to_result_priv(opt, result);\n }\n \n /*\n-- \ngitgitgadget\n\n"},{"id":"497106","messageId":"pull.1748.git.1718310307.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":null,"subject":"[PATCH 0/7] Fix and improve some error codepaths in merge-ort","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-13T20:25:00Z","receivedAt":"2024-06-13T20:25:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series started as a just a fix for the abort hit in merge-ort when\ncustom merge drivers error out (see\nhttps://lore.kernel.org/git/75F8BD12-7743-4863-B4C5-049FDEC4645E@gearset.com/).\nHowever, while working on that, I found a few other issues around error\ncodepaths in merge-ort. So this series:\n\n * Patches 1-2: fix the reported abort problem\n * Patches 3-4: make code in handle_content_merges() easier to handle when\n we hit errors\n * Patch 5: fix a misleading comment\n * Patches 6-7: make error handling (immediate print vs. letting callers get\n the error information) more consistent\n\nThe last two patches change the behavior slightly for error codepaths, and\nthere's a question about whether we should show only the error messages that\ncaused an early termination of the merge, or if we should also show any\nconflict messages for other paths that were handled before we hit the early\ntermination. These patches made a decision but feel free to take those last\ntwo patches as more of an RFC.\n\nElijah Newren (7):\n merge-ort: extract handling of priv member into reusable function\n merge-ort: maintain expected invariant for priv member\n merge-ort: fix type of local 'clean' var in handle_content_merge()\n merge-ort: clearer propagation of failure-to-function from\n merge_submodule\n merge-ort: loosen commented requirements\n merge-ort: upon merge abort, only show messages causing the abort\n merge-ort: convert more error() cases to path_msg()\n\n merge-ort.c | 167 +++++++++++++++++++++++++++++++-----------\n t/t6406-merge-attr.sh | 42 ++++++++++-\n 2 files changed, 164 insertions(+), 45 deletions(-)\n\n\nbase-commit: 8d94cfb54504f2ec9edc7ca3eb5c29a3dd3675ae\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1748%2Fnewren%2Ffix-error-cases-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1748/newren/fix-error-cases-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1748\n-- \ngitgitgadget\n"},{"id":"497103","messageId":"17c97301baa829a993cf8838deb9271add5bd1cd.1718310307.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.git.1718310307.gitgitgadget@gmail.com","subject":"[PATCH 2/7] merge-ort: maintain expected invariant for priv member","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-13T20:25:02Z","receivedAt":"2024-06-13T20:25:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe calling convention for the merge machinery is\n One call to init_merge_options()\n One or more calls to merge_incore_[non]recursive()\n One call to merge_finalize()\n (possibly indirectly via merge_switch_to_result())\nBoth merge_switch_to_result() and merge_finalize() expect\n opt->priv == NULL && result->priv != NULL\nwhich is supposed to be set up by our move_opt_priv_to_result_priv()\nfunction. However, two codepath dealing with error cases did not\nexecute this necessary logic, which could result in assertion failures\n(or, if assertions were compiled out, could result in segfaults). Fix\nthe oversight and add a test that would have caught one of these\nproblems.\n\nWhile at it, also tighten an existing test for a non-recursive merge\nto verify that it fails correctly, i.e. with the expected exit code\nrather than with an assertion failure.\n\nReported-by: Matt Cree \nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 3 ++-\n t/t6406-merge-attr.sh | 42 +++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 10f5a655f29..6ca7b0f9be4 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -5015,7 +5015,7 @@ static void move_opt_priv_to_result_priv(struct merge_options *opt,\n \t * to move it.\n \t */\n \tassert(opt->priv && !result->priv);\n-\tif (!opt->priv->call_depth) {\n+\tif (!opt->priv->call_depth || result->clean < 0) {\n \t\tresult->priv = opt->priv;\n \t\tresult->_properly_initialized = RESULT_INITIALIZED;\n \t\topt->priv = NULL;\n@@ -5052,6 +5052,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n \t\t oid_to_hex(&side1->object.oid),\n \t\t oid_to_hex(&side2->object.oid));\n \t\tresult->clean = -1;\n+\t\tmove_opt_priv_to_result_priv(opt, result);\n \t\treturn;\n \t}\n \ttrace2_region_leave(\"merge\", \"collect_merge_info\", opt->repo);\ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex 156a1efacfe..b6db5c2cc36 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -185,7 +185,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'\n \n \t>./please-abort &&\n \techo \"* merge=custom\" >.gitattributes &&\n-\ttest_must_fail git merge main 2>err &&\n+\ttest_expect_code 2 git merge main 2>err &&\n \tgrep \"^error: failed to execute internal merge\" err &&\n \tgit ls-files -u >output &&\n \tgit diff --name-only HEAD >>output &&\n@@ -261,4 +261,44 @@ test_expect_success 'binary files with union attribute' '\n \tgrep -i \"warning.*cannot merge.*HEAD vs. bin-main\" output\n '\n \n+test_expect_success !WINDOWS 'custom merge driver that is killed with a signal on recursive merge' '\n+\ttest_when_finished \"rm -f output please-abort\" &&\n+\ttest_when_finished \"git checkout side\" &&\n+\n+\tgit reset --hard anchor &&\n+\n+\tgit checkout -b base-a main^ &&\n+\techo base-a >text &&\n+\tgit commit -m base-a text &&\n+\n+\tgit checkout -b base-b main^ &&\n+\techo base-b >text &&\n+\tgit commit -m base-b text &&\n+\n+\tgit checkout -b recursive-a base-a &&\n+\ttest_must_fail git merge base-b &&\n+\techo recursive-a >text &&\n+\tgit add text &&\n+\tgit commit -m recursive-a &&\n+\n+\tgit checkout -b recursive-b base-b &&\n+\ttest_must_fail git merge base-a &&\n+\techo recursive-b >text &&\n+\tgit add text &&\n+\tgit commit -m recursive-b &&\n+\n+\tgit config --replace-all \\\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n+\tgit config --replace-all \\\n+\tmerge.custom.name \"custom merge driver for testing\" &&\n+\n+\t>./please-abort &&\n+\techo \"* merge=custom\" >.gitattributes &&\n+\ttest_expect_code 2 git merge recursive-a 2>err &&\n+\tgrep \"^error: failed to execute internal merge\" err &&\n+\tgit ls-files -u >output &&\n+\tgit diff --name-only HEAD >>output &&\n+\ttest_must_be_empty output\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"497104","messageId":"23bb3386114f3bb08de285e3d11e1029ca03850a.1718310307.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.git.1718310307.gitgitgadget@gmail.com","subject":"[PATCH 3/7] merge-ort: fix type of local 'clean' var in handle_content_merge()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-13T20:25:03Z","receivedAt":"2024-06-13T20:25:14Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nhandle_content_merge() returns an int. Every caller of\nhandle_content_merge() expects an int. However, we declare a local\nvariable 'clean' that we use for the return value to be unsigned. To\nmake matters worse, we also assign 'clean' the return value of\nmerge_submodule() in one codepath, which is defined to return an int.\nIt seems that the only reason to have 'clean' be unsigned was to allow a\ncutesy bit manipulation operation to be well-defined. Fix the type of\nthe 'clean' local in handle_content_merge().\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 6ca7b0f9be4..13b47d352fc 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2109,7 +2109,7 @@ static int handle_content_merge(struct merge_options *opt,\n \t * merges, which happens for example with rename/rename(2to1) and\n \t * rename/add conflicts.\n \t */\n-\tunsigned clean = 1;\n+\tint clean = 1;\n \n \t/*\n \t * handle_content_merge() needs both files to be of the same type, i.e.\n@@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt,\n \t\tfree(result_buf.ptr);\n \t\tif (ret)\n \t\t\treturn -1;\n-\t\tclean &= (merge_status == 0);\n+\t\tif (merge_status > 0)\n+\t\t\tclean = 0;\n \t\tpath_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL,\n \t\t\t _(\"Auto-merging %s\"), path);\n \t} else if (S_ISGITLINK(a->mode)) {\n-- \ngitgitgadget\n\n"},{"id":"497105","messageId":"2789c58cd3f0691a32cbd0a5aa96a438b799b9f3.1718310307.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.git.1718310307.gitgitgadget@gmail.com","subject":"[PATCH 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-13T20:25:04Z","receivedAt":"2024-06-13T20:25:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =\nconflicted, -1 = failure-to-determine), but we often like to think of\nit as binary (ignoring the possibility of a negative value) and use\nconstructs like '!clean' to reflect this. However, these constructs\ncan make codepaths more difficult to understand, unless we handle the\nnegative case early and return pre-emptively; do that in\nhandle_content_merge() to make the code a bit easier to read.\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 13b47d352fc..39799e65a36 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2193,6 +2193,8 @@ static int handle_content_merge(struct merge_options *opt,\n \t\tclean = merge_submodule(opt, pathnames[0],\n \t\t\t\t\ttwo_way ? null_oid() : &o->oid,\n \t\t\t\t\t&a->oid, &b->oid, &result->oid);\n+\t\tif (clean < 0)\n+\t\t\treturn -1;\n \t\tif (opt->priv->call_depth && two_way && !clean) {\n \t\t\tresult->mode = o->mode;\n \t\t\toidcpy(&result->oid, &o->oid);\n-- \ngitgitgadget\n\n"},{"id":"497107","messageId":"975fbddf3053d678df5cdfc75521b1be9e2533ce.1718310307.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.git.1718310307.gitgitgadget@gmail.com","subject":"[PATCH 5/7] merge-ort: loosen commented requirements","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-13T20:25:05Z","receivedAt":"2024-06-13T20:25:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe comment above type_short_descriptions claimed that the order had to\nmatch what was found in the conflict_info_and_types enum. Since\ntype_short_descriptions uses designated initializers, the order should\nnot actually matter; I am guessing that positional initializers may have\nbeen under consideration when that comment was added, but the comment\nwas not updated when designated initializers were chosen.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 39799e65a36..462bc6fb6e1 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -553,7 +553,7 @@ enum conflict_and_info_types {\n * Short description of conflict type, relied upon by external tools.\n *\n * We can add more entries, but DO NOT change any of these strings. Also,\n- * Order MUST match conflict_info_and_types.\n+ * please ensure the order matches what is used in conflict_info_and_types.\n */\n static const char *type_short_descriptions[] = {\n \t/*** \"Simple\" conflicts and informational messages ***/\n-- \ngitgitgadget\n\n"},{"id":"497108","messageId":"71391b18c1a711fee1f5aff6eedbd3f631d37ded.1718310307.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.git.1718310307.gitgitgadget@gmail.com","subject":"[PATCH 6/7] merge-ort: upon merge abort, only show messages causing the abort","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-13T20:25:06Z","receivedAt":"2024-06-13T20:25:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen something goes wrong enough that we need to abort early and not\neven attempt merging the remaining files, it probably does not make\nsense to report conflicts messages for the subset of files we processed\nbefore hitting the fatal error. Instead, only show the messages\nassociated with paths where we hit the fatal error. Also, print these\nmessages to stderr rather than stdout.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 76 ++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 52 insertions(+), 24 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 462bc6fb6e1..5410dec2b4f 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -543,10 +543,24 @@ enum conflict_and_info_types {\n \tCONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE,\n \tCONFLICT_SUBMODULE_MAY_HAVE_REWINDS,\n \tCONFLICT_SUBMODULE_NULL_MERGE_BASE,\n-\tCONFLICT_SUBMODULE_CORRUPT,\n+\n+\t/* INSERT NEW ENTRIES HERE */\n+\n+\t/*\n+\t * Keep this entry after all regular conflict and info types; only\n+\t * errors (failures causing immediate abort of the merge) should\n+\t * come after this.\n+\t */\n+\tNB_REGULAR_CONFLICT_TYPES,\n+\n+\t/*\n+\t * Something is seriously wrong; cannot even perform merge;\n+\t * Keep this group _last_ other than NB_CONFLICT_TYPES\n+\t */\n+\tERROR_SUBMODULE_CORRUPT,\n \n \t/* Keep this entry _last_ in the list */\n-\tNB_CONFLICT_TYPES,\n+\tNB_TOTAL_TYPES,\n };\n \n /*\n@@ -597,8 +611,10 @@ static const char *type_short_descriptions[] = {\n \t\t\"CONFLICT (submodule may have rewinds)\",\n \t[CONFLICT_SUBMODULE_NULL_MERGE_BASE] =\n \t\t\"CONFLICT (submodule lacks merge base)\",\n-\t[CONFLICT_SUBMODULE_CORRUPT] =\n-\t\t\"CONFLICT (submodule corrupt)\"\n+\n+\t/* Something is seriously wrong; cannot even perform merge */\n+\t[ERROR_SUBMODULE_CORRUPT] =\n+\t\t\"ERROR (submodule corrupt)\",\n };\n \n struct logical_conflict_info {\n@@ -762,7 +778,8 @@ static void path_msg(struct merge_options *opt,\n \n \t/* Sanity checks */\n \tassert(omittable_hint ==\n-\t !starts_with(type_short_descriptions[type], \"CONFLICT\") ||\n+\t (!starts_with(type_short_descriptions[type], \"CONFLICT\") &&\n+\t\t!starts_with(type_short_descriptions[type], \"ERROR\")) ||\n \t type == CONFLICT_DIR_RENAME_SUGGESTED);\n \tif (opt->record_conflict_msgs_as_headers && omittable_hint)\n \t\treturn; /* Do not record mere hints in headers */\n@@ -1817,9 +1834,9 @@ static int merge_submodule(struct merge_options *opt,\n \t/* check whether both changes are forward */\n \tret2 = repo_in_merge_bases(&subrepo, commit_o, commit_a);\n \tif (ret2 < 0) {\n-\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n+\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n-\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t _(\"error: failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n \t\tret = -1;\n@@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt,\n \tif (ret2 > 0)\n \t\tret2 = repo_in_merge_bases(&subrepo, commit_o, commit_b);\n \tif (ret2 < 0) {\n-\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n+\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n-\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t _(\"error: failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n \t\tret = -1;\n@@ -1848,7 +1865,7 @@ static int merge_submodule(struct merge_options *opt,\n \t/* Case #1: a is contained in b or vice versa */\n \tret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);\n \tif (ret2 < 0) {\n-\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n+\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n \t\t\t _(\"Failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n@@ -1867,9 +1884,9 @@ static int merge_submodule(struct merge_options *opt,\n \t}\n \tret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);\n \tif (ret2 < 0) {\n-\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n+\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n-\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t _(\"error: failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n \t\tret = -1;\n@@ -1901,9 +1918,9 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t\t\t &merges);\n \tswitch (parent_count) {\n \tcase -1:\n-\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n+\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n-\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t _(\"error: failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n \t\tret = -1;\n@@ -4646,6 +4663,7 @@ void merge_display_update_messages(struct merge_options *opt,\n \tstruct hashmap_iter iter;\n \tstruct strmap_entry *e;\n \tstruct string_list olist = STRING_LIST_INIT_NODUP;\n+\tFILE *o = stdout;\n \n \tif (opt->record_conflict_msgs_as_headers)\n \t\tBUG(\"Either display conflict messages or record them as headers, not both\");\n@@ -4662,6 +4680,10 @@ void merge_display_update_messages(struct merge_options *opt,\n \t}\n \tstring_list_sort(&olist);\n \n+\t/* Print to stderr if we hit errors rather than just conflicts */\n+\tif (result->clean < 0)\n+\t\to = stderr;\n+\n \t/* Iterate over the items, printing them */\n \tfor (int path_nr = 0; path_nr < olist.nr; ++path_nr) {\n \t\tstruct string_list *conflicts = olist.items[path_nr].util;\n@@ -4669,25 +4691,31 @@ void merge_display_update_messages(struct merge_options *opt,\n \t\t\tstruct logical_conflict_info *info =\n \t\t\t\tconflicts->items[i].util;\n \n+\t\t\t/* On failure, ignore regular conflict types */\n+\t\t\tif (result->clean < 0 &&\n+\t\t\t info->type < NB_REGULAR_CONFLICT_TYPES)\n+\t\t\t\tcontinue;\n+\n \t\t\tif (detailed) {\n-\t\t\t\tprintf(\"%lu\", (unsigned long)info->paths.nr);\n-\t\t\t\tputchar('\\0');\n+\t\t\t\tfprintf(o, \"%lu\", (unsigned long)info->paths.nr);\n+\t\t\t\tfputc('\\0', o);\n \t\t\t\tfor (int n = 0; n < info->paths.nr; n++) {\n-\t\t\t\t\tfputs(info->paths.v[n], stdout);\n-\t\t\t\t\tputchar('\\0');\n+\t\t\t\t\tfputs(info->paths.v[n], o);\n+\t\t\t\t\tfputc('\\0', o);\n \t\t\t\t}\n-\t\t\t\tfputs(type_short_descriptions[info->type],\n-\t\t\t\t stdout);\n-\t\t\t\tputchar('\\0');\n+\t\t\t\tfputs(type_short_descriptions[info->type], o);\n+\t\t\t\tfputc('\\0', o);\n \t\t\t}\n-\t\t\tputs(conflicts->items[i].string);\n+\t\t\tfputs(conflicts->items[i].string, o);\n+\t\t\tfputc('\\n', o);\n \t\t\tif (detailed)\n-\t\t\t\tputchar('\\0');\n+\t\t\t\tfputc('\\0', o);\n \t\t}\n \t}\n \tstring_list_clear(&olist, 0);\n \n-\tprint_submodule_conflict_suggestion(&opti->conflicted_submodules);\n+\tif (result->clean >= 0)\n+\t\tprint_submodule_conflict_suggestion(&opti->conflicted_submodules);\n \n \t/* Also include needed rename limit adjustment now */\n \tdiff_warn_rename_limit(\"merge.renamelimit\",\n-- \ngitgitgadget\n\n"},{"id":"497109","messageId":"32ae44b6260eea8dad7a428ed92e25b3891ddd9c.1718310307.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.git.1718310307.gitgitgadget@gmail.com","subject":"[PATCH 7/7] merge-ort: convert more error() cases to path_msg()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-13T20:25:07Z","receivedAt":"2024-06-13T20:25:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nmerge_submodule() stores errors using path_msg(), whereas other call\nsites make use of the error() function. This is inconsistent, and\nmoving towards path_msg() seems more friendly for libification efforts\nsince it will allow the caller to determine whether the error messages\nneed to be printed.\n\nNote that this deferred handling of error messages changes the error\nmessage in a recursive merge from\n error: failed to execute internal merge\nto\n From inner merge: error: failed to execute internal merge\nwhich provides a little more information about the error which may be\nuseful. Since the recursive merge strategy still only shows the older\nerror, we had to adjust the new testcase introduced a few commits ago to\njust search for the older message somewhere in the output.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 53 +++++++++++++++++++++++++++++++++----------\n t/t6406-merge-attr.sh | 2 +-\n 2 files changed, 42 insertions(+), 13 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 5410dec2b4f..493aa55aa80 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -558,6 +558,10 @@ enum conflict_and_info_types {\n \t * Keep this group _last_ other than NB_CONFLICT_TYPES\n \t */\n \tERROR_SUBMODULE_CORRUPT,\n+\tERROR_THREEWAY_CONTENT_MERGE_FAILED,\n+\tERROR_OBJECT_WRITE_FAILED,\n+\tERROR_OBJECT_READ_FAILED,\n+\tERROR_OBJECT_NOT_A_BLOB,\n \n \t/* Keep this entry _last_ in the list */\n \tNB_TOTAL_TYPES,\n@@ -615,6 +619,14 @@ static const char *type_short_descriptions[] = {\n \t/* Something is seriously wrong; cannot even perform merge */\n \t[ERROR_SUBMODULE_CORRUPT] =\n \t\t\"ERROR (submodule corrupt)\",\n+\t[ERROR_THREEWAY_CONTENT_MERGE_FAILED] =\n+\t\t\"ERROR (three-way content merge failed)\",\n+\t[ERROR_OBJECT_WRITE_FAILED] =\n+\t\t\"ERROR (object write failed)\",\n+\t[ERROR_OBJECT_READ_FAILED] =\n+\t\t\"ERROR (object read failed)\",\n+\t[ERROR_OBJECT_NOT_A_BLOB] =\n+\t\t\"ERROR (object is not a blob)\",\n };\n \n struct logical_conflict_info {\n@@ -2190,15 +2202,24 @@ static int handle_content_merge(struct merge_options *opt,\n \t\t\t\t\t pathnames, extra_marker_size,\n \t\t\t\t\t &result_buf);\n \n-\t\tif ((merge_status < 0) || !result_buf.ptr)\n-\t\t\tret = error(_(\"failed to execute internal merge\"));\n+\t\tif ((merge_status < 0) || !result_buf.ptr) {\n+\t\t\tpath_msg(opt, ERROR_THREEWAY_CONTENT_MERGE_FAILED, 0,\n+\t\t\t\t pathnames[0], pathnames[1], pathnames[2], NULL,\n+\t\t\t\t _(\"error: failed to execute internal merge for %s\"),\n+\t\t\t\t path);\n+\t\t\tret = -1;\n+\t\t}\n \n \t\tif (!ret &&\n \t\t write_object_file(result_buf.ptr, result_buf.size,\n-\t\t\t\t OBJ_BLOB, &result->oid))\n-\t\t\tret = error(_(\"unable to add %s to database\"), path);\n-\n+\t\t\t\t OBJ_BLOB, &result->oid)) {\n+\t\t\tpath_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0,\n+\t\t\t\t pathnames[0], pathnames[1], pathnames[2], NULL,\n+\t\t\t\t _(\"error: unable to add %s to database\"), path);\n+\t\t\tret = -1;\n+\t\t}\n \t\tfree(result_buf.ptr);\n+\n \t\tif (ret)\n \t\t\treturn -1;\n \t\tif (merge_status > 0)\n@@ -3577,18 +3598,26 @@ static int sort_dirs_next_to_their_children(const char *one, const char *two)\n \t\treturn c1 - c2;\n }\n \n-static int read_oid_strbuf(const struct object_id *oid,\n-\t\t\t struct strbuf *dst)\n+static int read_oid_strbuf(struct merge_options *opt,\n+\t\t\t const struct object_id *oid,\n+\t\t\t struct strbuf *dst,\n+\t\t\t const char *path)\n {\n \tvoid *buf;\n \tenum object_type type;\n \tunsigned long size;\n \tbuf = repo_read_object_file(the_repository, oid, &type, &size);\n-\tif (!buf)\n-\t\treturn error(_(\"cannot read object %s\"), oid_to_hex(oid));\n+\tif (!buf) {\n+\t\tpath_msg(opt, ERROR_OBJECT_READ_FAILED, 0,\n+\t\t\t path, NULL, NULL, NULL,\n+\t\t\t _(\"error: cannot read object %s\"), oid_to_hex(oid));\n+\t\treturn -1;\n+\t}\n \tif (type != OBJ_BLOB) {\n \t\tfree(buf);\n-\t\treturn error(_(\"object %s is not a blob\"), oid_to_hex(oid));\n+\t\tpath_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,\n+\t\t\t path, NULL, NULL, NULL,\n+\t\t\t _(\"error: object %s is not a blob\"), oid_to_hex(oid));\n \t}\n \tstrbuf_attach(dst, buf, size, size + 1);\n \treturn 0;\n@@ -3612,8 +3641,8 @@ static int blob_unchanged(struct merge_options *opt,\n \tif (oideq(&base->oid, &side->oid))\n \t\treturn 1;\n \n-\tif (read_oid_strbuf(&base->oid, &basebuf) ||\n-\t read_oid_strbuf(&side->oid, &sidebuf))\n+\tif (read_oid_strbuf(opt, &base->oid, &basebuf, path) ||\n+\t read_oid_strbuf(opt, &side->oid, &sidebuf, path))\n \t\tgoto error_return;\n \t/*\n \t * Note: binary | is used so that both renormalizations are\ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex b6db5c2cc36..9bf95249347 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -295,7 +295,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal o\n \t>./please-abort &&\n \techo \"* merge=custom\" >.gitattributes &&\n \ttest_expect_code 2 git merge recursive-a 2>err &&\n-\tgrep \"^error: failed to execute internal merge\" err &&\n+\tgrep \"error: failed to execute internal merge\" err &&\n \tgit ls-files -u >output &&\n \tgit diff --name-only HEAD >>output &&\n \ttest_must_be_empty output\n-- \ngitgitgadget\n"},{"id":"497111","messageId":"xmqqjzisimnj.fsf@gitster.g","threadId":"61631","inReplyTo":"d4ba1fccd9145db9b3fe1530881052315cfa16b8.1718310307.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/7] merge-ort: extract handling of priv member into reusable function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-13T21:00:00Z","receivedAt":"2024-06-13T21:00:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> +static void move_opt_priv_to_result_priv(struct merge_options *opt,\n> +\t\t\t\t\t struct merge_result *result)\n> +{\n> +\t/*\n> +\t * opt->priv and result->priv are a bit weird. opt->priv contains\n> +\t * information that we can re-use in subsequent merge operations to\n> +\t * enable our cached renames optimization. The best way to provide\n> +\t * that to subsequent merges is putting it in result->priv.\n> +\t * However, putting it directly there would mean retrofitting lots\n> +\t * of functions in this file to also take a merge_result pointer,\n> +\t * which is ugly and annoying. So, we just make sure at the end of\n> +\t * the merge (the outer merge if there are internal recursive ones)\n> +\t * to move it.\n> +\t */\n> +\tassert(opt->priv && !result->priv);\n> +\tif (!opt->priv->call_depth) {\n> +\t\tresult->priv = opt->priv;\n> +\t\tresult->_properly_initialized = RESULT_INITIALIZED;\n> +\t\topt->priv = NULL;\n> +\t}\n> +}\n> +\n> /*\n> * Originally from merge_trees_internal(); heavily adapted, though.\n> */\n> @@ -5060,11 +5082,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n> \t\t/* existence of conflicted entries implies unclean */\n> \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n> \t}\n> -\tif (!opt->priv->call_depth) {\n> -\t\tresult->priv = opt->priv;\n> -\t\tresult->_properly_initialized = RESULT_INITIALIZED;\n> -\t\topt->priv = NULL;\n> -\t}\n> +\tmove_opt_priv_to_result_priv(opt, result);\n> }\n\nI have a feeling that making it the caller's responsibility to check\n\"are we doing the outermost merge?\" and not the callee's problem\nwould result in a better code organization. If we write\n\n\tif (!opt->priv->call_depth)\n\t\tmove_opt_priv_to_result_priv(opt, result);\n\nthen for this call site, it is still crystal clear that this will\nhappen only at the outermost level. The new caller you add in the\nnext step would also be simpler to reason about.\n\nYou have the assert() to make sure callers do not call the \"move\"\nhelper at a wrong place already, and if the organization in this\npatch somehow comes from a desire that the \"move\" is done only at\nthe outermost level (and \"or immediately before an error causes the\nwhole thing to abort\" after the next patch), it does not have to be\na silent \"if call_depth is not zero we return silently\", but another\nassert() that insists that the callers are allowed to call it under\nthese two specific conditions.\n\nThanks.\n"},{"id":"497113","messageId":"Zmt4Jat252KkQApg@nand.local","threadId":"61631","inReplyTo":"xmqqjzisimnj.fsf@gitster.g","subject":"Re: [PATCH 1/7] merge-ort: extract handling of priv member into reusable function","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-13T22:52:37Z","receivedAt":"2024-06-13T22:52:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jun 13, 2024 at 02:00:00PM -0700, Junio C Hamano wrote:\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > +static void move_opt_priv_to_result_priv(struct merge_options *opt,\n> > +\t\t\t\t\t struct merge_result *result)\n> > +{\n> > +\t/*\n> > +\t * opt->priv and result->priv are a bit weird. opt->priv contains\n> > +\t * information that we can re-use in subsequent merge operations to\n> > +\t * enable our cached renames optimization. The best way to provide\n> > +\t * that to subsequent merges is putting it in result->priv.\n> > +\t * However, putting it directly there would mean retrofitting lots\n> > +\t * of functions in this file to also take a merge_result pointer,\n> > +\t * which is ugly and annoying. So, we just make sure at the end of\n> > +\t * the merge (the outer merge if there are internal recursive ones)\n> > +\t * to move it.\n> > +\t */\n> > +\tassert(opt->priv && !result->priv);\n> > +\tif (!opt->priv->call_depth) {\n> > +\t\tresult->priv = opt->priv;\n> > +\t\tresult->_properly_initialized = RESULT_INITIALIZED;\n> > +\t\topt->priv = NULL;\n> > +\t}\n> > +}\n> > +\n> > /*\n> > * Originally from merge_trees_internal(); heavily adapted, though.\n> > */\n> > @@ -5060,11 +5082,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n> > \t\t/* existence of conflicted entries implies unclean */\n> > \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n> > \t}\n> > -\tif (!opt->priv->call_depth) {\n> > -\t\tresult->priv = opt->priv;\n> > -\t\tresult->_properly_initialized = RESULT_INITIALIZED;\n> > -\t\topt->priv = NULL;\n> > -\t}\n> > +\tmove_opt_priv_to_result_priv(opt, result);\n> > }\n>\n> I have a feeling that making it the caller's responsibility to check\n> \"are we doing the outermost merge?\" and not the callee's problem\n> would result in a better code organization. If we write\n>\n> \tif (!opt->priv->call_depth)\n> \t\tmove_opt_priv_to_result_priv(opt, result);\n>\n> then for this call site, it is still crystal clear that this will\n> happen only at the outermost level. The new caller you add in the\n> next step would also be simpler to reason about.\n\nI had the same thought. Calling the function\nmove_opt_priv_to_result_priv() seems to indicate that reuslt->priv will\ndefinitely be updated to opt->priv.\n\nAnother approach would be to rename the function\nmaybe_move_opt_priv_to_result_priv() and have it be a noop if\nopt->priv->call_depth is non-zero.\n\nBut this is all fairly philosophical ;-). I do not really mind or feel\nstrongly either way whatsoever.\n\nThanks,\nTaylor\n"},{"id":"497114","messageId":"Zmt5tiuGIpTHLHRC@nand.local","threadId":"61631","inReplyTo":"17c97301baa829a993cf8838deb9271add5bd1cd.1718310307.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/7] merge-ort: maintain expected invariant for priv member","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-13T22:59:02Z","receivedAt":"2024-06-13T22:59:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jun 13, 2024 at 08:25:02PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n>\n> The calling convention for the merge machinery is\n> One call to init_merge_options()\n> One or more calls to merge_incore_[non]recursive()\n> One call to merge_finalize()\n> (possibly indirectly via merge_switch_to_result())\n> Both merge_switch_to_result() and merge_finalize() expect\n> opt->priv == NULL && result->priv != NULL\n> which is supposed to be set up by our move_opt_priv_to_result_priv()\n> function. However, two codepath dealing with error cases did not\n\ns/codepath/&s/ ?\n\n> execute this necessary logic, which could result in assertion failures\n> (or, if assertions were compiled out, could result in segfaults). Fix\n> the oversight and add a test that would have caught one of these\n> problems.\n>\n> While at it, also tighten an existing test for a non-recursive merge\n> to verify that it fails correctly, i.e. with the expected exit code\n> rather than with an assertion failure.\n\nI suspect that this test was flaky, too, since if the assertion errors\nwere compiled out and it died via a segfault, the test would have failed\noutright as test_must_fail does not allow for segfaults typically.\n\nSo I'm glad to see us tightening up that area of the test suite.\n\n> Reported-by: Matt Cree \n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 3 ++-\n> t/t6406-merge-attr.sh | 42 +++++++++++++++++++++++++++++++++++++++++-\n> 2 files changed, 43 insertions(+), 2 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 10f5a655f29..6ca7b0f9be4 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -5015,7 +5015,7 @@ static void move_opt_priv_to_result_priv(struct merge_options *opt,\n> \t * to move it.\n> \t */\n> \tassert(opt->priv && !result->priv);\n> -\tif (!opt->priv->call_depth) {\n> +\tif (!opt->priv->call_depth || result->clean < 0) {\n> \t\tresult->priv = opt->priv;\n> \t\tresult->_properly_initialized = RESULT_INITIALIZED;\n> \t\topt->priv = NULL;\n> @@ -5052,6 +5052,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n> \t\t oid_to_hex(&side1->object.oid),\n> \t\t oid_to_hex(&side2->object.oid));\n> \t\tresult->clean = -1;\n> +\t\tmove_opt_priv_to_result_priv(opt, result);\n> \t\treturn;\n> \t}\n> \ttrace2_region_leave(\"merge\", \"collect_merge_info\", opt->repo);\n> diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\n> index 156a1efacfe..b6db5c2cc36 100755\n> --- a/t/t6406-merge-attr.sh\n> +++ b/t/t6406-merge-attr.sh\n> @@ -185,7 +185,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'\n>\n> \t>./please-abort &&\n> \techo \"* merge=custom\" >.gitattributes &&\n> -\ttest_must_fail git merge main 2>err &&\n> +\ttest_expect_code 2 git merge main 2>err &&\n> \tgrep \"^error: failed to execute internal merge\" err &&\n> \tgit ls-files -u >output &&\n> \tgit diff --name-only HEAD >>output &&\n> @@ -261,4 +261,44 @@ test_expect_success 'binary files with union attribute' '\n> \tgrep -i \"warning.*cannot merge.*HEAD vs. bin-main\" output\n> '\n>\n> +test_expect_success !WINDOWS 'custom merge driver that is killed with a signal on recursive merge' '\n> +\ttest_when_finished \"rm -f output please-abort\" &&\n> +\ttest_when_finished \"git checkout side\" &&\n> +\n> +\tgit reset --hard anchor &&\n> +\n> +\tgit checkout -b base-a main^ &&\n> +\techo base-a >text &&\n> +\tgit commit -m base-a text &&\n> +\n> +\tgit checkout -b base-b main^ &&\n> +\techo base-b >text &&\n> +\tgit commit -m base-b text &&\n> +\n> +\tgit checkout -b recursive-a base-a &&\n> +\ttest_must_fail git merge base-b &&\n\nHere and below, do you care about the particular exit code of merging as\nabove? IOW, should these be `test_expect_code`'s as well?\n\nThanks,\nTaylor\n"},{"id":"497115","messageId":"Zmt68bR3v6+kxrfN@nand.local","threadId":"61631","inReplyTo":"pull.1748.git.1718310307.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/7] Fix and improve some error codepaths in merge-ort","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-13T23:04:17Z","receivedAt":"2024-06-13T23:04:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jun 13, 2024 at 08:25:00PM +0000, Elijah Newren via GitGitGadget wrote:\n> Elijah Newren (7):\n> merge-ort: extract handling of priv member into reusable function\n> merge-ort: maintain expected invariant for priv member\n> merge-ort: fix type of local 'clean' var in handle_content_merge()\n> merge-ort: clearer propagation of failure-to-function from\n> merge_submodule\n> merge-ort: loosen commented requirements\n> merge-ort: upon merge abort, only show messages causing the abort\n> merge-ort: convert more error() cases to path_msg()\n>\n> merge-ort.c | 167 +++++++++++++++++++++++++++++++-----------\n> t/t6406-merge-attr.sh | 42 ++++++++++-\n> 2 files changed, 164 insertions(+), 45 deletions(-)\n\nVery nice. I had a couple of minor thoughts on the earlier patches, but\nI agree with the substantive ones that printing only messages related to\npaths that we were processing when something went horribly wrong is a\ngood idea.\n\nI don't think that either of my comments alone merit a reroll, and I'd\nbe happy to see this version move along as-is. But I'm equally happy if\nyou want to fix a typo here or there or do some bikeshedding ;-).\n\nThanks for fixing this issue.\n\nThanks,\nTaylor\n"},{"id":"497121","messageId":"CAPig+cSZPC=41w8kG6n5hdSCUC4+jYJ6cM8UC-WCO3Bcmb2iNA@mail.gmail.com","threadId":"61631","inReplyTo":"71391b18c1a711fee1f5aff6eedbd3f631d37ded.1718310307.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/7] merge-ort: upon merge abort, only show messages causing the abort","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-06-14T04:19:25Z","receivedAt":"2024-06-14T04:19:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 13, 2024 at 4:25 PM Elijah Newren via GitGitGadget\n wrote:\n> When something goes wrong enough that we need to abort early and not\n> even attempt merging the remaining files, it probably does not make\n> sense to report conflicts messages for the subset of files we processed\n> before hitting the fatal error. Instead, only show the messages\n> associated with paths where we hit the fatal error. Also, print these\n> messages to stderr rather than stdout.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> diff --git a/merge-ort.c b/merge-ort.c\n> @@ -543,10 +543,24 @@ enum conflict_and_info_types {\n> - CONFLICT_SUBMODULE_CORRUPT,\n> +\n> + /* INSERT NEW ENTRIES HERE */\n> + /*\n> + * Something is seriously wrong; cannot even perform merge;\n> + * Keep this group _last_ other than NB_CONFLICT_TYPES\n> + */\n\nI'm probably missing something obvious, but here the new comment talks\nabout NB_CONFLICT_TYPES...\n\n> + ERROR_SUBMODULE_CORRUPT,\n>\n> /* Keep this entry _last_ in the list */\n> - NB_CONFLICT_TYPES,\n> + NB_TOTAL_TYPES,\n\n... but NB_CONFLICT_TYPES gets removed here.\n\n> @@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt,\n> - _(\"Failed to merge submodule %s \"\n> + _(\"error: failed to merge submodule %s \"\n> @@ -1848,7 +1865,7 @@ static int merge_submodule(struct merge_options *opt,\n> _(\"Failed to merge submodule %s \"\n> \"(repository corrupt)\"),\n\nDo you also want to apply the same \"error: failed...\" transformation\nto this error message as you did to other error messages?\n"},{"id":"497315","messageId":"CABPp-BGhWkjdd+9oa_RuUA07exVRtMHWqg8Ri_Sg1twUBkdxWA@mail.gmail.com","threadId":"61631","inReplyTo":"Zmt5tiuGIpTHLHRC@nand.local","subject":"Re: [PATCH 2/7] merge-ort: maintain expected invariant for priv member","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-19T02:58:42Z","receivedAt":"2024-06-19T02:58:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jun 13, 2024 at 10:59 PM Taylor Blau wrote:\n>\n> On Thu, Jun 13, 2024 at 08:25:02PM +0000, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > The calling convention for the merge machinery is\n> > One call to init_merge_options()\n> > One or more calls to merge_incore_[non]recursive()\n> > One call to merge_finalize()\n> > (possibly indirectly via merge_switch_to_result())\n> > Both merge_switch_to_result() and merge_finalize() expect\n> > opt->priv == NULL && result->priv != NULL\n> > which is supposed to be set up by our move_opt_priv_to_result_priv()\n> > function. However, two codepath dealing with error cases did not\n>\n> s/codepath/&s/ ?\n\nIndeed.\n\n> > execute this necessary logic, which could result in assertion failures\n> > (or, if assertions were compiled out, could result in segfaults). Fix\n> > the oversight and add a test that would have caught one of these\n> > problems.\n> >\n> > While at it, also tighten an existing test for a non-recursive merge\n> > to verify that it fails correctly, i.e. with the expected exit code\n> > rather than with an assertion failure.\n\nIt turns out my logic here was faulty; if a\n test_must_fail git ARGS...\ncommand is run and git hits an assertion failure, the test_must_fail\nwill fail with\n test_must_fail: died by signal 6: git ARGS...\nsimilar to how it would fail if git were to segfault.\n\nHowever, I still like the idea of testing the exit status here because\nof this comment in builtin/merge.c:\n /*\n * The backend exits with 1 when conflicts are\n * left to be resolved, with 2 when it does not\n * handle the given merge at all.\n */\nand this is one of the few tests in the testsuite where we are\nexplicitly testing a case where the merge is neither success nor\nconflicts, but failed-to-handle.\n\n> I suspect that this test was flaky, too, since if the assertion errors\n> were compiled out and it died via a segfault, the test would have failed\n> outright as test_must_fail does not allow for segfaults typically.\n>\n> So I'm glad to see us tightening up that area of the test suite.\n[...]\n> > + test_must_fail git merge base-b &&\n>\n> Here and below, do you care about the particular exit code of merging as\n> above? IOW, should these be `test_expect_code`'s as well?\n\nBoth of these comments led me on the path to check something out and\ndiscover my error above, which will lead to slightly different changes\nto the patch.\n"},{"id":"497316","messageId":"CABPp-BHAcXG=C5i0anuAxm0qb0i-i8AcbQXyQ4Rz4azWA0SU1A@mail.gmail.com","threadId":"61631","inReplyTo":"CAPig+cSZPC=41w8kG6n5hdSCUC4+jYJ6cM8UC-WCO3Bcmb2iNA@mail.gmail.com","subject":"Re: [PATCH 6/7] merge-ort: upon merge abort, only show messages causing the abort","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-19T02:58:58Z","receivedAt":"2024-06-19T02:59:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 14, 2024 at 4:19 AM Eric Sunshine wrote:\n>\n> On Thu, Jun 13, 2024 at 4:25 PM Elijah Newren via GitGitGadget\n> wrote:\n> > When something goes wrong enough that we need to abort early and not\n> > even attempt merging the remaining files, it probably does not make\n> > sense to report conflicts messages for the subset of files we processed\n> > before hitting the fatal error. Instead, only show the messages\n> > associated with paths where we hit the fatal error. Also, print these\n> > messages to stderr rather than stdout.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > @@ -543,10 +543,24 @@ enum conflict_and_info_types {\n> > - CONFLICT_SUBMODULE_CORRUPT,\n> > +\n> > + /* INSERT NEW ENTRIES HERE */\n> > + /*\n> > + * Something is seriously wrong; cannot even perform merge;\n> > + * Keep this group _last_ other than NB_CONFLICT_TYPES\n> > + */\n>\n> I'm probably missing something obvious, but here the new comment talks\n> about NB_CONFLICT_TYPES...\n>\n> > + ERROR_SUBMODULE_CORRUPT,\n> >\n> > /* Keep this entry _last_ in the list */\n> > - NB_CONFLICT_TYPES,\n> > + NB_TOTAL_TYPES,\n>\n> ... but NB_CONFLICT_TYPES gets removed here.\n>\n> > @@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt,\n> > - _(\"Failed to merge submodule %s \"\n> > + _(\"error: failed to merge submodule %s \"\n> > @@ -1848,7 +1865,7 @@ static int merge_submodule(struct merge_options *opt,\n> > _(\"Failed to merge submodule %s \"\n> > \"(repository corrupt)\"),\n>\n> Do you also want to apply the same \"error: failed...\" transformation\n> to this error message as you did to other error messages?\n\nDoh, I tried to re-read through my patches after making numerous\nadditional tweaks to try to catch stuff like this, but I clearly\nmissed a few cases here. Thanks for catching both of these issues;\nI'll fix them up.\n"},{"id":"497317","messageId":"5c50c0b3aadbefc7973a2e8d77890808c95875d6.1718766019.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.v2.git.1718766019.gitgitgadget@gmail.com","subject":"[PATCH v2 1/7] merge-ort: extract handling of priv member into reusable function","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-19T03:00:13Z","receivedAt":"2024-06-19T03:00:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn preparation for a subsequent commit which will ensure we do not\nforget to maintain our invariants for the priv member in error\ncodepaths, extract the necessary functionality out into a separate\nfunction. This change is cosmetic at this point, and introduces no\nchanges beyond an extra assertion sanity check.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 27 ++++++++++++++++++++++-----\n 1 file changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex eaede6cead9..700ddfccb90 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -5000,6 +5000,26 @@ static void merge_check_renames_reusable(struct merge_result *result,\n \n /*** Function Grouping: merge_incore_*() and their internal variants ***/\n \n+static void move_opt_priv_to_result_priv(struct merge_options *opt,\n+\t\t\t\t\t struct merge_result *result)\n+{\n+\t/*\n+\t * opt->priv and result->priv are a bit weird. opt->priv contains\n+\t * information that we can re-use in subsequent merge operations to\n+\t * enable our cached renames optimization. The best way to provide\n+\t * that to subsequent merges is putting it in result->priv.\n+\t * However, putting it directly there would mean retrofitting lots\n+\t * of functions in this file to also take a merge_result pointer,\n+\t * which is ugly and annoying. So, we just make sure at the end of\n+\t * the merge (the outer merge if there are internal recursive ones)\n+\t * to move it.\n+\t */\n+\tassert(opt->priv && !result->priv);\n+\tresult->priv = opt->priv;\n+\tresult->_properly_initialized = RESULT_INITIALIZED;\n+\topt->priv = NULL;\n+}\n+\n /*\n * Originally from merge_trees_internal(); heavily adapted, though.\n */\n@@ -5060,11 +5080,8 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n \t\t/* existence of conflicted entries implies unclean */\n \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n \t}\n-\tif (!opt->priv->call_depth) {\n-\t\tresult->priv = opt->priv;\n-\t\tresult->_properly_initialized = RESULT_INITIALIZED;\n-\t\topt->priv = NULL;\n-\t}\n+\tif (!opt->priv->call_depth)\n+\t\tmove_opt_priv_to_result_priv(opt, result);\n }\n \n /*\n-- \ngitgitgadget\n\n"},{"id":"497318","messageId":"pull.1748.v2.git.1718766019.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.git.1718310307.gitgitgadget@gmail.com","subject":"[PATCH v2 0/7] Fix and improve some error codepaths in merge-ort","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-19T03:00:12Z","receivedAt":"2024-06-19T03:00:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Changes since v1:\n\n * Remove the conditional outside of move_opt_priv_to_result_priv() for\n patches 1 & 2\n * Fix the consistency issues in patch 6 (refer to updated enum value in\n comment, use consistent prefix on error messages)\n * Fix typo in patch 2\n * Fix incorrect claim in commit message of patch 2 and provide more detail\n about which merges we are interested in checking for a very specific exit\n code.\n\nThis series started as a just a fix for the abort hit in merge-ort when\ncustom merge drivers error out (see\nhttps://lore.kernel.org/git/75F8BD12-7743-4863-B4C5-049FDEC4645E@gearset.com/).\nHowever, while working on that, I found a few other issues around error\ncodepaths in merge-ort. So this series:\n\n * Patches 1-2: fix the reported abort problem\n * Patches 3-4: make code in handle_content_merges() easier to handle when\n we hit errors\n * Patch 5: fix a misleading comment\n * Patches 6-7: make error handling (immediate print vs. letting callers get\n the error information) more consistent\n\nThe last two patches change the behavior slightly for error codepaths, and\nthere's a question about whether we should show only the error messages that\ncaused an early termination of the merge, or if we should also show any\nconflict messages for other paths that were handled before we hit the early\ntermination. These patches made a decision but feel free to take those last\ntwo patches as more of an RFC.\n\nElijah Newren (7):\n merge-ort: extract handling of priv member into reusable function\n merge-ort: maintain expected invariant for priv member\n merge-ort: fix type of local 'clean' var in handle_content_merge()\n merge-ort: clearer propagation of failure-to-function from\n merge_submodule\n merge-ort: loosen commented requirements\n merge-ort: upon merge abort, only show messages causing the abort\n merge-ort: convert more error() cases to path_msg()\n\n merge-ort.c | 168 +++++++++++++++++++++++++++++++-----------\n t/t6406-merge-attr.sh | 42 ++++++++++-\n 2 files changed, 164 insertions(+), 46 deletions(-)\n\n\nbase-commit: 8d94cfb54504f2ec9edc7ca3eb5c29a3dd3675ae\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1748%2Fnewren%2Ffix-error-cases-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1748/newren/fix-error-cases-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1748\n\nRange-diff vs v1:\n\n 1: d4ba1fccd91 ! 1: 5c50c0b3aad merge-ort: extract handling of priv member into reusable function\n @@ merge-ort.c: static void merge_check_renames_reusable(struct merge_result *resul\n +\t * to move it.\n +\t */\n +\tassert(opt->priv && !result->priv);\n -+\tif (!opt->priv->call_depth) {\n -+\t\tresult->priv = opt->priv;\n -+\t\tresult->_properly_initialized = RESULT_INITIALIZED;\n -+\t\topt->priv = NULL;\n -+\t}\n ++\tresult->priv = opt->priv;\n ++\tresult->_properly_initialized = RESULT_INITIALIZED;\n ++\topt->priv = NULL;\n +}\n +\n /*\n @@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *o\n -\t\tresult->_properly_initialized = RESULT_INITIALIZED;\n -\t\topt->priv = NULL;\n -\t}\n -+\tmove_opt_priv_to_result_priv(opt, result);\n ++\tif (!opt->priv->call_depth)\n ++\t\tmove_opt_priv_to_result_priv(opt, result);\n }\n \n /*\n 2: 17c97301baa ! 2: d1adec6d105 merge-ort: maintain expected invariant for priv member\n @@ Commit message\n Both merge_switch_to_result() and merge_finalize() expect\n opt->priv == NULL && result->priv != NULL\n which is supposed to be set up by our move_opt_priv_to_result_priv()\n - function. However, two codepath dealing with error cases did not\n + function. However, two codepaths dealing with error cases did not\n execute this necessary logic, which could result in assertion failures\n (or, if assertions were compiled out, could result in segfaults). Fix\n the oversight and add a test that would have caught one of these\n problems.\n \n While at it, also tighten an existing test for a non-recursive merge\n - to verify that it fails correctly, i.e. with the expected exit code\n - rather than with an assertion failure.\n + to verify that it fails with appropriate status. Most merge tests in\n + the testsuite check either for success or conflicts; those testing for\n + neither are rare and it is good to ensure they support the invariant\n + assumed by builtin/merge.c in this comment:\n + /*\n + * The backend exits with 1 when conflicts are\n + * left to be resolved, with 2 when it does not\n + * handle the given merge at all.\n + */\n + So, explicitly check for the exit status of 2 in these cases.\n \n Reported-by: Matt Cree \n Signed-off-by: Elijah Newren \n \n ## merge-ort.c ##\n -@@ merge-ort.c: static void move_opt_priv_to_result_priv(struct merge_options *opt,\n - \t * to move it.\n - \t */\n - \tassert(opt->priv && !result->priv);\n --\tif (!opt->priv->call_depth) {\n -+\tif (!opt->priv->call_depth || result->clean < 0) {\n - \t\tresult->priv = opt->priv;\n - \t\tresult->_properly_initialized = RESULT_INITIALIZED;\n - \t\topt->priv = NULL;\n @@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n \t\t oid_to_hex(&side1->object.oid),\n \t\t oid_to_hex(&side2->object.oid));\n @@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *o\n \t\treturn;\n \t}\n \ttrace2_region_leave(\"merge\", \"collect_merge_info\", opt->repo);\n +@@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n + \t\t/* existence of conflicted entries implies unclean */\n + \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n + \t}\n +-\tif (!opt->priv->call_depth)\n ++\tif (!opt->priv->call_depth || result->clean < 0)\n + \t\tmove_opt_priv_to_result_priv(opt, result);\n + }\n + \n \n ## t/t6406-merge-attr.sh ##\n @@ t/t6406-merge-attr.sh: test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'\n 3: 23bb3386114 = 3: 034b91db1d2 merge-ort: fix type of local 'clean' var in handle_content_merge()\n 4: 2789c58cd3f = 4: 2813a15b48b merge-ort: clearer propagation of failure-to-function from merge_submodule\n 5: 975fbddf305 = 5: 750acae4dba merge-ort: loosen commented requirements\n 6: 71391b18c1a ! 6: 6756956d0c7 merge-ort: upon merge abort, only show messages causing the abort\n @@ merge-ort.c: enum conflict_and_info_types {\n +\n +\t/*\n +\t * Something is seriously wrong; cannot even perform merge;\n -+\t * Keep this group _last_ other than NB_CONFLICT_TYPES\n ++\t * Keep this group _last_ other than NB_TOTAL_TYPES\n +\t */\n +\tERROR_SUBMODULE_CORRUPT,\n \n @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,\n -\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n +\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n - \t\t\t _(\"Failed to merge submodule %s \"\n +-\t\t\t _(\"Failed to merge submodule %s \"\n ++\t\t\t _(\"error: failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n + \t\t\t path);\n + \t\tret = -1;\n @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,\n \t}\n \tret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);\n 7: 32ae44b6260 ! 7: 500433edf49 merge-ort: convert more error() cases to path_msg()\n @@ Commit message\n \n ## merge-ort.c ##\n @@ merge-ort.c: enum conflict_and_info_types {\n - \t * Keep this group _last_ other than NB_CONFLICT_TYPES\n + \t * Keep this group _last_ other than NB_TOTAL_TYPES\n \t */\n \tERROR_SUBMODULE_CORRUPT,\n +\tERROR_THREEWAY_CONTENT_MERGE_FAILED,\n\n-- \ngitgitgadget\n"},{"id":"497319","messageId":"d1adec6d10556e247c21f94420879724fa2c6436.1718766019.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.v2.git.1718766019.gitgitgadget@gmail.com","subject":"[PATCH v2 2/7] merge-ort: maintain expected invariant for priv member","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-19T03:00:14Z","receivedAt":"2024-06-19T03:00:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe calling convention for the merge machinery is\n One call to init_merge_options()\n One or more calls to merge_incore_[non]recursive()\n One call to merge_finalize()\n (possibly indirectly via merge_switch_to_result())\nBoth merge_switch_to_result() and merge_finalize() expect\n opt->priv == NULL && result->priv != NULL\nwhich is supposed to be set up by our move_opt_priv_to_result_priv()\nfunction. However, two codepaths dealing with error cases did not\nexecute this necessary logic, which could result in assertion failures\n(or, if assertions were compiled out, could result in segfaults). Fix\nthe oversight and add a test that would have caught one of these\nproblems.\n\nWhile at it, also tighten an existing test for a non-recursive merge\nto verify that it fails with appropriate status. Most merge tests in\nthe testsuite check either for success or conflicts; those testing for\nneither are rare and it is good to ensure they support the invariant\nassumed by builtin/merge.c in this comment:\n /*\n * The backend exits with 1 when conflicts are\n * left to be resolved, with 2 when it does not\n * handle the given merge at all.\n */\nSo, explicitly check for the exit status of 2 in these cases.\n\nReported-by: Matt Cree \nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 3 ++-\n t/t6406-merge-attr.sh | 42 +++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 700ddfccb90..dc62aaf6d11 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -5050,6 +5050,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n \t\t oid_to_hex(&side1->object.oid),\n \t\t oid_to_hex(&side2->object.oid));\n \t\tresult->clean = -1;\n+\t\tmove_opt_priv_to_result_priv(opt, result);\n \t\treturn;\n \t}\n \ttrace2_region_leave(\"merge\", \"collect_merge_info\", opt->repo);\n@@ -5080,7 +5081,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n \t\t/* existence of conflicted entries implies unclean */\n \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n \t}\n-\tif (!opt->priv->call_depth)\n+\tif (!opt->priv->call_depth || result->clean < 0)\n \t\tmove_opt_priv_to_result_priv(opt, result);\n }\n \ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex 156a1efacfe..b6db5c2cc36 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -185,7 +185,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'\n \n \t>./please-abort &&\n \techo \"* merge=custom\" >.gitattributes &&\n-\ttest_must_fail git merge main 2>err &&\n+\ttest_expect_code 2 git merge main 2>err &&\n \tgrep \"^error: failed to execute internal merge\" err &&\n \tgit ls-files -u >output &&\n \tgit diff --name-only HEAD >>output &&\n@@ -261,4 +261,44 @@ test_expect_success 'binary files with union attribute' '\n \tgrep -i \"warning.*cannot merge.*HEAD vs. bin-main\" output\n '\n \n+test_expect_success !WINDOWS 'custom merge driver that is killed with a signal on recursive merge' '\n+\ttest_when_finished \"rm -f output please-abort\" &&\n+\ttest_when_finished \"git checkout side\" &&\n+\n+\tgit reset --hard anchor &&\n+\n+\tgit checkout -b base-a main^ &&\n+\techo base-a >text &&\n+\tgit commit -m base-a text &&\n+\n+\tgit checkout -b base-b main^ &&\n+\techo base-b >text &&\n+\tgit commit -m base-b text &&\n+\n+\tgit checkout -b recursive-a base-a &&\n+\ttest_must_fail git merge base-b &&\n+\techo recursive-a >text &&\n+\tgit add text &&\n+\tgit commit -m recursive-a &&\n+\n+\tgit checkout -b recursive-b base-b &&\n+\ttest_must_fail git merge base-a &&\n+\techo recursive-b >text &&\n+\tgit add text &&\n+\tgit commit -m recursive-b &&\n+\n+\tgit config --replace-all \\\n+\tmerge.custom.driver \"./custom-merge %O %A %B 0 %P %S %X %Y\" &&\n+\tgit config --replace-all \\\n+\tmerge.custom.name \"custom merge driver for testing\" &&\n+\n+\t>./please-abort &&\n+\techo \"* merge=custom\" >.gitattributes &&\n+\ttest_expect_code 2 git merge recursive-a 2>err &&\n+\tgrep \"^error: failed to execute internal merge\" err &&\n+\tgit ls-files -u >output &&\n+\tgit diff --name-only HEAD >>output &&\n+\ttest_must_be_empty output\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"497320","messageId":"034b91db1d2ed78995b52c014de313744972ff40.1718766019.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.v2.git.1718766019.gitgitgadget@gmail.com","subject":"[PATCH v2 3/7] merge-ort: fix type of local 'clean' var in handle_content_merge()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-19T03:00:15Z","receivedAt":"2024-06-19T03:00:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nhandle_content_merge() returns an int. Every caller of\nhandle_content_merge() expects an int. However, we declare a local\nvariable 'clean' that we use for the return value to be unsigned. To\nmake matters worse, we also assign 'clean' the return value of\nmerge_submodule() in one codepath, which is defined to return an int.\nIt seems that the only reason to have 'clean' be unsigned was to allow a\ncutesy bit manipulation operation to be well-defined. Fix the type of\nthe 'clean' local in handle_content_merge().\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex dc62aaf6d11..be0f5bc3838 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2109,7 +2109,7 @@ static int handle_content_merge(struct merge_options *opt,\n \t * merges, which happens for example with rename/rename(2to1) and\n \t * rename/add conflicts.\n \t */\n-\tunsigned clean = 1;\n+\tint clean = 1;\n \n \t/*\n \t * handle_content_merge() needs both files to be of the same type, i.e.\n@@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt,\n \t\tfree(result_buf.ptr);\n \t\tif (ret)\n \t\t\treturn -1;\n-\t\tclean &= (merge_status == 0);\n+\t\tif (merge_status > 0)\n+\t\t\tclean = 0;\n \t\tpath_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL,\n \t\t\t _(\"Auto-merging %s\"), path);\n \t} else if (S_ISGITLINK(a->mode)) {\n-- \ngitgitgadget\n\n"},{"id":"497321","messageId":"2813a15b48b70ead7e3fd062d1b49baee665fc9d.1718766019.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.v2.git.1718766019.gitgitgadget@gmail.com","subject":"[PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-19T03:00:16Z","receivedAt":"2024-06-19T03:00:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =\nconflicted, -1 = failure-to-determine), but we often like to think of\nit as binary (ignoring the possibility of a negative value) and use\nconstructs like '!clean' to reflect this. However, these constructs\ncan make codepaths more difficult to understand, unless we handle the\nnegative case early and return pre-emptively; do that in\nhandle_content_merge() to make the code a bit easier to read.\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 be0f5bc3838..d187c966c6a 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2193,6 +2193,8 @@ static int handle_content_merge(struct merge_options *opt,\n \t\tclean = merge_submodule(opt, pathnames[0],\n \t\t\t\t\ttwo_way ? null_oid() : &o->oid,\n \t\t\t\t\t&a->oid, &b->oid, &result->oid);\n+\t\tif (clean < 0)\n+\t\t\treturn -1;\n \t\tif (opt->priv->call_depth && two_way && !clean) {\n \t\t\tresult->mode = o->mode;\n \t\t\toidcpy(&result->oid, &o->oid);\n-- \ngitgitgadget\n\n"},{"id":"497322","messageId":"750acae4dba009d7a2f2e10b3767fcecf3a4748c.1718766019.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.v2.git.1718766019.gitgitgadget@gmail.com","subject":"[PATCH v2 5/7] merge-ort: loosen commented requirements","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-19T03:00:17Z","receivedAt":"2024-06-19T03:00:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe comment above type_short_descriptions claimed that the order had to\nmatch what was found in the conflict_info_and_types enum. Since\ntype_short_descriptions uses designated initializers, the order should\nnot actually matter; I am guessing that positional initializers may have\nbeen under consideration when that comment was added, but the comment\nwas not updated when designated initializers were chosen.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d187c966c6a..d0b13463283 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -553,7 +553,7 @@ enum conflict_and_info_types {\n * Short description of conflict type, relied upon by external tools.\n *\n * We can add more entries, but DO NOT change any of these strings. Also,\n- * Order MUST match conflict_info_and_types.\n+ * please ensure the order matches what is used in conflict_info_and_types.\n */\n static const char *type_short_descriptions[] = {\n \t/*** \"Simple\" conflicts and informational messages ***/\n-- \ngitgitgadget\n\n"},{"id":"497323","messageId":"6756956d0c7e3672a1a3b362b31b9d7e29bc5b9f.1718766019.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.v2.git.1718766019.gitgitgadget@gmail.com","subject":"[PATCH v2 6/7] merge-ort: upon merge abort, only show messages causing the abort","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-19T03:00:18Z","receivedAt":"2024-06-19T03:00:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen something goes wrong enough that we need to abort early and not\neven attempt merging the remaining files, it probably does not make\nsense to report conflicts messages for the subset of files we processed\nbefore hitting the fatal error. Instead, only show the messages\nassociated with paths where we hit the fatal error. Also, print these\nmessages to stderr rather than stdout.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 78 ++++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 53 insertions(+), 25 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d0b13463283..b337e4d74ef 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -543,10 +543,24 @@ enum conflict_and_info_types {\n \tCONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE,\n \tCONFLICT_SUBMODULE_MAY_HAVE_REWINDS,\n \tCONFLICT_SUBMODULE_NULL_MERGE_BASE,\n-\tCONFLICT_SUBMODULE_CORRUPT,\n+\n+\t/* INSERT NEW ENTRIES HERE */\n+\n+\t/*\n+\t * Keep this entry after all regular conflict and info types; only\n+\t * errors (failures causing immediate abort of the merge) should\n+\t * come after this.\n+\t */\n+\tNB_REGULAR_CONFLICT_TYPES,\n+\n+\t/*\n+\t * Something is seriously wrong; cannot even perform merge;\n+\t * Keep this group _last_ other than NB_TOTAL_TYPES\n+\t */\n+\tERROR_SUBMODULE_CORRUPT,\n \n \t/* Keep this entry _last_ in the list */\n-\tNB_CONFLICT_TYPES,\n+\tNB_TOTAL_TYPES,\n };\n \n /*\n@@ -597,8 +611,10 @@ static const char *type_short_descriptions[] = {\n \t\t\"CONFLICT (submodule may have rewinds)\",\n \t[CONFLICT_SUBMODULE_NULL_MERGE_BASE] =\n \t\t\"CONFLICT (submodule lacks merge base)\",\n-\t[CONFLICT_SUBMODULE_CORRUPT] =\n-\t\t\"CONFLICT (submodule corrupt)\"\n+\n+\t/* Something is seriously wrong; cannot even perform merge */\n+\t[ERROR_SUBMODULE_CORRUPT] =\n+\t\t\"ERROR (submodule corrupt)\",\n };\n \n struct logical_conflict_info {\n@@ -762,7 +778,8 @@ static void path_msg(struct merge_options *opt,\n \n \t/* Sanity checks */\n \tassert(omittable_hint ==\n-\t !starts_with(type_short_descriptions[type], \"CONFLICT\") ||\n+\t (!starts_with(type_short_descriptions[type], \"CONFLICT\") &&\n+\t\t!starts_with(type_short_descriptions[type], \"ERROR\")) ||\n \t type == CONFLICT_DIR_RENAME_SUGGESTED);\n \tif (opt->record_conflict_msgs_as_headers && omittable_hint)\n \t\treturn; /* Do not record mere hints in headers */\n@@ -1817,9 +1834,9 @@ static int merge_submodule(struct merge_options *opt,\n \t/* check whether both changes are forward */\n \tret2 = repo_in_merge_bases(&subrepo, commit_o, commit_a);\n \tif (ret2 < 0) {\n-\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n+\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n-\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t _(\"error: failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n \t\tret = -1;\n@@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt,\n \tif (ret2 > 0)\n \t\tret2 = repo_in_merge_bases(&subrepo, commit_o, commit_b);\n \tif (ret2 < 0) {\n-\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n+\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n-\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t _(\"error: failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n \t\tret = -1;\n@@ -1848,9 +1865,9 @@ static int merge_submodule(struct merge_options *opt,\n \t/* Case #1: a is contained in b or vice versa */\n \tret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);\n \tif (ret2 < 0) {\n-\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n+\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n-\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t _(\"error: failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n \t\tret = -1;\n@@ -1867,9 +1884,9 @@ static int merge_submodule(struct merge_options *opt,\n \t}\n \tret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);\n \tif (ret2 < 0) {\n-\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n+\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n-\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t _(\"error: failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n \t\tret = -1;\n@@ -1901,9 +1918,9 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t\t\t &merges);\n \tswitch (parent_count) {\n \tcase -1:\n-\t\tpath_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,\n+\t\tpath_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,\n \t\t\t path, NULL, NULL, NULL,\n-\t\t\t _(\"Failed to merge submodule %s \"\n+\t\t\t _(\"error: failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n \t\tret = -1;\n@@ -4646,6 +4663,7 @@ void merge_display_update_messages(struct merge_options *opt,\n \tstruct hashmap_iter iter;\n \tstruct strmap_entry *e;\n \tstruct string_list olist = STRING_LIST_INIT_NODUP;\n+\tFILE *o = stdout;\n \n \tif (opt->record_conflict_msgs_as_headers)\n \t\tBUG(\"Either display conflict messages or record them as headers, not both\");\n@@ -4662,6 +4680,10 @@ void merge_display_update_messages(struct merge_options *opt,\n \t}\n \tstring_list_sort(&olist);\n \n+\t/* Print to stderr if we hit errors rather than just conflicts */\n+\tif (result->clean < 0)\n+\t\to = stderr;\n+\n \t/* Iterate over the items, printing them */\n \tfor (int path_nr = 0; path_nr < olist.nr; ++path_nr) {\n \t\tstruct string_list *conflicts = olist.items[path_nr].util;\n@@ -4669,25 +4691,31 @@ void merge_display_update_messages(struct merge_options *opt,\n \t\t\tstruct logical_conflict_info *info =\n \t\t\t\tconflicts->items[i].util;\n \n+\t\t\t/* On failure, ignore regular conflict types */\n+\t\t\tif (result->clean < 0 &&\n+\t\t\t info->type < NB_REGULAR_CONFLICT_TYPES)\n+\t\t\t\tcontinue;\n+\n \t\t\tif (detailed) {\n-\t\t\t\tprintf(\"%lu\", (unsigned long)info->paths.nr);\n-\t\t\t\tputchar('\\0');\n+\t\t\t\tfprintf(o, \"%lu\", (unsigned long)info->paths.nr);\n+\t\t\t\tfputc('\\0', o);\n \t\t\t\tfor (int n = 0; n < info->paths.nr; n++) {\n-\t\t\t\t\tfputs(info->paths.v[n], stdout);\n-\t\t\t\t\tputchar('\\0');\n+\t\t\t\t\tfputs(info->paths.v[n], o);\n+\t\t\t\t\tfputc('\\0', o);\n \t\t\t\t}\n-\t\t\t\tfputs(type_short_descriptions[info->type],\n-\t\t\t\t stdout);\n-\t\t\t\tputchar('\\0');\n+\t\t\t\tfputs(type_short_descriptions[info->type], o);\n+\t\t\t\tfputc('\\0', o);\n \t\t\t}\n-\t\t\tputs(conflicts->items[i].string);\n+\t\t\tfputs(conflicts->items[i].string, o);\n+\t\t\tfputc('\\n', o);\n \t\t\tif (detailed)\n-\t\t\t\tputchar('\\0');\n+\t\t\t\tfputc('\\0', o);\n \t\t}\n \t}\n \tstring_list_clear(&olist, 0);\n \n-\tprint_submodule_conflict_suggestion(&opti->conflicted_submodules);\n+\tif (result->clean >= 0)\n+\t\tprint_submodule_conflict_suggestion(&opti->conflicted_submodules);\n \n \t/* Also include needed rename limit adjustment now */\n \tdiff_warn_rename_limit(\"merge.renamelimit\",\n-- \ngitgitgadget\n\n"},{"id":"497324","messageId":"500433edf49a4df448b330e4ed9201cfac83cecf.1718766019.git.gitgitgadget@gmail.com","threadId":"61631","inReplyTo":"pull.1748.v2.git.1718766019.gitgitgadget@gmail.com","subject":"[PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-06-19T03:00:19Z","receivedAt":"2024-06-19T03:00:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nmerge_submodule() stores errors using path_msg(), whereas other call\nsites make use of the error() function. This is inconsistent, and\nmoving towards path_msg() seems more friendly for libification efforts\nsince it will allow the caller to determine whether the error messages\nneed to be printed.\n\nNote that this deferred handling of error messages changes the error\nmessage in a recursive merge from\n error: failed to execute internal merge\nto\n From inner merge: error: failed to execute internal merge\nwhich provides a little more information about the error which may be\nuseful. Since the recursive merge strategy still only shows the older\nerror, we had to adjust the new testcase introduced a few commits ago to\njust search for the older message somewhere in the output.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 53 +++++++++++++++++++++++++++++++++----------\n t/t6406-merge-attr.sh | 2 +-\n 2 files changed, 42 insertions(+), 13 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex b337e4d74ef..8dfe80f1009 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -558,6 +558,10 @@ enum conflict_and_info_types {\n \t * Keep this group _last_ other than NB_TOTAL_TYPES\n \t */\n \tERROR_SUBMODULE_CORRUPT,\n+\tERROR_THREEWAY_CONTENT_MERGE_FAILED,\n+\tERROR_OBJECT_WRITE_FAILED,\n+\tERROR_OBJECT_READ_FAILED,\n+\tERROR_OBJECT_NOT_A_BLOB,\n \n \t/* Keep this entry _last_ in the list */\n \tNB_TOTAL_TYPES,\n@@ -615,6 +619,14 @@ static const char *type_short_descriptions[] = {\n \t/* Something is seriously wrong; cannot even perform merge */\n \t[ERROR_SUBMODULE_CORRUPT] =\n \t\t\"ERROR (submodule corrupt)\",\n+\t[ERROR_THREEWAY_CONTENT_MERGE_FAILED] =\n+\t\t\"ERROR (three-way content merge failed)\",\n+\t[ERROR_OBJECT_WRITE_FAILED] =\n+\t\t\"ERROR (object write failed)\",\n+\t[ERROR_OBJECT_READ_FAILED] =\n+\t\t\"ERROR (object read failed)\",\n+\t[ERROR_OBJECT_NOT_A_BLOB] =\n+\t\t\"ERROR (object is not a blob)\",\n };\n \n struct logical_conflict_info {\n@@ -2190,15 +2202,24 @@ static int handle_content_merge(struct merge_options *opt,\n \t\t\t\t\t pathnames, extra_marker_size,\n \t\t\t\t\t &result_buf);\n \n-\t\tif ((merge_status < 0) || !result_buf.ptr)\n-\t\t\tret = error(_(\"failed to execute internal merge\"));\n+\t\tif ((merge_status < 0) || !result_buf.ptr) {\n+\t\t\tpath_msg(opt, ERROR_THREEWAY_CONTENT_MERGE_FAILED, 0,\n+\t\t\t\t pathnames[0], pathnames[1], pathnames[2], NULL,\n+\t\t\t\t _(\"error: failed to execute internal merge for %s\"),\n+\t\t\t\t path);\n+\t\t\tret = -1;\n+\t\t}\n \n \t\tif (!ret &&\n \t\t write_object_file(result_buf.ptr, result_buf.size,\n-\t\t\t\t OBJ_BLOB, &result->oid))\n-\t\t\tret = error(_(\"unable to add %s to database\"), path);\n-\n+\t\t\t\t OBJ_BLOB, &result->oid)) {\n+\t\t\tpath_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0,\n+\t\t\t\t pathnames[0], pathnames[1], pathnames[2], NULL,\n+\t\t\t\t _(\"error: unable to add %s to database\"), path);\n+\t\t\tret = -1;\n+\t\t}\n \t\tfree(result_buf.ptr);\n+\n \t\tif (ret)\n \t\t\treturn -1;\n \t\tif (merge_status > 0)\n@@ -3577,18 +3598,26 @@ static int sort_dirs_next_to_their_children(const char *one, const char *two)\n \t\treturn c1 - c2;\n }\n \n-static int read_oid_strbuf(const struct object_id *oid,\n-\t\t\t struct strbuf *dst)\n+static int read_oid_strbuf(struct merge_options *opt,\n+\t\t\t const struct object_id *oid,\n+\t\t\t struct strbuf *dst,\n+\t\t\t const char *path)\n {\n \tvoid *buf;\n \tenum object_type type;\n \tunsigned long size;\n \tbuf = repo_read_object_file(the_repository, oid, &type, &size);\n-\tif (!buf)\n-\t\treturn error(_(\"cannot read object %s\"), oid_to_hex(oid));\n+\tif (!buf) {\n+\t\tpath_msg(opt, ERROR_OBJECT_READ_FAILED, 0,\n+\t\t\t path, NULL, NULL, NULL,\n+\t\t\t _(\"error: cannot read object %s\"), oid_to_hex(oid));\n+\t\treturn -1;\n+\t}\n \tif (type != OBJ_BLOB) {\n \t\tfree(buf);\n-\t\treturn error(_(\"object %s is not a blob\"), oid_to_hex(oid));\n+\t\tpath_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,\n+\t\t\t path, NULL, NULL, NULL,\n+\t\t\t _(\"error: object %s is not a blob\"), oid_to_hex(oid));\n \t}\n \tstrbuf_attach(dst, buf, size, size + 1);\n \treturn 0;\n@@ -3612,8 +3641,8 @@ static int blob_unchanged(struct merge_options *opt,\n \tif (oideq(&base->oid, &side->oid))\n \t\treturn 1;\n \n-\tif (read_oid_strbuf(&base->oid, &basebuf) ||\n-\t read_oid_strbuf(&side->oid, &sidebuf))\n+\tif (read_oid_strbuf(opt, &base->oid, &basebuf, path) ||\n+\t read_oid_strbuf(opt, &side->oid, &sidebuf, path))\n \t\tgoto error_return;\n \t/*\n \t * Note: binary | is used so that both renormalizations are\ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex b6db5c2cc36..9bf95249347 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -295,7 +295,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal o\n \t>./please-abort &&\n \techo \"* merge=custom\" >.gitattributes &&\n \ttest_expect_code 2 git merge recursive-a 2>err &&\n-\tgrep \"^error: failed to execute internal merge\" err &&\n+\tgrep \"error: failed to execute internal merge\" err &&\n \tgit ls-files -u >output &&\n \tgit diff --name-only HEAD >>output &&\n \ttest_must_be_empty output\n-- \ngitgitgadget\n"},{"id":"497778","messageId":"0df65763-9419-4ed4-ba28-a82ea1ac6dcb@gmail.com","threadId":"61631","inReplyTo":"d1adec6d10556e247c21f94420879724fa2c6436.1718766019.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/7] merge-ort: maintain expected invariant for priv member","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-06-28T02:09:42Z","receivedAt":"2024-06-28T02:09:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> The calling convention for the merge machinery is\n> One call to init_merge_options()\n> One or more calls to merge_incore_[non]recursive()\n> One call to merge_finalize()\n> (possibly indirectly via merge_switch_to_result())\n> Both merge_switch_to_result() and merge_finalize() expect\n> opt->priv == NULL && result->priv != NULL\n> which is supposed to be set up by our move_opt_priv_to_result_priv()\n> function. However, two codepaths dealing with error cases did not\n> execute this necessary logic, which could result in assertion failures\n> (or, if assertions were compiled out, could result in segfaults). Fix\n> the oversight and add a test that would have caught one of these\n> problems.\n\n\"one of these problems\" is key here.\n\n> @@ -5050,6 +5050,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n> \t\t oid_to_hex(&side1->object.oid),\n> \t\t oid_to_hex(&side2->object.oid));\n> \t\tresult->clean = -1;\n> +\t\tmove_opt_priv_to_result_priv(opt, result);\n> \t\treturn;\n> \t} > \ttrace2_region_leave(\"merge\", \"collect_merge_info\", opt->repo);\n\nRemoving this line does not cause your test script to fail. This is\nunderstandable as this case only happens during a parse failure, so\nit would be unreasonable to generate a test case that only fails\nfor such a reason.\n\n> @@ -5080,7 +5081,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,\n> \t\t/* existence of conflicted entries implies unclean */\n> \t\tresult->clean &= strmap_empty(&opt->priv->conflicted);\n> \t}\n> -\tif (!opt->priv->call_depth)\n> +\tif (!opt->priv->call_depth || result->clean < 0)\n> \t\tmove_opt_priv_to_result_priv(opt, result);\n> }\n\nRemoving this change does get caught by the new test.\n\nThanks,\n-Stolee\n"},{"id":"497779","messageId":"63caecb4-19cd-4b6f-91f0-bd00df2ecb54@gmail.com","threadId":"61631","inReplyTo":"2813a15b48b70ead7e3fd062d1b49baee665fc9d.1718766019.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-06-28T02:12:08Z","receivedAt":"2024-06-28T02:12:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =\n> conflicted, -1 = failure-to-determine), but we often like to think of\n> it as binary (ignoring the possibility of a negative value) and use\n> constructs like '!clean' to reflect this. However, these constructs\n> can make codepaths more difficult to understand, unless we handle the\n> negative case early and return pre-emptively; do that in\n> handle_content_merge() to make the code a bit easier to read.\n\nThis patch is correct and valuable.\n\nWould it be valuable to go a bit further and turn 'clean' into\nan enum that reflects these states? Perhaps that would prevent\nfuture changes from slipping into this mistake.\n\nThanks,\n-Stolee\n"},{"id":"497780","messageId":"CABPp-BHgd1Diqaiqc+a+UinHb947iwEx99cpVFqckUGXjmGVsQ@mail.gmail.com","threadId":"61631","inReplyTo":"63caecb4-19cd-4b6f-91f0-bd00df2ecb54@gmail.com","subject":"Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-28T02:38:18Z","receivedAt":"2024-06-28T02:38:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jun 27, 2024 at 7:12 PM Derrick Stolee wrote:\n>\n> On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =\n> > conflicted, -1 = failure-to-determine), but we often like to think of\n> > it as binary (ignoring the possibility of a negative value) and use\n> > constructs like '!clean' to reflect this. However, these constructs\n> > can make codepaths more difficult to understand, unless we handle the\n> > negative case early and return pre-emptively; do that in\n> > handle_content_merge() to make the code a bit easier to read.\n>\n> This patch is correct and valuable.\n>\n> Would it be valuable to go a bit further and turn 'clean' into\n> an enum that reflects these states? Perhaps that would prevent\n> future changes from slipping into this mistake.\n\nThat may make sense to investigate, but I suspect it may be a bigger\nchange and would recommend making such a clean up a separate series.\n\nAlso, I'm curious if it makes sense to finish off replacing recursive\nwith ort first; as long as recursive exists, it has the same problem\nand in fact was the source of using a tri-state 'clean' variable and\nthus would need the same cleanup. But if we replace recursive with\nort (making explicit requests for 'recursive' be handled by 'ort', as\noriginally designed and intended), that cuts the number of sites\nneeding this cleanup in half.\n"},{"id":"497781","messageId":"6db979ed-3d08-4ac4-b1c6-65a76939de35@gmail.com","threadId":"61631","inReplyTo":"034b91db1d2ed78995b52c014de313744972ff40.1718766019.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/7] merge-ort: fix type of local 'clean' var in handle_content_merge()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-06-28T02:44:42Z","receivedAt":"2024-06-28T02:44:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> handle_content_merge() returns an int. Every caller of\n> handle_content_merge() expects an int. However, we declare a local\n> variable 'clean' that we use for the return value to be unsigned. To\n> make matters worse, we also assign 'clean' the return value of\n> merge_submodule() in one codepath, which is defined to return an int.\n> It seems that the only reason to have 'clean' be unsigned was to allow a\n> cutesy bit manipulation operation to be well-defined. Fix the type of\n> the 'clean' local in handle_content_merge().\n\n> @@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt,\n> \t\tfree(result_buf.ptr);\n> \t\tif (ret)\n> \t\t\treturn -1;\n> -\t\tclean &= (merge_status == 0);\n> +\t\tif (merge_status > 0)\n> +\t\t\tclean = 0;\n> \t\tpath_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL,\n> \t\t\t _(\"Auto-merging %s\"), path);\n> \t} else if (S_ISGITLINK(a->mode)) {\n\nEven after this removal of this cute bitflip, there is one more\nsubtle use still in the code:\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8dfe80f1009..569014eef31 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2629,7 +2629,8 @@ static char *check_for_directory_rename(struct \nmerge_options *opt,\n \tnew_path = handle_path_level_conflicts(opt, path, side_index,\n \t\t\t\t\t rename_info,\n \t\t\t\t\t &collisions[side_index]);\n-\t*clean_merge &= (new_path != NULL);\n+\tif (*clean_merge && !new_path)\n+\t\t*clean_merge = 0;\n\n \treturn new_path;\n }\n\nI had to think very carefully about this cleverness to be sure\nthat this conversion is right (and I'm only mostly sure). When\n(new_path != NULL) is false, then we definitely set *clean_merge\nto zero. Otherwise, we set it to 1 (but only when it was already\n1 or -1). Technically, this does change the behavior by not\nsquashing -1 into 1, but that is less likely to be an existing\nvalue of *clean_merge.\n\nThere are other uses of \"clean &= collect_renames(...)\" but that\nappears to be fine because collect_renames() never results in an\nabort state (returning -1).\n\nThanks,\n-Stolee\n"},{"id":"497782","messageId":"3f1e155f-f559-42ac-9454-8ddcf7873f48@gmail.com","threadId":"61631","inReplyTo":"pull.1748.v2.git.1718766019.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/7] Fix and improve some error codepaths in merge-ort","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-06-28T02:45:34Z","receivedAt":"2024-06-28T02:45:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:\n> This series started as a just a fix for the abort hit in merge-ort when\n> custom merge drivers error out (see\n> https://lore.kernel.org/git/75F8BD12-7743-4863-B4C5-049FDEC4645E@gearset.com/).\n> However, while working on that, I found a few other issues around error\n> codepaths in merge-ort. So this series:\n> \n> * Patches 1-2: fix the reported abort problem\n> * Patches 3-4: make code in handle_content_merges() easier to handle when\n> we hit errors\n> * Patch 5: fix a misleading comment\n> * Patches 6-7: make error handling (immediate print vs. letting callers get\n> the error information) more consistent\n\nI saw this in Junio's email about series requiring review, so I took a\nlook despite missing v1. Stepping through each commit in my local copy\nhelped make sure that these changes were correct in their proper context.\n\n> The last two patches change the behavior slightly for error codepaths, and\n> there's a question about whether we should show only the error messages that\n> caused an early termination of the merge, or if we should also show any\n> conflict messages for other paths that were handled before we hit the early\n> termination. These patches made a decision but feel free to take those last\n> two patches as more of an RFC.\n\nI also support this change in the final two patches.\n\nOne thing I mentioned in an earlier reply was \"why not use an\nenum in this tri-state 'clean' variable?\" and then I tried to\nmake just such a patch. There were two problems:\n\n 1. There were hundreds of changes that would be difficult to\n do in small bits (but maybe doable).\n\n 2. There is already an 'enum ll_merge_result' in merge-ll.h\n that is silently passed back from merge_3way() in merge-ort.c.\n While it's technically four states, maybe it would suffice to\n use that enum instead of creating a new one.\n\nBut I'll leave that as something to think about another time. It\ndoes not change the merit of this series. I left a note about\nanother \"&=\" case that wasn't touched, but it's not wrong as-is.\n\nThanks,\n-Stolee\n"},{"id":"497783","messageId":"1b7f9579-e583-4393-8410-1bbbf882e70f@gmail.com","threadId":"61631","inReplyTo":"CABPp-BHgd1Diqaiqc+a+UinHb947iwEx99cpVFqckUGXjmGVsQ@mail.gmail.com","subject":"Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-06-28T02:47:17Z","receivedAt":"2024-06-28T02:47:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/27/24 10:38 PM, Elijah Newren wrote:\n> On Thu, Jun 27, 2024 at 7:12 PM Derrick Stolee wrote:\n>>\n>> On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:\n>>> From: Elijah Newren \n>>>\n>>> The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =\n>>> conflicted, -1 = failure-to-determine), but we often like to think of\n>>> it as binary (ignoring the possibility of a negative value) and use\n>>> constructs like '!clean' to reflect this. However, these constructs\n>>> can make codepaths more difficult to understand, unless we handle the\n>>> negative case early and return pre-emptively; do that in\n>>> handle_content_merge() to make the code a bit easier to read.\n>>\n>> This patch is correct and valuable.\n>>\n>> Would it be valuable to go a bit further and turn 'clean' into\n>> an enum that reflects these states? Perhaps that would prevent\n>> future changes from slipping into this mistake.\n> \n> That may make sense to investigate, but I suspect it may be a bigger\n> change and would recommend making such a clean up a separate series.\n> \n> Also, I'm curious if it makes sense to finish off replacing recursive\n> with ort first; as long as recursive exists, it has the same problem\n> and in fact was the source of using a tri-state 'clean' variable and\n> thus would need the same cleanup. But if we replace recursive with\n> ort (making explicit requests for 'recursive' be handled by 'ort', as\n> originally designed and intended), that cuts the number of sites\n> needing this cleanup in half.\n\nYour fast response to this message means that I didn't see this when\nI mentioned it in my closing of the review (in response to your\ncover letter).\n\nReducing the size of the conversion would definitely be good to do,\nand then you could also consider using the existing ll_merge_result\nenum, though it technically has four states.\n\nThanks,\n-Stolee\n"},{"id":"497816","messageId":"xmqq34ox6lh0.fsf@gitster.g","threadId":"61631","inReplyTo":"3f1e155f-f559-42ac-9454-8ddcf7873f48@gmail.com","subject":"Re: [PATCH v2 0/7] Fix and improve some error codepaths in merge-ort","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-28T17:11:39Z","receivedAt":"2024-06-28T17:11:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> I saw this in Junio's email about series requiring review, so I took a\n> look despite missing v1. Stepping through each commit in my local copy\n> helped make sure that these changes were correct in their proper context.\n>\n>> The last two patches change the behavior slightly for error codepaths, and\n>> there's a question about whether we should show only the error messages that\n>> caused an early termination of the merge, or if we should also show any\n>> conflict messages for other paths that were handled before we hit the early\n>> termination. These patches made a decision but feel free to take those last\n>> two patches as more of an RFC.\n>\n> I also support this change in the final two patches.\n>\n> One thing I mentioned in an earlier reply was \"why not use an\n> enum in this tri-state 'clean' variable?\" and then I tried to\n> make just such a patch. ...\n> ... But I'll leave that as something to think about another time. It\n> does not change the merit of this series. I left a note about\n> another \"&=\" case that wasn't touched, but it's not wrong as-is.\n\nThanks for having done a thorough review, including exploration of\nan alternative. Really appreciated.\n\n"},{"id":"498004","messageId":"20240702213326.GA591768@coredump.intra.peff.net","threadId":"61631","inReplyTo":"500433edf49a4df448b330e4ed9201cfac83cecf.1718766019.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-02T21:33:26Z","receivedAt":"2024-07-02T21:33:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 19, 2024 at 03:00:19AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> +static int read_oid_strbuf(struct merge_options *opt,\n> +\t\t\t const struct object_id *oid,\n> +\t\t\t struct strbuf *dst,\n> +\t\t\t const char *path)\n> {\n> \tvoid *buf;\n> \tenum object_type type;\n> \tunsigned long size;\n> \tbuf = repo_read_object_file(the_repository, oid, &type, &size);\n> -\tif (!buf)\n> -\t\treturn error(_(\"cannot read object %s\"), oid_to_hex(oid));\n> +\tif (!buf) {\n> +\t\tpath_msg(opt, ERROR_OBJECT_READ_FAILED, 0,\n> +\t\t\t path, NULL, NULL, NULL,\n> +\t\t\t _(\"error: cannot read object %s\"), oid_to_hex(oid));\n> +\t\treturn -1;\n> +\t}\n> \tif (type != OBJ_BLOB) {\n> \t\tfree(buf);\n> -\t\treturn error(_(\"object %s is not a blob\"), oid_to_hex(oid));\n> +\t\tpath_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,\n> +\t\t\t path, NULL, NULL, NULL,\n> +\t\t\t _(\"error: object %s is not a blob\"), oid_to_hex(oid));\n> \t}\n> \tstrbuf_attach(dst, buf, size, size + 1);\n> \treturn 0;\n\nThis loses the early return in the \"type != OBJ_BLOB\" code path. So we\nfree(buf), but then continue on to the strbuf_attach() call on the\ndangling pointer. Should it \"return -1\" like the earlier conditional?\n\n-Peff\n"},{"id":"498038","messageId":"CABPp-BEkh4K1156FWUdB9wSjSQcdeUF0dcTJ3CS_b8yvz5Vffg@mail.gmail.com","threadId":"61631","inReplyTo":"20240702213326.GA591768@coredump.intra.peff.net","subject":"Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-07-03T15:48:59Z","receivedAt":"2024-07-03T15:49:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jul 2, 2024 at 2:33 PM Jeff King wrote:\n>\n> On Wed, Jun 19, 2024 at 03:00:19AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > +static int read_oid_strbuf(struct merge_options *opt,\n> > + const struct object_id *oid,\n> > + struct strbuf *dst,\n> > + const char *path)\n> > {\n> > void *buf;\n> > enum object_type type;\n> > unsigned long size;\n> > buf = repo_read_object_file(the_repository, oid, &type, &size);\n> > - if (!buf)\n> > - return error(_(\"cannot read object %s\"), oid_to_hex(oid));\n> > + if (!buf) {\n> > + path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,\n> > + path, NULL, NULL, NULL,\n> > + _(\"error: cannot read object %s\"), oid_to_hex(oid));\n> > + return -1;\n> > + }\n> > if (type != OBJ_BLOB) {\n> > free(buf);\n> > - return error(_(\"object %s is not a blob\"), oid_to_hex(oid));\n> > + path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,\n> > + path, NULL, NULL, NULL,\n> > + _(\"error: object %s is not a blob\"), oid_to_hex(oid));\n> > }\n> > strbuf_attach(dst, buf, size, size + 1);\n> > return 0;\n>\n> This loses the early return in the \"type != OBJ_BLOB\" code path. So we\n> free(buf), but then continue on to the strbuf_attach() call on the\n> dangling pointer. Should it \"return -1\" like the earlier conditional?\n>\n> -Peff\n\nOops! That's embarrassing. Thanks for catching; I'll send in a patch\non top since this is already in next.\n"},{"id":"498052","messageId":"xmqqwmm2b9xl.fsf@gitster.g","threadId":"61631","inReplyTo":"CABPp-BEkh4K1156FWUdB9wSjSQcdeUF0dcTJ3CS_b8yvz5Vffg@mail.gmail.com","subject":"Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-03T18:35:34Z","receivedAt":"2024-07-03T18:35:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Oops! That's embarrassing. Thanks for catching; I'll send in a patch\n> on top since this is already in next.\n\nThanks.\n"},{"id":"498146","messageId":"20240706061104.GA700645@coredump.intra.peff.net","threadId":"61631","inReplyTo":"CABPp-BEkh4K1156FWUdB9wSjSQcdeUF0dcTJ3CS_b8yvz5Vffg@mail.gmail.com","subject":"Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-06T06:11:04Z","receivedAt":"2024-07-06T06:11:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 03, 2024 at 08:48:59AM -0700, Elijah Newren wrote:\n\n> > > if (type != OBJ_BLOB) {\n> > > free(buf);\n> > > - return error(_(\"object %s is not a blob\"), oid_to_hex(oid));\n> > > + path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,\n> > > + path, NULL, NULL, NULL,\n> > > + _(\"error: object %s is not a blob\"), oid_to_hex(oid));\n> > > }\n> > > strbuf_attach(dst, buf, size, size + 1);\n> > > return 0;\n> >\n> > This loses the early return in the \"type != OBJ_BLOB\" code path. So we\n> > free(buf), but then continue on to the strbuf_attach() call on the\n> > dangling pointer. Should it \"return -1\" like the earlier conditional?\n> \n> Oops! That's embarrassing. Thanks for catching; I'll send in a patch\n> on top since this is already in next.\n\nYeah, I only caught it because Coverity flagged it when it hit 'next'.\nIt is quite subtle. :)\n\n-Peff\n"}]}