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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 3, 2021, 22:36 UTC
Message-ID: <xmqqa6lytat9.fsf@gitster.g>
In-Reply-To: <5d2fd55c580abc2057f2e6fe9f7d9c94748bf8de.1627953383.git.gitgitgadget@gmail.com>

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: ZheNing Hu <adlternative@gmail.com>
>
> 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 <christian.couder@gmail.com>
> Mentored-by Hariom Verma <hariom18599@gmail.com>:
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Hepled-by: Junio C Hamano <gitster@pobox.com>

Heple?

Show 20 quoted lines

> diff --git a/sequencer.c b/sequencer.c
> index 0bec01cf38e..83cf6a5da3c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -397,24 +397,13 @@ static void free_message(struct commit *commit, struct commit_message *msg)
>  	unuse_commit_buffer(commit, msg->message);
>  }
>  
> -static void print_advice(struct repository *r, int show_hint,
> -			 struct replay_opts *opts)
> +static void print_advice(struct replay_opts *opts, int show_hint)
>  {
>  	char *msg = getenv("GIT_CHERRY_PICK_HELP");
>  
>  	if (msg) {
> +		advise("%s\n", msg);
> +	} else if (show_hint) {
>  		if (opts->no_commit)
>  			advise(_("after resolving the conflicts, mark the corrected paths\n"
>  				 "with 'git add <paths>' or 'git rm <paths>'"));

OK.  That makes sense.

Show 15 quoted lines

> @@ -2265,7 +2254,16 @@ static int do_pick_commit(struct repository *r,
>  		      ? _("could not revert %s... %s")
>  		      : _("could not apply %s... %s"),
>  		      short_commit_name(commit), msg.subject);
> -		print_advice(r, res == 1, opts);
> +		print_advice(opts, res == 1);
> +		if (opts->delete_cherry_pick_head) {
> +			/*
> +			 * A conflict has occurred but the porcelain
> +			 * (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);
> +		}

OK, this separation makes sense, too.

Show 9 quoted lines

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