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

[Phillip Wood <phillip.wood123@gmail.com>]

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