Re: [PATCH v2 1/2] [GSOC] cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

From: ZheNing Hu <redacted>

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

set -> sets

value in `git-rebase--preserve-merges.sh`, `git rebase --merge` set

set -> sets

GIT_CHERRY_PICK_HELP value in run_specific_rebase().

"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.

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.

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

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`.

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.

Mentored-by: Christian Couder [off-list ref]
Mentored-by Hariom Verma [off-list ref]:
Helped-by: Phillip Wood [off-list ref]
Hepled-by: Junio C Hamano [off-list ref]

Heple?

OK.  That makes sense.

OK, this separation makes sense, too.

-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
-'

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 [off-list ref] 于2021年8月4日周三 上午6:36写道：

quoted

GIT_CHERRY_PICK_HELP value in run_specific_rebase().

"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.

Thanks, such a supplement is very good.

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")?

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:

Junio C Hamano [off-list ref] 于2021年8月4日周三 上午6:36写道：

quoted

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")?

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.

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

Thanks.
--
ZheNing Hu