Re: [PATCH 4/9] merge_recursive: abort properly upon errors
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 29, 2016, 20:08 UTC
- Message-ID
- <xmqqvb0r3gi4.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <81a74b02ac714a4fa3734dfb774cff6dea3a3471.1467199553.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 24 quoted lines
> There are a couple of places where return values indicating errors > are ignored. Let's teach them manners. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > merge-recursive.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index bcb53f0..c4ece96 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -1944,8 +1944,9 @@ int merge_recursive(struct merge_options *o, > saved_b2 = o->branch2; > o->branch1 = "Temporary merge branch 1"; > o->branch2 = "Temporary merge branch 2"; > - merge_recursive(o, merged_common_ancestors, iter->item, > - NULL, &merged_common_ancestors); > + if (merge_recursive(o, merged_common_ancestors, iter->item, > + NULL, &merged_common_ancestors) < 0) > + return -1; > o->branch1 = saved_b1; > o->branch2 = saved_b2; > o->call_depth--;
OK, this early return (and others in this patch) are only for negative (i.e. error) cases, and "attempted a merge, resulted in conflicts" cases are handled as before.
Which is good.
I wonder if o->branch[12] need to be restored, though. The only sensible thing the caller can do is to punt, but would it expect to be able to do some error reporting based on these fields, e.g. printf("merge of %s and %s failed", o->branch1, o->branch2) or something? In addition to that kind of "state restoration", we may need to watch out for resource leaks, but I think there is none at least these three early returns.
Thanks.
Show 19 quoted lines
> @@ -1961,6 +1962,8 @@ int merge_recursive(struct merge_options *o,
> o->ancestor = "merged common ancestors";
> clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,
> &mrtree);
> + if (clean < 0)
> + return clean;
>
> if (o->call_depth) {
> *result = make_virtual_commit(mrtree, "merged tree");
> @@ -2017,6 +2020,9 @@ int merge_recursive_generic(struct merge_options *o,
> hold_locked_index(lock, 1);
> clean = merge_recursive(o, head_commit, next_commit, ca,
> result);
> + if (clean < 0)
> + return clean;
> +
> if (active_cache_changed &&
> write_locked_index(&the_index, lock, COMMIT_LOCK))
> return error(_("Unable to write index."));