Hi Junio,
On Wed, 29 Jun 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>
> > 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@xxxxxx>
> > ---
> > 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.
I do not think that the caller can do anything sensible with *o after we
return an error: it means that we failed *somewhere* in that operation,
but does not say exactly where.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html