Johannes Schindelin [off-list ref] writes:
quoted
quoted
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?
Hi Junio,
On Fri, 1 Jul 2016, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
quoted
quoted
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