patch, 2 parts[GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

This patch fixes the bug when git cherry-pick is used with environment variable GIT_CHERRY_PICK_HELP, and makes git chery-pick advice message better.

v3: https://lore.kernel.org/git/pull.1007.git.1627561665.gitgitgadget@gmail.com/

v3-->v4:

 1. Add hidden option --delete-cherry-pick-head to git cherry-pick wihch
    used to delete CHERRY_PICK_HEAD when conflict occurs.
 2. add delete_cherry_pick_head flag to struct replay_opts and struct
    rebase_options.
 3. Split print_advice() into print advice and delete CHERRY_PICK_HEAD two
    part.
 4. Use better git cherry-pick advice message.

ZheNing Hu (2):
  [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP
  [GSOC] cherry-pick: use better advice message

 builtin/rebase.c                |  3 +++
 builtin/revert.c                |  2 ++
 git-rebase--preserve-merges.sh  |  2 +-
 sequencer.c                     | 37 +++++++++++++++++-------------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 40 ++++++++++++++++++++-------------
 6 files changed, 53 insertions(+), 32 deletions(-)

base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

From: ZheNing Hu <adlternative@gmail.com>

In the past, git cherry-pick would print such advice when there was a conflict:

hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit'

But in fact, when we want to cherry-pick multiple commits, we should not use "git commit" after resolving conflicts, which will make Git generate some errors. We should recommend users to use `git cherry-pick --continue`, `git cherry-pick --abort`, just like git rebase does.

This is the improved advice:

hint: Resolve all conflicts manually, mark them as resolved with hint: "git add/rm <conflicted_files>", then run "git cherry-pick \ --continue". hint: You can instead skip this commit: run "git cherry-pick --skip". hint: To abort and get back to the state before "git cherry-pick", hint: run "git cherry-pick --abort".

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 sequencer.c                     |  9 ++++++++-
 t/t3507-cherry-pick-conflict.sh | 15 ++++++++++-----
 2 files changed, 18 insertions(+), 6 deletions(-)

On 31/07/2021 08:01, ZheNing Hu via GitGitGadget wrote:

This new wording matches what we have for rebase which is good, I am slightly worried that the lines end up being quite long though they are just under 80 characters. It might be worth splitting the line that mentions running "git cherry-pick --continue" so it is a bit shorter.

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> 于2021年8月1日周日 下午6:14写道：

Agree.

Thanks. -- ZheNing Hu

From: ZheNing Hu <adlternative@gmail.com>

GIT_CHERRY_PICK_HELP is an environment variable, as the implementation detail of some porcelain in git to help realize the rebasing steps. E.g. `git rebase -p` set GIT_CHERRY_PICK_HELP value in `git-rebase--preserve-merges.sh`, `git rebase --merge` set GIT_CHERRY_PICK_HELP value in run_specific_rebase(). But If we set the value of GIT_CHERRY_PICK_HELP when using `git cherry-pick`, CHERRY_PICK_HEAD will be deleted, then we will get an error when we try to use `git cherry-pick --continue` or other cherr-pick command.

Introduce new "hidden" option `--delete-cherry-pick-head` for git cherry-pick which indicates that CHERRY_PICK_HEAD will be deleted when conflict occurs, which provided for some porcelain commands of git like `git-rebase--preserve-merges.sh`. After `git rebase -p` completely abolished, this option should be removed. At the same time, add the flag `delete_cherry_pick_head` to `struct rebase_options` and `struct replay_opts`, We can decide whether to delete CHERRY_PICK_HEAD by setting, passing, and checking this flag bit.

Then we split print_advice() into two part: Firstly, print_advice() will only be responsible for outputting content; Secondly, check if we set the `delete_cherry_pick_head` flag; if set, delete CHERRY_PICK_HEAD. In this way, the steps of printing advice and deleting CHERRY_PICK_HEAD are decoupled. Finally, let `git-rebase--preserve-merges.sh` use the `--delete-cherry-pick-head` option when it executes git cherry-pick, and set the `delete_cherry_pick_head` flag in run_specific_rebase() when we are using `git rebase --merge`, which can fix this breakage.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/rebase.c                |  3 +++
 builtin/revert.c                |  2 ++
 git-rebase--preserve-merges.sh  |  2 +-
 sequencer.c                     | 28 +++++++++++++---------------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 25 +++++++++++++++----------
 6 files changed, 35 insertions(+), 26 deletions(-)

On 31/07/2021 08:01, ZheNing Hu via GitGitGadget wrote:

I think we could just have
	replay.delete_cherry_pick_head = 1;
and not add a new member to rebase_options as we always want this set

This is a change in behavior as the advice will now be prefixed with "hint: ". I think that is probably an improvement so long as it does not make the lines too long when the advice is printed.

I think it would be useful to add a test for the new cherry-pick option that is added in this patch. Overall this patch is looking pretty good it just needs a few small changes, thanks for working on it.

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> 于2021年8月1日周日 下午6:10写道：

Maybe you are right.

Yeah, maybe I should mention this in commit messages.

Make sence.

Thanks. -- ZheNing Hu

This patch fixes the bug when git cherry-pick is used with environment variable GIT_CHERRY_PICK_HELP, and makes git chery-pick advice message better.

v4: https://lore.kernel.org/git/pull.1010.git.1627714877.gitgitgadget@gmail.com/

v4-->v5:

 1. Delete struct rebase_options member delete_cherry_pick_head, and set the
    delete_cherry_pick_head in struct replay_opts replay to 1 in
    get_replay_opts().
 2. Avoid too long line length of advice of cherry-pick, split "git
    cherry-pick --continue" in advice to new line.

ZheNing Hu (2):
  [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP
  [GSOC] cherry-pick: use better advice message

 builtin/rebase.c                |  1 +
 builtin/revert.c                |  2 ++
 git-rebase--preserve-merges.sh  |  2 +-
 sequencer.c                     | 38 +++++++++++++++-----------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 48 ++++++++++++++++++++++-----------
 6 files changed, 60 insertions(+), 32 deletions(-)

base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v1:

 1:  0d0a55bd9c4 ! 1:  5d2fd55c580 [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP
     @@ Commit message
          conflict occurs, which provided for some porcelain commands of git like
          `git-rebase--preserve-merges.sh`. After `git rebase -p` completely
          abolished, this option should be removed. At the same time, add the flag
     -    `delete_cherry_pick_head` to `struct rebase_options` and
     -    `struct replay_opts`, We can decide whether to delete CHERRY_PICK_HEAD
     -    by setting, passing, and checking this flag bit.
     +    `delete_cherry_pick_head` to `struct replay_opts`, We can decide whether
     +    to delete CHERRY_PICK_HEAD by setting and checking this flag bit.
      
          Then we split print_advice() into two part: Firstly, print_advice()
          will only be responsible for outputting content; Secondly, check if
     @@ Commit message
          In this way, the steps of printing advice and deleting CHERRY_PICK_HEAD
          are decoupled. Finally, let `git-rebase--preserve-merges.sh` use the
          `--delete-cherry-pick-head` option when it executes git cherry-pick, and
     -    set the `delete_cherry_pick_head` flag in run_specific_rebase() when we
     +    set the `delete_cherry_pick_head` flag in get_replay_opts() when we
          are using `git rebase --merge`, which can fix this breakage.
      
     +    It is worth mentioning that now we use advice() to print the content
     +    of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will
     +    start with "hint: ".
     +
          Mentored-by: Christian Couder <christian.couder@gmail.com>
          Mentored-by Hariom Verma <hariom18599@gmail.com>:
          Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     @@ Commit message
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
      
       ## builtin/rebase.c ##
     -@@ builtin/rebase.c: struct rebase_options {
     - 		REBASE_FORCE = 1<<3,
     - 		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
     - 	} flags;
     -+	int delete_cherry_pick_head;
     - 	struct strvec git_am_opts;
     - 	const char *action;
     - 	int signoff;
      @@ builtin/rebase.c: static struct replay_opts get_replay_opts(const struct rebase_options *opts)
       		oidcpy(&replay.squash_onto, opts->squash_onto);
       		replay.have_squash_onto = 1;
       	}
     -+	replay.delete_cherry_pick_head = opts->delete_cherry_pick_head;
     ++	replay.delete_cherry_pick_head = 1;
       
       	return replay;
       }
     -@@ builtin/rebase.c: static int run_specific_rebase(struct rebase_options *opts, enum action action)
     - 	if (opts->type == REBASE_MERGE) {
     - 		/* Run sequencer-based rebase */
     - 		setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
     -+		opts->delete_cherry_pick_head = 1;
     - 		if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
     - 			setenv("GIT_SEQUENCE_EDITOR", ":", 1);
     - 			opts->autosquash = 0;
      
       ## builtin/revert.c ##
      @@ builtin/revert.c: static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
     @@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-
       test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '
       	pristine_detach initial &&
       	test_must_fail git cherry-pick picked &&
     + 	test_cmp_rev picked CHERRY_PICK_HEAD
     + '
     + 
     ++test_expect_success 'failed cherry-pick with --delete-cherry-pick-head does not set CHERRY_PICK_HEAD' '
     ++	pristine_detach initial &&
     ++	test_must_fail git cherry-pick --delete-cherry-pick-head picked &&
     ++	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
     ++'
     ++
     + test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' '
     + 	pristine_detach initial &&
     + 	git cherry-pick base &&
      @@ t/t3507-cherry-pick-conflict.sh: test_expect_success \
       	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
       '
 2:  7e1ed49728d ! 2:  5279bca7a79 [GSOC] cherry-pick: use better advice message
     @@ Commit message
          This is the improved advice:
      
          hint: Resolve all conflicts manually, mark them as resolved with
     -    hint: "git add/rm <conflicted_files>", then run "git cherry-pick \
     -    --continue".
     +    hint: "git add/rm <conflicted_files>", then run
     +    hint: "git cherry-pick --continue".
          hint: You can instead skip this commit: run "git cherry-pick --skip".
          hint: To abort and get back to the state before "git cherry-pick",
          hint: run "git cherry-pick --abort".
     @@ sequencer.c: static void print_advice(struct replay_opts *opts, int show_hint)
      -		if (opts->no_commit)
      +		if (opts->action == REPLAY_PICK) {
      +			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
     -+				 "\"git add/rm <conflicted_files>\", then run \"git cherry-pick --continue\".\n"
     ++				 "\"git add/rm <conflicted_files>\", then run\n"
     ++				 "\"git cherry-pick --continue\".\n"
      +				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
      +				 "To abort and get back to the state before \"git cherry-pick\",\n"
      +				 "run \"git cherry-pick --abort\"."));
     @@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-
      -	hint: with 'git add <paths>' or 'git rm <paths>'
      -	hint: and commit the result with 'git commit'
      +	hint: Resolve all conflicts manually, mark them as resolved with
     -+	hint: \"git add/rm <conflicted_files>\", then run \"git cherry-pick --continue\".
     ++	hint: \"git add/rm <conflicted_files>\", then run
     ++	hint: \"git cherry-pick --continue\".
      +	hint: You can instead skip this commit: run \"git cherry-pick --skip\".
      +	hint: To abort and get back to the state before \"git cherry-pick\",
      +	hint: run \"git cherry-pick --abort\".
     @@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-
      -	hint: after resolving the conflicts, mark the corrected paths
      -	hint: with 'git add <paths>' or 'git rm <paths>'
      +	hint: Resolve all conflicts manually, mark them as resolved with
     -+	hint: \"git add/rm <conflicted_files>\", then run \"git cherry-pick --continue\".
     ++	hint: \"git add/rm <conflicted_files>\", then run
     ++	hint: \"git cherry-pick --continue\".
      +	hint: You can instead skip this commit: run \"git cherry-pick --skip\".
      +	hint: To abort and get back to the state before \"git cherry-pick\",
      +	hint: run \"git cherry-pick --abort\".

From: ZheNing Hu <adlternative@gmail.com>

GIT_CHERRY_PICK_HELP is an environment variable, as the implementation detail of some porcelain in git to help realize the rebasing steps. E.g. `git rebase -p` set GIT_CHERRY_PICK_HELP value in `git-rebase--preserve-merges.sh`, `git rebase --merge` set GIT_CHERRY_PICK_HELP value in run_specific_rebase(). But If we set the value of GIT_CHERRY_PICK_HELP when using `git cherry-pick`, CHERRY_PICK_HEAD will be deleted, then we will get an error when we try to use `git cherry-pick --continue` or other cherr-pick command.

Introduce new "hidden" option `--delete-cherry-pick-head` for git cherry-pick which indicates that CHERRY_PICK_HEAD will be deleted when conflict occurs, which provided for some porcelain commands of git like `git-rebase--preserve-merges.sh`. After `git rebase -p` completely abolished, this option should be removed. At the same time, add the flag `delete_cherry_pick_head` to `struct replay_opts`, We can decide whether to delete CHERRY_PICK_HEAD by setting and checking this flag bit.

Then we split print_advice() into two part: Firstly, print_advice() will only be responsible for outputting content; Secondly, check if we set the `delete_cherry_pick_head` flag; if set, delete CHERRY_PICK_HEAD. In this way, the steps of printing advice and deleting CHERRY_PICK_HEAD are decoupled. Finally, let `git-rebase--preserve-merges.sh` use the `--delete-cherry-pick-head` option when it executes git cherry-pick, and set the `delete_cherry_pick_head` flag in get_replay_opts() when we are using `git rebase --merge`, which can fix this breakage.

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/rebase.c                |  1 +
 builtin/revert.c                |  2 ++
 git-rebase--preserve-merges.sh  |  2 +-
 sequencer.c                     | 28 +++++++++++++---------------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 31 +++++++++++++++++++++----------
 6 files changed, 39 insertions(+), 26 deletions(-)

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

set -> sets

set -> sets

"help realize the rebasing steps" did not tell us much on "how" the environment variable helps or what it is used for. A sentence at this point, e.g.

    The variable carries a custom help message to be shown when one
    step of replaying an existing commit fails in conflict.

may help.  And there is one leap in the logic flow here.

    However, the code also removes CHERRY_PICK_HEAD pseudoref when
    this environment variable exists, assuming that the presence of
    it means the sequencer machinery and not end-user is doing the
    cherry-picking.

And then we can drop "But" before "If" here.

indicates that CHERRY_PICK_HEAD will be ... ->

tells Git remove CHERRY_PICK_HEAD to separate the decision from message customization to clean up this mess.

Heple?

OK.  That makes sense.

OK, this separation makes sense, too.

Hmph, this is a bit troubling. So has this been part of the "published" behaviour since d7e5c0cb (Introduce CHERRY_PICK_HEAD, 2011-02-19) that introduced this test, and there are people who are relying on it? IOW, should the resolution to the original problem report have been "if it hurts, don't do it" (in other words, "setting GIT_CHERRY_PICK_HELP will remove CHERRY_PICK_HEAD, so if you do not want to get the latter removed, do not set the former")?

Junio C Hamano <gitster@pobox.com> 于2021年8月4日周三 上午6:36写道：

Thanks, such a supplement is very good.

You mean that cherry_pick with GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD is not even a bug?

It is reasonable for `git rebase -p` and `git rebase -m` to delete CHERRY_PICK_HEAD when a conflict occurs, but it is not necessarily for git cherry-pick to delete it too. IOW, I suspect that instead of letting users not touch the trap here, it is better to remove the trap completely.

Thanks. -- ZheNing Hu

On 04/08/2021 09:35, ZheNing Hu wrote:

Looking at the history I think it is fair to conclude that GIT_CHERRY_PICK_HELP was introduced as a way to help people writing scripts built on top of 'git cherry-pick' like 'git rebase' that want to have a custom message and do not want to leave CHERRY_PICK_HEAD around if there are conflicts. I don't think it was intended as a way for users to change the help when cherry-picking and has never been documented as such. I think we'd be better to focus on improving the default help that cherry-pick prints as the second patch in this series does.

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

I think that it is what the existing test is telling us. Of course, with a good reason, an earlier design can be updated as long as we make sure we won't hurt existing users who may rely on the current design, but ...

... I think that is a reasonable stance to take [*1*]. If the default help message can be improved, that is a good thing to do regardless.

Thanks.

[Footnote]

*1* Tying the "here is a custom HELP text" environment variable to "having a customization means whoever is driving the cherry-pick machinery is ALSO responsible for sequencing and we will remove CHERRY_PICK_HEAD" is a rather unfortunate design, but as long as that is documented, it is a workable design.

Junio C Hamano <gitster@pobox.com> 于2021年8月5日周四 上午1:31写道：

Well, this is indeed a bit strange, but maybe your and Phillip's intuitions are right, then I will delete the content of the first patch and keep the second.

Thanks. -- ZheNing Hu

From: ZheNing Hu <adlternative@gmail.com>

In the past, git cherry-pick would print such advice when there was a conflict:

hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit'

But in fact, when we want to cherry-pick multiple commits, we should not use "git commit" after resolving conflicts, which will make Git generate some errors. We should recommend users to use `git cherry-pick --continue`, `git cherry-pick --abort`, just like git rebase does.

This is the improved advice:

hint: Resolve all conflicts manually, mark them as resolved with hint: "git add/rm <conflicted_files>", then run hint: "git cherry-pick --continue". hint: You can instead skip this commit: run "git cherry-pick --skip". hint: To abort and get back to the state before "git cherry-pick", hint: run "git cherry-pick --abort".

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 sequencer.c                     | 10 +++++++++-
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----
 2 files changed, 21 insertions(+), 6 deletions(-)

From: ZheNing Hu <adlternative@gmail.com>

In the past, git cherry-pick would print such advice when there was a conflict:

hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit'

But in fact, when we want to cherry-pick multiple commits, we should not use "git commit" after resolving conflicts, which will make Git generate some errors. We should recommend users to use `git cherry-pick --continue`, `git cherry-pick --abort`, just like git rebase does.

This is the improved advice:

hint: Resolve all conflicts manually, mark them as resolved with hint: "git add/rm <conflicted_files>", then run hint: "git cherry-pick --continue". hint: You can instead skip this commit: run "git cherry-pick --skip". hint: To abort and get back to the state before "git cherry-pick", hint: run "git cherry-pick --abort".

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [GSOC] cherry-pick: use better advice message
    
    Because git cherry-pick's past advice message is not good enough, it
    often misleads new users of Git, so this patch makes git chery-pick
    advice message better.
    
    v5:
    https://lore.kernel.org/git/pull.1010.git.1627714877.gitgitgadget@gmail.com/
    
    v5-->v6:
    
     1. Deleted the first patch, which could have made git cherry-pick not
        delete CHERRY_PICK_HEAD when using GIT_CHERRY_PICK_HELP, but both
        Junio and Phillip believe that this design does not need to be
        changed.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v2:

 1:  5d2fd55c580 < -:  ----------- [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP
 2:  5279bca7a79 ! 1:  701645dde17 [GSOC] cherry-pick: use better advice message
     @@ Commit message
          hint: To abort and get back to the state before "git cherry-pick",
          hint: run "git cherry-pick --abort".
      
     +    It is worth mentioning that now we use advice() to print the content
     +    of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will
     +    start with "hint: ".
     +
          Mentored-by: Christian Couder <christian.couder@gmail.com>
          Mentored-by Hariom Verma <hariom18599@gmail.com>:
          Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     @@ Commit message
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
      
       ## sequencer.c ##
     -@@ sequencer.c: static void print_advice(struct replay_opts *opts, int show_hint)
     +@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
     + 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
     + 
       	if (msg) {
     - 		advise("%s\n", msg);
     - 	} else if (show_hint) {
     +-		fprintf(stderr, "%s\n", msg);
     ++		advise("%s\n", msg);
     + 		/*
     + 		 * A conflict has occurred but the porcelain
     + 		 * (typically rebase --interactive) wants to take care
     +@@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
     + 	}
     + 
     + 	if (show_hint) {
      -		if (opts->no_commit)
      +		if (opts->action == REPLAY_PICK) {
      +			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
     @@ sequencer.c: static void print_advice(struct replay_opts *opts, int show_hint)
      +				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
      +				 "To abort and get back to the state before \"git cherry-pick\",\n"
      +				 "run \"git cherry-pick --abort\"."));
     -+
      +		} else if (opts->no_commit)
       			advise(_("after resolving the conflicts, mark the corrected paths\n"
       				 "with 'git add <paths>' or 'git rm <paths>'"));

 sequencer.c                     | 11 +++++++++--
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++-----
 2 files changed, 21 insertions(+), 7 deletions(-)

On 05/08/2021 06:48, ZheNing Hu via GitGitGadget wrote:

This changes means we give the wrong advice for 'git cherry-pick --no-commit'. I think you want to keep the existing clause as the first one and insert this before the else. The advice itself looks good. It would be nice to improve the advice for 'git revert' in the same way.

If you quote the here doc end marker then there is no substitution in the here doc so writing

	cat <<-\EOF >expected &&

means you can replace \" with " here

Best Wishes

Phillip

Hi, sorry for the late reply, I am busy processing the patches on the ref-filter side.

Phillip Wood <phillip.wood123@gmail.com> 于2021年8月11日周三 下午6:00写道：

Make sense.

Thanks, I haven't paid attention to this detail before.

Thanks, -- ZheNing Hu

Phillip Wood <phillip.wood123@gmail.com> writes:

Hmph, actually the double-quote that opens the body of test_expect_success should be stared at with a very cautious eyes, as they often do not mean what the author of the patch thought they do. I can see that it already fooled your eyes into thinking that there is no substitution, but $picked needs to be substituted into its value. The backslash before it is *not* about guarding it from substitution inside here-doc; it is to pass literal "$" into the string, which is the last parameter to test_expect_success, that gets eval'ed.

The original of this one, for example, would probably have been better if written like so:

test_expect_success 'advice from failed cherry-pick' '
	pristine_detach initial &&
	SQ='\'' &&

	picked=$(git rev-parse --short picked) &&
	cat <<-EOF >expected &&
	error: could not apply $picked... picked
	hint: after resolving the conflicts, mark the corrected paths
	hint: with ${SQ}git add <paths>${SQ} or ${SQ}git rm <paths>${SQ}
	hint: and commit the result with ${SQ}git commit${SQ}
	EOF
	test_must_fail git cherry-pick picked 2>actual &&

	test_cmp expected actual
'

And because there is no single quote in the updated text, it would become:

test_expect_success 'advice from failed cherry-pick' '
	pristine_detach initial &&

	picked=$(git rev-parse --short picked) &&
	cat <<-EOF >expected &&
	error: could not apply $picked... picked
	hint: Resolve all conflicts manually, mark them as resolved with
	hint: "git add/rm <conflicted_files>", then run
	EOF
	test_must_fail git cherry-pick picked 2>actual &&

	test_cmp expected actual
'

which makes it far easier to see that $picked needs to be substituted, and the "git add/rm" are to be enclosed in dq pair.

Junio C Hamano <gitster@pobox.com> 于2021年8月14日周六 上午4:14写道：

Yes, the escaping here comes from double-quote of test_expect_success instead of here-doc.

Um? This section is not working for me.

./test-lib.sh: eval: line 917: unexpected EOF while looking for matching `'' ./test-lib.sh: eval: line 932: syntax error: unexpected end of file

This section is exactly what I want. Using single quotes does make it look a lot easier.

-- ZheNing Hu

On 13/08/2021 21:14, Junio C Hamano wrote:

Good point, you're right I had completely missed that, thanks for pointing it out

Phillip

From: ZheNing Hu <adlternative@gmail.com>

In the past, git cherry-pick would print such advice when there was a conflict:

hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit'

But in fact, when we want to cherry-pick multiple commits, we should not use "git commit" after resolving conflicts, which will make Git generate some errors. We should recommend users to use `git cherry-pick --continue`, `git cherry-pick --abort`, just like git rebase does.

This is the improved advice:

hint: Resolve all conflicts manually, mark them as resolved with hint: "git add/rm <conflicted_files>", then run hint: "git cherry-pick --continue". hint: You can instead skip this commit: run "git cherry-pick --skip". hint: To abort and get back to the state before "git cherry-pick", hint: run "git cherry-pick --abort".

Similarly, this optimization can be applied to git revert:

hint: Resolve all conflicts manually, mark them as resolved with hint: "git add/rm <conflicted_files>", then run hint: "git revert --continue". hint: You can instead skip this commit: run "git revert --skip". hint: To abort and get back to the state before "git revert", hint: run "git revert --abort".

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hepled-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [GSOC] cherry-pick: use better advice message
    
    Because git cherry-pick's past advice message is not good enough, it
    often misleads new users of Git, so this patch makes git chery-pick
    advice message better.
    
    v6:
    https://lore.kernel.org/git/pull.1010.v3.git.1628142482640.gitgitgadget@gmail.com/
    
    v6-->v7:
    
     1. Modify the advice order, respect git cherry-pick --no-commit advice.
     2. Let git revert also use better message.
     3. Use double quotes instead of single quotes for the test body.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v3:

 1:  701645dde17 ! 1:  dc51c0b8c2b [GSOC] cherry-pick: use better advice message
     @@ Commit message
          hint: To abort and get back to the state before "git cherry-pick",
          hint: run "git cherry-pick --abort".
      
     +    Similarly, this optimization can be applied to git revert:
     +
     +    hint: Resolve all conflicts manually, mark them as resolved with
     +    hint: "git add/rm <conflicted_files>", then run
     +    hint: "git revert --continue".
     +    hint: You can instead skip this commit: run "git revert --skip".
     +    hint: To abort and get back to the state before "git revert",
     +    hint: run "git revert --abort".
     +
          It is worth mentioning that now we use advice() to print the content
          of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will
          start with "hint: ".
     @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
       		 * A conflict has occurred but the porcelain
       		 * (typically rebase --interactive) wants to take care
      @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
     - 	}
     - 
     - 	if (show_hint) {
     --		if (opts->no_commit)
     -+		if (opts->action == REPLAY_PICK) {
     + 		if (opts->no_commit)
     + 			advise(_("after resolving the conflicts, mark the corrected paths\n"
     + 				 "with 'git add <paths>' or 'git rm <paths>'"));
     ++		else if (opts->action == REPLAY_PICK)
      +			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
      +				 "\"git add/rm <conflicted_files>\", then run\n"
      +				 "\"git cherry-pick --continue\".\n"
      +				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
      +				 "To abort and get back to the state before \"git cherry-pick\",\n"
      +				 "run \"git cherry-pick --abort\"."));
     -+		} else if (opts->no_commit)
     - 			advise(_("after resolving the conflicts, mark the corrected paths\n"
     - 				 "with 'git add <paths>' or 'git rm <paths>'"));
     ++		else if (opts->action == REPLAY_REVERT)
     ++			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
     ++				 "\"git add/rm <conflicted_files>\", then run\n"
     ++				 "\"git revert --continue\".\n"
     ++				 "You can instead skip this commit: run \"git revert --skip\".\n"
     ++				 "To abort and get back to the state before \"git revert\",\n"
     ++				 "run \"git revert --abort\"."));
       		else
     + 			advise(_("after resolving the conflicts, mark the corrected paths\n"
     + 				 "with 'git add <paths>' or 'git rm <paths>'\n"
     +
     + ## t/t3501-revert-cherry-pick.sh ##
     +@@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty renamed file' '
     + 	grep -q "^modified$" renamed
     + '
     + 
     ++test_expect_success 'advice from failed revert' '
     ++	echo dream >dream &&
     ++	git add dream &&
     ++	git commit -m "add dream" &&
     ++	dream_oid=$(git rev-parse --short HEAD) &&
     ++	cat <<-EOF >expected &&
     ++	error: could not revert $dream_oid... add dream
     ++	hint: Resolve all conflicts manually, mark them as resolved with
     ++	hint: "git add/rm <conflicted_files>", then run
     ++	hint: "git revert --continue".
     ++	hint: You can instead skip this commit: run "git revert --skip".
     ++	hint: To abort and get back to the state before "git revert",
     ++	hint: run "git revert --abort".
     ++	EOF
     ++	echo dream >>dream &&
     ++	git add dream &&
     ++	git commit -m "double-add dream" &&
     ++	test_must_fail git revert HEAD^ 2>actual &&
     ++	test_cmp expected actual
     ++'
     + test_done
      
       ## t/t3507-cherry-pick-conflict.sh ##
     -@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-pick' "
     - 	picked=\$(git rev-parse --short picked) &&
     +@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'failed cherry-pick does not advance HEAD' '
     + 	test "$head" = "$newhead"
     + '
     + 
     +-test_expect_success 'advice from failed cherry-pick' "
     ++test_expect_success 'advice from failed cherry-pick' '
     + 	pristine_detach initial &&
     + 
     +-	picked=\$(git rev-parse --short picked) &&
     ++	picked=$(git rev-parse --short picked) &&
       	cat <<-EOF >expected &&
     - 	error: could not apply \$picked... picked
     +-	error: could not apply \$picked... picked
      -	hint: after resolving the conflicts, mark the corrected paths
      -	hint: with 'git add <paths>' or 'git rm <paths>'
      -	hint: and commit the result with 'git commit'
     ++	error: could not apply $picked... picked
      +	hint: Resolve all conflicts manually, mark them as resolved with
     -+	hint: \"git add/rm <conflicted_files>\", then run
     -+	hint: \"git cherry-pick --continue\".
     -+	hint: You can instead skip this commit: run \"git cherry-pick --skip\".
     -+	hint: To abort and get back to the state before \"git cherry-pick\",
     -+	hint: run \"git cherry-pick --abort\".
     ++	hint: "git add/rm <conflicted_files>", then run
     ++	hint: "git cherry-pick --continue".
     ++	hint: You can instead skip this commit: run "git cherry-pick --skip".
     ++	hint: To abort and get back to the state before "git cherry-pick",
     ++	hint: run "git cherry-pick --abort".
       	EOF
       	test_must_fail git cherry-pick picked 2>actual &&
       
     -@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-pick --no-commit' "
     - 	picked=\$(git rev-parse --short picked) &&
     - 	cat <<-EOF >expected &&
     - 	error: could not apply \$picked... picked
     --	hint: after resolving the conflicts, mark the corrected paths
     --	hint: with 'git add <paths>' or 'git rm <paths>'
     -+	hint: Resolve all conflicts manually, mark them as resolved with
     -+	hint: \"git add/rm <conflicted_files>\", then run
     -+	hint: \"git cherry-pick --continue\".
     -+	hint: You can instead skip this commit: run \"git cherry-pick --skip\".
     -+	hint: To abort and get back to the state before \"git cherry-pick\",
     -+	hint: run \"git cherry-pick --abort\".
     - 	EOF
     - 	test_must_fail git cherry-pick --no-commit picked 2>actual &&
     + 	test_cmp expected actual
     +-"
     ++'
       
     + test_expect_success 'advice from failed cherry-pick --no-commit' "
     + 	pristine_detach initial &&

 sequencer.c                     | 16 +++++++++++++++-
 t/t3501-revert-cherry-pick.sh   | 20 ++++++++++++++++++++
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------
 3 files changed, 45 insertions(+), 8 deletions(-)

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

In our log messages, we desciribe the current state in the present tense.

It may be worth mentioning that this is because the program originally was about picking only one commit and the hint was inherited from those days. Or it may not. I dunno.

I am not sure "should not" is the right phrase. Also it does not help readers to have a vague "generate some errors" than not saying anything---it leaves readers puzzled with "what errors???"

Whether picking a single commit or a series of commits, after resolving conflicts in the current step, wouldn't

    $ git commit ;# to conclude the resolution
    $ git cherry-pick --continue

do the right thing?

It may be an improved advice, but just omit it and say something like:

	Suggest use of "git cherry-pick --contiue", so that it would
	also apply to cases where multiple commits are being picked.

The actual message does not have to be reproduced here, as it is in the source, and it can be seen in the test ;-)

That would make the proposed log message conform to our norm, i.e. brief description of what happens with the current system, followed by description of the perceived problem, followed by an order to the codebase to become different in a specific way that solves the problem.

Taken together, perhaps

	"git cherry-pick", upon seeing a conflict, says:

	    hint: ...

	as if running "git commit" to conclude the resolution of
	this single step were the end of the story.  This stems from
	the fact that the command originally was to pick a single
	commit and not a range of commits, and the message was
	written back then and has not been adjusted.

	When picking a range of commits and the command stops with a
	conflict in the middle of the range, however, after
	resolving the conflict and (optionally) recording the result
	with "git commit", the user has to run "git cherry-pick
	--continue" to have the rest of the range dealt with,
	"--skip" to drop the current commit, or "--abort" to discard
	the series.

	Suggest use of "git cherry-pick --continue/--skip/--abort"
	so that the message also covers the case where a range of
	commits are being picked.

It seems that unlike other people I keep hepling you, whatever that verb means ;-).

Thanks.

Junio C Hamano <gitster@pobox.com> 于2021年8月15日周日 上午4:33写道：

Yes, may look at the time, the design is correct.

yes, this error is specifically that if we want pick a series of commits, after resolving conflicts and use git commit, `.git/sequencer` still exists, but git cherry-pick has ended (the next step cherry-pick is not performed). This may be a very strange critical state.

I think you mean that git commit is used to end a single cherry-pick. But in the face of a series of commits, errors will occur.

Yes, maybe the commit message should be shorter and more concise.

Ok, I understand now, git commit just stopped git cherry-pick by delete CHERRY_PICK_HEAD in sequencer_post_commit_cleanup(). we can still resume the process with git cherry-pick --continue.

Ehhh, I seem to have forgotten the correction here.

Thanks. -- ZheNing Hu

From: ZheNing Hu <adlternative@gmail.com>

"git cherry-pick", upon seeing a conflict, says:

hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit'

as if running "git commit" to conclude the resolution of this single step were the end of the story. This stems from the fact that the command originally was to pick a single commit and not a range of commits, and the message was written back then and has not been adjusted.

When picking a range of commits and the command stops with a conflict in the middle of the range, however, after resolving the conflict and (optionally) recording the result with "git commit", the user has to run "git cherry-pick --continue" to have the rest of the range dealt with, "--skip" to drop the current commit, or "--abort" to discard the series.

Suggest use of "git cherry-pick --continue/--skip/--abort" so that the message also covers the case where a range of commits are being picked.

Similarly, this optimization can be applied to git revert, suggest use of "git revert --continue/--skip/--abort" so that the message also covers the case where a range of commits are being reverted.

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>:
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [GSOC] cherry-pick: use better advice message
    
    The cherry-pick and revert advice message are only suitable for picking
    one commit or reverting one commit, but not for multiple commits. So
    correct the advice message to have the rest of the range dealt with.
    
    v7:
    https://lore.kernel.org/git/pull.1010.v4.git.1628936863733.gitgitgadget@gmail.com/
    
    v7-->v8:
    
     1. Modify the commit message to make it more in line with the
        specification.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v4:

 1:  dc51c0b8c2b ! 1:  6cf78ffd088 [GSOC] cherry-pick: use better advice message
     @@ Metadata
       ## Commit message ##
          [GSOC] cherry-pick: use better advice message
      
     -    In the past, git cherry-pick would print such advice when
     -    there was a conflict:
     +    "git cherry-pick", upon seeing a conflict, says:
      
          hint: after resolving the conflicts, mark the corrected paths
          hint: with 'git add <paths>' or 'git rm <paths>'
          hint: and commit the result with 'git commit'
      
     -    But in fact, when we want to cherry-pick multiple commits,
     -    we should not use "git commit" after resolving conflicts, which
     -    will make Git generate some errors. We should recommend users to
     -    use `git cherry-pick --continue`, `git cherry-pick --abort`, just
     -    like git rebase does.
     +    as if running "git commit" to conclude the resolution of
     +    this single step were the end of the story.  This stems from
     +    the fact that the command originally was to pick a single
     +    commit and not a range of commits, and the message was
     +    written back then and has not been adjusted.
      
     -    This is the improved advice:
     +    When picking a range of commits and the command stops with a
     +    conflict in the middle of the range, however, after
     +    resolving the conflict and (optionally) recording the result
     +    with "git commit", the user has to run "git cherry-pick
     +    --continue" to have the rest of the range dealt with,
     +    "--skip" to drop the current commit, or "--abort" to discard
     +    the series.
      
     -    hint: Resolve all conflicts manually, mark them as resolved with
     -    hint: "git add/rm <conflicted_files>", then run
     -    hint: "git cherry-pick --continue".
     -    hint: You can instead skip this commit: run "git cherry-pick --skip".
     -    hint: To abort and get back to the state before "git cherry-pick",
     -    hint: run "git cherry-pick --abort".
     +    Suggest use of "git cherry-pick --continue/--skip/--abort"
     +    so that the message also covers the case where a range of
     +    commits are being picked.
      
     -    Similarly, this optimization can be applied to git revert:
     +    Similarly, this optimization can be applied to git revert,
     +    suggest use of "git revert --continue/--skip/--abort" so
     +    that the message also covers the case where a range of
     +    commits are being reverted.
      
     -    hint: Resolve all conflicts manually, mark them as resolved with
     -    hint: "git add/rm <conflicted_files>", then run
     -    hint: "git revert --continue".
     -    hint: You can instead skip this commit: run "git revert --skip".
     -    hint: To abort and get back to the state before "git revert",
     -    hint: run "git revert --abort".
     -
     -    It is worth mentioning that now we use advice() to print the content
     -    of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will
     -    start with "hint: ".
     +    It is worth mentioning that now we use advice() to print
     +    the content of GIT_CHERRY_PICK_HELP in print_advice(), each
     +    line of output will start with "hint: ".
      
          Mentored-by: Christian Couder <christian.couder@gmail.com>
          Mentored-by Hariom Verma <hariom18599@gmail.com>:
          Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     -    Hepled-by: Junio C Hamano <gitster@pobox.com>
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
      
       ## sequencer.c ##

 sequencer.c                     | 16 +++++++++++++++-
 t/t3501-revert-cherry-pick.sh   | 20 ++++++++++++++++++++
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------
 3 files changed, 45 insertions(+), 8 deletions(-)

On 16/08/2021 01:55, ZheNing Hu via GitGitGadget wrote:

Thanks for making the revert advice better as well

I think this last else arm should probably be a bug now as anything other than cherry-pick or revert should be setting GIT_CHERRY_PICK_HELP

	else
		BUG("unexpected pick action in print_advice()");

A minor comment: you can condense these three lines by using test_commit (see test-lib-functions.sh for the documentation)

	test_commit "add dream" dream dream

test_commit can also append to a file

	test_commit --append "double-add dream" dream dream

This is looking very close to being ready now I think

Thanks

Phillip

Phillip Wood <phillip.wood123@gmail.com> 于2021年8月18日周三 下午5:51写道：

Thanks for reviewing too.

Maybe you are right, If no one else will touch it, it may be reasonable to set it as BUG().

Thanks, I may also need a --no-tag option, because "add dream" is not a valid tag name.

Same too, test_commit --append --no-tag "double-add dream" dream dream

Thanks. -- ZheNing Hu

ZheNing Hu <adlternative@gmail.com> 于2021年8月19日周四 上午9:55写道：

GIT_CHERRY_PICK_HELP should be set for other commands except cherry-pick and revert.

-- ZheNing

From: ZheNing Hu <adlternative@gmail.com>

"git cherry-pick", upon seeing a conflict, says:

hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit'

as if running "git commit" to conclude the resolution of this single step were the end of the story. This stems from the fact that the command originally was to pick a single commit and not a range of commits, and the message was written back then and has not been adjusted.

When picking a range of commits and the command stops with a conflict in the middle of the range, however, after resolving the conflict and (optionally) recording the result with "git commit", the user has to run "git cherry-pick --continue" to have the rest of the range dealt with, "--skip" to drop the current commit, or "--abort" to discard the series.

Suggest use of "git cherry-pick --continue/--skip/--abort" so that the message also covers the case where a range of commits are being picked.

Similarly, this optimization can be applied to git revert, suggest use of "git revert --continue/--skip/--abort" so that the message also covers the case where a range of commits are being reverted.

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by Hariom Verma <hariom18599@gmail.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [GSOC] cherry-pick: use better advice message
    
    The cherry-pick and revert advice message are only suitable for picking
    one commit or reverting one commit, but not for multiple commits. So
    correct the advice message to have the rest of the range dealt with.
    
    v8:
    https://lore.kernel.org/git/pull.1010.v5.git.1629075306706.gitgitgadget@gmail.com/
    
    v8-->v9:
    
     1. Use the test_commit function in the test.
     2. Treat these commands which are not using cherry-pick or revert and
        without setting GIT_CHERRY_PICK_HELP as a BUG.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v5:

 1:  6cf78ffd088 ! 1:  84676c475ee [GSOC] cherry-pick: use better advice message
     @@ Commit message
          line of output will start with "hint: ".
      
          Mentored-by: Christian Couder <christian.couder@gmail.com>
     -    Mentored-by Hariom Verma <hariom18599@gmail.com>:
     +    Mentored-by Hariom Verma <hariom18599@gmail.com>
          Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
          Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
     @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
      +				 "To abort and get back to the state before \"git revert\",\n"
      +				 "run \"git revert --abort\"."));
       		else
     - 			advise(_("after resolving the conflicts, mark the corrected paths\n"
     - 				 "with 'git add <paths>' or 'git rm <paths>'\n"
     +-			advise(_("after resolving the conflicts, mark the corrected paths\n"
     +-				 "with 'git add <paths>' or 'git rm <paths>'\n"
     +-				 "and commit the result with 'git commit'"));
     ++			BUG("unexpected pick action in print_advice()");
     + 	}
     + }
     + 
      
       ## t/t3501-revert-cherry-pick.sh ##
      @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty renamed file' '
     @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty
       '
       
      +test_expect_success 'advice from failed revert' '
     -+	echo dream >dream &&
     -+	git add dream &&
     -+	git commit -m "add dream" &&
     ++	test_commit --no-tag "add dream" dream dream &&
      +	dream_oid=$(git rev-parse --short HEAD) &&
      +	cat <<-EOF >expected &&
      +	error: could not revert $dream_oid... add dream
     @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty
      +	hint: To abort and get back to the state before "git revert",
      +	hint: run "git revert --abort".
      +	EOF
     -+	echo dream >>dream &&
     -+	git add dream &&
     -+	git commit -m "double-add dream" &&
     ++	test_commit --append --no-tag "double-add dream" dream dream &&
      +	test_must_fail git revert HEAD^ 2>actual &&
      +	test_cmp expected actual
      +'

 sequencer.c                     | 20 ++++++++++++++++----
 t/t3501-revert-cherry-pick.sh   | 16 ++++++++++++++++
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------
 3 files changed, 42 insertions(+), 11 deletions(-)

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

I think you meant to move ':' not remove it.

Do we need to say "manually"? Dropping the word would make the message more concise. Not repeating the word "resolve" by saying something like "after resolving the conflicts, mark them with ..." would probably be a much better phrasing, too.

<paths> (or <pathspec>) not <conflicted_files>; you can resolve many files in a directory and give the pathspec to match the directory to mark all the files in there as resolved.

Inconsistent use of prose in the above and ": run" here.

        You can instead skip this commit with "git cherry-pick --skip"

Likewise.

Thanks.

Junio C Hamano <gitster@pobox.com> 于2021年8月20日周五 上午1:10写道：

Yeah. Note the $reslovemsg in git-rebase--preserve-merges.sh, I copied it from here. Perhaps it is also worth modifying?

Indeed so.

Thanks. -- ZheNing Hu

From: ZheNing Hu <adlternative@gmail.com>

"git cherry-pick", upon seeing a conflict, says:

hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit'

as if running "git commit" to conclude the resolution of this single step were the end of the story. This stems from the fact that the command originally was to pick a single commit and not a range of commits, and the message was written back then and has not been adjusted.

When picking a range of commits and the command stops with a conflict in the middle of the range, however, after resolving the conflict and (optionally) recording the result with "git commit", the user has to run "git cherry-pick --continue" to have the rest of the range dealt with, "--skip" to drop the current commit, or "--abort" to discard the series.

Suggest use of "git cherry-pick --continue/--skip/--abort" so that the message also covers the case where a range of commits are being picked.

Similarly, this optimization can be applied to git revert, suggest use of "git revert --continue/--skip/--abort" so that the message also covers the case where a range of commits are being reverted.

It is worth mentioning that now we use advice() to print the content of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will start with "hint: ".

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    [GSOC] cherry-pick: use better advice message
    
    The cherry-pick and revert advice message are only suitable for picking
    one commit or reverting one commit, but not for multiple commits. So
    correct the advice message to have the rest of the range dealt with.
    
    v9:
    https://lore.kernel.org/git/pull.1010.v6.git.1629352277151.gitgitgadget@gmail.com/
    
    v9-->v10:
    
     1. Correct the wording of the advice message.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1010%2Fadlternative%2Fcherry-pick-help-fix-3-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1010/adlternative/cherry-pick-help-fix-3-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/1010

Range-diff vs v6:

 1:  84676c475ee ! 1:  07440ea5ffe [GSOC] cherry-pick: use better advice message
     @@ Commit message
          line of output will start with "hint: ".
      
          Mentored-by: Christian Couder <christian.couder@gmail.com>
     -    Mentored-by Hariom Verma <hariom18599@gmail.com>
     +    Mentored-by: Hariom Verma <hariom18599@gmail.com>
          Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
          Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: ZheNing Hu <adlternative@gmail.com>
     @@ sequencer.c: static void print_advice(struct repository *r, int show_hint,
       			advise(_("after resolving the conflicts, mark the corrected paths\n"
       				 "with 'git add <paths>' or 'git rm <paths>'"));
      +		else if (opts->action == REPLAY_PICK)
     -+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
     -+				 "\"git add/rm <conflicted_files>\", then run\n"
     ++			advise(_("After resolving the conflicts, mark them with\n"
     ++				 "\"git add/rm <pathspec>\", then run\n"
      +				 "\"git cherry-pick --continue\".\n"
     -+				 "You can instead skip this commit: run \"git cherry-pick --skip\".\n"
     ++				 "You can instead skip this commit with \"git cherry-pick --skip\".\n"
      +				 "To abort and get back to the state before \"git cherry-pick\",\n"
      +				 "run \"git cherry-pick --abort\"."));
      +		else if (opts->action == REPLAY_REVERT)
     -+			advise(_("Resolve all conflicts manually, mark them as resolved with\n"
     -+				 "\"git add/rm <conflicted_files>\", then run\n"
     ++			advise(_("After resolving the conflicts, mark them with\n"
     ++				 "\"git add/rm <pathspec>\", then run\n"
      +				 "\"git revert --continue\".\n"
     -+				 "You can instead skip this commit: run \"git revert --skip\".\n"
     ++				 "You can instead skip this commit with \"git revert --skip\".\n"
      +				 "To abort and get back to the state before \"git revert\",\n"
      +				 "run \"git revert --abort\"."));
       		else
     @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'cherry-pick works with dirty
      +	dream_oid=$(git rev-parse --short HEAD) &&
      +	cat <<-EOF >expected &&
      +	error: could not revert $dream_oid... add dream
     -+	hint: Resolve all conflicts manually, mark them as resolved with
     -+	hint: "git add/rm <conflicted_files>", then run
     ++	hint: After resolving the conflicts, mark them with
     ++	hint: "git add/rm <pathspec>", then run
      +	hint: "git revert --continue".
     -+	hint: You can instead skip this commit: run "git revert --skip".
     ++	hint: You can instead skip this commit with "git revert --skip".
      +	hint: To abort and get back to the state before "git revert",
      +	hint: run "git revert --abort".
      +	EOF
     @@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'failed cherry-pick does no
      -	hint: with 'git add <paths>' or 'git rm <paths>'
      -	hint: and commit the result with 'git commit'
      +	error: could not apply $picked... picked
     -+	hint: Resolve all conflicts manually, mark them as resolved with
     -+	hint: "git add/rm <conflicted_files>", then run
     ++	hint: After resolving the conflicts, mark them with
     ++	hint: "git add/rm <pathspec>", then run
      +	hint: "git cherry-pick --continue".
     -+	hint: You can instead skip this commit: run "git cherry-pick --skip".
     ++	hint: You can instead skip this commit with "git cherry-pick --skip".
      +	hint: To abort and get back to the state before "git cherry-pick",
      +	hint: run "git cherry-pick --abort".
       	EOF

 sequencer.c                     | 20 ++++++++++++++++----
 t/t3501-revert-cherry-pick.sh   | 16 ++++++++++++++++
 t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------
 3 files changed, 42 insertions(+), 11 deletions(-)