{"thread":{"id":"24402","subject":"[PATCH 1/4] revert: report success when using option --strategy","startedAt":"2010-07-13T23:28:12Z","lastAt":"2010-07-15T22:37:06Z","messageCount":2,"participants":["Christian Couder","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"145611","messageId":"20100713232816.7738.17380.chriscool@tuxfamily.org","threadId":"24402","inReplyTo":null,"subject":"[PATCH 1/4] revert: report success when using option --strategy","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-07-13T23:28:12Z","receivedAt":"2010-07-13T23:28:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"\"git cherry-pick foo\" has always reported success with\n\"Finished one cherry-pick\" but \"cherry-pick --strategy\"\ndoes not print anything. So move the code to write that\nmessage from do_recursive_merge() to do_cherry_pick()\nso other strategies can share it.\n\nThis patch also refactors the code that prints a message\nlike \"Automatic cherry-pick failed. <help message>\". This\ncode was duplicated in both do_recursive_merge() and\ndo_pick_commit().\n\nTo do that, now do_recursive_merge() returns an int to signal\nsuccess or failure. And in case of failure we just return 1\nfrom do_pick_commit() instead of doing \"exit(1)\" from either\ndo_recursive_merge() or do_pick_commit().\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/revert.c                    |   51 ++++++++++++++++++++---------------\n t/t3508-cherry-pick-many-commits.sh |   26 +++++++++++++++++-\n 2 files changed, 54 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 8b9d829..b082bb4 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -311,10 +311,9 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n \treturn write_ref_sha1(ref_lock, to, \"cherry-pick\");\n }\n \n-static void do_recursive_merge(struct commit *base, struct commit *next,\n-\t\t\t       const char *base_label, const char *next_label,\n-\t\t\t       unsigned char *head, struct strbuf *msgbuf,\n-\t\t\t       char *defmsg)\n+static int do_recursive_merge(struct commit *base, struct commit *next,\n+\t\t\t      const char *base_label, const char *next_label,\n+\t\t\t      unsigned char *head, struct strbuf *msgbuf)\n {\n \tstruct merge_options o;\n \tstruct tree *result, *next_tree, *base_tree, *head_tree;\n@@ -357,14 +356,9 @@ static void do_recursive_merge(struct commit *base, struct commit *next,\n \t\t\t\t\ti++;\n \t\t\t}\n \t\t}\n-\t\twrite_message(msgbuf, defmsg);\n-\t\tfprintf(stderr, \"Automatic %s failed.%s\\n\",\n-\t\t\tme, help_msg());\n-\t\trerere(allow_rerere_auto);\n-\t\texit(1);\n \t}\n-\twrite_message(msgbuf, defmsg);\n-\tfprintf(stderr, \"Finished one %s.\\n\", me);\n+\n+\treturn !clean;\n }\n \n static int do_pick_commit(void)\n@@ -375,6 +369,8 @@ static int do_pick_commit(void)\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n+\tstruct strbuf mebuf = STRBUF_INIT;\n+\tint res;\n \n \tif (no_commit) {\n \t\t/*\n@@ -470,30 +466,41 @@ static int do_pick_commit(void)\n \t\t}\n \t}\n \n-\tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT)\n-\t\tdo_recursive_merge(base, next, base_label, next_label,\n-\t\t\t\t   head, &msgbuf, defmsg);\n-\telse {\n-\t\tint res;\n+\tstrbuf_addstr(&mebuf, me);\n+\n+\tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n+\t\tres = do_recursive_merge(base, next, base_label, next_label,\n+\t\t\t\t\t head, &msgbuf);\n+\t\twrite_message(&msgbuf, defmsg);\n+\t} else {\n \t\tstruct commit_list *common = NULL;\n \t\tstruct commit_list *remotes = NULL;\n+\n+\t\tstrbuf_addf(&mebuf, \" with strategy %s\", strategy);\n \t\twrite_message(&msgbuf, defmsg);\n+\n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n \t\tres = try_merge_command(strategy, common,\n \t\t\t\t\tsha1_to_hex(head), remotes);\n \t\tfree_commit_list(common);\n \t\tfree_commit_list(remotes);\n-\t\tif (res) {\n-\t\t\tfprintf(stderr, \"Automatic %s with strategy %s failed.%s\\n\",\n-\t\t\t\tme, strategy, help_msg());\n-\t\t\trerere(allow_rerere_auto);\n-\t\t\texit(1);\n-\t\t}\n \t}\n \n+\tif (res) {\n+\t\tfprintf(stderr, \"Automatic %s failed.%s\\n\",\n+\t\t\tmebuf.buf, help_msg());\n+\t\trerere(allow_rerere_auto);\n+\t} else {\n+\t\tfprintf(stderr, \"Finished one %s.\\n\", mebuf.buf);\n+\t}\n+\n+\tstrbuf_release(&mebuf);\n \tfree_message(&msg);\n \n+\tif (res)\n+\t\treturn 1;\n+\n \t/*\n \t *\n \t * If we are cherry-pick, and if the merge did not result in\ndiff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\nindex f90ed3d..6555f92 100755\n--- a/t/t3508-cherry-pick-many-commits.sh\n+++ b/t/t3508-cherry-pick-many-commits.sh\n@@ -23,12 +23,36 @@ test_expect_success setup '\n '\n \n test_expect_success 'cherry-pick first..fourth works' '\n+\tcat <<-EOF > expected &&\n+\tFinished one cherry-pick.\n+\tFinished one cherry-pick.\n+\tFinished one cherry-pick.\n+\tEOF\n+\n+\tgit checkout -f master &&\n+\tgit reset --hard first &&\n+\ttest_tick &&\n+\tgit cherry-pick first..fourth 2>actual &&\n+\tgit diff --quiet other &&\n+\tgit diff --quiet HEAD other &&\n+\ttest_cmp expected actual &&\n+\ttest \"$(git rev-parse --verify HEAD)\" != \"$(git rev-parse --verify fourth)\"\n+'\n+\n+test_expect_success 'cherry-pick --strategy resolve first..fourth works' '\n+\tcat <<-EOF > expected &&\n+\tFinished one cherry-pick with strategy resolve.\n+\tFinished one cherry-pick with strategy resolve.\n+\tFinished one cherry-pick with strategy resolve.\n+\tEOF\n+\n \tgit checkout -f master &&\n \tgit reset --hard first &&\n \ttest_tick &&\n-\tgit cherry-pick first..fourth &&\n+\tgit cherry-pick --strategy resolve first..fourth 2>actual &&\n \tgit diff --quiet other &&\n \tgit diff --quiet HEAD other &&\n+\ttest_cmp expected actual &&\n \ttest \"$(git rev-parse --verify HEAD)\" != \"$(git rev-parse --verify fourth)\"\n '\n \n-- \n1.7.2.rc1.215.g2dff2.dirty\n"},{"id":"145666","messageId":"20100715223706.GA4788@burratino","threadId":"24402","inReplyTo":"20100713232816.7738.17380.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/4] revert: report success when using option --strategy","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-15T22:37:06Z","receivedAt":"2010-07-15T22:37:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Christian Couder wrote:\n\n> \"git cherry-pick foo\" has always reported success with\n> \"Finished one cherry-pick\" but \"cherry-pick --strategy\"\n> does not print anything.\n[...]\n> This patch also refactors the code that prints a message\n> like \"Automatic cherry-pick failed. <help message>\". This\n> code was duplicated in both do_recursive_merge() and\n> do_pick_commit().\n\nThis is good.  Nitpicks below.\n\n> +++ b/builtin/revert.c\n> diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\n> index f90ed3d..6555f92 100755\n> --- a/t/t3508-cherry-pick-many-commits.sh\n> +++ b/t/t3508-cherry-pick-many-commits.sh\n> @@ -23,12 +23,36 @@ test_expect_success setup '\n>  '\n>  \n>  test_expect_success 'cherry-pick first..fourth works' '\n> +\tcat <<-EOF > expected &&\n\nStyle:\n\tcat <<-\\EOF >expected &&\n\nThe \\quoting lets reviewers skim through a message without looking\nfor characters that might be expanded.  The decreased whitespace is\njust for consistency.\n\n> +\tFinished one cherry-pick.\n> +\tFinished one cherry-pick.\n> +\tFinished one cherry-pick.\n> +\tEOF\n> +\n> +\tgit checkout -f master &&\n> +\tgit reset --hard first &&\n> +\ttest_tick &&\n> +\tgit cherry-pick first..fourth 2>actual &&\n> +\tgit diff --quiet other &&\n> +\tgit diff --quiet HEAD other &&\n> +\ttest_cmp expected actual &&\n> +\ttest \"$(git rev-parse --verify HEAD)\" != \"$(git rev-parse --verify fourth)\"\n\nThe --verify here does not accomplish much, since the exit status of\ngit rev-parse is not propagated to test.  So strictly speaking it\nwould be more robust to do something like\n\n\thead=$(git rev-parse --verify HEAD) &&\n\tfourth=$(git rev-parse --verify fourth) &&\n\ttest \"$head\" = \"$fourth\"\n\nthough I do not think it is worth changing.\n\nWith or without the following changes,\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n---\ndiff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\nindex 6555f92..d90b365 100755\n--- a/t/t3508-cherry-pick-many-commits.sh\n+++ b/t/t3508-cherry-pick-many-commits.sh\n@@ -23,7 +23,7 @@ test_expect_success setup '\n '\n \n test_expect_success 'cherry-pick first..fourth works' '\n-\tcat <<-EOF > expected &&\n+\tcat <<-\\EOF >expected &&\n \tFinished one cherry-pick.\n \tFinished one cherry-pick.\n \tFinished one cherry-pick.\n@@ -40,7 +40,7 @@ test_expect_success 'cherry-pick first..fourth works' '\n '\n \n test_expect_success 'cherry-pick --strategy resolve first..fourth works' '\n-\tcat <<-EOF > expected &&\n+\tcat <<-\\EOF >expected &&\n \tFinished one cherry-pick with strategy resolve.\n \tFinished one cherry-pick with strategy resolve.\n \tFinished one cherry-pick with strategy resolve.\n-- \n"}]}