Hi Junio, On Fri, 1 Jul 2016, Junio C Hamano wrote: > 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? No, not really. We do not really know *where* in the recursive merge the failure happened. All we know is that it happened while trying to merge the temporary branches. > 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. But would restoring the branch names not give the false impression that the error occurred during a different phase of the recursive merge? > 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. Well, not really: 1) please note the o->call_depth-- that is *also* skipped in case of an error after my patch, and 2) what use is the recursion level if an arbitrary number of merges could happen at the same recursion depth? In short, I do not think that resetting those values in *o could bring any benefit to the caller, short of introducing those fine-grained error values I mentioned elsewhere. 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