Re: [PATCH 1/5] merge-ort: propagate callback errors from traverse_trees_wrapper()

[Junio C Hamano <gitster@pobox.com>]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> 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.



> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  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,