| Message ID | pull.1010.v3.git.1628142482640.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,GSOC] cherry-pick: use better advice message | expand |
On 05/08/2021 06:48, ZheNing Hu via GitGitGadget wrote: > From: ZheNing Hu <adlternative@gmail.com> > [...] > sequencer.c | 11 +++++++++-- > t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++----- > 2 files changed, 21 insertions(+), 7 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 0bec01cf38e..7fa91b99870 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 > @@ -415,7 +415,14 @@ static void print_advice(struct repository *r, int show_hint, > } > > if (show_hint) { > - if (opts->no_commit) > + if (opts->action == REPLAY_PICK) { 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. > + 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 > diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh > index 014001b8f32..d3ed9d7ce0d 100755 > --- a/t/t3507-cherry-pick-conflict.sh > +++ b/t/t3507-cherry-pick-conflict.sh > @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' " > picked=\$(git rev-parse --short picked) && > cat <<-EOF >expected && If you quote the here doc end marker then there is no substitution in the here doc so writing 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' > + hint: Resolve all conflicts manually, mark them as resolved with > + hint: \"git add/rm <conflicted_files>\", then run means you can replace \" with " here Best Wishes Phillip > + 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 && > > @@ -68,8 +71,12 @@ 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 && > > > base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006 >
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写道： > > On 05/08/2021 06:48, ZheNing Hu via GitGitGadget wrote: > > From: ZheNing Hu <adlternative@gmail.com> > > [...] > > sequencer.c | 11 +++++++++-- > > t/t3507-cherry-pick-conflict.sh | 17 ++++++++++++----- > > 2 files changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/sequencer.c b/sequencer.c > > index 0bec01cf38e..7fa91b99870 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 > > @@ -415,7 +415,14 @@ static void print_advice(struct repository *r, int show_hint, > > } > > > > if (show_hint) { > > - if (opts->no_commit) > > + if (opts->action == REPLAY_PICK) { > > 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. > Make sense. > > + 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 > > diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh > > index 014001b8f32..d3ed9d7ce0d 100755 > > --- a/t/t3507-cherry-pick-conflict.sh > > +++ b/t/t3507-cherry-pick-conflict.sh > > @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' " > > picked=\$(git rev-parse --short picked) && > > cat <<-EOF >expected && > > If you quote the here doc end marker then there is no substitution in > the here doc so writing > > 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' > > + hint: Resolve all conflicts manually, mark them as resolved with > > + hint: \"git add/rm <conflicted_files>\", then run > > means you can replace \" with " here > Thanks, I haven't paid attention to this detail before. > Best Wishes > > Phillip > Thanks, -- ZheNing Hu
Phillip Wood <phillip.wood123@gmail.com> writes: >> +++ b/t/t3507-cherry-pick-conflict.sh >> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' " >> picked=\$(git rev-parse --short picked) && >> cat <<-EOF >expected && > > If you quote the here doc end marker then there is no substitution in > the here doc so writing > > 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' >> + hint: Resolve all conflicts manually, mark them as resolved with >> + hint: \"git add/rm <conflicted_files>\", then run > > means you can replace \" with " here 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写道： > > Phillip Wood <phillip.wood123@gmail.com> writes: > > >> +++ b/t/t3507-cherry-pick-conflict.sh > >> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' " > >> picked=\$(git rev-parse --short picked) && > >> cat <<-EOF >expected && > > > > If you quote the here doc end marker then there is no substitution in > > the here doc so writing > > > > 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' > >> + hint: Resolve all conflicts manually, mark them as resolved with > >> + hint: \"git add/rm <conflicted_files>\", then run > > > > means you can replace \" with " here > > 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. > Yes, the escaping here comes from double-quote of test_expect_success instead of here-doc. > 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 > ' > 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 > 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. 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: > Phillip Wood <phillip.wood123@gmail.com> writes: > >>> +++ b/t/t3507-cherry-pick-conflict.sh >>> @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' " >>> picked=\$(git rev-parse --short picked) && >>> cat <<-EOF >expected && >> >> If you quote the here doc end marker then there is no substitution in >> the here doc so writing >> >> 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' >>> + hint: Resolve all conflicts manually, mark them as resolved with >>> + hint: \"git add/rm <conflicted_files>\", then run >> >> means you can replace \" with " here > > 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. Good point, you're right I had completely missed that, thanks for pointing it out Phillip > > 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. >
diff --git a/sequencer.c b/sequencer.c index 0bec01cf38e..7fa91b99870 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 @@ -415,7 +415,14 @@ 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" + "\"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 diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index 014001b8f32..d3ed9d7ce0d 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -53,9 +53,12 @@ test_expect_success 'advice from failed cherry-pick' " 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' + 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 && @@ -68,8 +71,12 @@ 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 &&