diff mbox series

[v5,GSOC] cherry-pick: use better advice message

Message ID pull.1010.v5.git.1629075306706.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v5,GSOC] cherry-pick: use better advice message | expand

Commit Message

ZheNing Hu Aug. 16, 2021, 12:55 a.m. UTC 
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(-)


base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006

Comments

Phillip Wood Aug. 18, 2021, 9:51 a.m. UTC | #1 
On 16/08/2021 01:55, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>
> [...]
>   sequencer.c                     | 16 +++++++++++++++-
>   t/t3501-revert-cherry-pick.sh   | 20 ++++++++++++++++++++
>   t/t3507-cherry-pick-conflict.sh | 17 ++++++++++-------
>   3 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 0bec01cf38e..2dd73d24a87 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -403,7 +403,7 @@ static void print_advice(struct repository *r, int show_hint,
>   	char *msg = getenv("GIT_CHERRY_PICK_HELP");
>   
>   	if (msg) {
> -		fprintf(stderr, "%s\n", msg);
> +		advise("%s\n", msg);
>   		/*
>   		 * A conflict has occurred but the porcelain
>   		 * (typically rebase --interactive) wants to take care
> @@ -418,6 +418,20 @@ static void print_advice(struct repository *r, int show_hint,
>   		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->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\"."));

Thanks for making the revert advice better as well

>   		else
>   			advise(_("after resolving the conflicts, mark the corrected paths\n"
>   				 "with 'git add <paths>' or 'git rm <paths>'\n"

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()");

> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 9d100cd1884..6766aed7282 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -158,4 +158,24 @@ 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" &&

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

> +	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_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

> +	test_must_fail git revert HEAD^ 2>actual &&
> +	test_cmp expected actual
> +'
>   test_done
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 014001b8f32..cb2ebea9ad3 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -47,20 +47,23 @@ 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
> -	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".
>   	EOF
>   	test_must_fail git cherry-pick picked 2>actual &&
>   
>   	test_cmp expected actual
> -"
> +'
>   
>   test_expect_success 'advice from failed cherry-pick --no-commit' "
>   	pristine_detach initial &&
> 
> base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006
>
ZheNing Hu Aug. 19, 2021, 1:55 a.m. UTC | #2 
Phillip Wood <phillip.wood123@gmail.com> 于2021年8月18日周三 下午5:51写道：
>
> Thanks for making the revert advice better as well
>

Thanks for reviewing too.

> >               else
> >                       advise(_("after resolving the conflicts, mark the corrected paths\n"
> >                                "with 'git add <paths>' or 'git rm <paths>'\n"
>
> 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()");
>

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

> > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> > index 9d100cd1884..6766aed7282 100755
> > --- a/t/t3501-revert-cherry-pick.sh
> > +++ b/t/t3501-revert-cherry-pick.sh
> > @@ -158,4 +158,24 @@ 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" &&
>
> 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
>

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

> > +     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_commit can also append to a file
>
>         test_commit --append "double-add dream" dream dream
>

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

> This is looking very close to being ready now I think
>
> Thanks
>
> Phillip
>

Thanks.
--
ZheNing Hu
ZheNing Hu Aug. 19, 2021, 2:07 a.m. UTC | #3 
ZheNing Hu <adlternative@gmail.com> 于2021年8月19日周四 上午9:55写道：
>
> Phillip Wood <phillip.wood123@gmail.com> 于2021年8月18日周三 下午5:51写道：
> >
> > Thanks for making the revert advice better as well
> >
>
> Thanks for reviewing too.
>
> > >               else
> > >                       advise(_("after resolving the conflicts, mark the corrected paths\n"
> > >                                "with 'git add <paths>' or 'git rm <paths>'\n"
> >
> > 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
> >

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

> >         else
> >                 BUG("unexpected pick action in print_advice()");
> >
>
> Maybe you are right, If no one else will touch it, it may be
> reasonable to set it as BUG().
>

--
ZheNing
diff mbox series

Patch

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..2dd73d24a87 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -403,7 +403,7 @@  static void print_advice(struct repository *r, int show_hint,
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	if (msg) {
-		fprintf(stderr, "%s\n", msg);
+		advise("%s\n", msg);
 		/*
 		 * A conflict has occurred but the porcelain
 		 * (typically rebase --interactive) wants to take care
@@ -418,6 +418,20 @@  static void print_advice(struct repository *r, int show_hint,
 		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->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"
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 9d100cd1884..6766aed7282 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -158,4 +158,24 @@  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
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 014001b8f32..cb2ebea9ad3 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -47,20 +47,23 @@  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
-	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".
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 
 	test_cmp expected actual
-"
+'
 
 test_expect_success 'advice from failed cherry-pick --no-commit' "
 	pristine_detach initial &&