| Message ID | pull.1001.v2.git.1627135281887.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP | expand |
On 24/07/2021 15:01, ZheNing Hu via GitGitGadget wrote: > From: ZheNing Hu <adlternative@gmail.com> > > If we set the value of the environment variable 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. > > So unsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick(), to avoid > deleting CHERRY_PICK_HEAD when we are truly cherry-picking, which can > fix this breakage. > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > Mentored-by: Christian Couder <christian.couder@gmail.com> > Mentored-by Hariom Verma <hariom18599@gmail.com>: > --- > [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. > > Change from last version: > > 1. Only unsetenv(GIT_CHERRY_PICK_HELP) without touching anything in > sequencer.c. Now git cherry-pick will ignore GIT_CHERRY_PICK_HELP, > > $ GIT_CHERRY_PICK_HELP="213" git cherry-pick dev~3..dev > > will only output default advice: > > hint: after resolving the conflicts, mark the corrected paths hint: with > 'git add ' or 'git rm ' hint: and commit the result with 'git commit' > > This may still not be good enough, hope that cherry-pick will not advice > anything related to "git commit". Maybe we should make --no-commit as > cherry-pick default behavior? > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1001%2Fadlternative%2Fcherry-pick-help-fix-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1001/adlternative/cherry-pick-help-fix-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/1001 > > Range-diff vs v1: > > 1: c2a6a625ac8 ! 1: fbb9c166502 [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP > @@ Commit message > we will get an error when we try to use `git cherry-pick --continue` > or other cherr-pick command. > > - So add option action check in print_advice(), we will not remove > - CHERRY_PICK_HEAD if we are indeed cherry-picking instead of rebasing. > + So unsetenv(GIT_CHERRY_PICK_HELP) in cmd_cherry_pick(), to avoid > + deleting CHERRY_PICK_HEAD when we are truly cherry-picking, which can > + fix this breakage. > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > + Mentored-by: Christian Couder <christian.couder@gmail.com> > + Mentored-by Hariom Verma <hariom18599@gmail.com>: > > - ## sequencer.c ## > -@@ sequencer.c: static void print_advice(struct repository *r, int show_hint, > - * (typically rebase --interactive) wants to take care > - * of the commit itself so remove CHERRY_PICK_HEAD > - */ > -- refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD", > -- NULL, 0); > -+ if (opts->action != REPLAY_PICK) > -+ refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD", > -+ NULL, 0); > - return; > - } > + ## builtin/revert.c ## > +@@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *prefix) > > + opts.action = REPLAY_PICK; > + sequencer_init_config(&opts); > ++ unsetenv("GIT_CHERRY_PICK_HELP"); > + res = run_sequencer(argc, argv, &opts); > + if (res < 0) > + die(_("cherry-pick failed")); > > ## t/t3507-cherry-pick-conflict.sh ## > +@@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry-pick --no-commit' " > + test_cmp expected actual > + " > + > ++test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' " > ++ pristine_detach initial && > ++ ( > ++ 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' > ++ EOF > ++ GIT_CHERRY_PICK_HELP='and then do something else' && > ++ export GIT_CHERRY_PICK_HELP && > ++ test_must_fail git cherry-pick picked 2>actual && > ++ test_cmp expected actual > ++ ) > ++" > ++ > + test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' ' > + pristine_detach initial && > + test_must_fail git cherry-pick picked && > @@ t/t3507-cherry-pick-conflict.sh: test_expect_success \ > test_must_fail git rev-parse --verify CHERRY_PICK_HEAD > ' > > -test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' ' > - pristine_detach initial && > -+test_expect_success 'GIT_CHERRY_PICK_HELP respects CHERRY_PICK_HEAD' ' > -+ git init repo && > - ( > -+ cd repo && > -+ git branch -M main && > -+ echo 1 >file && > -+ git add file && > -+ git commit -m 1 && > -+ echo 2 >file && > -+ git add file && > -+ git commit -m 2 && > -+ git checkout HEAD~ && > -+ echo 3 >file && > -+ git add file && > -+ git commit -m 3 && > - GIT_CHERRY_PICK_HELP="and then do something else" && > - export GIT_CHERRY_PICK_HELP && > +- ( > +- GIT_CHERRY_PICK_HELP="and then do something else" && > +- export GIT_CHERRY_PICK_HELP && > - test_must_fail git cherry-pick picked > - ) && > - test_must_fail git rev-parse --verify CHERRY_PICK_HEAD > -+ test_must_fail git cherry-pick main && > -+ git rev-parse --verify CHERRY_PICK_HEAD >actual && > -+ git rev-parse --verify main >expect && > -+ test_cmp expect actual && > -+ git cherry-pick --abort > -+ ) > - ' > - > +-' > +- > test_expect_success 'git reset clears CHERRY_PICK_HEAD' ' > + pristine_detach initial && > + > > > builtin/revert.c | 1 + > t/t3507-cherry-pick-conflict.sh | 27 +++++++++++++++++---------- > 2 files changed, 18 insertions(+), 10 deletions(-) > > diff --git a/builtin/revert.c b/builtin/revert.c > index 237f2f18d4c..ec0abe7db73 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) > > opts.action = REPLAY_PICK; > sequencer_init_config(&opts); > + unsetenv("GIT_CHERRY_PICK_HELP"); This will break git-rebase--preserve-merges.sh which uses GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is removed when picking commits. I'm a bit confused as to what the problem is - how is 'git cherry-pick' being run with GIT_CHERRY_PICK_HELP set in the environment outside of a rebase (your explanation in [1] does not mention how GIT_CHERRY_PICK_HELP is set)? As far as I can see 'git rebase -i' does not set it so the only case I can think of is cherry-picking from an exec command while running 'git rebase -p' Best Wishes Phillip [1] https://lore.kernel.org/git/CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com/ > res = run_sequencer(argc, argv, &opts); > if (res < 0) > die(_("cherry-pick failed")); > diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh > index 014001b8f32..6f8035399d9 100755 > --- a/t/t3507-cherry-pick-conflict.sh > +++ b/t/t3507-cherry-pick-conflict.sh > @@ -76,6 +76,23 @@ test_expect_success 'advice from failed cherry-pick --no-commit' " > test_cmp expected actual > " > > +test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' " > + pristine_detach initial && > + ( > + 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' > + EOF > + GIT_CHERRY_PICK_HELP='and then do something else' && > + export GIT_CHERRY_PICK_HELP && > + test_must_fail git cherry-pick picked 2>actual && > + test_cmp expected actual > + ) > +" > + > test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' ' > pristine_detach initial && > test_must_fail git cherry-pick picked && > @@ -109,16 +126,6 @@ test_expect_success \ > test_must_fail git rev-parse --verify CHERRY_PICK_HEAD > ' > > -test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' ' > - pristine_detach initial && > - ( > - GIT_CHERRY_PICK_HELP="and then do something else" && > - export GIT_CHERRY_PICK_HELP && > - test_must_fail git cherry-pick picked > - ) && > - test_must_fail git rev-parse --verify CHERRY_PICK_HEAD > -' > - > test_expect_success 'git reset clears CHERRY_PICK_HEAD' ' > pristine_detach initial && > > > base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006 >
Phillip Wood <phillip.wood123@gmail.com> writes: > This will break git-rebase--preserve-merges.sh which uses > GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is > removed when picking commits. Ahh, I didn't realize we still had scripted rebase backends that called cherry-pick as an executable. I was hoping that all rebase backends by now would be calling into the cherry-pick machinery directly, bypassing cmd_cherry_pick(), and that was why I suggested to catch stray one the end-users set manually in the environment and clear it there. > I'm a bit confused as to what the > problem is - how is 'git cherry-pick' being run with > GIT_CHERRY_PICK_HELP set in the environment outside of a rebase (your > explanation in [1] does not mention how GIT_CHERRY_PICK_HELP is set)? I didn't press for the information too hard, but I guessed that it was perhaps because somebody like stackoverflow suggested to set a message in their environment to get a "better message."
Junio C Hamano <gitster@pobox.com> writes: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> This will break git-rebase--preserve-merges.sh which uses >> GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is >> removed when picking commits. > > Ahh, I didn't realize we still had scripted rebase backends that > called cherry-pick as an executable. I was hoping that all rebase > backends by now would be calling into the cherry-pick machinery > directly, bypassing cmd_cherry_pick(), and that was why I suggested > to catch stray one the end-users set manually in the environment > and clear it there. > >> I'm a bit confused as to what the >> problem is - how is 'git cherry-pick' being run with >> GIT_CHERRY_PICK_HELP set in the environment outside of a rebase (your >> explanation in [1] does not mention how GIT_CHERRY_PICK_HELP is set)? > > I didn't press for the information too hard, but I guessed that it > was perhaps because somebody like stackoverflow suggested to set a > message in their environment to get a "better message." A good way forward may be to relieve sequencer.c::print_advice() of the responsibility of optinally removing CHERRY_PICK_HEAD; make it a separate function that bases its decision on a more direct cue, not on the presense of a custom message in GIT_CHERRY_PICK_HELP, make do_pick_commit(), which is the sole caller of print_advice(), call it after calling print_advice(). I do not offhand know what that "direct cue" should be, but we may already have an appropriate field in the replay_opts structure; "replay.action is neither REVERT nor PICK" could be a good enough approximation, I dunno. Otherwise we can allocate a new bit in the structure, have relevant callers set it, and teach cherry-pick an unadvertised command line option that sets the bit, and use that option only from git-rebase--preserve-merges when it makes a call to cherry-pick. When "rebase -p" is either retired or rewritten in C, we can retire the option from cherry-pick. Workable?
Phillip Wood <phillip.wood123@gmail.com> 于2021年7月28日周三 上午3:43写道： > > > diff --git a/builtin/revert.c b/builtin/revert.c > > index 237f2f18d4c..ec0abe7db73 100644 > > --- a/builtin/revert.c > > +++ b/builtin/revert.c > > @@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) > > > > opts.action = REPLAY_PICK; > > sequencer_init_config(&opts); > > + unsetenv("GIT_CHERRY_PICK_HELP"); > > This will break git-rebase--preserve-merges.sh which uses > GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is > removed when picking commits. I'm a bit confused as to what the problem Yeah, I thought it would call some rebase-related code before, I didn’t expect it to call cherry-pick. On the other hand, I passed all tests, so I ignore it, there should be a test for it. > is - how is 'git cherry-pick' being run with GIT_CHERRY_PICK_HELP set in > the environment outside of a rebase (your explanation in [1] does not > mention how GIT_CHERRY_PICK_HELP is set)? As far as I can see 'git > rebase -i' does not set it so the only case I can think of is > cherry-picking from an exec command while running 'git rebase -p' > Ah, because I want to find a way to suppress its advice messages about "git commit", and I don’t think anyone else is using this "feature". > Best Wishes > > Phillip > > [1] > https://lore.kernel.org/git/CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com/ > Thanks. -- ZheNing Hu
Hi ZheNing On 28/07/2021 08:39, ZheNing Hu wrote: > Phillip Wood <phillip.wood123@gmail.com> 于2021年7月28日周三 上午3:43写道： >> >>> diff --git a/builtin/revert.c b/builtin/revert.c >>> index 237f2f18d4c..ec0abe7db73 100644 >>> --- a/builtin/revert.c >>> +++ b/builtin/revert.c >>> @@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) >>> >>> opts.action = REPLAY_PICK; >>> sequencer_init_config(&opts); >>> + unsetenv("GIT_CHERRY_PICK_HELP"); >> >> This will break git-rebase--preserve-merges.sh which uses >> GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is >> removed when picking commits. I'm a bit confused as to what the problem > > Yeah, I thought it would call some rebase-related code before, I > didn’t expect it to > call cherry-pick. On the other hand, I passed all tests, so I ignore > it, there should be > a test for it. > >> is - how is 'git cherry-pick' being run with GIT_CHERRY_PICK_HELP set in >> the environment outside of a rebase (your explanation in [1] does not >> mention how GIT_CHERRY_PICK_HELP is set)? As far as I can see 'git >> rebase -i' does not set it so the only case I can think of is >> cherry-picking from an exec command while running 'git rebase -p' >> > > Ah, because I want to find a way to suppress its advice messages about > "git commit", > and I don’t think anyone else is using this "feature". I'd welcome a patch to improve the advice. I suspect the current advice predates the introduction of the '--continue' flag for cherry-pick. I think that would be a better route forward as it would benefit all users. Setting GIT_CHERRY_PICK_HELP is undocumented and has always removed CHERRY_PICK_HEAD since CHERRY_PICK_HEAD was introduced in commit 7e5c0cbf (Introduce CHERRY_PICK_HEAD, 2011-02-19). Best Wishes Phillip >> Best Wishes >> >> Phillip >> >> [1] >> https://lore.kernel.org/git/CAOLTT8Ty47fyY7T3d68CYPKh9k+HAHsnCLJ=F0KaLm+0gp3+EQ@mail.gmail.com/ >> > > Thanks. > -- > ZheNing Hu >
On 27/07/2021 22:00, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Phillip Wood <phillip.wood123@gmail.com> writes: >> >>> This will break git-rebase--preserve-merges.sh which uses >>> GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is >>> removed when picking commits. >> >> Ahh, I didn't realize we still had scripted rebase backends that >> called cherry-pick as an executable. I was hoping that all rebase >> backends by now would be calling into the cherry-pick machinery >> directly, bypassing cmd_cherry_pick(), and that was why I suggested >> to catch stray one the end-users set manually in the environment >> and clear it there. >> >>> I'm a bit confused as to what the >>> problem is - how is 'git cherry-pick' being run with >>> GIT_CHERRY_PICK_HELP set in the environment outside of a rebase (your >>> explanation in [1] does not mention how GIT_CHERRY_PICK_HELP is set)? >> >> I didn't press for the information too hard, but I guessed that it >> was perhaps because somebody like stackoverflow suggested to set a >> message in their environment to get a "better message." > > A good way forward may be to relieve sequencer.c::print_advice() of > the responsibility of optinally removing CHERRY_PICK_HEAD; make it a > separate function that bases its decision on a more direct cue, not > on the presense of a custom message in GIT_CHERRY_PICK_HELP, make > do_pick_commit(), which is the sole caller of print_advice(), call > it after calling print_advice(). > > I do not offhand know what that "direct cue" should be, but we may > already have an appropriate field in the replay_opts structure; > "replay.action is neither REVERT nor PICK" could be a good enough > approximation, I dunno. > > Otherwise we can allocate a new bit in the structure, have relevant > callers set it, and teach cherry-pick an unadvertised command line > option that sets the bit, and use that option only from > git-rebase--preserve-merges when it makes a call to cherry-pick. > When "rebase -p" is either retired or rewritten in C, we can retire > the option from cherry-pick. > > Workable? Most of the time the builtin rebase should not be writing CHERRY_PICK_HEAD in the first place (it needs it when a commit becomes empty but not otherwise). For 'rebase -p' adding a command line option to cherry-pick as you suggest is probably the cleanest solution - in the short term 'rebase -i' could set it until we refactor the code that creates CHERRY_PICK_HEAD. One thing to note is that I think GIT_CHERRY_PICK_HELP was introduced to allow assist scripted rebase like porcelains rather than as a way to allow users to customize the help and setting it has always removed CHERRY_PICK_HEAD. There are however no users of GIT_CHERRY_PICK_HELP on codesearch.debian.org Best Wishes Phillip
Junio C Hamano <gitster@pobox.com> 于2021年7月28日周三 上午5:00写道： > > Junio C Hamano <gitster@pobox.com> writes: > > > Phillip Wood <phillip.wood123@gmail.com> writes: > > > >> This will break git-rebase--preserve-merges.sh which uses > >> GIT_CHERRY_PICK_HELP to set the help and ensure CHERRY_PICK_HEAD is > >> removed when picking commits. > > > > Ahh, I didn't realize we still had scripted rebase backends that > > called cherry-pick as an executable. I was hoping that all rebase > > backends by now would be calling into the cherry-pick machinery > > directly, bypassing cmd_cherry_pick(), and that was why I suggested > > to catch stray one the end-users set manually in the environment > > and clear it there. > > > >> I'm a bit confused as to what the > >> problem is - how is 'git cherry-pick' being run with > >> GIT_CHERRY_PICK_HELP set in the environment outside of a rebase (your > >> explanation in [1] does not mention how GIT_CHERRY_PICK_HELP is set)? > > > > I didn't press for the information too hard, but I guessed that it > > was perhaps because somebody like stackoverflow suggested to set a > > message in their environment to get a "better message." > > A good way forward may be to relieve sequencer.c::print_advice() of > the responsibility of optinally removing CHERRY_PICK_HEAD; make it a > separate function that bases its decision on a more direct cue, not > on the presense of a custom message in GIT_CHERRY_PICK_HELP, make > do_pick_commit(), which is the sole caller of print_advice(), call > it after calling print_advice(). > I think this function "check_need_delete_cherry_pick_head()" should be before print_advice(), like this: + const char *help_msgs = NULL; + error(command == TODO_REVERT ? _("could not revert %s... %s") : _("could not apply %s... %s"), short_commit_name(commit), msg.subject); - print_advice(r, res == 1, opts); + if (((opts->action == REPLAY_PICK && + !opts->rebase_preserve_merges_mode) || + (help_msgs = check_need_delete_cherry_pick_head(r))) && + res == 1) + print_advice(opts, help_msgs); > I do not offhand know what that "direct cue" should be, but we may > already have an appropriate field in the replay_opts structure; > "replay.action is neither REVERT nor PICK" could be a good enough > approximation, I dunno. > > Otherwise we can allocate a new bit in the structure, have relevant > callers set it, and teach cherry-pick an unadvertised command line > option that sets the bit, and use that option only from > git-rebase--preserve-merges when it makes a call to cherry-pick. > When "rebase -p" is either retired or rewritten in C, we can retire > the option from cherry-pick. > I think this one can be easily achieved. > Workable? Thanks. -- ZheNing Hu
Phillip Wood <phillip.wood123@gmail.com> 于2021年7月28日周三 下午5:46写道： > > Hi ZheNing > > > Ah, because I want to find a way to suppress its advice messages about > > "git commit", > > and I don’t think anyone else is using this "feature". > > I'd welcome a patch to improve the advice. I suspect the current advice > predates the introduction of the '--continue' flag for cherry-pick. I > think that would be a better route forward as it would benefit all > users. Setting GIT_CHERRY_PICK_HELP is undocumented and has always > removed CHERRY_PICK_HEAD since CHERRY_PICK_HEAD was introduced in commit > 7e5c0cbf (Introduce CHERRY_PICK_HEAD, 2011-02-19). > After I modify the interface of print_advice() in the way suggested by Junio, I can provide a help_msg parameter for print_advice(), and maybe we can use it to provide better advice later. Something like this: +static void print_advice(struct replay_opts *opts, const char *help_msgs) +{ + if (help_msgs) + fprintf(stderr, "%s\n", help_msgs); + else if (opts->no_commit) + advise(_("after resolving the conflicts, mark the corrected paths\n" + "with 'git add <paths>' or 'git rm <paths>'")); + else + 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'")); } > Best Wishes > > Phillip > Thanks. -- ZheNing Hu
Junio C Hamano <gitster@pobox.com> 于2021年7月28日周三 上午5:00写道： > Otherwise we can allocate a new bit in the structure, have relevant > callers set it, and teach cherry-pick an unadvertised command line > option that sets the bit, and use that option only from > git-rebase--preserve-merges when it makes a call to cherry-pick. > When "rebase -p" is either retired or rewritten in C, we can retire > the option from cherry-pick. > Just use a PARSE_OPT_HIDDEN cannot prevent the user from using the option... Is there a better way? -- ZheNing Hu
On 28/07/2021 12:01, ZheNing Hu wrote: > Phillip Wood <phillip.wood123@gmail.com> 于2021年7月28日周三 下午5:46写道： >> >> Hi ZheNing >> >>> Ah, because I want to find a way to suppress its advice messages about >>> "git commit", >>> and I don’t think anyone else is using this "feature". >> >> I'd welcome a patch to improve the advice. I suspect the current advice >> predates the introduction of the '--continue' flag for cherry-pick. I >> think that would be a better route forward as it would benefit all >> users. Setting GIT_CHERRY_PICK_HELP is undocumented and has always >> removed CHERRY_PICK_HEAD since CHERRY_PICK_HEAD was introduced in commit >> 7e5c0cbf (Introduce CHERRY_PICK_HEAD, 2011-02-19). >> > > After I modify the interface of print_advice() in the way suggested by > Junio, I can provide a > help_msg parameter for print_advice(), and maybe we can use it to > provide better advice later. I think that we could change the advice now to suggest using 'git cherry-pick --continue' instead of running 'git commit'. I think that in the long term we want to move away from being able to customize the advice when 'git rebase -p' is retired. Best Wishes Phillip > Something like this: > > +static void print_advice(struct replay_opts *opts, const char *help_msgs) > +{ > + if (help_msgs) > + fprintf(stderr, "%s\n", help_msgs); > + else if (opts->no_commit) > + advise(_("after resolving the conflicts, mark the > corrected paths\n" > + "with 'git add <paths>' or 'git rm <paths>'")); > + else > + 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'")); > } > >> Best Wishes >> >> Phillip >> > > Thanks. > -- > ZheNing Hu >
ZheNing Hu <adlternative@gmail.com> writes: > I think this function "check_need_delete_cherry_pick_head()" should be before > print_advice(), like this: > > + const char *help_msgs = NULL; > + > error(command == TODO_REVERT > ? _("could not revert %s... %s") > : _("could not apply %s... %s"), > short_commit_name(commit), msg.subject); > - print_advice(r, res == 1, opts); > + if (((opts->action == REPLAY_PICK && > + !opts->rebase_preserve_merges_mode) || > + (help_msgs = check_need_delete_cherry_pick_head(r))) && > + res == 1) > + print_advice(opts, help_msgs); Sorry, but I am not sure what problem this separation is trying to solve. The root cause of the issue we have, I think, is because the decision to delete or keep the cherry-pick-head pseudoref is tied to what message we give to users in the current code, and the suggested split of concern is to limit print_advice() to only print the advice message, and a new helper to decide and remove the pseudoref, without relying on what is in the GIT_CHERRY_PICK_HELP environment. It is unclear where you are making the decision to keep or remove the pseudoref in the above arrangement that lets the new check_need_delete_cherry_pick_head() decide if the advice is printed or not.
ZheNing Hu <adlternative@gmail.com> writes: > Just use a PARSE_OPT_HIDDEN cannot prevent the user from using > the option... Is there a better way? If a command can be invoked from a script, you can invoke it interactively the same way. Not advertising on short help, not completing in the completion and documenting it for internal use is the standard arrangement for such "implementation detail only" options.
diff --git a/builtin/revert.c b/builtin/revert.c index 237f2f18d4c..ec0abe7db73 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -245,6 +245,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) opts.action = REPLAY_PICK; sequencer_init_config(&opts); + unsetenv("GIT_CHERRY_PICK_HELP"); res = run_sequencer(argc, argv, &opts); if (res < 0) die(_("cherry-pick failed")); diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index 014001b8f32..6f8035399d9 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -76,6 +76,23 @@ test_expect_success 'advice from failed cherry-pick --no-commit' " test_cmp expected actual " +test_expect_success 'advice from failed cherry-pick with GIT_CHERRY_PICK_HELP' " + pristine_detach initial && + ( + 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' + EOF + GIT_CHERRY_PICK_HELP='and then do something else' && + export GIT_CHERRY_PICK_HELP && + test_must_fail git cherry-pick picked 2>actual && + test_cmp expected actual + ) +" + test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' ' pristine_detach initial && test_must_fail git cherry-pick picked && @@ -109,16 +126,6 @@ test_expect_success \ test_must_fail git rev-parse --verify CHERRY_PICK_HEAD ' -test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' ' - pristine_detach initial && - ( - GIT_CHERRY_PICK_HELP="and then do something else" && - export GIT_CHERRY_PICK_HELP && - test_must_fail git cherry-pick picked - ) && - test_must_fail git rev-parse --verify CHERRY_PICK_HEAD -' - test_expect_success 'git reset clears CHERRY_PICK_HEAD' ' pristine_detach initial &&