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

Junio C Hamano
2024-02-22T17:18:05Z

"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);