{"thread":{"id":"56187","subject":"[PATCH 0/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","startedAt":"2021-07-31T07:01:26Z","lastAt":"2021-08-22T13:09:18Z","messageCount":32,"participants":["ZheNing Hu via GitGitGadget","Phillip Wood","ZheNing Hu","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"431618","messageId":"pull.1010.git.1627714877.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":null,"subject":"[PATCH 0/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-31T07:01:15Z","receivedAt":"2021-07-31T07:01:26Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"This patch fixes the bug when git cherry-pick is used with environment\nvariable GIT_CHERRY_PICK_HELP, and makes git chery-pick advice message\nbetter.\n\nv3:\nhttps://lore.kernel.org/git/pull.1007.git.1627561665.gitgitgadget@gmail.com/\n\nv3-->v4:\n\n 1. Add hidden option --delete-cherry-pick-head to git cherry-pick wihch\n used to delete CHERRY_PICK_HEAD when conflict occurs.\n 2. add delete_cherry_pick_head flag to struct replay_opts and struct\n rebase_options.\n 3. Split print_advice() into print advice and delete CHERRY_PICK_HEAD two\n part.\n 4. Use better git cherry-pick advice message.\n\nZheNing Hu (2):\n [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP\n [GSOC] cherry-pick: use better advice message\n\n builtin/rebase.c | 3 +++\n builtin/revert.c | 2 ++\n git-rebase--preserve-merges.sh | 2 +-\n sequencer.c | 37 +++++++++++++++++-------------\n sequencer.h | 1 +\n t/t3507-cherry-pick-conflict.sh | 40 ++++++++++++++++++++-------------\n 6 files changed, 53 insertions(+), 32 deletions(-)\n\n\nbase-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1010\n-- \ngitgitgadget\n"},{"id":"431619","messageId":"7e1ed49728df8dab771d77f1a076f0fa30975718.1627714878.git.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":"pull.1010.git.1627714877.gitgitgadget@gmail.com","subject":"[PATCH 2/2] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-31T07:01:17Z","receivedAt":"2021-07-31T07:01:28Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nIn the past, git cherry-pick would print such advice when\nthere was a conflict:\n\nhint: after resolving the conflicts, mark the corrected paths\nhint: with 'git add ' or 'git rm '\nhint: and commit the result with 'git commit'\n\nBut in fact, when we want to cherry-pick multiple commits,\nwe should not use \"git commit\" after resolving conflicts, which\nwill make Git generate some errors. We should recommend users to\nuse `git cherry-pick --continue`, `git cherry-pick --abort`, just\nlike git rebase does.\n\nThis is the improved advice:\n\nhint: Resolve all conflicts manually, mark them as resolved with\nhint: \"git add/rm \", then run \"git cherry-pick \\\n--continue\".\nhint: You can instead skip this commit: run \"git cherry-pick --skip\".\nhint: To abort and get back to the state before \"git cherry-pick\",\nhint: run \"git cherry-pick --abort\".\n\nMentored-by: Christian Couder \nMentored-by Hariom Verma :\nHelped-by: Phillip Wood \nHepled-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n sequencer.c | 9 ++++++++-\n t/t3507-cherry-pick-conflict.sh | 15 ++++++++++-----\n 2 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 83cf6a5da3c..f6e9d1fddd8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -404,7 +404,14 @@ static void print_advice(struct replay_opts *opts, int show_hint)\n \tif (msg) {\n \t\tadvise(\"%s\\n\", msg);\n \t} else if (show_hint) {\n-\t\tif (opts->no_commit)\n+\t\tif (opts->action == REPLAY_PICK) {\n+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run \\\"git cherry-pick --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n+\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n+\n+\t\t} else if (opts->no_commit)\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n \t\telse\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex f17621d1915..2cc3977f5a6 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -53,9 +53,11 @@ test_expect_success 'advice from failed cherry-pick' \"\n \tpicked=\\$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n \terror: could not apply \\$picked... picked\n-\thint: after resolving the conflicts, mark the corrected paths\n-\thint: with 'git add ' or 'git rm '\n-\thint: and commit the result with 'git commit'\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \\\"git add/rm \\\", then run \\\"git cherry-pick --continue\\\".\n+\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n+\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n+\thint: run \\\"git cherry-pick --abort\\\".\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n \n@@ -68,8 +70,11 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \tpicked=\\$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n \terror: could not apply \\$picked... picked\n-\thint: after resolving the conflicts, mark the corrected paths\n-\thint: with 'git add ' or 'git rm '\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \\\"git add/rm \\\", then run \\\"git cherry-pick --continue\\\".\n+\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n+\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n+\thint: run \\\"git cherry-pick --abort\\\".\n \tEOF\n \ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n \n-- \ngitgitgadget\n"},{"id":"431620","messageId":"0d0a55bd9c4094450749fa20a30d0d11203768d6.1627714878.git.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":"pull.1010.git.1627714877.gitgitgadget@gmail.com","subject":"[PATCH 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-31T07:01:16Z","receivedAt":"2021-07-31T07:01:30Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nGIT_CHERRY_PICK_HELP is an environment variable, as the\nimplementation detail of some porcelain in git to help realize\nthe rebasing steps. E.g. `git rebase -p` set GIT_CHERRY_PICK_HELP\nvalue in `git-rebase--preserve-merges.sh`, `git rebase --merge` set\nGIT_CHERRY_PICK_HELP value in run_specific_rebase(). But If we set\nthe value of GIT_CHERRY_PICK_HELP when using `git cherry-pick`,\nCHERRY_PICK_HEAD will be deleted, then we will get an error when we\ntry to use `git cherry-pick --continue` or other cherr-pick command.\n\nIntroduce new \"hidden\" option `--delete-cherry-pick-head` for git\ncherry-pick which indicates that CHERRY_PICK_HEAD will be deleted when\nconflict occurs, which provided for some porcelain commands of git like\n`git-rebase--preserve-merges.sh`. After `git rebase -p` completely\nabolished, this option should be removed. At the same time, add the flag\n`delete_cherry_pick_head` to `struct rebase_options` and\n`struct replay_opts`, We can decide whether to delete CHERRY_PICK_HEAD\nby setting, passing, and checking this flag bit.\n\nThen we split print_advice() into two part: Firstly, print_advice()\nwill only be responsible for outputting content; Secondly, check if\nwe set the `delete_cherry_pick_head` flag; if set, delete CHERRY_PICK_HEAD.\nIn this way, the steps of printing advice and deleting CHERRY_PICK_HEAD\nare decoupled. Finally, let `git-rebase--preserve-merges.sh` use the\n`--delete-cherry-pick-head` option when it executes git cherry-pick, and\nset the `delete_cherry_pick_head` flag in run_specific_rebase() when we\nare using `git rebase --merge`, which can fix this breakage.\n\nMentored-by: Christian Couder \nMentored-by Hariom Verma :\nHelped-by: Phillip Wood \nHepled-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n builtin/rebase.c | 3 +++\n builtin/revert.c | 2 ++\n git-rebase--preserve-merges.sh | 2 +-\n sequencer.c | 28 +++++++++++++---------------\n sequencer.h | 1 +\n t/t3507-cherry-pick-conflict.sh | 25 +++++++++++++++----------\n 6 files changed, 35 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 12f093121d9..5983f37d531 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -84,6 +84,7 @@ struct rebase_options {\n \t\tREBASE_FORCE = 1<<3,\n \t\tREBASE_INTERACTIVE_EXPLICIT = 1<<4,\n \t} flags;\n+\tint delete_cherry_pick_head;\n \tstruct strvec git_am_opts;\n \tconst char *action;\n \tint signoff;\n@@ -152,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \t\toidcpy(&replay.squash_onto, opts->squash_onto);\n \t\treplay.have_squash_onto = 1;\n \t}\n+\treplay.delete_cherry_pick_head = opts->delete_cherry_pick_head;\n \n \treturn replay;\n }\n@@ -948,6 +950,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)\n \tif (opts->type == REBASE_MERGE) {\n \t\t/* Run sequencer-based rebase */\n \t\tsetenv(\"GIT_CHERRY_PICK_HELP\", resolvemsg, 1);\n+\t\topts->delete_cherry_pick_head = 1;\n \t\tif (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {\n \t\t\tsetenv(\"GIT_SEQUENCE_EDITOR\", \":\", 1);\n \t\t\topts->autosquash = 0;\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 237f2f18d4c..15a4b6fe4ee 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -127,6 +127,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tOPT_BOOL(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n \t\t\tOPT_BOOL(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n \t\t\tOPT_BOOL(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n+\t\t\tOPT_BOOL_F(0, \"delete-cherry-pick-head\", &opts->delete_cherry_pick_head,\n+\t\t\t\t N_(\"delete CHERRY_PICK_HEAD when conflict occurs\"), PARSE_OPT_HIDDEN),\n \t\t\tOPT_END(),\n \t\t};\n \t\toptions = parse_options_concat(options, cp_extra);\ndiff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh\nindex b9c71d2a71b..eaa8f9de2c5 100644\n--- a/git-rebase--preserve-merges.sh\n+++ b/git-rebase--preserve-merges.sh\n@@ -444,7 +444,7 @@ pick_one_preserving_merges () {\n \t\t\toutput eval git cherry-pick $allow_rerere_autoupdate \\\n \t\t\t\t$allow_empty_message \\\n \t\t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n-\t\t\t\t\"$strategy_args\" \"$@\" ||\n+\t\t\t\t\"$strategy_args\" --delete-cherry-pick-head \"$@\" ||\n \t\t\t\tdie_with_patch $sha1 \"$(eval_gettext \"Could not pick \\$sha1\")\"\n \t\t\t;;\n \t\tesac\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..83cf6a5da3c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -397,24 +397,13 @@ static void free_message(struct commit *commit, struct commit_message *msg)\n \tunuse_commit_buffer(commit, msg->message);\n }\n \n-static void print_advice(struct repository *r, int show_hint,\n-\t\t\t struct replay_opts *opts)\n+static void print_advice(struct replay_opts *opts, int show_hint)\n {\n \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n \n \tif (msg) {\n-\t\tfprintf(stderr, \"%s\\n\", msg);\n-\t\t/*\n-\t\t * A conflict has occurred but the porcelain\n-\t\t * (typically rebase --interactive) wants to take care\n-\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n-\t\t */\n-\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n-\t\t\t\tNULL, 0);\n-\t\treturn;\n-\t}\n-\n-\tif (show_hint) {\n+\t\tadvise(\"%s\\n\", msg);\n+\t} else if (show_hint) {\n \t\tif (opts->no_commit)\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n@@ -2265,7 +2254,16 @@ static int do_pick_commit(struct repository *r,\n \t\t ? _(\"could not revert %s... %s\")\n \t\t : _(\"could not apply %s... %s\"),\n \t\t short_commit_name(commit), msg.subject);\n-\t\tprint_advice(r, res == 1, opts);\n+\t\tprint_advice(opts, res == 1);\n+\t\tif (opts->delete_cherry_pick_head) {\n+\t\t\t/*\n+\t\t\t * A conflict has occurred but the porcelain\n+\t\t\t * (typically rebase --interactive) wants to take care\n+\t\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n+\t\t\t */\n+\t\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n+\t\t\t\t\tNULL, 0);\n+\t\t}\n \t\trepo_rerere(r, opts->allow_rerere_auto);\n \t\tgoto leave;\n \t}\ndiff --git a/sequencer.h b/sequencer.h\nindex d57d8ea23d7..76fb4af56fd 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -49,6 +49,7 @@ struct replay_opts {\n \tint reschedule_failed_exec;\n \tint committer_date_is_author_date;\n \tint ignore_date;\n+\tint delete_cherry_pick_head;\n \n \tint mainline;\n \ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 014001b8f32..f17621d1915 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -76,6 +76,21 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \ttest_cmp expected actual\n \"\n \n+test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' \"\n+\tpristine_detach initial &&\n+\t(\n+\t\tpicked=\\$(git rev-parse --short picked) &&\n+\t\tcat <<-EOF >expected &&\n+\t\terror: could not apply \\$picked... picked\n+\t\thint: and then do something else\n+\t\tEOF\n+\t\tGIT_CHERRY_PICK_HELP='and then do something else' &&\n+\t\texport GIT_CHERRY_PICK_HELP &&\n+\t\ttest_must_fail git cherry-pick picked 2>actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+\"\n+\n test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick picked &&\n@@ -109,16 +124,6 @@ test_expect_success \\\n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n \n-test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n-\tpristine_detach initial &&\n-\t(\n-\t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n-\t\texport GIT_CHERRY_PICK_HELP &&\n-\t\ttest_must_fail git cherry-pick picked\n-\t) &&\n-\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n-'\n-\n test_expect_success 'git reset clears CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \n-- \ngitgitgadget\n\n"},{"id":"431651","messageId":"273d1864-422e-f31f-13ef-20cfe4871947@gmail.com","threadId":"56187","inReplyTo":"0d0a55bd9c4094450749fa20a30d0d11203768d6.1627714878.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-01T10:09:59Z","receivedAt":"2021-08-01T10:10:05Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 31/07/2021 08:01, ZheNing Hu via GitGitGadget wrote:\n> From: ZheNing Hu \n> \n> GIT_CHERRY_PICK_HELP is an environment variable, as the\n> implementation detail of some porcelain in git to help realize\n> the rebasing steps. E.g. `git rebase -p` set GIT_CHERRY_PICK_HELP\n> value in `git-rebase--preserve-merges.sh`, `git rebase --merge` set\n> GIT_CHERRY_PICK_HELP value in run_specific_rebase(). But If we set\n> the value of GIT_CHERRY_PICK_HELP when using `git cherry-pick`,\n> CHERRY_PICK_HEAD will be deleted, then we will get an error when we\n> try to use `git cherry-pick --continue` or other cherr-pick command.\n> \n> Introduce new \"hidden\" option `--delete-cherry-pick-head` for git\n> cherry-pick which indicates that CHERRY_PICK_HEAD will be deleted when\n> conflict occurs, which provided for some porcelain commands of git like\n> `git-rebase--preserve-merges.sh`. After `git rebase -p` completely\n> abolished, this option should be removed. At the same time, add the flag\n> `delete_cherry_pick_head` to `struct rebase_options` and\n> `struct replay_opts`, We can decide whether to delete CHERRY_PICK_HEAD\n> by setting, passing, and checking this flag bit.\n> \n> Then we split print_advice() into two part: Firstly, print_advice()\n> will only be responsible for outputting content; Secondly, check if\n> we set the `delete_cherry_pick_head` flag; if set, delete CHERRY_PICK_HEAD.\n> In this way, the steps of printing advice and deleting CHERRY_PICK_HEAD\n> are decoupled. Finally, let `git-rebase--preserve-merges.sh` use the\n> `--delete-cherry-pick-head` option when it executes git cherry-pick, and\n> set the `delete_cherry_pick_head` flag in run_specific_rebase() when we\n> are using `git rebase --merge`, which can fix this breakage.\n> \n> Mentored-by: Christian Couder \n> Mentored-by Hariom Verma :\n> Helped-by: Phillip Wood \n> Hepled-by: Junio C Hamano \n> Signed-off-by: ZheNing Hu \n> ---\n> builtin/rebase.c | 3 +++\n> builtin/revert.c | 2 ++\n> git-rebase--preserve-merges.sh | 2 +-\n> sequencer.c | 28 +++++++++++++---------------\n> sequencer.h | 1 +\n> t/t3507-cherry-pick-conflict.sh | 25 +++++++++++++++----------\n> 6 files changed, 35 insertions(+), 26 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 12f093121d9..5983f37d531 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -84,6 +84,7 @@ struct rebase_options {\n> \t\tREBASE_FORCE = 1<<3,\n> \t\tREBASE_INTERACTIVE_EXPLICIT = 1<<4,\n> \t} flags;\n> +\tint delete_cherry_pick_head;\n> \tstruct strvec git_am_opts;\n> \tconst char *action;\n> \tint signoff;\n> @@ -152,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n> \t\toidcpy(&replay.squash_onto, opts->squash_onto);\n> \t\treplay.have_squash_onto = 1;\n> \t}\n> +\treplay.delete_cherry_pick_head = opts->delete_cherry_pick_head;\n\nI think we could just have\n\treplay.delete_cherry_pick_head = 1;\nand not add a new member to rebase_options as we always want this set\n> \n> \treturn replay;\n> }\n> @@ -948,6 +950,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)\n> \tif (opts->type == REBASE_MERGE) {\n> \t\t/* Run sequencer-based rebase */\n> \t\tsetenv(\"GIT_CHERRY_PICK_HELP\", resolvemsg, 1);\n> +\t\topts->delete_cherry_pick_head = 1;\n> \t\tif (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {\n> \t\t\tsetenv(\"GIT_SEQUENCE_EDITOR\", \":\", 1);\n> \t\t\topts->autosquash = 0;\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 237f2f18d4c..15a4b6fe4ee 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -127,6 +127,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n> \t\t\tOPT_BOOL(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n> \t\t\tOPT_BOOL(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n> \t\t\tOPT_BOOL(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n> +\t\t\tOPT_BOOL_F(0, \"delete-cherry-pick-head\", &opts->delete_cherry_pick_head,\n> +\t\t\t\t N_(\"delete CHERRY_PICK_HEAD when conflict occurs\"), PARSE_OPT_HIDDEN),\n> \t\t\tOPT_END(),\n> \t\t};\n> \t\toptions = parse_options_concat(options, cp_extra);\n> diff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh\n> index b9c71d2a71b..eaa8f9de2c5 100644\n> --- a/git-rebase--preserve-merges.sh\n> +++ b/git-rebase--preserve-merges.sh\n> @@ -444,7 +444,7 @@ pick_one_preserving_merges () {\n> \t\t\toutput eval git cherry-pick $allow_rerere_autoupdate \\\n> \t\t\t\t$allow_empty_message \\\n> \t\t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n> -\t\t\t\t\"$strategy_args\" \"$@\" ||\n> +\t\t\t\t\"$strategy_args\" --delete-cherry-pick-head \"$@\" ||\n> \t\t\t\tdie_with_patch $sha1 \"$(eval_gettext \"Could not pick \\$sha1\")\"\n> \t\t\t;;\n> \t\tesac\n> diff --git a/sequencer.c b/sequencer.c\n> index 0bec01cf38e..83cf6a5da3c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -397,24 +397,13 @@ static void free_message(struct commit *commit, struct commit_message *msg)\n> \tunuse_commit_buffer(commit, msg->message);\n> }\n> \n> -static void print_advice(struct repository *r, int show_hint,\n> -\t\t\t struct replay_opts *opts)\n> +static void print_advice(struct replay_opts *opts, int show_hint)\n> {\n> \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n> \n> \tif (msg) {\n> -\t\tfprintf(stderr, \"%s\\n\", msg);\n> -\t\t/*\n> -\t\t * A conflict has occurred but the porcelain\n> -\t\t * (typically rebase --interactive) wants to take care\n> -\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n> -\t\t */\n> -\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n> -\t\t\t\tNULL, 0);\n> -\t\treturn;\n> -\t}\n> -\n> -\tif (show_hint) {\n> +\t\tadvise(\"%s\\n\", msg);\n\nThis is a change in behavior as the advice will now be prefixed with \n\"hint: \". I think that is probably an improvement so long as it does not \nmake the lines too long when the advice is printed.\n\n> +\t} else if (show_hint) {\n> \t\tif (opts->no_commit)\n> \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> \t\t\t\t \"with 'git add ' or 'git rm '\"));\n> @@ -2265,7 +2254,16 @@ static int do_pick_commit(struct repository *r,\n> \t\t ? _(\"could not revert %s... %s\")\n> \t\t : _(\"could not apply %s... %s\"),\n> \t\t short_commit_name(commit), msg.subject);\n> -\t\tprint_advice(r, res == 1, opts);\n> +\t\tprint_advice(opts, res == 1);\n> +\t\tif (opts->delete_cherry_pick_head) {\n> +\t\t\t/*\n> +\t\t\t * A conflict has occurred but the porcelain\n> +\t\t\t * (typically rebase --interactive) wants to take care\n> +\t\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n> +\t\t\t */\n> +\t\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n> +\t\t\t\t\tNULL, 0);\n> +\t\t}\n> \t\trepo_rerere(r, opts->allow_rerere_auto);\n> \t\tgoto leave;\n> \t}\n> diff --git a/sequencer.h b/sequencer.h\n> index d57d8ea23d7..76fb4af56fd 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -49,6 +49,7 @@ struct replay_opts {\n> \tint reschedule_failed_exec;\n> \tint committer_date_is_author_date;\n> \tint ignore_date;\n> +\tint delete_cherry_pick_head;\n> \n> \tint mainline;\n> \n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 014001b8f32..f17621d1915 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -76,6 +76,21 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n> \ttest_cmp expected actual\n> \"\n> \n> +test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' \"\n> +\tpristine_detach initial &&\n> +\t(\n> +\t\tpicked=\\$(git rev-parse --short picked) &&\n> +\t\tcat <<-EOF >expected &&\n> +\t\terror: could not apply \\$picked... picked\n> +\t\thint: and then do something else\n> +\t\tEOF\n> +\t\tGIT_CHERRY_PICK_HELP='and then do something else' &&\n> +\t\texport GIT_CHERRY_PICK_HELP &&\n> +\t\ttest_must_fail git cherry-pick picked 2>actual &&\n> +\t\ttest_cmp expected actual\n> +\t)\n> +\"\n> +\n> test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n> \tpristine_detach initial &&\n> \ttest_must_fail git cherry-pick picked &&\n> @@ -109,16 +124,6 @@ test_expect_success \\\n> \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> '\n> \n> -test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n> -\tpristine_detach initial &&\n> -\t(\n> -\t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n> -\t\texport GIT_CHERRY_PICK_HELP &&\n> -\t\ttest_must_fail git cherry-pick picked\n> -\t) &&\n> -\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> -'\n> -\n> test_expect_success 'git reset clears CHERRY_PICK_HEAD' '\n> \tpristine_detach initial &&\n\nI think it would be useful to add a test for the new cherry-pick option \nthat is added in this patch. Overall this patch is looking pretty good \nit just needs a few small changes, thanks for working on it.\n\nBest Wishes\n\nPhillip\n"},{"id":"431652","messageId":"0a2ab793-de4d-d8d1-4d1d-a018fa0f252c@gmail.com","threadId":"56187","inReplyTo":"7e1ed49728df8dab771d77f1a076f0fa30975718.1627714878.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] [GSOC] cherry-pick: use better advice message","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-01T10:14:32Z","receivedAt":"2021-08-01T10:14:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 31/07/2021 08:01, ZheNing Hu via GitGitGadget wrote:\n> From: ZheNing Hu \n> \n> In the past, git cherry-pick would print such advice when\n> there was a conflict:\n> \n> hint: after resolving the conflicts, mark the corrected paths\n> hint: with 'git add ' or 'git rm '\n> hint: and commit the result with 'git commit'\n> \n> But in fact, when we want to cherry-pick multiple commits,\n> we should not use \"git commit\" after resolving conflicts, which\n> will make Git generate some errors. We should recommend users to\n> use `git cherry-pick --continue`, `git cherry-pick --abort`, just\n> like git rebase does.\n> \n> This is the improved advice:\n> \n> hint: Resolve all conflicts manually, mark them as resolved with\n> hint: \"git add/rm \", then run \"git cherry-pick \\\n> --continue\".\n> hint: You can instead skip this commit: run \"git cherry-pick --skip\".\n> hint: To abort and get back to the state before \"git cherry-pick\",\n> hint: run \"git cherry-pick --abort\".\n\nThis new wording matches what we have for rebase which is good, I am \nslightly worried that the lines end up being quite long though they are \njust under 80 characters. It might be worth splitting the line that \nmentions running \"git cherry-pick --continue\" so it is a bit shorter.\n\nBest Wishes\n\nPhillip\n\n> Mentored-by: Christian Couder \n> Mentored-by Hariom Verma :\n> Helped-by: Phillip Wood \n> Hepled-by: Junio C Hamano \n> Signed-off-by: ZheNing Hu \n> ---\n> sequencer.c | 9 ++++++++-\n> t/t3507-cherry-pick-conflict.sh | 15 ++++++++++-----\n> 2 files changed, 18 insertions(+), 6 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 83cf6a5da3c..f6e9d1fddd8 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -404,7 +404,14 @@ static void print_advice(struct replay_opts *opts, int show_hint)\n> \tif (msg) {\n> \t\tadvise(\"%s\\n\", msg);\n> \t} else if (show_hint) {\n> -\t\tif (opts->no_commit)\n> +\t\tif (opts->action == REPLAY_PICK) {\n> +\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n> +\t\t\t\t \"\\\"git add/rm \\\", then run \\\"git cherry-pick --continue\\\".\\n\"\n> +\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n> +\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n> +\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n> +\n> +\t\t} else if (opts->no_commit)\n> \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> \t\t\t\t \"with 'git add ' or 'git rm '\"));\n> \t\telse\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index f17621d1915..2cc3977f5a6 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -53,9 +53,11 @@ test_expect_success 'advice from failed cherry-pick' \"\n> \tpicked=\\$(git rev-parse --short picked) &&\n> \tcat <<-EOF >expected &&\n> \terror: could not apply \\$picked... picked\n> -\thint: after resolving the conflicts, mark the corrected paths\n> -\thint: with 'git add ' or 'git rm '\n> -\thint: and commit the result with 'git commit'\n> +\thint: Resolve all conflicts manually, mark them as resolved with\n> +\thint: \\\"git add/rm \\\", then run \\\"git cherry-pick --continue\\\".\n> +\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n> +\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n> +\thint: run \\\"git cherry-pick --abort\\\".\n> \tEOF\n> \ttest_must_fail git cherry-pick picked 2>actual &&\n> \n> @@ -68,8 +70,11 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n> \tpicked=\\$(git rev-parse --short picked) &&\n> \tcat <<-EOF >expected &&\n> \terror: could not apply \\$picked... picked\n> -\thint: after resolving the conflicts, mark the corrected paths\n> -\thint: with 'git add ' or 'git rm '\n> +\thint: Resolve all conflicts manually, mark them as resolved with\n> +\thint: \\\"git add/rm \\\", then run \\\"git cherry-pick --continue\\\".\n> +\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n> +\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n> +\thint: run \\\"git cherry-pick --abort\\\".\n> \tEOF\n> \ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n> \n> \n"},{"id":"431686","messageId":"CAOLTT8TberYK1DtwMaEjtZLeVr1JM0tbU1DXW=Us8D_Yrsg-mA@mail.gmail.com","threadId":"56187","inReplyTo":"0a2ab793-de4d-d8d1-4d1d-a018fa0f252c@gmail.com","subject":"Re: [PATCH 2/2] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-02T13:35:57Z","receivedAt":"2021-08-02T13:35:17Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Phillip Wood 于2021年8月1日周日 下午6:14写道：\n>\n\n> > This is the improved advice:\n> >\n> > hint: Resolve all conflicts manually, mark them as resolved with\n> > hint: \"git add/rm \", then run \"git cherry-pick \\\n> > --continue\".\n> > hint: You can instead skip this commit: run \"git cherry-pick --skip\".\n> > hint: To abort and get back to the state before \"git cherry-pick\",\n> > hint: run \"git cherry-pick --abort\".\n>\n> This new wording matches what we have for rebase which is good, I am\n> slightly worried that the lines end up being quite long though they are\n> just under 80 characters. It might be worth splitting the line that\n> mentions running \"git cherry-pick --continue\" so it is a bit shorter.\n>\n\nAgree.\n\n> Best Wishes\n>\n> Phillip\n>\nThanks.\n--\nZheNing Hu\n"},{"id":"431687","messageId":"CAOLTT8S=v41p_uZQ4u0LBnHkRp3pMpVK0Pt5mykzbfX9ReTC7g@mail.gmail.com","threadId":"56187","inReplyTo":"273d1864-422e-f31f-13ef-20cfe4871947@gmail.com","subject":"Re: [PATCH 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-02T13:34:46Z","receivedAt":"2021-08-02T13:36:02Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Phillip Wood 于2021年8月1日周日 下午6:10写道：\n>\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index 12f093121d9..5983f37d531 100644\n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -84,6 +84,7 @@ struct rebase_options {\n> > REBASE_FORCE = 1<<3,\n> > REBASE_INTERACTIVE_EXPLICIT = 1<<4,\n> > } flags;\n> > + int delete_cherry_pick_head;\n> > struct strvec git_am_opts;\n> > const char *action;\n> > int signoff;\n> > @@ -152,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n> > oidcpy(&replay.squash_onto, opts->squash_onto);\n> > replay.have_squash_onto = 1;\n> > }\n> > + replay.delete_cherry_pick_head = opts->delete_cherry_pick_head;\n>\n> I think we could just have\n> replay.delete_cherry_pick_head = 1;\n> and not add a new member to rebase_options as we always want this set\n\nMaybe you are right.\n\n> > -static void print_advice(struct repository *r, int show_hint,\n> > - struct replay_opts *opts)\n> > +static void print_advice(struct replay_opts *opts, int show_hint)\n> > {\n> > char *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n> >\n> > if (msg) {\n> > - fprintf(stderr, \"%s\\n\", msg);\n> > - /*\n> > - * A conflict has occurred but the porcelain\n> > - * (typically rebase --interactive) wants to take care\n> > - * of the commit itself so remove CHERRY_PICK_HEAD\n> > - */\n> > - refs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n> > - NULL, 0);\n> > - return;\n> > - }\n> > -\n> > - if (show_hint) {\n> > + advise(\"%s\\n\", msg);\n>\n> This is a change in behavior as the advice will now be prefixed with\n> \"hint: \". I think that is probably an improvement so long as it does not\n> make the lines too long when the advice is printed.\n>\n\nYeah, maybe I should mention this in commit messages.\n\n>\n> I think it would be useful to add a test for the new cherry-pick option\n> that is added in this patch. Overall this patch is looking pretty good\n> it just needs a few small changes, thanks for working on it.\n>\n\nMake sence.\n\n> Best Wishes\n>\n> Phillip\n\nThanks.\n--\nZheNing Hu\n"},{"id":"431790","messageId":"pull.1010.v2.git.1627953383.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":"pull.1010.git.1627714877.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-03T01:16:20Z","receivedAt":"2021-08-03T01:16:28Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"This patch fixes the bug when git cherry-pick is used with environment\nvariable GIT_CHERRY_PICK_HELP, and makes git chery-pick advice message\nbetter.\n\nv4:\nhttps://lore.kernel.org/git/pull.1010.git.1627714877.gitgitgadget@gmail.com/\n\nv4-->v5:\n\n 1. Delete struct rebase_options member delete_cherry_pick_head, and set the\n delete_cherry_pick_head in struct replay_opts replay to 1 in\n get_replay_opts().\n 2. Avoid too long line length of advice of cherry-pick, split \"git\n cherry-pick --continue\" in advice to new line.\n\nZheNing Hu (2):\n [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP\n [GSOC] cherry-pick: use better advice message\n\n builtin/rebase.c | 1 +\n builtin/revert.c | 2 ++\n git-rebase--preserve-merges.sh | 2 +-\n sequencer.c | 38 +++++++++++++++-----------\n sequencer.h | 1 +\n t/t3507-cherry-pick-conflict.sh | 48 ++++++++++++++++++++++-----------\n 6 files changed, 60 insertions(+), 32 deletions(-)\n\n\nbase-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1010\n\nRange-diff vs v1:\n\n 1: 0d0a55bd9c4 ! 1: 5d2fd55c580 [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP\n @@ Commit message\n conflict occurs, which provided for some porcelain commands of git like\n `git-rebase--preserve-merges.sh`. After `git rebase -p` completely\n abolished, this option should be removed. At the same time, add the flag\n - `delete_cherry_pick_head` to `struct rebase_options` and\n - `struct replay_opts`, We can decide whether to delete CHERRY_PICK_HEAD\n - by setting, passing, and checking this flag bit.\n + `delete_cherry_pick_head` to `struct replay_opts`, We can decide whether\n + to delete CHERRY_PICK_HEAD by setting and checking this flag bit.\n \n Then we split print_advice() into two part: Firstly, print_advice()\n will only be responsible for outputting content; Secondly, check if\n @@ Commit message\n In this way, the steps of printing advice and deleting CHERRY_PICK_HEAD\n are decoupled. Finally, let `git-rebase--preserve-merges.sh` use the\n `--delete-cherry-pick-head` option when it executes git cherry-pick, and\n - set the `delete_cherry_pick_head` flag in run_specific_rebase() when we\n + set the `delete_cherry_pick_head` flag in get_replay_opts() when we\n are using `git rebase --merge`, which can fix this breakage.\n \n + It is worth mentioning that now we use advice() to print the content\n + of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will\n + start with \"hint: \".\n +\n Mentored-by: Christian Couder \n Mentored-by Hariom Verma :\n Helped-by: Phillip Wood \n @@ Commit message\n Signed-off-by: ZheNing Hu \n \n ## builtin/rebase.c ##\n -@@ builtin/rebase.c: struct rebase_options {\n - \t\tREBASE_FORCE = 1<<3,\n - \t\tREBASE_INTERACTIVE_EXPLICIT = 1<<4,\n - \t} flags;\n -+\tint delete_cherry_pick_head;\n - \tstruct strvec git_am_opts;\n - \tconst char *action;\n - \tint signoff;\n @@ builtin/rebase.c: static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \t\toidcpy(&replay.squash_onto, opts->squash_onto);\n \t\treplay.have_squash_onto = 1;\n \t}\n -+\treplay.delete_cherry_pick_head = opts->delete_cherry_pick_head;\n ++\treplay.delete_cherry_pick_head = 1;\n \n \treturn replay;\n }\n -@@ builtin/rebase.c: static int run_specific_rebase(struct rebase_options *opts, enum action action)\n - \tif (opts->type == REBASE_MERGE) {\n - \t\t/* Run sequencer-based rebase */\n - \t\tsetenv(\"GIT_CHERRY_PICK_HELP\", resolvemsg, 1);\n -+\t\topts->delete_cherry_pick_head = 1;\n - \t\tif (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {\n - \t\t\tsetenv(\"GIT_SEQUENCE_EDITOR\", \":\", 1);\n - \t\t\topts->autosquash = 0;\n \n ## builtin/revert.c ##\n @@ builtin/revert.c: static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n @@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-\n test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick picked &&\n + \ttest_cmp_rev picked CHERRY_PICK_HEAD\n + '\n + \n ++test_expect_success 'failed cherry-pick with --delete-cherry-pick-head does not set CHERRY_PICK_HEAD' '\n ++\tpristine_detach initial &&\n ++\ttest_must_fail git cherry-pick --delete-cherry-pick-head picked &&\n ++\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n ++'\n ++\n + test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' '\n + \tpristine_detach initial &&\n + \tgit cherry-pick base &&\n @@ t/t3507-cherry-pick-conflict.sh: test_expect_success \\\n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n 2: 7e1ed49728d ! 2: 5279bca7a79 [GSOC] cherry-pick: use better advice message\n @@ Commit message\n This is the improved advice:\n \n hint: Resolve all conflicts manually, mark them as resolved with\n - hint: \"git add/rm \", then run \"git cherry-pick \\\n - --continue\".\n + hint: \"git add/rm \", then run\n + hint: \"git cherry-pick --continue\".\n hint: You can instead skip this commit: run \"git cherry-pick --skip\".\n hint: To abort and get back to the state before \"git cherry-pick\",\n hint: run \"git cherry-pick --abort\".\n @@ sequencer.c: static void print_advice(struct replay_opts *opts, int show_hint)\n -\t\tif (opts->no_commit)\n +\t\tif (opts->action == REPLAY_PICK) {\n +\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n -+\t\t\t\t \"\\\"git add/rm \\\", then run \\\"git cherry-pick --continue\\\".\\n\"\n ++\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n ++\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n +\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n +\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n +\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n @@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-\n -\thint: with 'git add ' or 'git rm '\n -\thint: and commit the result with 'git commit'\n +\thint: Resolve all conflicts manually, mark them as resolved with\n -+\thint: \\\"git add/rm \\\", then run \\\"git cherry-pick --continue\\\".\n ++\thint: \\\"git add/rm \\\", then run\n ++\thint: \\\"git cherry-pick --continue\\\".\n +\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n +\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n +\thint: run \\\"git cherry-pick --abort\\\".\n @@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-\n -\thint: after resolving the conflicts, mark the corrected paths\n -\thint: with 'git add ' or 'git rm '\n +\thint: Resolve all conflicts manually, mark them as resolved with\n -+\thint: \\\"git add/rm \\\", then run \\\"git cherry-pick --continue\\\".\n ++\thint: \\\"git add/rm \\\", then run\n ++\thint: \\\"git cherry-pick --continue\\\".\n +\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n +\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n +\thint: run \\\"git cherry-pick --abort\\\".\n\n-- \ngitgitgadget\n"},{"id":"431791","messageId":"5d2fd55c580abc2057f2e6fe9f7d9c94748bf8de.1627953383.git.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":"pull.1010.v2.git.1627953383.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-03T01:16:21Z","receivedAt":"2021-08-03T01:16:31Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nGIT_CHERRY_PICK_HELP is an environment variable, as the\nimplementation detail of some porcelain in git to help realize\nthe rebasing steps. E.g. `git rebase -p` set GIT_CHERRY_PICK_HELP\nvalue in `git-rebase--preserve-merges.sh`, `git rebase --merge` set\nGIT_CHERRY_PICK_HELP value in run_specific_rebase(). But If we set\nthe value of GIT_CHERRY_PICK_HELP when using `git cherry-pick`,\nCHERRY_PICK_HEAD will be deleted, then we will get an error when we\ntry to use `git cherry-pick --continue` or other cherr-pick command.\n\nIntroduce new \"hidden\" option `--delete-cherry-pick-head` for git\ncherry-pick which indicates that CHERRY_PICK_HEAD will be deleted when\nconflict occurs, which provided for some porcelain commands of git like\n`git-rebase--preserve-merges.sh`. After `git rebase -p` completely\nabolished, this option should be removed. At the same time, add the flag\n`delete_cherry_pick_head` to `struct replay_opts`, We can decide whether\nto delete CHERRY_PICK_HEAD by setting and checking this flag bit.\n\nThen we split print_advice() into two part: Firstly, print_advice()\nwill only be responsible for outputting content; Secondly, check if\nwe set the `delete_cherry_pick_head` flag; if set, delete CHERRY_PICK_HEAD.\nIn this way, the steps of printing advice and deleting CHERRY_PICK_HEAD\nare decoupled. Finally, let `git-rebase--preserve-merges.sh` use the\n`--delete-cherry-pick-head` option when it executes git cherry-pick, and\nset the `delete_cherry_pick_head` flag in get_replay_opts() when we\nare using `git rebase --merge`, which can fix this breakage.\n\nIt is worth mentioning that now we use advice() to print the content\nof GIT_CHERRY_PICK_HELP in print_advice(), each line of output will\nstart with \"hint: \".\n\nMentored-by: Christian Couder \nMentored-by Hariom Verma :\nHelped-by: Phillip Wood \nHepled-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n builtin/rebase.c | 1 +\n builtin/revert.c | 2 ++\n git-rebase--preserve-merges.sh | 2 +-\n sequencer.c | 28 +++++++++++++---------------\n sequencer.h | 1 +\n t/t3507-cherry-pick-conflict.sh | 31 +++++++++++++++++++++----------\n 6 files changed, 39 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 12f093121d9..08ba437c6a0 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -152,6 +152,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \t\toidcpy(&replay.squash_onto, opts->squash_onto);\n \t\treplay.have_squash_onto = 1;\n \t}\n+\treplay.delete_cherry_pick_head = 1;\n \n \treturn replay;\n }\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 237f2f18d4c..15a4b6fe4ee 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -127,6 +127,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\t\tOPT_BOOL(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n \t\t\tOPT_BOOL(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n \t\t\tOPT_BOOL(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, N_(\"keep redundant, empty commits\")),\n+\t\t\tOPT_BOOL_F(0, \"delete-cherry-pick-head\", &opts->delete_cherry_pick_head,\n+\t\t\t\t N_(\"delete CHERRY_PICK_HEAD when conflict occurs\"), PARSE_OPT_HIDDEN),\n \t\t\tOPT_END(),\n \t\t};\n \t\toptions = parse_options_concat(options, cp_extra);\ndiff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh\nindex b9c71d2a71b..eaa8f9de2c5 100644\n--- a/git-rebase--preserve-merges.sh\n+++ b/git-rebase--preserve-merges.sh\n@@ -444,7 +444,7 @@ pick_one_preserving_merges () {\n \t\t\toutput eval git cherry-pick $allow_rerere_autoupdate \\\n \t\t\t\t$allow_empty_message \\\n \t\t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n-\t\t\t\t\"$strategy_args\" \"$@\" ||\n+\t\t\t\t\"$strategy_args\" --delete-cherry-pick-head \"$@\" ||\n \t\t\t\tdie_with_patch $sha1 \"$(eval_gettext \"Could not pick \\$sha1\")\"\n \t\t\t;;\n \t\tesac\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..83cf6a5da3c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -397,24 +397,13 @@ static void free_message(struct commit *commit, struct commit_message *msg)\n \tunuse_commit_buffer(commit, msg->message);\n }\n \n-static void print_advice(struct repository *r, int show_hint,\n-\t\t\t struct replay_opts *opts)\n+static void print_advice(struct replay_opts *opts, int show_hint)\n {\n \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n \n \tif (msg) {\n-\t\tfprintf(stderr, \"%s\\n\", msg);\n-\t\t/*\n-\t\t * A conflict has occurred but the porcelain\n-\t\t * (typically rebase --interactive) wants to take care\n-\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n-\t\t */\n-\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n-\t\t\t\tNULL, 0);\n-\t\treturn;\n-\t}\n-\n-\tif (show_hint) {\n+\t\tadvise(\"%s\\n\", msg);\n+\t} else if (show_hint) {\n \t\tif (opts->no_commit)\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n@@ -2265,7 +2254,16 @@ static int do_pick_commit(struct repository *r,\n \t\t ? _(\"could not revert %s... %s\")\n \t\t : _(\"could not apply %s... %s\"),\n \t\t short_commit_name(commit), msg.subject);\n-\t\tprint_advice(r, res == 1, opts);\n+\t\tprint_advice(opts, res == 1);\n+\t\tif (opts->delete_cherry_pick_head) {\n+\t\t\t/*\n+\t\t\t * A conflict has occurred but the porcelain\n+\t\t\t * (typically rebase --interactive) wants to take care\n+\t\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n+\t\t\t */\n+\t\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n+\t\t\t\t\tNULL, 0);\n+\t\t}\n \t\trepo_rerere(r, opts->allow_rerere_auto);\n \t\tgoto leave;\n \t}\ndiff --git a/sequencer.h b/sequencer.h\nindex d57d8ea23d7..76fb4af56fd 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -49,6 +49,7 @@ struct replay_opts {\n \tint reschedule_failed_exec;\n \tint committer_date_is_author_date;\n \tint ignore_date;\n+\tint delete_cherry_pick_head;\n \n \tint mainline;\n \ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 014001b8f32..af5678d981a 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -76,12 +76,33 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \ttest_cmp expected actual\n \"\n \n+test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' \"\n+\tpristine_detach initial &&\n+\t(\n+\t\tpicked=\\$(git rev-parse --short picked) &&\n+\t\tcat <<-EOF >expected &&\n+\t\terror: could not apply \\$picked... picked\n+\t\thint: and then do something else\n+\t\tEOF\n+\t\tGIT_CHERRY_PICK_HELP='and then do something else' &&\n+\t\texport GIT_CHERRY_PICK_HELP &&\n+\t\ttest_must_fail git cherry-pick picked 2>actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+\"\n+\n test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick picked &&\n \ttest_cmp_rev picked CHERRY_PICK_HEAD\n '\n \n+test_expect_success 'failed cherry-pick with --delete-cherry-pick-head does not set CHERRY_PICK_HEAD' '\n+\tpristine_detach initial &&\n+\ttest_must_fail git cherry-pick --delete-cherry-pick-head picked &&\n+\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n+'\n+\n test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \tgit cherry-pick base &&\n@@ -109,16 +130,6 @@ test_expect_success \\\n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n \n-test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n-\tpristine_detach initial &&\n-\t(\n-\t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n-\t\texport GIT_CHERRY_PICK_HELP &&\n-\t\ttest_must_fail git cherry-pick picked\n-\t) &&\n-\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n-'\n-\n test_expect_success 'git reset clears CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \n-- \ngitgitgadget\n\n"},{"id":"431792","messageId":"5279bca7a794bd66681d420147e193d2b5415860.1627953383.git.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":"pull.1010.v2.git.1627953383.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-03T01:16:22Z","receivedAt":"2021-08-03T01:16:33Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nIn the past, git cherry-pick would print such advice when\nthere was a conflict:\n\nhint: after resolving the conflicts, mark the corrected paths\nhint: with 'git add ' or 'git rm '\nhint: and commit the result with 'git commit'\n\nBut in fact, when we want to cherry-pick multiple commits,\nwe should not use \"git commit\" after resolving conflicts, which\nwill make Git generate some errors. We should recommend users to\nuse `git cherry-pick --continue`, `git cherry-pick --abort`, just\nlike git rebase does.\n\nThis is the improved advice:\n\nhint: Resolve all conflicts manually, mark them as resolved with\nhint: \"git add/rm \", then run\nhint: \"git cherry-pick --continue\".\nhint: You can instead skip this commit: run \"git cherry-pick --skip\".\nhint: To abort and get back to the state before \"git cherry-pick\",\nhint: run \"git cherry-pick --abort\".\n\nMentored-by: Christian Couder \nMentored-by Hariom Verma :\nHelped-by: Phillip Wood \nHepled-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n sequencer.c | 10 +++++++++-\n t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 83cf6a5da3c..bf7dbea81dd 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -404,7 +404,15 @@ static void print_advice(struct replay_opts *opts, int show_hint)\n \tif (msg) {\n \t\tadvise(\"%s\\n\", msg);\n \t} else if (show_hint) {\n-\t\tif (opts->no_commit)\n+\t\tif (opts->action == REPLAY_PICK) {\n+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n+\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n+\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n+\n+\t\t} else if (opts->no_commit)\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n \t\telse\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex af5678d981a..e953b54e54d 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' \"\n \tpicked=\\$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n \terror: could not apply \\$picked... picked\n-\thint: after resolving the conflicts, mark the corrected paths\n-\thint: with 'git add ' or 'git rm '\n-\thint: and commit the result with 'git commit'\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \\\"git add/rm \\\", then run\n+\thint: \\\"git cherry-pick --continue\\\".\n+\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n+\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n+\thint: run \\\"git cherry-pick --abort\\\".\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n \n@@ -68,8 +71,12 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \tpicked=\\$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n \terror: could not apply \\$picked... picked\n-\thint: after resolving the conflicts, mark the corrected paths\n-\thint: with 'git add ' or 'git rm '\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \\\"git add/rm \\\", then run\n+\thint: \\\"git cherry-pick --continue\\\".\n+\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n+\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n+\thint: run \\\"git cherry-pick --abort\\\".\n \tEOF\n \ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n \n-- \ngitgitgadget\n"},{"id":"431890","messageId":"xmqqa6lytat9.fsf@gitster.g","threadId":"56187","inReplyTo":"5d2fd55c580abc2057f2e6fe9f7d9c94748bf8de.1627953383.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-03T22:36:50Z","receivedAt":"2021-08-03T22:37:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> From: ZheNing Hu \n>\n> GIT_CHERRY_PICK_HELP is an environment variable, as the\n> implementation detail of some porcelain in git to help realize\n> the rebasing steps. E.g. `git rebase -p` set GIT_CHERRY_PICK_HELP\n\nset -> sets\n\n> value in `git-rebase--preserve-merges.sh`, `git rebase --merge` set\n\nset -> sets\n\n> GIT_CHERRY_PICK_HELP value in run_specific_rebase().\n\n\"help realize the rebasing steps\" did not tell us much on \"how\" the\nenvironment variable helps or what it is used for. A sentence at\nthis point, e.g.\n\n The variable carries a custom help message to be shown when one\n step of replaying an existing commit fails in conflict.\n\nmay help. And there is one leap in the logic flow here.\n\n However, the code also removes CHERRY_PICK_HEAD pseudoref when\n this environment variable exists, assuming that the presence of\n it means the sequencer machinery and not end-user is doing the\n cherry-picking.\n\n> But If we set\n> the value of GIT_CHERRY_PICK_HELP when using `git cherry-pick`,\n> CHERRY_PICK_HEAD will be deleted, then we will get an error when we\n> try to use `git cherry-pick --continue` or other cherr-pick command.\n\nAnd then we can drop \"But\" before \"If\" here.\n\n> Introduce new \"hidden\" option `--delete-cherry-pick-head` for git\n> cherry-pick which indicates that CHERRY_PICK_HEAD will be deleted when\n> conflict occurs, which provided for some porcelain commands of git like\n> `git-rebase--preserve-merges.sh`.\n\nindicates that CHERRY_PICK_HEAD will be ... ->\n\ntells Git remove CHERRY_PICK_HEAD to separate the decision from\nmessage customization to clean up this mess.\n\n> Mentored-by: Christian Couder \n> Mentored-by Hariom Verma :\n> Helped-by: Phillip Wood \n> Hepled-by: Junio C Hamano \n\nHeple?\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 0bec01cf38e..83cf6a5da3c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -397,24 +397,13 @@ static void free_message(struct commit *commit, struct commit_message *msg)\n> \tunuse_commit_buffer(commit, msg->message);\n> }\n> \n> -static void print_advice(struct repository *r, int show_hint,\n> -\t\t\t struct replay_opts *opts)\n> +static void print_advice(struct replay_opts *opts, int show_hint)\n> {\n> \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n> \n> \tif (msg) {\n> +\t\tadvise(\"%s\\n\", msg);\n> +\t} else if (show_hint) {\n> \t\tif (opts->no_commit)\n> \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> \t\t\t\t \"with 'git add ' or 'git rm '\"));\n\nOK. That makes sense.\n\n> @@ -2265,7 +2254,16 @@ static int do_pick_commit(struct repository *r,\n> \t\t ? _(\"could not revert %s... %s\")\n> \t\t : _(\"could not apply %s... %s\"),\n> \t\t short_commit_name(commit), msg.subject);\n> -\t\tprint_advice(r, res == 1, opts);\n> +\t\tprint_advice(opts, res == 1);\n> +\t\tif (opts->delete_cherry_pick_head) {\n> +\t\t\t/*\n> +\t\t\t * A conflict has occurred but the porcelain\n> +\t\t\t * (typically rebase --interactive) wants to take care\n> +\t\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n> +\t\t\t */\n> +\t\t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n> +\t\t\t\t\tNULL, 0);\n> +\t\t}\n\nOK, this separation makes sense, too.\n\n> -test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n> -\tpristine_detach initial &&\n> -\t(\n> -\t\tGIT_CHERRY_PICK_HELP=\"and then do something else\" &&\n> -\t\texport GIT_CHERRY_PICK_HELP &&\n> -\t\ttest_must_fail git cherry-pick picked\n> -\t) &&\n> -\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> -'\n\nHmph, this is a bit troubling. So has this been part of the\n\"published\" behaviour since d7e5c0cb (Introduce CHERRY_PICK_HEAD,\n2011-02-19) that introduced this test, and there are people who are\nrelying on it? IOW, should the resolution to the original problem\nreport have been \"if it hurts, don't do it\" (in other words, \"setting\nGIT_CHERRY_PICK_HELP will remove CHERRY_PICK_HEAD, so if you do not\nwant to get the latter removed, do not set the former\")?\n\n"},{"id":"431942","messageId":"CAOLTT8SkbNMcVocU9Lg3PfqTGHVEX8y27BMcP55HytfWH60w6g@mail.gmail.com","threadId":"56187","inReplyTo":"xmqqa6lytat9.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-04T08:35:20Z","receivedAt":"2021-08-04T08:34:40Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年8月4日周三 上午6:36写道：\n>\n\n> > GIT_CHERRY_PICK_HELP value in run_specific_rebase().\n>\n> \"help realize the rebasing steps\" did not tell us much on \"how\" the\n> environment variable helps or what it is used for. A sentence at\n> this point, e.g.\n>\n> The variable carries a custom help message to be shown when one\n> step of replaying an existing commit fails in conflict.\n>\n> may help. And there is one leap in the logic flow here.\n>\n> However, the code also removes CHERRY_PICK_HEAD pseudoref when\n> this environment variable exists, assuming that the presence of\n> it means the sequencer machinery and not end-user is doing the\n> cherry-picking.\n>\n\nThanks, such a supplement is very good.\n\n> Hmph, this is a bit troubling. So has this been part of the\n> \"published\" behaviour since d7e5c0cb (Introduce CHERRY_PICK_HEAD,\n> 2011-02-19) that introduced this test, and there are people who are\n> relying on it? IOW, should the resolution to the original problem\n> report have been \"if it hurts, don't do it\" (in other words, \"setting\n> GIT_CHERRY_PICK_HELP will remove CHERRY_PICK_HEAD, so if you do not\n> want to get the latter removed, do not set the former\")?\n>\n\nYou mean that cherry_pick with GIT_CHERRY_PICK_HELP suppresses\nCHERRY_PICK_HEAD is not even a bug?\n\nIt is reasonable for `git rebase -p` and `git rebase -m` to delete\nCHERRY_PICK_HEAD when a conflict occurs, but it is not necessarily\nfor git cherry-pick to delete it too. IOW, I suspect that instead of\nletting users\nnot touch the trap here, it is better to remove the trap completely.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"431947","messageId":"2e3e8379-99e3-521b-1666-30fcf0e7909b@gmail.com","threadId":"56187","inReplyTo":"CAOLTT8SkbNMcVocU9Lg3PfqTGHVEX8y27BMcP55HytfWH60w6g@mail.gmail.com","subject":"Re: [PATCH v2 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-04T10:10:11Z","receivedAt":"2021-08-04T10:12:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/08/2021 09:35, ZheNing Hu wrote:\n> Junio C Hamano 于2021年8月4日周三 上午6:36写道：\n>> Hmph, this is a bit troubling. So has this been part of the\n>> \"published\" behaviour since d7e5c0cb (Introduce CHERRY_PICK_HEAD,\n>> 2011-02-19) that introduced this test, and there are people who are\n>> relying on it? IOW, should the resolution to the original problem\n>> report have been \"if it hurts, don't do it\" (in other words, \"setting\n>> GIT_CHERRY_PICK_HELP will remove CHERRY_PICK_HEAD, so if you do not\n>> want to get the latter removed, do not set the former\")?\n>>\n> \n> You mean that cherry_pick with GIT_CHERRY_PICK_HELP suppresses\n> CHERRY_PICK_HEAD is not even a bug?\n> \n> It is reasonable for `git rebase -p` and `git rebase -m` to delete\n> CHERRY_PICK_HEAD when a conflict occurs, but it is not necessarily\n> for git cherry-pick to delete it too. IOW, I suspect that instead of\n> letting users\n> not touch the trap here, it is better to remove the trap completely.\n\nLooking at the history I think it is fair to conclude that \nGIT_CHERRY_PICK_HELP was introduced as a way to help people writing \nscripts built on top of 'git cherry-pick' like 'git rebase' that want to \nhave a custom message and do not want to leave CHERRY_PICK_HEAD around \nif there are conflicts. I don't think it was intended as a way for users \nto change the help when cherry-picking and has never been documented as \nsuch. I think we'd be better to focus on improving the default help that \ncherry-pick prints as the second patch in this series does.\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n> --\n> ZheNing Hu\n> \n"},{"id":"431966","messageId":"xmqqsfzpp15j.fsf@gitster.g","threadId":"56187","inReplyTo":"2e3e8379-99e3-521b-1666-30fcf0e7909b@gmail.com","subject":"Re: [PATCH v2 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-04T17:31:20Z","receivedAt":"2021-08-04T17:31:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> You mean that cherry_pick with GIT_CHERRY_PICK_HELP suppresses\n>> CHERRY_PICK_HEAD is not even a bug?\n\nI think that it is what the existing test is telling us. Of course,\nwith a good reason, an earlier design can be updated as long as we\nmake sure we won't hurt existing users who may rely on the current\ndesign, but ...\n\n> Looking at the history I think it is fair to conclude that\n> GIT_CHERRY_PICK_HELP was introduced as a way to help people writing \n> scripts built on top of 'git cherry-pick' like 'git rebase' that want\n> to have a custom message and do not want to leave CHERRY_PICK_HEAD\n> around if there are conflicts. I don't think it was intended as a way\n> for users to change the help when cherry-picking and has never been\n> documented as such. I think we'd be better to focus on improving the\n> default help that cherry-pick prints as the second patch in this\n> series does.\n\n... I think that is a reasonable stance to take [*1*]. If the\ndefault help message can be improved, that is a good thing to do\nregardless.\n\nThanks.\n\n[Footnote]\n\n*1* Tying the \"here is a custom HELP text\" environment variable to\n\"having a customization means whoever is driving the cherry-pick\nmachinery is ALSO responsible for sequencing and we will remove\nCHERRY_PICK_HEAD\" is a rather unfortunate design, but as long as\nthat is documented, it is a workable design.\n"},{"id":"432052","messageId":"CAOLTT8S+x3aGRopiXwWcNYsyQtiGWChR+f8u+7nM5A0xpusR3A@mail.gmail.com","threadId":"56187","inReplyTo":"xmqqsfzpp15j.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-05T05:36:41Z","receivedAt":"2021-08-05T05:35:59Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年8月5日周四 上午1:31写道：\n>\n> Phillip Wood writes:\n>\n> >> You mean that cherry_pick with GIT_CHERRY_PICK_HELP suppresses\n> >> CHERRY_PICK_HEAD is not even a bug?\n>\n> I think that it is what the existing test is telling us. Of course,\n> with a good reason, an earlier design can be updated as long as we\n> make sure we won't hurt existing users who may rely on the current\n> design, but ...\n>\n> > Looking at the history I think it is fair to conclude that\n> > GIT_CHERRY_PICK_HELP was introduced as a way to help people writing\n> > scripts built on top of 'git cherry-pick' like 'git rebase' that want\n> > to have a custom message and do not want to leave CHERRY_PICK_HEAD\n> > around if there are conflicts. I don't think it was intended as a way\n> > for users to change the help when cherry-picking and has never been\n> > documented as such. I think we'd be better to focus on improving the\n> > default help that cherry-pick prints as the second patch in this\n> > series does.\n>\n> ... I think that is a reasonable stance to take [*1*]. If the\n> default help message can be improved, that is a good thing to do\n> regardless.\n>\n\nWell, this is indeed a bit strange, but maybe your and Phillip's\nintuitions are right,\nthen I will delete the content of the first patch and keep the second.\n\n> Thanks.\n>\n> [Footnote]\n>\n> *1* Tying the \"here is a custom HELP text\" environment variable to\n> \"having a customization means whoever is driving the cherry-pick\n> machinery is ALSO responsible for sequencing and we will remove\n> CHERRY_PICK_HEAD\" is a rather unfortunate design, but as long as\n> that is documented, it is a workable design.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"432053","messageId":"pull.1010.v3.git.1628142482640.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":"pull.1010.v2.git.1627953383.gitgitgadget@gmail.com","subject":"[PATCH v3] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-05T05:48:02Z","receivedAt":"2021-08-05T05:48:11Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nIn the past, git cherry-pick would print such advice when\nthere was a conflict:\n\nhint: after resolving the conflicts, mark the corrected paths\nhint: with 'git add ' or 'git rm '\nhint: and commit the result with 'git commit'\n\nBut in fact, when we want to cherry-pick multiple commits,\nwe should not use \"git commit\" after resolving conflicts, which\nwill make Git generate some errors. We should recommend users to\nuse `git cherry-pick --continue`, `git cherry-pick --abort`, just\nlike git rebase does.\n\nThis is the improved advice:\n\nhint: Resolve all conflicts manually, mark them as resolved with\nhint: \"git add/rm \", then run\nhint: \"git cherry-pick --continue\".\nhint: You can instead skip this commit: run \"git cherry-pick --skip\".\nhint: To abort and get back to the state before \"git cherry-pick\",\nhint: run \"git cherry-pick --abort\".\n\nIt is worth mentioning that now we use advice() to print the content\nof GIT_CHERRY_PICK_HELP in print_advice(), each line of output will\nstart with \"hint: \".\n\nMentored-by: Christian Couder \nMentored-by Hariom Verma :\nHelped-by: Phillip Wood \nHepled-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] cherry-pick: use better advice message\n \n Because git cherry-pick's past advice message is not good enough, it\n often misleads new users of Git, so this patch makes git chery-pick\n advice message better.\n \n v5:\n https://lore.kernel.org/git/pull.1010.git.1627714877.gitgitgadget@gmail.com/\n \n v5-->v6:\n \n 1. Deleted the first patch, which could have made git cherry-pick not\n delete CHERRY_PICK_HEAD when using GIT_CHERRY_PICK_HELP, but both\n Junio and Phillip believe that this design does not need to be\n changed.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1010\n\nRange-diff vs v2:\n\n 1: 5d2fd55c580 < -: ----------- [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP\n 2: 5279bca7a79 ! 1: 701645dde17 [GSOC] cherry-pick: use better advice message\n @@ Commit message\n hint: To abort and get back to the state before \"git cherry-pick\",\n hint: run \"git cherry-pick --abort\".\n \n + It is worth mentioning that now we use advice() to print the content\n + of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will\n + start with \"hint: \".\n +\n Mentored-by: Christian Couder \n Mentored-by Hariom Verma :\n Helped-by: Phillip Wood \n @@ Commit message\n Signed-off-by: ZheNing Hu \n \n ## sequencer.c ##\n -@@ sequencer.c: static void print_advice(struct replay_opts *opts, int show_hint)\n +@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,\n + \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n + \n \tif (msg) {\n - \t\tadvise(\"%s\\n\", msg);\n - \t} else if (show_hint) {\n +-\t\tfprintf(stderr, \"%s\\n\", msg);\n ++\t\tadvise(\"%s\\n\", msg);\n + \t\t/*\n + \t\t * A conflict has occurred but the porcelain\n + \t\t * (typically rebase --interactive) wants to take care\n +@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,\n + \t}\n + \n + \tif (show_hint) {\n -\t\tif (opts->no_commit)\n +\t\tif (opts->action == REPLAY_PICK) {\n +\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n @@ sequencer.c: static void print_advice(struct replay_opts *opts, int show_hint)\n +\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n +\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n +\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n -+\n +\t\t} else if (opts->no_commit)\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n\n\n sequencer.c | 11 +++++++++--\n t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..7fa91b99870 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,\n \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n \n \tif (msg) {\n-\t\tfprintf(stderr, \"%s\\n\", msg);\n+\t\tadvise(\"%s\\n\", msg);\n \t\t/*\n \t\t * A conflict has occurred but the porcelain\n \t\t * (typically rebase --interactive) wants to take care\n@@ -415,7 +415,14 @@ static void print_advice(struct repository *r, int show_hint,\n \t}\n \n \tif (show_hint) {\n-\t\tif (opts->no_commit)\n+\t\tif (opts->action == REPLAY_PICK) {\n+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n+\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n+\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n+\t\t} else if (opts->no_commit)\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n \t\telse\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 014001b8f32..d3ed9d7ce0d 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' \"\n \tpicked=\\$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n \terror: could not apply \\$picked... picked\n-\thint: after resolving the conflicts, mark the corrected paths\n-\thint: with 'git add ' or 'git rm '\n-\thint: and commit the result with 'git commit'\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \\\"git add/rm \\\", then run\n+\thint: \\\"git cherry-pick --continue\\\".\n+\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n+\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n+\thint: run \\\"git cherry-pick --abort\\\".\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n \n@@ -68,8 +71,12 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \tpicked=\\$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n \terror: could not apply \\$picked... picked\n-\thint: after resolving the conflicts, mark the corrected paths\n-\thint: with 'git add ' or 'git rm '\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \\\"git add/rm \\\", then run\n+\thint: \\\"git cherry-pick --continue\\\".\n+\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n+\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n+\thint: run \\\"git cherry-pick --abort\\\".\n \tEOF\n \ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n \n\nbase-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n-- \ngitgitgadget\n"},{"id":"432472","messageId":"a510722c-d5a2-f00c-dd20-dd7f76da2408@gmail.com","threadId":"56187","inReplyTo":"pull.1010.v3.git.1628142482640.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] [GSOC] cherry-pick: use better advice message","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-11T10:00:32Z","receivedAt":"2021-08-11T10:00:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 05/08/2021 06:48, ZheNing Hu via GitGitGadget wrote:\n> From: ZheNing Hu \n> [...]\n> sequencer.c | 11 +++++++++--\n> t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----\n> 2 files changed, 21 insertions(+), 7 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 0bec01cf38e..7fa91b99870 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,\n> \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n> \n> \tif (msg) {\n> -\t\tfprintf(stderr, \"%s\\n\", msg);\n> +\t\tadvise(\"%s\\n\", msg);\n> \t\t/*\n> \t\t * A conflict has occurred but the porcelain\n> \t\t * (typically rebase --interactive) wants to take care\n> @@ -415,7 +415,14 @@ static void print_advice(struct repository *r, int show_hint,\n> \t}\n> \n> \tif (show_hint) {\n> -\t\tif (opts->no_commit)\n> +\t\tif (opts->action == REPLAY_PICK) {\n\nThis changes means we give the wrong advice for 'git cherry-pick \n--no-commit'. I think you want to keep the existing clause as the first \none and insert this before the else. The advice itself looks good. It \nwould be nice to improve the advice for 'git revert' in the same way.\n\n> +\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n> +\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n> +\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n> +\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n> +\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n> +\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n> +\t\t} else if (opts->no_commit)\n> \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> \t\t\t\t \"with 'git add ' or 'git rm '\"));\n> \t\telse\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 014001b8f32..d3ed9d7ce0d 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' \"\n> \tpicked=\\$(git rev-parse --short picked) &&\n> \tcat <<-EOF >expected &&\n\nIf you quote the here doc end marker then there is no substitution in \nthe here doc so writing\n\n\tcat <<-\\EOF >expected &&\n\n> \terror: could not apply \\$picked... picked\n> -\thint: after resolving the conflicts, mark the corrected paths\n> -\thint: with 'git add ' or 'git rm '\n> -\thint: and commit the result with 'git commit'\n> +\thint: Resolve all conflicts manually, mark them as resolved with\n> +\thint: \\\"git add/rm \\\", then run\n\nmeans you can replace \\\" with \" here\n\nBest Wishes\n\nPhillip\n\n> +\thint: \\\"git cherry-pick --continue\\\".\n> +\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n> +\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n> +\thint: run \\\"git cherry-pick --abort\\\".\n> \tEOF\n> \ttest_must_fail git cherry-pick picked 2>actual &&\n> \n> @@ -68,8 +71,12 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n> \tpicked=\\$(git rev-parse --short picked) &&\n> \tcat <<-EOF >expected &&\n> \terror: could not apply \\$picked... picked\n> -\thint: after resolving the conflicts, mark the corrected paths\n> -\thint: with 'git add ' or 'git rm '\n> +\thint: Resolve all conflicts manually, mark them as resolved with\n> +\thint: \\\"git add/rm \\\", then run\n> +\thint: \\\"git cherry-pick --continue\\\".\n> +\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n> +\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n> +\thint: run \\\"git cherry-pick --abort\\\".\n> \tEOF\n> \ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n> \n> \n> base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n> \n\n"},{"id":"432631","messageId":"CAOLTT8QGD08BRuJ5Z8kLdS8gFx+HBeZfsgqotTz0Xch3BNHGPg@mail.gmail.com","threadId":"56187","inReplyTo":"a510722c-d5a2-f00c-dd20-dd7f76da2408@gmail.com","subject":"Re: [PATCH v3] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-13T08:08:08Z","receivedAt":"2021-08-13T08:08:21Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Hi,\nsorry for the late reply, I am busy processing the patches on the\nref-filter side.\n\nPhillip Wood 于2021年8月11日周三 下午6:00写道：\n>\n> On 05/08/2021 06:48, ZheNing Hu via GitGitGadget wrote:\n> > From: ZheNing Hu \n> > [...]\n> > sequencer.c | 11 +++++++++--\n> > t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----\n> > 2 files changed, 21 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 0bec01cf38e..7fa91b99870 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,\n> > char *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n> >\n> > if (msg) {\n> > - fprintf(stderr, \"%s\\n\", msg);\n> > + advise(\"%s\\n\", msg);\n> > /*\n> > * A conflict has occurred but the porcelain\n> > * (typically rebase --interactive) wants to take care\n> > @@ -415,7 +415,14 @@ static void print_advice(struct repository *r, int show_hint,\n> > }\n> >\n> > if (show_hint) {\n> > - if (opts->no_commit)\n> > + if (opts->action == REPLAY_PICK) {\n>\n> This changes means we give the wrong advice for 'git cherry-pick\n> --no-commit'. I think you want to keep the existing clause as the first\n> one and insert this before the else. The advice itself looks good. It\n> would be nice to improve the advice for 'git revert' in the same way.\n>\n\nMake sense.\n\n> > + advise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n> > + \"\\\"git add/rm \\\", then run\\n\"\n> > + \"\\\"git cherry-pick --continue\\\".\\n\"\n> > + \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n> > + \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n> > + \"run \\\"git cherry-pick --abort\\\".\"));\n> > + } else if (opts->no_commit)\n> > advise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> > \"with 'git add ' or 'git rm '\"));\n> > else\n> > diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> > index 014001b8f32..d3ed9d7ce0d 100755\n> > --- a/t/t3507-cherry-pick-conflict.sh\n> > +++ b/t/t3507-cherry-pick-conflict.sh\n> > @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' \"\n> > picked=\\$(git rev-parse --short picked) &&\n> > cat <<-EOF >expected &&\n>\n> If you quote the here doc end marker then there is no substitution in\n> the here doc so writing\n>\n> cat <<-\\EOF >expected &&\n>\n> > error: could not apply \\$picked... picked\n> > - hint: after resolving the conflicts, mark the corrected paths\n> > - hint: with 'git add ' or 'git rm '\n> > - hint: and commit the result with 'git commit'\n> > + hint: Resolve all conflicts manually, mark them as resolved with\n> > + hint: \\\"git add/rm \\\", then run\n>\n> means you can replace \\\" with \" here\n>\n\nThanks, I haven't paid attention to this detail before.\n\n> Best Wishes\n>\n> Phillip\n>\n\nThanks,\n--\nZheNing Hu\n"},{"id":"432701","messageId":"xmqqzgtlumob.fsf@gitster.g","threadId":"56187","inReplyTo":"a510722c-d5a2-f00c-dd20-dd7f76da2408@gmail.com","subject":"Re: [PATCH v3] [GSOC] cherry-pick: use better advice message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-13T20:14:44Z","receivedAt":"2021-08-13T20:14:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> +++ b/t/t3507-cherry-pick-conflict.sh\n>> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' \"\n>> \tpicked=\\$(git rev-parse --short picked) &&\n>> \tcat <<-EOF >expected &&\n>\n> If you quote the here doc end marker then there is no substitution in\n> the here doc so writing\n>\n> \tcat <<-\\EOF >expected &&\n>\n>> \terror: could not apply \\$picked... picked\n>> -\thint: after resolving the conflicts, mark the corrected paths\n>> -\thint: with 'git add ' or 'git rm '\n>> -\thint: and commit the result with 'git commit'\n>> +\thint: Resolve all conflicts manually, mark them as resolved with\n>> +\thint: \\\"git add/rm \\\", then run\n>\n> means you can replace \\\" with \" here\n\nHmph, actually the double-quote that opens the body of\ntest_expect_success should be stared at with a very cautious eyes,\nas they often do not mean what the author of the patch thought they\ndo. I can see that it already fooled your eyes into thinking that\nthere is no substitution, but $picked needs to be substituted into\nits value. The backslash before it is *not* about guarding it from\nsubstitution inside here-doc; it is to pass literal \"$\" into the\nstring, which is the last parameter to test_expect_success, that\ngets eval'ed.\n\nThe original of this one, for example, would probably have been\nbetter if written like so:\n\ntest_expect_success 'advice from failed cherry-pick' '\n\tpristine_detach initial &&\n\tSQ='\\'' &&\n\n\tpicked=$(git rev-parse --short picked) &&\n\tcat <<-EOF >expected &&\n\terror: could not apply $picked... picked\n\thint: after resolving the conflicts, mark the corrected paths\n\thint: with ${SQ}git add ${SQ} or ${SQ}git rm ${SQ}\n\thint: and commit the result with ${SQ}git commit${SQ}\n\tEOF\n\ttest_must_fail git cherry-pick picked 2>actual &&\n\n\ttest_cmp expected actual\n'\n\nAnd because there is no single quote in the updated text, it would\nbecome:\n\ntest_expect_success 'advice from failed cherry-pick' '\n\tpristine_detach initial &&\n\n\tpicked=$(git rev-parse --short picked) &&\n\tcat <<-EOF >expected &&\n\terror: could not apply $picked... picked\n\thint: Resolve all conflicts manually, mark them as resolved with\n\thint: \"git add/rm \", then run\n\tEOF\n\ttest_must_fail git cherry-pick picked 2>actual &&\n\n\ttest_cmp expected actual\n'\n\nwhich makes it far easier to see that $picked needs to be\nsubstituted, and the \"git add/rm\" are to be enclosed in dq pair.\n"},{"id":"432724","messageId":"CAOLTT8Qrz_t60i6U0YvrgXKGkqVEx13ts5T1J1qKht3iMzC2bg@mail.gmail.com","threadId":"56187","inReplyTo":"xmqqzgtlumob.fsf@gitster.g","subject":"Re: [PATCH v3] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-14T02:07:40Z","receivedAt":"2021-08-14T02:07:53Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年8月14日周六 上午4:14写道：\n>\n> Phillip Wood writes:\n>\n> >> +++ b/t/t3507-cherry-pick-conflict.sh\n> >> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' \"\n> >> picked=\\$(git rev-parse --short picked) &&\n> >> cat <<-EOF >expected &&\n> >\n> > If you quote the here doc end marker then there is no substitution in\n> > the here doc so writing\n> >\n> > cat <<-\\EOF >expected &&\n> >\n> >> error: could not apply \\$picked... picked\n> >> - hint: after resolving the conflicts, mark the corrected paths\n> >> - hint: with 'git add ' or 'git rm '\n> >> - hint: and commit the result with 'git commit'\n> >> + hint: Resolve all conflicts manually, mark them as resolved with\n> >> + hint: \\\"git add/rm \\\", then run\n> >\n> > means you can replace \\\" with \" here\n>\n> Hmph, actually the double-quote that opens the body of\n> test_expect_success should be stared at with a very cautious eyes,\n> as they often do not mean what the author of the patch thought they\n> do. I can see that it already fooled your eyes into thinking that\n> there is no substitution, but $picked needs to be substituted into\n> its value. The backslash before it is *not* about guarding it from\n> substitution inside here-doc; it is to pass literal \"$\" into the\n> string, which is the last parameter to test_expect_success, that\n> gets eval'ed.\n>\n\nYes, the escaping here comes from double-quote of test_expect_success\ninstead of here-doc.\n\n> The original of this one, for example, would probably have been\n> better if written like so:\n>\n> test_expect_success 'advice from failed cherry-pick' '\n> pristine_detach initial &&\n> SQ='\\'' &&\n>\n> picked=$(git rev-parse --short picked) &&\n> cat <<-EOF >expected &&\n> error: could not apply $picked... picked\n> hint: after resolving the conflicts, mark the corrected paths\n> hint: with ${SQ}git add ${SQ} or ${SQ}git rm ${SQ}\n> hint: and commit the result with ${SQ}git commit${SQ}\n> EOF\n> test_must_fail git cherry-pick picked 2>actual &&\n>\n> test_cmp expected actual\n> '\n>\n\nUm? This section is not working for me.\n\n./test-lib.sh: eval: line 917: unexpected EOF while looking for matching `''\n./test-lib.sh: eval: line 932: syntax error: unexpected end of file\n\n> And because there is no single quote in the updated text, it would\n> become:\n>\n> test_expect_success 'advice from failed cherry-pick' '\n> pristine_detach initial &&\n>\n> picked=$(git rev-parse --short picked) &&\n> cat <<-EOF >expected &&\n> error: could not apply $picked... picked\n> hint: Resolve all conflicts manually, mark them as resolved with\n> hint: \"git add/rm \", then run\n> EOF\n> test_must_fail git cherry-pick picked 2>actual &&\n>\n> test_cmp expected actual\n> '\n>\n> which makes it far easier to see that $picked needs to be\n> substituted, and the \"git add/rm\" are to be enclosed in dq pair.\n\nThis section is exactly what I want. Using single quotes does make it\nlook a lot easier.\n\n--\nZheNing Hu\n"},{"id":"432735","messageId":"pull.1010.v4.git.1628936863733.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":"pull.1010.v3.git.1628142482640.gitgitgadget@gmail.com","subject":"[PATCH v4] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-14T10:27:43Z","receivedAt":"2021-08-14T10:27:56Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nIn the past, git cherry-pick would print such advice when\nthere was a conflict:\n\nhint: after resolving the conflicts, mark the corrected paths\nhint: with 'git add ' or 'git rm '\nhint: and commit the result with 'git commit'\n\nBut in fact, when we want to cherry-pick multiple commits,\nwe should not use \"git commit\" after resolving conflicts, which\nwill make Git generate some errors. We should recommend users to\nuse `git cherry-pick --continue`, `git cherry-pick --abort`, just\nlike git rebase does.\n\nThis is the improved advice:\n\nhint: Resolve all conflicts manually, mark them as resolved with\nhint: \"git add/rm \", then run\nhint: \"git cherry-pick --continue\".\nhint: You can instead skip this commit: run \"git cherry-pick --skip\".\nhint: To abort and get back to the state before \"git cherry-pick\",\nhint: run \"git cherry-pick --abort\".\n\nSimilarly, this optimization can be applied to git revert:\n\nhint: Resolve all conflicts manually, mark them as resolved with\nhint: \"git add/rm \", then run\nhint: \"git revert --continue\".\nhint: You can instead skip this commit: run \"git revert --skip\".\nhint: To abort and get back to the state before \"git revert\",\nhint: run \"git revert --abort\".\n\nIt is worth mentioning that now we use advice() to print the content\nof GIT_CHERRY_PICK_HELP in print_advice(), each line of output will\nstart with \"hint: \".\n\nMentored-by: Christian Couder \nMentored-by Hariom Verma :\nHelped-by: Phillip Wood \nHepled-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] cherry-pick: use better advice message\n \n Because git cherry-pick's past advice message is not good enough, it\n often misleads new users of Git, so this patch makes git chery-pick\n advice message better.\n \n v6:\n https://lore.kernel.org/git/pull.1010.v3.git.1628142482640.gitgitgadget@gmail.com/\n \n v6-->v7:\n \n 1. Modify the advice order, respect git cherry-pick --no-commit advice.\n 2. Let git revert also use better message.\n 3. Use double quotes instead of single quotes for the test body.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1010\n\nRange-diff vs v3:\n\n 1: 701645dde17 ! 1: dc51c0b8c2b [GSOC] cherry-pick: use better advice message\n @@ Commit message\n hint: To abort and get back to the state before \"git cherry-pick\",\n hint: run \"git cherry-pick --abort\".\n \n + Similarly, this optimization can be applied to git revert:\n +\n + hint: Resolve all conflicts manually, mark them as resolved with\n + hint: \"git add/rm \", then run\n + hint: \"git revert --continue\".\n + hint: You can instead skip this commit: run \"git revert --skip\".\n + hint: To abort and get back to the state before \"git revert\",\n + hint: run \"git revert --abort\".\n +\n It is worth mentioning that now we use advice() to print the content\n of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will\n start with \"hint: \".\n @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,\n \t\t * A conflict has occurred but the porcelain\n \t\t * (typically rebase --interactive) wants to take care\n @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,\n - \t}\n - \n - \tif (show_hint) {\n --\t\tif (opts->no_commit)\n -+\t\tif (opts->action == REPLAY_PICK) {\n + \t\tif (opts->no_commit)\n + \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n + \t\t\t\t \"with 'git add ' or 'git rm '\"));\n ++\t\telse if (opts->action == REPLAY_PICK)\n +\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n +\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n +\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n +\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n +\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n +\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n -+\t\t} else if (opts->no_commit)\n - \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n - \t\t\t\t \"with 'git add ' or 'git rm '\"));\n ++\t\telse if (opts->action == REPLAY_REVERT)\n ++\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n ++\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n ++\t\t\t\t \"\\\"git revert --continue\\\".\\n\"\n ++\t\t\t\t \"You can instead skip this commit: run \\\"git revert --skip\\\".\\n\"\n ++\t\t\t\t \"To abort and get back to the state before \\\"git revert\\\",\\n\"\n ++\t\t\t\t \"run \\\"git revert --abort\\\".\"));\n \t\telse\n + \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n + \t\t\t\t \"with 'git add ' or 'git rm '\\n\"\n +\n + ## t/t3501-revert-cherry-pick.sh ##\n +@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty renamed file' '\n + \tgrep -q \"^modified$\" renamed\n + '\n + \n ++test_expect_success 'advice from failed revert' '\n ++\techo dream >dream &&\n ++\tgit add dream &&\n ++\tgit commit -m \"add dream\" &&\n ++\tdream_oid=$(git rev-parse --short HEAD) &&\n ++\tcat <<-EOF >expected &&\n ++\terror: could not revert $dream_oid... add dream\n ++\thint: Resolve all conflicts manually, mark them as resolved with\n ++\thint: \"git add/rm \", then run\n ++\thint: \"git revert --continue\".\n ++\thint: You can instead skip this commit: run \"git revert --skip\".\n ++\thint: To abort and get back to the state before \"git revert\",\n ++\thint: run \"git revert --abort\".\n ++\tEOF\n ++\techo dream >>dream &&\n ++\tgit add dream &&\n ++\tgit commit -m \"double-add dream\" &&\n ++\ttest_must_fail git revert HEAD^ 2>actual &&\n ++\ttest_cmp expected actual\n ++'\n + test_done\n \n ## t/t3507-cherry-pick-conflict.sh ##\n -@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-pick' \"\n - \tpicked=\\$(git rev-parse --short picked) &&\n +@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'failed cherry-pick does not advance HEAD' '\n + \ttest \"$head\" = \"$newhead\"\n + '\n + \n +-test_expect_success 'advice from failed cherry-pick' \"\n ++test_expect_success 'advice from failed cherry-pick' '\n + \tpristine_detach initial &&\n + \n +-\tpicked=\\$(git rev-parse --short picked) &&\n ++\tpicked=$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n - \terror: could not apply \\$picked... picked\n +-\terror: could not apply \\$picked... picked\n -\thint: after resolving the conflicts, mark the corrected paths\n -\thint: with 'git add ' or 'git rm '\n -\thint: and commit the result with 'git commit'\n ++\terror: could not apply $picked... picked\n +\thint: Resolve all conflicts manually, mark them as resolved with\n -+\thint: \\\"git add/rm \\\", then run\n -+\thint: \\\"git cherry-pick --continue\\\".\n -+\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n -+\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n -+\thint: run \\\"git cherry-pick --abort\\\".\n ++\thint: \"git add/rm \", then run\n ++\thint: \"git cherry-pick --continue\".\n ++\thint: You can instead skip this commit: run \"git cherry-pick --skip\".\n ++\thint: To abort and get back to the state before \"git cherry-pick\",\n ++\thint: run \"git cherry-pick --abort\".\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n \n -@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-pick --no-commit' \"\n - \tpicked=\\$(git rev-parse --short picked) &&\n - \tcat <<-EOF >expected &&\n - \terror: could not apply \\$picked... picked\n --\thint: after resolving the conflicts, mark the corrected paths\n --\thint: with 'git add ' or 'git rm '\n -+\thint: Resolve all conflicts manually, mark them as resolved with\n -+\thint: \\\"git add/rm \\\", then run\n -+\thint: \\\"git cherry-pick --continue\\\".\n -+\thint: You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\n -+\thint: To abort and get back to the state before \\\"git cherry-pick\\\",\n -+\thint: run \\\"git cherry-pick --abort\\\".\n - \tEOF\n - \ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n + \ttest_cmp expected actual\n +-\"\n ++'\n \n + test_expect_success 'advice from failed cherry-pick --no-commit' \"\n + \tpristine_detach initial &&\n\n\n sequencer.c | 16 +++++++++++++++-\n t/t3501-revert-cherry-pick.sh | 20 ++++++++++++++++++++\n t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------\n 3 files changed, 45 insertions(+), 8 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..2dd73d24a87 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,\n \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n \n \tif (msg) {\n-\t\tfprintf(stderr, \"%s\\n\", msg);\n+\t\tadvise(\"%s\\n\", msg);\n \t\t/*\n \t\t * A conflict has occurred but the porcelain\n \t\t * (typically rebase --interactive) wants to take care\n@@ -418,6 +418,20 @@ static void print_advice(struct repository *r, int show_hint,\n \t\tif (opts->no_commit)\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n+\t\telse if (opts->action == REPLAY_PICK)\n+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n+\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n+\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n+\t\telse if (opts->action == REPLAY_REVERT)\n+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n+\t\t\t\t \"\\\"git revert --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit: run \\\"git revert --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git revert\\\",\\n\"\n+\t\t\t\t \"run \\\"git revert --abort\\\".\"));\n \t\telse\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\\n\"\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 9d100cd1884..6766aed7282 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -158,4 +158,24 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n \tgrep -q \"^modified$\" renamed\n '\n \n+test_expect_success 'advice from failed revert' '\n+\techo dream >dream &&\n+\tgit add dream &&\n+\tgit commit -m \"add dream\" &&\n+\tdream_oid=$(git rev-parse --short HEAD) &&\n+\tcat <<-EOF >expected &&\n+\terror: could not revert $dream_oid... add dream\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \"git add/rm \", then run\n+\thint: \"git revert --continue\".\n+\thint: You can instead skip this commit: run \"git revert --skip\".\n+\thint: To abort and get back to the state before \"git revert\",\n+\thint: run \"git revert --abort\".\n+\tEOF\n+\techo dream >>dream &&\n+\tgit add dream &&\n+\tgit commit -m \"double-add dream\" &&\n+\ttest_must_fail git revert HEAD^ 2>actual &&\n+\ttest_cmp expected actual\n+'\n test_done\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 014001b8f32..cb2ebea9ad3 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -47,20 +47,23 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '\n \ttest \"$head\" = \"$newhead\"\n '\n \n-test_expect_success 'advice from failed cherry-pick' \"\n+test_expect_success 'advice from failed cherry-pick' '\n \tpristine_detach initial &&\n \n-\tpicked=\\$(git rev-parse --short picked) &&\n+\tpicked=$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n-\terror: could not apply \\$picked... picked\n-\thint: after resolving the conflicts, mark the corrected paths\n-\thint: with 'git add ' or 'git rm '\n-\thint: and commit the result with 'git commit'\n+\terror: could not apply $picked... picked\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \"git add/rm \", then run\n+\thint: \"git cherry-pick --continue\".\n+\thint: You can instead skip this commit: run \"git cherry-pick --skip\".\n+\thint: To abort and get back to the state before \"git cherry-pick\",\n+\thint: run \"git cherry-pick --abort\".\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n \n \ttest_cmp expected actual\n-\"\n+'\n \n test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \tpristine_detach initial &&\n\nbase-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n-- \ngitgitgadget\n"},{"id":"432745","messageId":"xmqqa6lju5qd.fsf@gitster.g","threadId":"56187","inReplyTo":"pull.1010.v4.git.1628936863733.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] [GSOC] cherry-pick: use better advice message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-14T20:32:58Z","receivedAt":"2021-08-14T20:33:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> From: ZheNing Hu \n>\n> In the past, git cherry-pick would print such advice when\n> there was a conflict:\n> hint: after resolving the conflicts, mark the corrected paths\n> hint: with 'git add ' or 'git rm '\n> hint: and commit the result with 'git commit'\n\nIn our log messages, we desciribe the current state in the present\ntense.\n\nIt may be worth mentioning that this is because the program\noriginally was about picking only one commit and the hint was\ninherited from those days. Or it may not. I dunno.\n\n> But in fact, when we want to cherry-pick multiple commits,\n> we should not use \"git commit\" after resolving conflicts, which\n> will make Git generate some errors. We should recommend users to\n> use `git cherry-pick --continue`, `git cherry-pick --abort`, just\n> like git rebase does.\n\nI am not sure \"should not\" is the right phrase. Also it does not\nhelp readers to have a vague \"generate some errors\" than not saying\nanything---it leaves readers puzzled with \"what errors???\"\n\nWhether picking a single commit or a series of commits, after\nresolving conflicts in the current step, wouldn't\n\n $ git commit ;# to conclude the resolution\n $ git cherry-pick --continue\n\ndo the right thing?\n\n> This is the improved advice:\n\nIt may be an improved advice, but just omit it and say something\nlike:\n\n\tSuggest use of \"git cherry-pick --contiue\", so that it would\n\talso apply to cases where multiple commits are being picked.\n\nThe actual message does not have to be reproduced here, as it is in\nthe source, and it can be seen in the test ;-)\n\nThat would make the proposed log message conform to our norm,\ni.e. brief description of what happens with the current system,\nfollowed by description of the perceived problem, followed by\nan order to the codebase to become different in a specific way\nthat solves the problem.\n\nTaken together, perhaps\n\n\t\"git cherry-pick\", upon seeing a conflict, says:\n\n\t hint: ...\n\n\tas if running \"git commit\" to conclude the resolution of\n\tthis single step were the end of the story. This stems from\n\tthe fact that the command originally was to pick a single\n\tcommit and not a range of commits, and the message was\n\twritten back then and has not been adjusted.\n\n\tWhen picking a range of commits and the command stops with a\n\tconflict in the middle of the range, however, after\n\tresolving the conflict and (optionally) recording the result\n\twith \"git commit\", the user has to run \"git cherry-pick\n\t--continue\" to have the rest of the range dealt with,\n\t\"--skip\" to drop the current commit, or \"--abort\" to discard\n\tthe series.\n\n\tSuggest use of \"git cherry-pick --continue/--skip/--abort\"\n\tso that the message also covers the case where a range of\n\tcommits are being picked.\n\n\n> Helped-by: Phillip Wood \n> Hepled-by: Junio C Hamano \n\nIt seems that unlike other people I keep hepling you, whatever that\nverb means ;-).\n\nThanks.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 0bec01cf38e..2dd73d24a87 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,\n> \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n> \n> \tif (msg) {\n> -\t\tfprintf(stderr, \"%s\\n\", msg);\n> +\t\tadvise(\"%s\\n\", msg);\n> \t\t/*\n> \t\t * A conflict has occurred but the porcelain\n> \t\t * (typically rebase --interactive) wants to take care\n> @@ -418,6 +418,20 @@ static void print_advice(struct repository *r, int show_hint,\n> \t\tif (opts->no_commit)\n> \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> \t\t\t\t \"with 'git add ' or 'git rm '\"));\n> +\t\telse if (opts->action == REPLAY_PICK)\n> +\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n> +\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n> +\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n> +\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n> +\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n> +\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n> +\t\telse if (opts->action == REPLAY_REVERT)\n> +\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n> +\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n> +\t\t\t\t \"\\\"git revert --continue\\\".\\n\"\n> +\t\t\t\t \"You can instead skip this commit: run \\\"git revert --skip\\\".\\n\"\n> +\t\t\t\t \"To abort and get back to the state before \\\"git revert\\\",\\n\"\n> +\t\t\t\t \"run \\\"git revert --abort\\\".\"));\n> \t\telse\n> \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> \t\t\t\t \"with 'git add ' or 'git rm '\\n\"\n> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\n> index 9d100cd1884..6766aed7282 100755\n> --- a/t/t3501-revert-cherry-pick.sh\n> +++ b/t/t3501-revert-cherry-pick.sh\n> @@ -158,4 +158,24 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n> \tgrep -q \"^modified$\" renamed\n> '\n> \n> +test_expect_success 'advice from failed revert' '\n> +\techo dream >dream &&\n> +\tgit add dream &&\n> +\tgit commit -m \"add dream\" &&\n> +\tdream_oid=$(git rev-parse --short HEAD) &&\n> +\tcat <<-EOF >expected &&\n> +\terror: could not revert $dream_oid... add dream\n> +\thint: Resolve all conflicts manually, mark them as resolved with\n> +\thint: \"git add/rm \", then run\n> +\thint: \"git revert --continue\".\n> +\thint: You can instead skip this commit: run \"git revert --skip\".\n> +\thint: To abort and get back to the state before \"git revert\",\n> +\thint: run \"git revert --abort\".\n> +\tEOF\n> +\techo dream >>dream &&\n> +\tgit add dream &&\n> +\tgit commit -m \"double-add dream\" &&\n> +\ttest_must_fail git revert HEAD^ 2>actual &&\n> +\ttest_cmp expected actual\n> +'\n> test_done\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 014001b8f32..cb2ebea9ad3 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -47,20 +47,23 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '\n> \ttest \"$head\" = \"$newhead\"\n> '\n> \n> -test_expect_success 'advice from failed cherry-pick' \"\n> +test_expect_success 'advice from failed cherry-pick' '\n> \tpristine_detach initial &&\n> \n> -\tpicked=\\$(git rev-parse --short picked) &&\n> +\tpicked=$(git rev-parse --short picked) &&\n> \tcat <<-EOF >expected &&\n> -\terror: could not apply \\$picked... picked\n> -\thint: after resolving the conflicts, mark the corrected paths\n> -\thint: with 'git add ' or 'git rm '\n> -\thint: and commit the result with 'git commit'\n> +\terror: could not apply $picked... picked\n> +\thint: Resolve all conflicts manually, mark them as resolved with\n> +\thint: \"git add/rm \", then run\n> +\thint: \"git cherry-pick --continue\".\n> +\thint: You can instead skip this commit: run \"git cherry-pick --skip\".\n> +\thint: To abort and get back to the state before \"git cherry-pick\",\n> +\thint: run \"git cherry-pick --abort\".\n> \tEOF\n> \ttest_must_fail git cherry-pick picked 2>actual &&\n> \n> \ttest_cmp expected actual\n> -\"\n> +'\n> \n> test_expect_success 'advice from failed cherry-pick --no-commit' \"\n> \tpristine_detach initial &&\n>\n> base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n"},{"id":"432760","messageId":"CAOLTT8S6yOLuLh88wc6T1Wr2zJe=Wo2cYw=2ahOwAeN=e43Smw@mail.gmail.com","threadId":"56187","inReplyTo":"xmqqa6lju5qd.fsf@gitster.g","subject":"Re: [PATCH v4] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-15T12:48:54Z","receivedAt":"2021-08-15T12:49:28Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年8月15日周日 上午4:33写道：\n>\n> \"ZheNing Hu via GitGitGadget\" writes:\n>\n> > From: ZheNing Hu \n> >\n> > In the past, git cherry-pick would print such advice when\n> > there was a conflict:\n> > hint: after resolving the conflicts, mark the corrected paths\n> > hint: with 'git add ' or 'git rm '\n> > hint: and commit the result with 'git commit'\n>\n> In our log messages, we desciribe the current state in the present\n> tense.\n>\n> It may be worth mentioning that this is because the program\n> originally was about picking only one commit and the hint was\n> inherited from those days. Or it may not. I dunno.\n>\n\nYes, may look at the time, the design is correct.\n\n> > But in fact, when we want to cherry-pick multiple commits,\n> > we should not use \"git commit\" after resolving conflicts, which\n> > will make Git generate some errors. We should recommend users to\n> > use `git cherry-pick --continue`, `git cherry-pick --abort`, just\n> > like git rebase does.\n>\n> I am not sure \"should not\" is the right phrase. Also it does not\n> help readers to have a vague \"generate some errors\" than not saying\n> anything---it leaves readers puzzled with \"what errors???\"\n>\n\nyes, this error is specifically that if we want pick a series of commits,\nafter resolving conflicts and use git commit, `.git/sequencer` still exists,\nbut git cherry-pick has ended (the next step cherry-pick is not performed).\nThis may be a very strange critical state.\n\n> Whether picking a single commit or a series of commits, after\n> resolving conflicts in the current step, wouldn't\n>\n> $ git commit ;# to conclude the resolution\n> $ git cherry-pick --continue\n>\n> do the right thing?\n>\n\nI think you mean that git commit is used to end a single cherry-pick.\nBut in the face of a series of commits, errors will occur.\n\n> > This is the improved advice:\n>\n> It may be an improved advice, but just omit it and say something\n> like:\n>\n> Suggest use of \"git cherry-pick --contiue\", so that it would\n> also apply to cases where multiple commits are being picked.\n>\n> The actual message does not have to be reproduced here, as it is in\n> the source, and it can be seen in the test ;-)\n>\n\nYes, maybe the commit message should be shorter and more concise.\n\n> That would make the proposed log message conform to our norm,\n> i.e. brief description of what happens with the current system,\n> followed by description of the perceived problem, followed by\n> an order to the codebase to become different in a specific way\n> that solves the problem.\n>\n> Taken together, perhaps\n>\n> \"git cherry-pick\", upon seeing a conflict, says:\n>\n> hint: ...\n>\n> as if running \"git commit\" to conclude the resolution of\n> this single step were the end of the story. This stems from\n> the fact that the command originally was to pick a single\n> commit and not a range of commits, and the message was\n> written back then and has not been adjusted.\n>\n> When picking a range of commits and the command stops with a\n> conflict in the middle of the range, however, after\n> resolving the conflict and (optionally) recording the result\n> with \"git commit\", the user has to run \"git cherry-pick\n> --continue\" to have the rest of the range dealt with,\n> \"--skip\" to drop the current commit, or \"--abort\" to discard\n> the series.\n>\n\nOk, I understand now, git commit just stopped git cherry-pick by\ndelete CHERRY_PICK_HEAD in sequencer_post_commit_cleanup().\nwe can still resume the process with git cherry-pick --continue.\n\n> Suggest use of \"git cherry-pick --continue/--skip/--abort\"\n> so that the message also covers the case where a range of\n> commits are being picked.\n>\n>\n> > Helped-by: Phillip Wood \n> > Hepled-by: Junio C Hamano \n>\n> It seems that unlike other people I keep hepling you, whatever that\n> verb means ;-).\n>\n\nEhhh, I seem to have forgotten the correction here.\n\n> Thanks.\n>\n\nThanks.\n--\nZheNing Hu\n"},{"id":"432773","messageId":"pull.1010.v5.git.1629075306706.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":"pull.1010.v4.git.1628936863733.gitgitgadget@gmail.com","subject":"[PATCH v5] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-16T00:55:06Z","receivedAt":"2021-08-16T00:55:13Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\n\"git cherry-pick\", upon seeing a conflict, says:\n\nhint: after resolving the conflicts, mark the corrected paths\nhint: with 'git add ' or 'git rm '\nhint: and commit the result with 'git commit'\n\nas if running \"git commit\" to conclude the resolution of\nthis single step were the end of the story. This stems from\nthe fact that the command originally was to pick a single\ncommit and not a range of commits, and the message was\nwritten back then and has not been adjusted.\n\nWhen picking a range of commits and the command stops with a\nconflict in the middle of the range, however, after\nresolving the conflict and (optionally) recording the result\nwith \"git commit\", the user has to run \"git cherry-pick\n--continue\" to have the rest of the range dealt with,\n\"--skip\" to drop the current commit, or \"--abort\" to discard\nthe series.\n\nSuggest use of \"git cherry-pick --continue/--skip/--abort\"\nso that the message also covers the case where a range of\ncommits are being picked.\n\nSimilarly, this optimization can be applied to git revert,\nsuggest use of \"git revert --continue/--skip/--abort\" so\nthat the message also covers the case where a range of\ncommits are being reverted.\n\nIt is worth mentioning that now we use advice() to print\nthe content of GIT_CHERRY_PICK_HELP in print_advice(), each\nline of output will start with \"hint: \".\n\nMentored-by: Christian Couder \nMentored-by Hariom Verma :\nHelped-by: Phillip Wood \nHelped-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] cherry-pick: use better advice message\n \n The cherry-pick and revert advice message are only suitable for picking\n one commit or reverting one commit, but not for multiple commits. So\n correct the advice message to have the rest of the range dealt with.\n \n v7:\n https://lore.kernel.org/git/pull.1010.v4.git.1628936863733.gitgitgadget@gmail.com/\n \n v7-->v8:\n \n 1. Modify the commit message to make it more in line with the\n specification.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1010\n\nRange-diff vs v4:\n\n 1: dc51c0b8c2b ! 1: 6cf78ffd088 [GSOC] cherry-pick: use better advice message\n @@ Metadata\n ## Commit message ##\n [GSOC] cherry-pick: use better advice message\n \n - In the past, git cherry-pick would print such advice when\n - there was a conflict:\n + \"git cherry-pick\", upon seeing a conflict, says:\n \n hint: after resolving the conflicts, mark the corrected paths\n hint: with 'git add ' or 'git rm '\n hint: and commit the result with 'git commit'\n \n - But in fact, when we want to cherry-pick multiple commits,\n - we should not use \"git commit\" after resolving conflicts, which\n - will make Git generate some errors. We should recommend users to\n - use `git cherry-pick --continue`, `git cherry-pick --abort`, just\n - like git rebase does.\n + as if running \"git commit\" to conclude the resolution of\n + this single step were the end of the story. This stems from\n + the fact that the command originally was to pick a single\n + commit and not a range of commits, and the message was\n + written back then and has not been adjusted.\n \n - This is the improved advice:\n + When picking a range of commits and the command stops with a\n + conflict in the middle of the range, however, after\n + resolving the conflict and (optionally) recording the result\n + with \"git commit\", the user has to run \"git cherry-pick\n + --continue\" to have the rest of the range dealt with,\n + \"--skip\" to drop the current commit, or \"--abort\" to discard\n + the series.\n \n - hint: Resolve all conflicts manually, mark them as resolved with\n - hint: \"git add/rm \", then run\n - hint: \"git cherry-pick --continue\".\n - hint: You can instead skip this commit: run \"git cherry-pick --skip\".\n - hint: To abort and get back to the state before \"git cherry-pick\",\n - hint: run \"git cherry-pick --abort\".\n + Suggest use of \"git cherry-pick --continue/--skip/--abort\"\n + so that the message also covers the case where a range of\n + commits are being picked.\n \n - Similarly, this optimization can be applied to git revert:\n + Similarly, this optimization can be applied to git revert,\n + suggest use of \"git revert --continue/--skip/--abort\" so\n + that the message also covers the case where a range of\n + commits are being reverted.\n \n - hint: Resolve all conflicts manually, mark them as resolved with\n - hint: \"git add/rm \", then run\n - hint: \"git revert --continue\".\n - hint: You can instead skip this commit: run \"git revert --skip\".\n - hint: To abort and get back to the state before \"git revert\",\n - hint: run \"git revert --abort\".\n -\n - It is worth mentioning that now we use advice() to print the content\n - of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will\n - start with \"hint: \".\n + It is worth mentioning that now we use advice() to print\n + the content of GIT_CHERRY_PICK_HELP in print_advice(), each\n + line of output will start with \"hint: \".\n \n Mentored-by: Christian Couder \n Mentored-by Hariom Verma :\n Helped-by: Phillip Wood \n - Hepled-by: Junio C Hamano \n + Helped-by: Junio C Hamano \n Signed-off-by: ZheNing Hu \n \n ## sequencer.c ##\n\n\n sequencer.c | 16 +++++++++++++++-\n t/t3501-revert-cherry-pick.sh | 20 ++++++++++++++++++++\n t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------\n 3 files changed, 45 insertions(+), 8 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..2dd73d24a87 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,\n \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n \n \tif (msg) {\n-\t\tfprintf(stderr, \"%s\\n\", msg);\n+\t\tadvise(\"%s\\n\", msg);\n \t\t/*\n \t\t * A conflict has occurred but the porcelain\n \t\t * (typically rebase --interactive) wants to take care\n@@ -418,6 +418,20 @@ static void print_advice(struct repository *r, int show_hint,\n \t\tif (opts->no_commit)\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n+\t\telse if (opts->action == REPLAY_PICK)\n+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n+\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n+\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n+\t\telse if (opts->action == REPLAY_REVERT)\n+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n+\t\t\t\t \"\\\"git revert --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit: run \\\"git revert --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git revert\\\",\\n\"\n+\t\t\t\t \"run \\\"git revert --abort\\\".\"));\n \t\telse\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\\n\"\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 9d100cd1884..6766aed7282 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -158,4 +158,24 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n \tgrep -q \"^modified$\" renamed\n '\n \n+test_expect_success 'advice from failed revert' '\n+\techo dream >dream &&\n+\tgit add dream &&\n+\tgit commit -m \"add dream\" &&\n+\tdream_oid=$(git rev-parse --short HEAD) &&\n+\tcat <<-EOF >expected &&\n+\terror: could not revert $dream_oid... add dream\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \"git add/rm \", then run\n+\thint: \"git revert --continue\".\n+\thint: You can instead skip this commit: run \"git revert --skip\".\n+\thint: To abort and get back to the state before \"git revert\",\n+\thint: run \"git revert --abort\".\n+\tEOF\n+\techo dream >>dream &&\n+\tgit add dream &&\n+\tgit commit -m \"double-add dream\" &&\n+\ttest_must_fail git revert HEAD^ 2>actual &&\n+\ttest_cmp expected actual\n+'\n test_done\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 014001b8f32..cb2ebea9ad3 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -47,20 +47,23 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '\n \ttest \"$head\" = \"$newhead\"\n '\n \n-test_expect_success 'advice from failed cherry-pick' \"\n+test_expect_success 'advice from failed cherry-pick' '\n \tpristine_detach initial &&\n \n-\tpicked=\\$(git rev-parse --short picked) &&\n+\tpicked=$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n-\terror: could not apply \\$picked... picked\n-\thint: after resolving the conflicts, mark the corrected paths\n-\thint: with 'git add ' or 'git rm '\n-\thint: and commit the result with 'git commit'\n+\terror: could not apply $picked... picked\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \"git add/rm \", then run\n+\thint: \"git cherry-pick --continue\".\n+\thint: You can instead skip this commit: run \"git cherry-pick --skip\".\n+\thint: To abort and get back to the state before \"git cherry-pick\",\n+\thint: run \"git cherry-pick --abort\".\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n \n \ttest_cmp expected actual\n-\"\n+'\n \n test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \tpristine_detach initial &&\n\nbase-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n-- \ngitgitgadget\n"},{"id":"432935","messageId":"ed09ebd8-69f7-6a66-bd62-3c18b7021104@gmail.com","threadId":"56187","inReplyTo":"xmqqzgtlumob.fsf@gitster.g","subject":"Re: [PATCH v3] [GSOC] cherry-pick: use better advice message","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-17T10:09:40Z","receivedAt":"2021-08-17T10:09:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 13/08/2021 21:14, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>>> +++ b/t/t3507-cherry-pick-conflict.sh\n>>> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' \"\n>>> \tpicked=\\$(git rev-parse --short picked) &&\n>>> \tcat <<-EOF >expected &&\n>>\n>> If you quote the here doc end marker then there is no substitution in\n>> the here doc so writing\n>>\n>> \tcat <<-\\EOF >expected &&\n>>\n>>> \terror: could not apply \\$picked... picked\n>>> -\thint: after resolving the conflicts, mark the corrected paths\n>>> -\thint: with 'git add ' or 'git rm '\n>>> -\thint: and commit the result with 'git commit'\n>>> +\thint: Resolve all conflicts manually, mark them as resolved with\n>>> +\thint: \\\"git add/rm \\\", then run\n>>\n>> means you can replace \\\" with \" here\n> \n> Hmph, actually the double-quote that opens the body of\n> test_expect_success should be stared at with a very cautious eyes,\n> as they often do not mean what the author of the patch thought they\n> do. I can see that it already fooled your eyes into thinking that\n> there is no substitution, but $picked needs to be substituted into\n> its value. The backslash before it is *not* about guarding it from\n> substitution inside here-doc; it is to pass literal \"$\" into the\n> string, which is the last parameter to test_expect_success, that\n> gets eval'ed.\n\nGood point, you're right I had completely missed that, thanks for \npointing it out\n\nPhillip\n\n> \n> The original of this one, for example, would probably have been\n> better if written like so:\n> \n> test_expect_success 'advice from failed cherry-pick' '\n> \tpristine_detach initial &&\n> \tSQ='\\'' &&\n> \n> \tpicked=$(git rev-parse --short picked) &&\n> \tcat <<-EOF >expected &&\n> \terror: could not apply $picked... picked\n> \thint: after resolving the conflicts, mark the corrected paths\n> \thint: with ${SQ}git add ${SQ} or ${SQ}git rm ${SQ}\n> \thint: and commit the result with ${SQ}git commit${SQ}\n> \tEOF\n> \ttest_must_fail git cherry-pick picked 2>actual &&\n> \n> \ttest_cmp expected actual\n> '\n> \n> And because there is no single quote in the updated text, it would\n> become:\n> \n> test_expect_success 'advice from failed cherry-pick' '\n> \tpristine_detach initial &&\n> \n> \tpicked=$(git rev-parse --short picked) &&\n> \tcat <<-EOF >expected &&\n> \terror: could not apply $picked... picked\n> \thint: Resolve all conflicts manually, mark them as resolved with\n> \thint: \"git add/rm \", then run\n> \tEOF\n> \ttest_must_fail git cherry-pick picked 2>actual &&\n> \n> \ttest_cmp expected actual\n> '\n> \n> which makes it far easier to see that $picked needs to be\n> substituted, and the \"git add/rm\" are to be enclosed in dq pair.\n> \n\n"},{"id":"433055","messageId":"dcccd4af-e8b1-31e2-f271-4161ec4853d0@gmail.com","threadId":"56187","inReplyTo":"pull.1010.v5.git.1629075306706.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] [GSOC] cherry-pick: use better advice message","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-08-18T09:51:40Z","receivedAt":"2021-08-18T09:51:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/08/2021 01:55, ZheNing Hu via GitGitGadget wrote:\n> From: ZheNing Hu \n> [...]\n> sequencer.c | 16 +++++++++++++++-\n> t/t3501-revert-cherry-pick.sh | 20 ++++++++++++++++++++\n> t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------\n> 3 files changed, 45 insertions(+), 8 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 0bec01cf38e..2dd73d24a87 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,\n> \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n> \n> \tif (msg) {\n> -\t\tfprintf(stderr, \"%s\\n\", msg);\n> +\t\tadvise(\"%s\\n\", msg);\n> \t\t/*\n> \t\t * A conflict has occurred but the porcelain\n> \t\t * (typically rebase --interactive) wants to take care\n> @@ -418,6 +418,20 @@ static void print_advice(struct repository *r, int show_hint,\n> \t\tif (opts->no_commit)\n> \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> \t\t\t\t \"with 'git add ' or 'git rm '\"));\n> +\t\telse if (opts->action == REPLAY_PICK)\n> +\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n> +\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n> +\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n> +\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n> +\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n> +\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n> +\t\telse if (opts->action == REPLAY_REVERT)\n> +\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n> +\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n> +\t\t\t\t \"\\\"git revert --continue\\\".\\n\"\n> +\t\t\t\t \"You can instead skip this commit: run \\\"git revert --skip\\\".\\n\"\n> +\t\t\t\t \"To abort and get back to the state before \\\"git revert\\\",\\n\"\n> +\t\t\t\t \"run \\\"git revert --abort\\\".\"));\n\nThanks for making the revert advice better as well\n\n> \t\telse\n> \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> \t\t\t\t \"with 'git add ' or 'git rm '\\n\"\n\nI think this last else arm should probably be a bug now as anything \nother than cherry-pick or revert should be setting GIT_CHERRY_PICK_HELP\n\n\telse\n\t\tBUG(\"unexpected pick action in print_advice()\");\n\n> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\n> index 9d100cd1884..6766aed7282 100755\n> --- a/t/t3501-revert-cherry-pick.sh\n> +++ b/t/t3501-revert-cherry-pick.sh\n> @@ -158,4 +158,24 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n> \tgrep -q \"^modified$\" renamed\n> '\n> \n> +test_expect_success 'advice from failed revert' '\n> +\techo dream >dream &&\n> +\tgit add dream &&\n> +\tgit commit -m \"add dream\" &&\n\nA minor comment: you can condense these three lines by using test_commit \n(see test-lib-functions.sh for the documentation)\n\n\ttest_commit \"add dream\" dream dream\n\n> +\tdream_oid=$(git rev-parse --short HEAD) &&\n> +\tcat <<-EOF >expected &&\n> +\terror: could not revert $dream_oid... add dream\n> +\thint: Resolve all conflicts manually, mark them as resolved with\n> +\thint: \"git add/rm \", then run\n> +\thint: \"git revert --continue\".\n> +\thint: You can instead skip this commit: run \"git revert --skip\".\n> +\thint: To abort and get back to the state before \"git revert\",\n> +\thint: run \"git revert --abort\".\n> +\tEOF\n> +\techo dream >>dream &&\n> +\tgit add dream &&\n> +\tgit commit -m \"double-add dream\" &&\n\ntest_commit can also append to a file\n\n\ttest_commit --append \"double-add dream\" dream dream\n\nThis is looking very close to being ready now I think\n\nThanks\n\nPhillip\n\n> +\ttest_must_fail git revert HEAD^ 2>actual &&\n> +\ttest_cmp expected actual\n> +'\n> test_done\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 014001b8f32..cb2ebea9ad3 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -47,20 +47,23 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '\n> \ttest \"$head\" = \"$newhead\"\n> '\n> \n> -test_expect_success 'advice from failed cherry-pick' \"\n> +test_expect_success 'advice from failed cherry-pick' '\n> \tpristine_detach initial &&\n> \n> -\tpicked=\\$(git rev-parse --short picked) &&\n> +\tpicked=$(git rev-parse --short picked) &&\n> \tcat <<-EOF >expected &&\n> -\terror: could not apply \\$picked... picked\n> -\thint: after resolving the conflicts, mark the corrected paths\n> -\thint: with 'git add ' or 'git rm '\n> -\thint: and commit the result with 'git commit'\n> +\terror: could not apply $picked... picked\n> +\thint: Resolve all conflicts manually, mark them as resolved with\n> +\thint: \"git add/rm \", then run\n> +\thint: \"git cherry-pick --continue\".\n> +\thint: You can instead skip this commit: run \"git cherry-pick --skip\".\n> +\thint: To abort and get back to the state before \"git cherry-pick\",\n> +\thint: run \"git cherry-pick --abort\".\n> \tEOF\n> \ttest_must_fail git cherry-pick picked 2>actual &&\n> \n> \ttest_cmp expected actual\n> -\"\n> +'\n> \n> test_expect_success 'advice from failed cherry-pick --no-commit' \"\n> \tpristine_detach initial &&\n> \n> base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n> \n\n"},{"id":"433098","messageId":"CAOLTT8T0A5_5sSxpJBSLgbbCWW61q6tVSesAAzf5SrhDxf1bcA@mail.gmail.com","threadId":"56187","inReplyTo":"dcccd4af-e8b1-31e2-f271-4161ec4853d0@gmail.com","subject":"Re: [PATCH v5] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-19T01:55:03Z","receivedAt":"2021-08-19T01:55:17Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Phillip Wood 于2021年8月18日周三 下午5:51写道：\n>\n> Thanks for making the revert advice better as well\n>\n\nThanks for reviewing too.\n\n> > else\n> > advise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> > \"with 'git add ' or 'git rm '\\n\"\n>\n> I think this last else arm should probably be a bug now as anything\n> other than cherry-pick or revert should be setting GIT_CHERRY_PICK_HELP\n>\n> else\n> BUG(\"unexpected pick action in print_advice()\");\n>\n\nMaybe you are right, If no one else will touch it, it may be\nreasonable to set it as BUG().\n\n> > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\n> > index 9d100cd1884..6766aed7282 100755\n> > --- a/t/t3501-revert-cherry-pick.sh\n> > +++ b/t/t3501-revert-cherry-pick.sh\n> > @@ -158,4 +158,24 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n> > grep -q \"^modified$\" renamed\n> > '\n> >\n> > +test_expect_success 'advice from failed revert' '\n> > + echo dream >dream &&\n> > + git add dream &&\n> > + git commit -m \"add dream\" &&\n>\n> A minor comment: you can condense these three lines by using test_commit\n> (see test-lib-functions.sh for the documentation)\n>\n> test_commit \"add dream\" dream dream\n>\n\nThanks, I may also need a --no-tag option, because \"add dream\" is not a valid\ntag name.\n\n> > + dream_oid=$(git rev-parse --short HEAD) &&\n> > + cat <<-EOF >expected &&\n> > + error: could not revert $dream_oid... add dream\n> > + hint: Resolve all conflicts manually, mark them as resolved with\n> > + hint: \"git add/rm \", then run\n> > + hint: \"git revert --continue\".\n> > + hint: You can instead skip this commit: run \"git revert --skip\".\n> > + hint: To abort and get back to the state before \"git revert\",\n> > + hint: run \"git revert --abort\".\n> > + EOF\n> > + echo dream >>dream &&\n> > + git add dream &&\n> > + git commit -m \"double-add dream\" &&\n>\n> test_commit can also append to a file\n>\n> test_commit --append \"double-add dream\" dream dream\n>\n\nSame too, test_commit --append --no-tag \"double-add dream\" dream dream\n\n> This is looking very close to being ready now I think\n>\n> Thanks\n>\n> Phillip\n>\n\nThanks.\n--\nZheNing Hu\n"},{"id":"433099","messageId":"CAOLTT8QuL-yws77-N9j6Qu2r_1ALPiYasj+6EO_J0v0QcmcCmw@mail.gmail.com","threadId":"56187","inReplyTo":"CAOLTT8T0A5_5sSxpJBSLgbbCWW61q6tVSesAAzf5SrhDxf1bcA@mail.gmail.com","subject":"Re: [PATCH v5] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-19T02:07:23Z","receivedAt":"2021-08-19T02:07:50Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"ZheNing Hu 于2021年8月19日周四 上午9:55写道：\n>\n> Phillip Wood 于2021年8月18日周三 下午5:51写道：\n> >\n> > Thanks for making the revert advice better as well\n> >\n>\n> Thanks for reviewing too.\n>\n> > > else\n> > > advise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> > > \"with 'git add ' or 'git rm '\\n\"\n> >\n> > I think this last else arm should probably be a bug now as anything\n> > other than cherry-pick or revert should be setting GIT_CHERRY_PICK_HELP\n> >\n\nGIT_CHERRY_PICK_HELP should be set for other commands except\ncherry-pick and revert.\n\n> > else\n> > BUG(\"unexpected pick action in print_advice()\");\n> >\n>\n> Maybe you are right, If no one else will touch it, it may be\n> reasonable to set it as BUG().\n>\n\n--\nZheNing\n"},{"id":"433110","messageId":"pull.1010.v6.git.1629352277151.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":"pull.1010.v5.git.1629075306706.gitgitgadget@gmail.com","subject":"[PATCH v6] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-19T05:51:16Z","receivedAt":"2021-08-19T05:51:22Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\n\"git cherry-pick\", upon seeing a conflict, says:\n\nhint: after resolving the conflicts, mark the corrected paths\nhint: with 'git add ' or 'git rm '\nhint: and commit the result with 'git commit'\n\nas if running \"git commit\" to conclude the resolution of\nthis single step were the end of the story. This stems from\nthe fact that the command originally was to pick a single\ncommit and not a range of commits, and the message was\nwritten back then and has not been adjusted.\n\nWhen picking a range of commits and the command stops with a\nconflict in the middle of the range, however, after\nresolving the conflict and (optionally) recording the result\nwith \"git commit\", the user has to run \"git cherry-pick\n--continue\" to have the rest of the range dealt with,\n\"--skip\" to drop the current commit, or \"--abort\" to discard\nthe series.\n\nSuggest use of \"git cherry-pick --continue/--skip/--abort\"\nso that the message also covers the case where a range of\ncommits are being picked.\n\nSimilarly, this optimization can be applied to git revert,\nsuggest use of \"git revert --continue/--skip/--abort\" so\nthat the message also covers the case where a range of\ncommits are being reverted.\n\nIt is worth mentioning that now we use advice() to print\nthe content of GIT_CHERRY_PICK_HELP in print_advice(), each\nline of output will start with \"hint: \".\n\nMentored-by: Christian Couder \nMentored-by Hariom Verma \nHelped-by: Phillip Wood \nHelped-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] cherry-pick: use better advice message\n \n The cherry-pick and revert advice message are only suitable for picking\n one commit or reverting one commit, but not for multiple commits. So\n correct the advice message to have the rest of the range dealt with.\n \n v8:\n https://lore.kernel.org/git/pull.1010.v5.git.1629075306706.gitgitgadget@gmail.com/\n \n v8-->v9:\n \n 1. Use the test_commit function in the test.\n 2. Treat these commands which are not using cherry-pick or revert and\n without setting GIT_CHERRY_PICK_HELP as a BUG.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v6\nPull-Request: https://github.com/gitgitgadget/git/pull/1010\n\nRange-diff vs v5:\n\n 1: 6cf78ffd088 ! 1: 84676c475ee [GSOC] cherry-pick: use better advice message\n @@ Commit message\n line of output will start with \"hint: \".\n \n Mentored-by: Christian Couder \n - Mentored-by Hariom Verma :\n + Mentored-by Hariom Verma \n Helped-by: Phillip Wood \n Helped-by: Junio C Hamano \n Signed-off-by: ZheNing Hu \n @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,\n +\t\t\t\t \"To abort and get back to the state before \\\"git revert\\\",\\n\"\n +\t\t\t\t \"run \\\"git revert --abort\\\".\"));\n \t\telse\n - \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n - \t\t\t\t \"with 'git add ' or 'git rm '\\n\"\n +-\t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n +-\t\t\t\t \"with 'git add ' or 'git rm '\\n\"\n +-\t\t\t\t \"and commit the result with 'git commit'\"));\n ++\t\t\tBUG(\"unexpected pick action in print_advice()\");\n + \t}\n + }\n + \n \n ## t/t3501-revert-cherry-pick.sh ##\n @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty renamed file' '\n @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty\n '\n \n +test_expect_success 'advice from failed revert' '\n -+\techo dream >dream &&\n -+\tgit add dream &&\n -+\tgit commit -m \"add dream\" &&\n ++\ttest_commit --no-tag \"add dream\" dream dream &&\n +\tdream_oid=$(git rev-parse --short HEAD) &&\n +\tcat <<-EOF >expected &&\n +\terror: could not revert $dream_oid... add dream\n @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty\n +\thint: To abort and get back to the state before \"git revert\",\n +\thint: run \"git revert --abort\".\n +\tEOF\n -+\techo dream >>dream &&\n -+\tgit add dream &&\n -+\tgit commit -m \"double-add dream\" &&\n ++\ttest_commit --append --no-tag \"double-add dream\" dream dream &&\n +\ttest_must_fail git revert HEAD^ 2>actual &&\n +\ttest_cmp expected actual\n +'\n\n\n sequencer.c | 20 ++++++++++++++++----\n t/t3501-revert-cherry-pick.sh | 16 ++++++++++++++++\n t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------\n 3 files changed, 42 insertions(+), 11 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..f114b4ba80e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,\n \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n \n \tif (msg) {\n-\t\tfprintf(stderr, \"%s\\n\", msg);\n+\t\tadvise(\"%s\\n\", msg);\n \t\t/*\n \t\t * A conflict has occurred but the porcelain\n \t\t * (typically rebase --interactive) wants to take care\n@@ -418,10 +418,22 @@ static void print_advice(struct repository *r, int show_hint,\n \t\tif (opts->no_commit)\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n+\t\telse if (opts->action == REPLAY_PICK)\n+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n+\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n+\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n+\t\telse if (opts->action == REPLAY_REVERT)\n+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n+\t\t\t\t \"\\\"git revert --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit: run \\\"git revert --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git revert\\\",\\n\"\n+\t\t\t\t \"run \\\"git revert --abort\\\".\"));\n \t\telse\n-\t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n-\t\t\t\t \"with 'git add ' or 'git rm '\\n\"\n-\t\t\t\t \"and commit the result with 'git commit'\"));\n+\t\t\tBUG(\"unexpected pick action in print_advice()\");\n \t}\n }\n \ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 9d100cd1884..e70a33f3e60 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -158,4 +158,20 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n \tgrep -q \"^modified$\" renamed\n '\n \n+test_expect_success 'advice from failed revert' '\n+\ttest_commit --no-tag \"add dream\" dream dream &&\n+\tdream_oid=$(git rev-parse --short HEAD) &&\n+\tcat <<-EOF >expected &&\n+\terror: could not revert $dream_oid... add dream\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \"git add/rm \", then run\n+\thint: \"git revert --continue\".\n+\thint: You can instead skip this commit: run \"git revert --skip\".\n+\thint: To abort and get back to the state before \"git revert\",\n+\thint: run \"git revert --abort\".\n+\tEOF\n+\ttest_commit --append --no-tag \"double-add dream\" dream dream &&\n+\ttest_must_fail git revert HEAD^ 2>actual &&\n+\ttest_cmp expected actual\n+'\n test_done\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 014001b8f32..cb2ebea9ad3 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -47,20 +47,23 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '\n \ttest \"$head\" = \"$newhead\"\n '\n \n-test_expect_success 'advice from failed cherry-pick' \"\n+test_expect_success 'advice from failed cherry-pick' '\n \tpristine_detach initial &&\n \n-\tpicked=\\$(git rev-parse --short picked) &&\n+\tpicked=$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n-\terror: could not apply \\$picked... picked\n-\thint: after resolving the conflicts, mark the corrected paths\n-\thint: with 'git add ' or 'git rm '\n-\thint: and commit the result with 'git commit'\n+\terror: could not apply $picked... picked\n+\thint: Resolve all conflicts manually, mark them as resolved with\n+\thint: \"git add/rm \", then run\n+\thint: \"git cherry-pick --continue\".\n+\thint: You can instead skip this commit: run \"git cherry-pick --skip\".\n+\thint: To abort and get back to the state before \"git cherry-pick\",\n+\thint: run \"git cherry-pick --abort\".\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n \n \ttest_cmp expected actual\n-\"\n+'\n \n test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \tpristine_detach initial &&\n\nbase-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n-- \ngitgitgadget\n"},{"id":"433133","messageId":"xmqqlf4x2weh.fsf@gitster.g","threadId":"56187","inReplyTo":"pull.1010.v6.git.1629352277151.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] [GSOC] cherry-pick: use better advice message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-19T17:10:14Z","receivedAt":"2021-08-19T17:10:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> Mentored-by: Christian Couder \n> - Mentored-by Hariom Verma :\n> + Mentored-by Hariom Verma \n\nI think you meant to move ':' not remove it.\n\n> +\t\telse if (opts->action == REPLAY_PICK)\n> +\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n\nDo we need to say \"manually\"? Dropping the word would make the\nmessage more concise. Not repeating the word \"resolve\" by saying\nsomething like \"after resolving the conflicts, mark them with ...\"\nwould probably be a much better phrasing, too.\n\n> +\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n\n (or ) not ; you can resolve many\nfiles in a directory and give the pathspec to match the directory to\nmark all the files in there as resolved.\n\n> +\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n> +\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n\nInconsistent use of prose in the above and \": run\" here.\n\n You can instead skip this commit with \"git cherry-pick --skip\"\n\n> +\t\telse if (opts->action == REPLAY_REVERT)\n\nLikewise.\n\nThanks.\n"},{"id":"433293","messageId":"CAOLTT8T3=GCvkdiXvO9wprXRoDJ58P_CZBjJ2YDQVHv_eCiXqw@mail.gmail.com","threadId":"56187","inReplyTo":"xmqqlf4x2weh.fsf@gitster.g","subject":"Re: [PATCH v6] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-21T01:40:27Z","receivedAt":"2021-08-21T01:40:39Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年8月20日周五 上午1:10写道：\n>\n> > + else if (opts->action == REPLAY_PICK)\n> > + advise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n>\n> Do we need to say \"manually\"? Dropping the word would make the\n> message more concise. Not repeating the word \"resolve\" by saying\n> something like \"after resolving the conflicts, mark them with ...\"\n> would probably be a much better phrasing, too.\n>\n\nYeah. Note the $reslovemsg in git-rebase--preserve-merges.sh, I copied\nit from here. Perhaps it is also worth modifying?\n\n> > + \"\\\"git add/rm \\\", then run\\n\"\n>\n> (or ) not ; you can resolve many\n> files in a directory and give the pathspec to match the directory to\n> mark all the files in there as resolved.\n>\n\nIndeed so.\n\n> > + \"\\\"git cherry-pick --continue\\\".\\n\"\n> > + \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n>\n> Inconsistent use of prose in the above and \": run\" here.\n>\n> You can instead skip this commit with \"git cherry-pick --skip\"\n>\n> > + else if (opts->action == REPLAY_REVERT)\n>\n> Likewise.\n>\n> Thanks.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"433322","messageId":"pull.1010.v7.git.1629637721426.gitgitgadget@gmail.com","threadId":"56187","inReplyTo":"pull.1010.v6.git.1629352277151.gitgitgadget@gmail.com","subject":"[PATCH v7] [GSOC] cherry-pick: use better advice message","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-22T13:08:41Z","receivedAt":"2021-08-22T13:09:18Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\n\"git cherry-pick\", upon seeing a conflict, says:\n\nhint: after resolving the conflicts, mark the corrected paths\nhint: with 'git add ' or 'git rm '\nhint: and commit the result with 'git commit'\n\nas if running \"git commit\" to conclude the resolution of\nthis single step were the end of the story. This stems from\nthe fact that the command originally was to pick a single\ncommit and not a range of commits, and the message was\nwritten back then and has not been adjusted.\n\nWhen picking a range of commits and the command stops with a\nconflict in the middle of the range, however, after\nresolving the conflict and (optionally) recording the result\nwith \"git commit\", the user has to run \"git cherry-pick\n--continue\" to have the rest of the range dealt with,\n\"--skip\" to drop the current commit, or \"--abort\" to discard\nthe series.\n\nSuggest use of \"git cherry-pick --continue/--skip/--abort\"\nso that the message also covers the case where a range of\ncommits are being picked.\n\nSimilarly, this optimization can be applied to git revert,\nsuggest use of \"git revert --continue/--skip/--abort\" so\nthat the message also covers the case where a range of\ncommits are being reverted.\n\nIt is worth mentioning that now we use advice() to print\nthe content of GIT_CHERRY_PICK_HELP in print_advice(), each\nline of output will start with \"hint: \".\n\nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nHelped-by: Phillip Wood \nHelped-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] cherry-pick: use better advice message\n \n The cherry-pick and revert advice message are only suitable for picking\n one commit or reverting one commit, but not for multiple commits. So\n correct the advice message to have the rest of the range dealt with.\n \n v9:\n https://lore.kernel.org/git/pull.1010.v6.git.1629352277151.gitgitgadget@gmail.com/\n \n v9-->v10:\n \n 1. Correct the wording of the advice message.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v7\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v7\nPull-Request: https://github.com/gitgitgadget/git/pull/1010\n\nRange-diff vs v6:\n\n 1: 84676c475ee ! 1: 07440ea5ffe [GSOC] cherry-pick: use better advice message\n @@ Commit message\n line of output will start with \"hint: \".\n \n Mentored-by: Christian Couder \n - Mentored-by Hariom Verma \n + Mentored-by: Hariom Verma \n Helped-by: Phillip Wood \n Helped-by: Junio C Hamano \n Signed-off-by: ZheNing Hu \n @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n +\t\telse if (opts->action == REPLAY_PICK)\n -+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n -+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n ++\t\t\tadvise(_(\"After resolving the conflicts, mark them with\\n\"\n ++\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n +\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n -+\t\t\t\t \"You can instead skip this commit: run \\\"git cherry-pick --skip\\\".\\n\"\n ++\t\t\t\t \"You can instead skip this commit with \\\"git cherry-pick --skip\\\".\\n\"\n +\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n +\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n +\t\telse if (opts->action == REPLAY_REVERT)\n -+\t\t\tadvise(_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n -+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n ++\t\t\tadvise(_(\"After resolving the conflicts, mark them with\\n\"\n ++\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n +\t\t\t\t \"\\\"git revert --continue\\\".\\n\"\n -+\t\t\t\t \"You can instead skip this commit: run \\\"git revert --skip\\\".\\n\"\n ++\t\t\t\t \"You can instead skip this commit with \\\"git revert --skip\\\".\\n\"\n +\t\t\t\t \"To abort and get back to the state before \\\"git revert\\\",\\n\"\n +\t\t\t\t \"run \\\"git revert --abort\\\".\"));\n \t\telse\n @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty\n +\tdream_oid=$(git rev-parse --short HEAD) &&\n +\tcat <<-EOF >expected &&\n +\terror: could not revert $dream_oid... add dream\n -+\thint: Resolve all conflicts manually, mark them as resolved with\n -+\thint: \"git add/rm \", then run\n ++\thint: After resolving the conflicts, mark them with\n ++\thint: \"git add/rm \", then run\n +\thint: \"git revert --continue\".\n -+\thint: You can instead skip this commit: run \"git revert --skip\".\n ++\thint: You can instead skip this commit with \"git revert --skip\".\n +\thint: To abort and get back to the state before \"git revert\",\n +\thint: run \"git revert --abort\".\n +\tEOF\n @@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'failed cherry-pick does no\n -\thint: with 'git add ' or 'git rm '\n -\thint: and commit the result with 'git commit'\n +\terror: could not apply $picked... picked\n -+\thint: Resolve all conflicts manually, mark them as resolved with\n -+\thint: \"git add/rm \", then run\n ++\thint: After resolving the conflicts, mark them with\n ++\thint: \"git add/rm \", then run\n +\thint: \"git cherry-pick --continue\".\n -+\thint: You can instead skip this commit: run \"git cherry-pick --skip\".\n ++\thint: You can instead skip this commit with \"git cherry-pick --skip\".\n +\thint: To abort and get back to the state before \"git cherry-pick\",\n +\thint: run \"git cherry-pick --abort\".\n \tEOF\n\n\n sequencer.c | 20 ++++++++++++++++----\n t/t3501-revert-cherry-pick.sh | 16 ++++++++++++++++\n t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------\n 3 files changed, 42 insertions(+), 11 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..03c7b3dcadc 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,\n \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n \n \tif (msg) {\n-\t\tfprintf(stderr, \"%s\\n\", msg);\n+\t\tadvise(\"%s\\n\", msg);\n \t\t/*\n \t\t * A conflict has occurred but the porcelain\n \t\t * (typically rebase --interactive) wants to take care\n@@ -418,10 +418,22 @@ static void print_advice(struct repository *r, int show_hint,\n \t\tif (opts->no_commit)\n \t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n \t\t\t\t \"with 'git add ' or 'git rm '\"));\n+\t\telse if (opts->action == REPLAY_PICK)\n+\t\t\tadvise(_(\"After resolving the conflicts, mark them with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n+\t\t\t\t \"\\\"git cherry-pick --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit with \\\"git cherry-pick --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git cherry-pick\\\",\\n\"\n+\t\t\t\t \"run \\\"git cherry-pick --abort\\\".\"));\n+\t\telse if (opts->action == REPLAY_REVERT)\n+\t\t\tadvise(_(\"After resolving the conflicts, mark them with\\n\"\n+\t\t\t\t \"\\\"git add/rm \\\", then run\\n\"\n+\t\t\t\t \"\\\"git revert --continue\\\".\\n\"\n+\t\t\t\t \"You can instead skip this commit with \\\"git revert --skip\\\".\\n\"\n+\t\t\t\t \"To abort and get back to the state before \\\"git revert\\\",\\n\"\n+\t\t\t\t \"run \\\"git revert --abort\\\".\"));\n \t\telse\n-\t\t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n-\t\t\t\t \"with 'git add ' or 'git rm '\\n\"\n-\t\t\t\t \"and commit the result with 'git commit'\"));\n+\t\t\tBUG(\"unexpected pick action in print_advice()\");\n \t}\n }\n \ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 9d100cd1884..4b5b6076733 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -158,4 +158,20 @@ test_expect_success 'cherry-pick works with dirty renamed file' '\n \tgrep -q \"^modified$\" renamed\n '\n \n+test_expect_success 'advice from failed revert' '\n+\ttest_commit --no-tag \"add dream\" dream dream &&\n+\tdream_oid=$(git rev-parse --short HEAD) &&\n+\tcat <<-EOF >expected &&\n+\terror: could not revert $dream_oid... add dream\n+\thint: After resolving the conflicts, mark them with\n+\thint: \"git add/rm \", then run\n+\thint: \"git revert --continue\".\n+\thint: You can instead skip this commit with \"git revert --skip\".\n+\thint: To abort and get back to the state before \"git revert\",\n+\thint: run \"git revert --abort\".\n+\tEOF\n+\ttest_commit --append --no-tag \"double-add dream\" dream dream &&\n+\ttest_must_fail git revert HEAD^ 2>actual &&\n+\ttest_cmp expected actual\n+'\n test_done\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 014001b8f32..979e843c65a 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -47,20 +47,23 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '\n \ttest \"$head\" = \"$newhead\"\n '\n \n-test_expect_success 'advice from failed cherry-pick' \"\n+test_expect_success 'advice from failed cherry-pick' '\n \tpristine_detach initial &&\n \n-\tpicked=\\$(git rev-parse --short picked) &&\n+\tpicked=$(git rev-parse --short picked) &&\n \tcat <<-EOF >expected &&\n-\terror: could not apply \\$picked... picked\n-\thint: after resolving the conflicts, mark the corrected paths\n-\thint: with 'git add ' or 'git rm '\n-\thint: and commit the result with 'git commit'\n+\terror: could not apply $picked... picked\n+\thint: After resolving the conflicts, mark them with\n+\thint: \"git add/rm \", then run\n+\thint: \"git cherry-pick --continue\".\n+\thint: You can instead skip this commit with \"git cherry-pick --skip\".\n+\thint: To abort and get back to the state before \"git cherry-pick\",\n+\thint: run \"git cherry-pick --abort\".\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n \n \ttest_cmp expected actual\n-\"\n+'\n \n test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \tpristine_detach initial &&\n\nbase-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006\n-- \ngitgitgadget\n"}]}