From 7b53b92fdb22b90d2be558db84c725641c4ad170 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 14 Jul 2010 01:28:12 +0200 Subject: [PATCH 1/5] revert: report success when using option --strategy "git cherry-pick foo" has always reported success with "Finished one cherry-pick" but "cherry-pick --strategy" does not print anything. So move the code to write that message from do_recursive_merge() to do_cherry_pick() so other strategies can share it. This patch also refactors the code that prints a message like "Automatic cherry-pick failed. ". 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(). Signed-off-by: Christian Couder Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 51 ++++++++++++++++------------- t/t3508-cherry-pick-many-commits.sh | 26 ++++++++++++++- 2 files changed, 54 insertions(+), 23 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 8b9d829a73..b082bb425c 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 f90ed3da3e..d90b365fd2 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 && + 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 --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)" ' From 5df16453d4da41e5a36de536acf9acc8aab98041 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 14 Jul 2010 01:28:13 +0200 Subject: [PATCH 2/5] revert: refactor commit code into a new run_git_commit() function Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/revert.c | 52 +++++++++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index b082bb425c..b84b5b8645 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -361,6 +361,32 @@ static int do_recursive_merge(struct commit *base, struct commit *next, return !clean; } +/* + * If we are cherry-pick, and if the merge did not result in + * hand-editing, we will hit this commit and inherit the original + * author date and name. + * If we are revert, or if our cherry-pick results in a hand merge, + * we had better say that the current user is responsible for that. + */ +static int run_git_commit(const char *defmsg) +{ + /* 6 is max possible length of our args array including NULL */ + const char *args[6]; + int i = 0; + + args[i++] = "commit"; + args[i++] = "-n"; + if (signoff) + args[i++] = "-s"; + if (!edit) { + args[i++] = "-F"; + args[i++] = defmsg; + } + args[i] = NULL; + + return run_command_v_opt(args, RUN_GIT_CMD); +} + static int do_pick_commit(void) { unsigned char head[20]; @@ -501,33 +527,9 @@ static int do_pick_commit(void) if (res) return 1; - /* - * - * If we are cherry-pick, and if the merge did not result in - * hand-editing, we will hit this commit and inherit the original - * author date and name. - * If we are revert, or if our cherry-pick results in a hand merge, - * we had better say that the current user is responsible for that. - */ - if (!no_commit) { - /* 6 is max possible length of our args array including NULL */ - const char *args[6]; - int res; - int i = 0; - - args[i++] = "commit"; - args[i++] = "-n"; - if (signoff) - args[i++] = "-s"; - if (!edit) { - args[i++] = "-F"; - args[i++] = defmsg; - } - args[i] = NULL; - res = run_command_v_opt(args, RUN_GIT_CMD); + res = run_git_commit(defmsg); free(defmsg); - return res; } From 3b2c5b6df4be14cac6b36cf0db0468efa1f42916 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 14 Jul 2010 01:28:14 +0200 Subject: [PATCH 3/5] revert: don't print "Finished one cherry-pick." if commit failed Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/revert.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index b84b5b8645..ec931bdcfc 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -518,24 +518,17 @@ static int do_pick_commit(void) mebuf.buf, help_msg()); rerere(allow_rerere_auto); } else { - fprintf(stderr, "Finished one %s.\n", mebuf.buf); + if (!no_commit) + res = run_git_commit(defmsg); + if (!res) + fprintf(stderr, "Finished one %s.\n", mebuf.buf); } strbuf_release(&mebuf); free_message(&msg); - - if (res) - return 1; - - if (!no_commit) { - res = run_git_commit(defmsg); - free(defmsg); - return res; - } - free(defmsg); - return 0; + return res; } static void prepare_revs(struct rev_info *revs) From f29b5e06b3bdbe3c66a01db8ee67e3c87ae55705 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 14 Jul 2010 01:28:15 +0200 Subject: [PATCH 4/5] revert: improve success message by adding abbreviated commit sha1 Instead of saying "Finished one cherry-pick." or "Finished one revert.", we now say "Finished cherry-pick of commit ." or "Finished revert of commit ." which is more informative, especially when cherry-picking or reverting many commits. In case of failure the message is now "Automatic cherry-pick of commit failed." instead of "Automatic cherry-pick failed." Signed-off-by: Christian Couder Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 5 +++-- t/t3508-cherry-pick-many-commits.sh | 16 ++++++++-------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index ec931bdcfc..e261fb2391 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -492,7 +492,8 @@ static int do_pick_commit(void) } } - strbuf_addstr(&mebuf, me); + strbuf_addf(&mebuf, "%s of commit %s", me, + find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV)); if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) { res = do_recursive_merge(base, next, base_label, next_label, @@ -521,7 +522,7 @@ static int do_pick_commit(void) if (!no_commit) res = run_git_commit(defmsg); if (!res) - fprintf(stderr, "Finished one %s.\n", mebuf.buf); + fprintf(stderr, "Finished %s.\n", mebuf.buf); } strbuf_release(&mebuf); diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh index d90b365fd2..6044173007 100755 --- a/t/t3508-cherry-pick-many-commits.sh +++ b/t/t3508-cherry-pick-many-commits.sh @@ -23,10 +23,10 @@ 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. + cat <<-EOF >expected && + Finished cherry-pick of commit $(git rev-parse --short second). + Finished cherry-pick of commit $(git rev-parse --short third). + Finished cherry-pick of commit $(git rev-parse --short fourth). EOF git checkout -f master && @@ -40,10 +40,10 @@ test_expect_success 'cherry-pick first..fourth works' ' ' 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. + cat <<-EOF >expected && + Finished cherry-pick of commit $(git rev-parse --short second) with strategy resolve. + Finished cherry-pick of commit $(git rev-parse --short third) with strategy resolve. + Finished cherry-pick of commit $(git rev-parse --short fourth) with strategy resolve. EOF git checkout -f master && From 6bc83cdd0b9c7eab365e10d46d1e2acdb769728a Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 21 Jul 2010 06:25:02 +0200 Subject: [PATCH 5/5] t3508: add check_head_differs_from() helper function and use it In a test like: test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify fourth)" the --verify does not accomplish much, since the exit status of git rev-parse is not propagated to test. So it is more robust to define and use the helper functions check_head_differs_from() and check_head_equals() as done by this patch. Suggested-by: Jonathan Nieder Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- t/t3508-cherry-pick-many-commits.sh | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh index 6044173007..0f61495b2c 100755 --- a/t/t3508-cherry-pick-many-commits.sh +++ b/t/t3508-cherry-pick-many-commits.sh @@ -4,6 +4,18 @@ test_description='test cherry-picking many commits' . ./test-lib.sh +check_head_differs_from() { + head=$(git rev-parse --verify HEAD) && + arg=$(git rev-parse --verify "$1") && + test "$head" != "$arg" +} + +check_head_equals() { + head=$(git rev-parse --verify HEAD) && + arg=$(git rev-parse --verify "$1") && + test "$head" = "$arg" +} + test_expect_success setup ' echo first > file1 && git add file1 && @@ -36,7 +48,7 @@ test_expect_success 'cherry-pick first..fourth works' ' git diff --quiet other && git diff --quiet HEAD other && test_cmp expected actual && - test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify fourth)" + check_head_differs_from fourth ' test_expect_success 'cherry-pick --strategy resolve first..fourth works' ' @@ -53,7 +65,7 @@ test_expect_success 'cherry-pick --strategy resolve first..fourth works' ' git diff --quiet other && git diff --quiet HEAD other && test_cmp expected actual && - test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify fourth)" + check_head_differs_from fourth ' test_expect_success 'cherry-pick --ff first..fourth works' ' @@ -63,7 +75,7 @@ test_expect_success 'cherry-pick --ff first..fourth works' ' git cherry-pick --ff first..fourth && git diff --quiet other && git diff --quiet HEAD other && - test "$(git rev-parse --verify HEAD)" = "$(git rev-parse --verify fourth)" + check_head_equals fourth ' test_expect_success 'cherry-pick -n first..fourth works' ' @@ -113,7 +125,7 @@ test_expect_success 'cherry-pick -3 fourth works' ' git cherry-pick -3 fourth && git diff --quiet other && git diff --quiet HEAD other && - test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify fourth)" + check_head_differs_from fourth ' test_expect_success 'cherry-pick --stdin works' ' @@ -123,7 +135,7 @@ test_expect_success 'cherry-pick --stdin works' ' git rev-list --reverse first..fourth | git cherry-pick --stdin && git diff --quiet other && git diff --quiet HEAD other && - test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify fourth)" + check_head_differs_from fourth ' test_done