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

Commit Message

ZheNing Hu Aug. 3, 2021, 1:16 a.m. UTC

From: ZheNing Hu <[e-mail]>

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
value in `git-rebase--preserve-merges.sh`, `git rebase --merge` set
GIT_CHERRY_PICK_HELP value in run_specific_rebase(). 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.

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`. After `git rebase -p` completely
abolished, this option should be removed. At the same time, add the flag
`delete_cherry_pick_head` to `struct replay_opts`, We can decide whether
to delete CHERRY_PICK_HEAD by setting and checking this flag bit.

Then we split print_advice() into two part: Firstly, print_advice()
will only be responsible for outputting content; Secondly, check if
we set the `delete_cherry_pick_head` flag; if set, delete CHERRY_PICK_HEAD.
In this way, the steps of printing advice and deleting CHERRY_PICK_HEAD
are decoupled. Finally, let `git-rebase--preserve-merges.sh` use the
`--delete-cherry-pick-head` option when it executes git cherry-pick, and
set the `delete_cherry_pick_head` flag in get_replay_opts() when we
are using `git rebase --merge`, which can fix this breakage.

It is worth mentioning that now we use advice() to print the content
of GIT_CHERRY_PICK_HELP in print_advice(), each line of output will
start with "hint: ".

Mentored-by: Christian Couder <[e-mail]>
Mentored-by Hariom Verma <[e-mail]>:
Helped-by: Phillip Wood <[e-mail]>
Hepled-by: Junio C Hamano <[e-mail]>
Signed-off-by: ZheNing Hu <[e-mail]>
---
 builtin/rebase.c                |  1 +
 builtin/revert.c                |  2 ++
 git-rebase--preserve-merges.sh  |  2 +-
 sequencer.c                     | 28 +++++++++++++---------------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 31 +++++++++++++++++++++----------
 6 files changed, 39 insertions(+), 26 deletions(-)

Comments

Junio C Hamano Aug. 3, 2021, 10:36 p.m. UTC | #1

"ZheNing Hu via GitGitGadget" <[e-mail]> writes:

> From: ZheNing Hu <[e-mail]>
>
> 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 <[e-mail]>
> Mentored-by Hariom Verma <[e-mail]>:
> Helped-by: Phillip Wood <[e-mail]>
> Hepled-by: Junio C Hamano <[e-mail]>

Heple?

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

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

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

ZheNing Hu Aug. 4, 2021, 8:35 a.m. UTC | #2

Junio C Hamano <[e-mail]> 于2021年8月4日周三 上午6:36写道：
>

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

Phillip Wood Aug. 4, 2021, 10:10 a.m. UTC | #3

On 04/08/2021 09:35, ZheNing Hu wrote:
> Junio C Hamano <[e-mail]> 于2021年8月4日周三 上午6:36写道：
>> 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
>

Junio C Hamano Aug. 4, 2021, 5:31 p.m. UTC | #4

Phillip Wood <[e-mail]> writes:

>> You mean that cherry_pick with GIT_CHERRY_PICK_HELP suppresses
>> CHERRY_PICK_HEAD is not even a bug?

I think that it is what the existing test is telling us.  Of course,
with a good reason, an earlier design can be updated as long as we
make sure we won't hurt existing users who may rely on the current
design, but ...

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

... I think that is a reasonable stance to take [*1*].  If the
default help message can be improved, that is a good thing to do
regardless.

Thanks.

[Footnote]

*1* Tying the "here is a custom HELP text" environment variable to
"having a customization means whoever is driving the cherry-pick
machinery is ALSO responsible for sequencing and we will remove
CHERRY_PICK_HEAD" is a rather unfortunate design, but as long as
that is documented, it is a workable design.

ZheNing Hu Aug. 5, 2021, 5:36 a.m. UTC | #5

Junio C Hamano <[e-mail]> 于2021年8月5日周四 上午1:31写道：
>
> Phillip Wood <[e-mail]> writes:
>
> >> You mean that cherry_pick with GIT_CHERRY_PICK_HELP suppresses
> >> CHERRY_PICK_HEAD is not even a bug?
>
> I think that it is what the existing test is telling us.  Of course,
> with a good reason, an earlier design can be updated as long as we
> make sure we won't hurt existing users who may rely on the current
> design, but ...
>
> > 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.
>
> ... I think that is a reasonable stance to take [*1*].  If the
> default help message can be improved, that is a good thing to do
> regardless.
>

Well, this is indeed a bit strange, but maybe your and Phillip's
intuitions are right,
then I will delete the content of the first patch and keep the second.

> Thanks.
>
> [Footnote]
>
> *1* Tying the "here is a custom HELP text" environment variable to
> "having a customization means whoever is driving the cherry-pick
> machinery is ALSO responsible for sequencing and we will remove
> CHERRY_PICK_HEAD" is a rather unfortunate design, but as long as
> that is documented, it is a workable design.

Thanks.
--
ZheNing Hu

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 12f093121d9..08ba437c6a0 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -152,6 +152,7 @@  static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 		oidcpy(&replay.squash_onto, opts->squash_onto);
 		replay.have_squash_onto = 1;
 	}
+	replay.delete_cherry_pick_head = 1;
 
 	return replay;
 }
diff --git a/builtin/revert.c b/builtin/revert.c
index 237f2f18d4c..15a4b6fe4ee 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -127,6 +127,8 @@  static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 			OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
 			OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
 			OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
+			OPT_BOOL_F(0, "delete-cherry-pick-head", &opts->delete_cherry_pick_head,
+				   N_("delete CHERRY_PICK_HEAD when conflict occurs"), PARSE_OPT_HIDDEN),
 			OPT_END(),
 		};
 		options = parse_options_concat(options, cp_extra);
diff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh
index b9c71d2a71b..eaa8f9de2c5 100644
--- a/git-rebase--preserve-merges.sh
+++ b/git-rebase--preserve-merges.sh
@@ -444,7 +444,7 @@  pick_one_preserving_merges () {
 			output eval git cherry-pick $allow_rerere_autoupdate \
 				$allow_empty_message \
 				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-				"$strategy_args" "$@" ||
+				"$strategy_args" --delete-cherry-pick-head "$@" ||
 				die_with_patch $sha1 "$(eval_gettext "Could not pick \$sha1")"
 			;;
 		esac
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) {
-		fprintf(stderr, "%s\n", msg);
-		/*
-		 * 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);
-		return;
-	}
-
-	if (show_hint) {
+		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>'"));
@@ -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);
+		}
 		repo_rerere(r, opts->allow_rerere_auto);
 		goto leave;
 	}
diff --git a/sequencer.h b/sequencer.h
index d57d8ea23d7..76fb4af56fd 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -49,6 +49,7 @@  struct replay_opts {
 	int reschedule_failed_exec;
 	int committer_date_is_author_date;
 	int ignore_date;
+	int delete_cherry_pick_head;
 
 	int mainline;
 
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 014001b8f32..af5678d981a 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -76,12 +76,33 @@  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: and then do something else
+		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 &&
 	test_cmp_rev picked CHERRY_PICK_HEAD
 '
 
+test_expect_success 'failed cherry-pick with --delete-cherry-pick-head does not set CHERRY_PICK_HEAD' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --delete-cherry-pick-head picked &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
+'
+
 test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 	git cherry-pick base &&
@@ -109,16 +130,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 &&