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

Commit Message

Johannes Schindelin Feb. 22, 2024, 2:36 p.m. UTC

From: Johannes Schindelin <[e-mail]>

The previous commit fixed a bug where a missing tree was reported, but
not treated as an error.

This patch addresses the same issue for the remaining two callers of
`parse_tree()`.

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).

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 merge-ort.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Feb. 22, 2024, 5:18 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
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 <[e-mail]>
> ---
>  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);

Patch

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