Re: [PATCH 3/9] Prepare the builtins for a libified merge_recursive()
From: Junio C Hamano <hidden>
Date: 2016-06-29 19:04:55
Johannes Schindelin [off-list ref] writes:
A truly libified function does not die() just for fun.
The sentence is wasting bits. After all, a helper function in run-once-and-exit program does not die() just for fun, either. So what's more interesting to know for the readers?
As such, the recursive merge will convert all die() calls to return -1 instead in the next commits, giving the caller a chance at least to print some helpful message. Let's prepare the builtins for this fatal error condition, even if we do not really do more than imitating the previous die()'s exit(128): this is what callers of e.g. `git merge` have come to expect.
One thing missing is your design decision and justification. For example, the above explanation hints that the original code in this hunk expected merge_trees() to die() with some useful message when there is an error condition, but merge_trees() is going to be improved not to die() itself and return -1 instead to signal an error, so that the caller can react more flexibly, and this is a step to prepare for the version of merge_trees() that no longer dies.
- merge_trees(&o, new->commit->tree, work, + ret = merge_trees(&o, new->commit->tree, work, old->commit->tree, &result); + if (ret < 0) + exit(128);
The postimage of the patch tells us that the caller is now responsible for exiting with status 128, but neither the proposed log message nor the above hunk tells us where the message the original code must have given to the end user from die() inside merge_trees(). The updated caller just exits, so a natural guess is that the calls to die() have been changed to fprintf(stderr) with the patch. But that does not mesh very well with the stated objective of the patch. The callers want flexibility to do their own error handling, including giving their own message, so letting merge_trees() to still write the same message to the standard error stream would not work well for them. A caller may want to do merge_trees() just to see if it succeeds, without wanting to give _any_ indication of that is happening to the user, because it has an alternate/fallback code if merge_trees() fails, for example (analogy: "am -3" first tries a straight patch application before fallking back to 3-way merge; it may not want to show the error from the first attempt). The reader _can_ guess that this step ignores the error-message issue, and improving it later (or keep ignoring that issue) might be OK in the context of this patch series, but it is necessary to be upfront to the readers what the design choices were and which one of those choices the proposed patch adopted as its design for them to be able to evaluate the patch series correctly. One design alternative we've seen used in our code to help callers who want no default messages is to pass struct strbuf &err down the callchain and collect the default messages without emitting them directly to the standard error stream when an error is diagnosed. I do not know if that pattern is applicable or should be applied to this codepath.
quoted hunk
Note that the callers of the sequencer (revert and cherry-pick) already fail fast even for the return value -1; The only difference is that they now get a chance to say "<command> failed". Signed-off-by: Johannes Schindelin <redacted> --- builtin/checkout.c | 4 +++- builtin/merge.c | 4 ++++ sequencer.c | 4 ++++ 3 files changed, 11 insertions(+), 1 deletion(-)diff --git a/builtin/checkout.c b/builtin/checkout.c index c3486bd..14312f7 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c@@ -567,8 +567,10 @@ static int merge_working_tree(const struct checkout_opts *opts, o.ancestor = old->name; o.branch1 = new->name; o.branch2 = "local"; - merge_trees(&o, new->commit->tree, work, + ret = merge_trees(&o, new->commit->tree, work, old->commit->tree, &result); + if (ret < 0) + exit(128); ret = reset_tree(new->commit->tree, opts, 0, writeout_error); if (ret)diff --git a/builtin/merge.c b/builtin/merge.c index b555a1b..133b853 100644 --- a/builtin/merge.c +++ b/builtin/merge.c@@ -682,6 +682,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, hold_locked_index(&lock, 1); clean = merge_recursive(&o, head, remoteheads->item, reversed, &result); + if (clean < 0) + exit(128); if (active_cache_changed && write_locked_index(&the_index, &lock, COMMIT_LOCK)) die (_("unable to write %s"), get_index_file());@@ -1550,6 +1552,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) ret = try_merge_strategy(use_strategies[i]->name, common, remoteheads, head_commit, head_arg); + if (ret < 0) + exit(128); if (!option_commit && !ret) { merge_was_ok = 1; /*diff --git a/sequencer.c b/sequencer.c index c6362d6..13b794a 100644 --- a/sequencer.c +++ b/sequencer.c@@ -293,6 +293,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, clean = merge_trees(&o, head_tree, next_tree, base_tree, &result); + if (clean < 0) + return clean; if (active_cache_changed && write_locked_index(&the_index, &index_lock, COMMIT_LOCK))@@ -561,6 +563,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) { res = do_recursive_merge(base, next, base_label, next_label, head, &msgbuf, opts); + if (res < 0) + return res; write_message(&msgbuf, git_path_merge_msg()); } else { struct commit_list *common = NULL;