Re: [PATCH v3 2/5] merge-ort: do check `parse_tree()`'s return value

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> This change is not accompanied by a regression test because the code in
> question is only reached at the `checkout` stage, i.e. after the merge
> has happened (and therefore the tree objects could only be missing if
> the disk had gone bad in that short time window, or something similarly
> tricky to recreate in the test suite).

Makes sense.

A complete tangent I wonder is if unit-test-minded folks have clever
ideas to allow better test coverage, perhaps injecting a failure on
demand to any codepath (in this case, the codepath to write the
resulting tree) to simulate a situation where we fail to parse the
tree.

In any case, the patch looks good, of course, and I see no need for
further comments.

Thanks.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  merge-ort.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index c37fc035f13..79d9e18f63d 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,
>  	unpack_opts.verbose_update = (opt->verbosity > 2);
>  	unpack_opts.fn = twoway_merge;
>  	unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
> -	parse_tree(prev);
> +	if (parse_tree(prev) < 0)
> +		return -1;
>  	init_tree_desc(&trees[0], prev->buffer, prev->size);
> -	parse_tree(next);
> +	if (parse_tree(next) < 0)
> +		return -1;
>  	init_tree_desc(&trees[1], next->buffer, next->size);
>  
>  	ret = unpack_trees(2, trees, &unpack_opts);