Re: [PATCH 1/5] merge-ort: propagate callback errors from traverse_trees_wrapper()
From: Junio C Hamano <hidden>
Date: 2026-06-01 12:13:13
"Elijah Newren via GitGitGadget" [off-list ref] writes:
From: Elijah Newren <redacted> traverse_trees_wrapper() saves entries from a first pass through traverse_trees() and then replays them through the real callback (collect_merge_info_callback). However, the replay loop silently discards the callback return value. This means any error reported by the callback during replay -- including a future check for malformed trees -- would be ignored, allowing the merge to proceed with corrupt state. Capture the return value, stop the loop on negative (error) returns, and propagate the error to the caller. Note that the callback returns a positive mask value on success, so we normalize non-negative returns to 0 for the caller.
All makes perfect sense. How would the externally visible behaviour change at this step? Upon an error from the callback, we used to keep going and processed other callback data in the renames structure. We now leave the rest unprocessed. The caller of this helper would never have seen a failure, but now they will. Both callers, collect_merge_info_callback() and handle_deferred_entries(), are reacting to a negative "error" return well (perhaps because they sometimes call traverse_trees() in the same control flow, which does return an error already), so presumably there is no downside caused by aborting the innermost process upon the first error return.
quoted hunk ↗ jump to hunk
Signed-off-by: Elijah Newren <redacted> --- merge-ort.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)diff --git a/merge-ort.c b/merge-ort.c index 00923ce3cd..4b8e32209d 100644 --- a/merge-ort.c +++ b/merge-ort.c@@ -1008,18 +1008,20 @@ static int traverse_trees_wrapper(struct index_state *istate, info->traverse_path = renames->callback_data_traverse_path; info->fn = old_fn; for (i = old_offset; i < renames->callback_data_nr; ++i) { - info->fn(n, - renames->callback_data[i].mask, - renames->callback_data[i].dirmask, - renames->callback_data[i].names, - info); + ret = info->fn(n, + renames->callback_data[i].mask, + renames->callback_data[i].dirmask, + renames->callback_data[i].names, + info); + if (ret < 0) + break; } renames->callback_data_nr = old_offset; free(renames->callback_data_traverse_path); renames->callback_data_traverse_path = old_callback_data_traverse_path; info->traverse_path = NULL; - return 0; + return ret < 0 ? ret : 0; } static void setup_path_info(struct merge_options *opt,