| Message ID | pull.1010.v4.git.1628936863733.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,GSOC] cherry-pick: use better advice message | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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' 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. > 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. 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? > This is the improved advice: 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. > Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> > Hepled-by: Junio C Hamano <gitster@pobox.com> It seems that unlike other people I keep hepling you, whatever that verb means ;-). Thanks. > 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 && > > base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006
Junio C Hamano <gitster@pobox.com> 于2021年8月15日周日 上午4:33写道： > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > 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' > > 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. > Yes, may look at the time, the design is correct. > > 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. > > 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???" > 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. > 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? > 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. > > This is the improved advice: > > 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 ;-) > Yes, maybe the commit message should be shorter and more concise. > 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. > 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. > 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. > > > > Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> > > Hepled-by: Junio C Hamano <gitster@pobox.com> > > It seems that unlike other people I keep hepling you, whatever that > verb means ;-). > Ehhh, I seem to have forgotten the correction here. > Thanks. > Thanks. -- ZheNing Hu
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 &&