Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: >> > 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--; >> >> I wonder if o->branch[12] need to be restored, though. The only >> sensible thing the caller can do is to punt,... > > I do not think that the caller can do anything sensible with *o after we > return an error... That is totally up to what this patch does, isn't it? By deliberately keeping o->branch[12] to point at the temporary names and not restoring, this patch declares "the caller cannot do anything sensible with *o". If it restores, the caller still can. Even with this step as-is, the caller can tell at which recursion level the merge failed by looking at o->call_depth, for example. I do not think the current set of callers, and a new one you will be introducing, would prefer to be able to do something with *o after the failure return from this function, so in that sense, I do not care deeply either way. But if the patch is making a policy decision "*o is undefined upon error return from this function", it would help people who want to build on top of this codebase to add that to the comment before the function, wouldn't it? -- 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