Christian CouderThis patch refactors the code that prints a message like "Automatic cherry-pick failed. <help message>". This code was duplicated in both do_recursive_merge() and do_pick_commit().
To do that, now do_recursive_merge() returns an int to signal success or failure. And in case of failure we just return 1 from do_pick_commit() instead of doing "exit(1)" from either do_recursive_merge() or do_pick_commit().
In case of successful merge, do_recursive_merge() used to print something like "Finished one cherry-pick." but nothing was printed in case a special strategy like "resolve" was used. Now in this case a message like "Finished one cherry-pick with strategy resolve." is printed.
So the command behavior should be more consistent wether we are using a special strategy or not.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- I also wonder why messages like "Finished one cherry-pick." are printed on stderr instead of stdout. Maybe this could be changed in this patch or in another one.
And while at making a backward incompatible change, maybe we could change the message to something like:
"Finished cherry-pick <sha1>"
builtin/revert.c | 51 ++++++++++++++++++++--------------- t/t3508-cherry-pick-many-commits.sh | 26 +++++++++++++++++- 2 files changed, 54 insertions(+), 23 deletions(-)
Show changes to 2 files +54 −23
builtin/revert.c, t/t3508-cherry-pick-many-commits.sh
diff --git a/builtin/revert.c b/builtin/revert.c index 8b9d829..b082bb4 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -311,10 +311,9 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from) return write_ref_sha1(ref_lock, to, "cherry-pick"); } -static void do_recursive_merge(struct commit *base, struct commit *next, - const char *base_label, const char *next_label, - unsigned char *head, struct strbuf *msgbuf, - char *defmsg) +static int do_recursive_merge(struct commit *base, struct commit *next, + const char *base_label, const char *next_label, + unsigned char *head, struct strbuf *msgbuf) { struct merge_options o; struct tree *result, *next_tree, *base_tree, *head_tree; @@ -357,14 +356,9 @@ static void do_recursive_merge(struct commit *base, struct commit *next, i++; } } - write_message(msgbuf, defmsg); - fprintf(stderr, "Automatic %s failed.%s\n", - me, help_msg()); - rerere(allow_rerere_auto); - exit(1); } - write_message(msgbuf, defmsg); - fprintf(stderr, "Finished one %s.\n", me); + + return !clean; } static int do_pick_commit(void) @@ -375,6 +369,8 @@ static int do_pick_commit(void) struct commit_message msg = { NULL, NULL, NULL, NULL, NULL }; char *defmsg = NULL; struct strbuf msgbuf = STRBUF_INIT; + struct strbuf mebuf = STRBUF_INIT; + int res; if (no_commit) { /* @@ -470,30 +466,41 @@ static int do_pick_commit(void) } } - if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) - do_recursive_merge(base, next, base_label, next_label, - head, &msgbuf, defmsg); - else { - int res; + strbuf_addstr(&mebuf, me); + + if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) { + res = do_recursive_merge(base, next, base_label, next_label, + head, &msgbuf); + write_message(&msgbuf, defmsg); + } else { struct commit_list *common = NULL; struct commit_list *remotes = NULL; + + strbuf_addf(&mebuf, " with strategy %s", strategy); write_message(&msgbuf, defmsg); + commit_list_insert(base, &common); commit_list_insert(next, &remotes); res = try_merge_command(strategy, common, sha1_to_hex(head), remotes); free_commit_list(common); free_commit_list(remotes); - if (res) { - fprintf(stderr, "Automatic %s with strategy %s failed.%s\n", - me, strategy, help_msg()); - rerere(allow_rerere_auto); - exit(1); - } } + if (res) { + fprintf(stderr, "Automatic %s failed.%s\n", + mebuf.buf, help_msg()); + rerere(allow_rerere_auto); + } else { + fprintf(stderr, "Finished one %s.\n", mebuf.buf); + } + + strbuf_release(&mebuf); free_message(&msg); + if (res) + return 1; + /* * * If we are cherry-pick, and if the merge did not result in diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh index f90ed3d..6555f92 100755 --- a/t/t3508-cherry-pick-many-commits.sh +++ b/t/t3508-cherry-pick-many-commits.sh @@ -23,12 +23,36 @@ test_expect_success setup ' ' test_expect_success 'cherry-pick first..fourth works' ' + cat <<-EOF > expected && + Finished one cherry-pick. + Finished one cherry-pick. + Finished one cherry-pick. + EOF + + git checkout -f master && + git reset --hard first && + test_tick && + git cherry-pick first..fourth 2>actual && + git diff --quiet other && + git diff --quiet HEAD other && + test_cmp expected actual && + test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify fourth)" +' + +test_expect_success 'cherry-pick --strategy resolve first..fourth works' ' + cat <<-EOF > expected && + Finished one cherry-pick with strategy resolve. + Finished one cherry-pick with strategy resolve. + Finished one cherry-pick with strategy resolve. + EOF + git checkout -f master && git reset --hard first && test_tick && - git cherry-pick first..fourth && + git cherry-pick --strategy resolve first..fourth 2>actual && git diff --quiet other && git diff --quiet HEAD other && + test_cmp expected actual && test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify fourth)" '
-- 1.7.2.rc1.213.gf3fb81
