diff mbox series

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

Message ID 5c5ec310b9230ee19bde4b3c8733b359d5218b32.1627561665.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series cherry-pick: fix bug when used with GIT_CHERRY_PICK_HELP | expand

Commit Message

ZheNing Hu July 29, 2021, 12:27 p.m. UTC 
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
value in git-rebase--preserve-merges.sh. 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 --rebase-preserve-merges-mode for git
cherry-pick which indicates that git cherry-pick is currently called by
git-rebase--preserve-merges.sh. After `git rebase -p` completely
abolished, this option should be removed.

And then we split print_advice() into two part:
check_need_delete_cherry_pick_head() check if we set
GIT_CHERRY_PICK_HELP, if set, delete CHERRY_PICK_HEAD and return
GIT_CHERRY_PICK_HELP's value, if not set, return NULL; The parameters
of print_advice() have changed, which now accept a
`struct replay_opts *opt` and a `const char *help_msgs`. We can pass
the value of GIT_CHERRY_PICK_HELP into print_advice(), and output it.
In this way, the steps of printing advice and checking
GIT_CHERRY_PICK_HELP are decoupled.

Finally, avoid deleting CHERRY_PICK_HEAD when we are truly
cherry-picking, which can fix this breakage.

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>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/revert.c                |  2 ++
 git-rebase--preserve-merges.sh  |  2 +-
 sequencer.c                     | 36 ++++++++++++++++++++-------------
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 27 ++++++++++++++++---------
 5 files changed, 43 insertions(+), 25 deletions(-)

Comments

Junio C Hamano July 29, 2021, 8:56 p.m. UTC | #1 
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -static void print_advice(struct repository *r, int show_hint,
> -			 struct replay_opts *opts)
> +static char *check_need_delete_cherry_pick_head(struct repository *r)
>  {
>  	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
> @@ -411,18 +409,22 @@ static void print_advice(struct repository *r, int show_hint,
>  		 */
>  		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
>  				NULL, 0);
> -		return;
> +		return msg;
>  	}
> +	return NULL;
> +}

The patch is a bit hard to read, but I do not think this improves
the root cause of the issue at all, which is that "do we need to
delete the pseudoref?" decision is tied to and based on the presense
of the custom message in the environment variable.

The original issue was that we declared (erroneously) that the
presence of a custom message _means_ the caller is not a human but a
sequencing machinery that wants to deal with what is done via the
CHERRY_PICK_HEAD pseudoref itself, and your original issue was that
when a human user has the custom message envirnoment (either on
purpose or by mistake), the logic _misidentifies_ the call as if it
is made from a sequencing machinery and we lose the CHERRY_PICK_HEAD
pseudoref.  The root cause is mistaking the correlation (sequencing
machinery uses the environment and it also does not want pseudoref)
as the causation (presense of the environment means we should remove
pseudoref).
ZheNing Hu July 30, 2021, 2:15 p.m. UTC | #2 
Junio C Hamano <gitster@pobox.com> 于2021年7月30日周五 上午4:56写道：
>
> The patch is a bit hard to read, but I do not think this improves
> the root cause of the issue at all, which is that "do we need to
> delete the pseudoref?" decision is tied to and based on the presense
> of the custom message in the environment variable.
>

My original idea was to distinguish between cherry-pick calls by machines
(git-rebase--preserve-merges.sh) and cherry-pick calls by people through
an additional option "rebase-preserve-merges-mode".

> The original issue was that we declared (erroneously) that the
> presence of a custom message _means_ the caller is not a human but a
> sequencing machinery that wants to deal with what is done via the
> CHERRY_PICK_HEAD pseudoref itself, and your original issue was that
> when a human user has the custom message envirnoment (either on
> purpose or by mistake), the logic _misidentifies_ the call as if it
> is made from a sequencing machinery and we lose the CHERRY_PICK_HEAD
> pseudoref.  The root cause is mistaking the correlation (sequencing
> machinery uses the environment and it also does not want pseudoref)
> as the causation (presense of the environment means we should remove
> pseudoref).

Ok... Now I guess what you mean is to provide an option for git cherry-pick
to specifically delete "CHERRY_PICK_HEAD", and this options is "hidden",
it is not provided to users, it is only reserved for sequencing
machinery. In this
way, we separate print_advice() from deleting "CHERRY_PICK_HEAD".
Then in the future, GIT_CHERRY_PICK_HELP will be deleted.

Thanks,
--
ZheNing Hu
diff mbox series

Patch

diff --git a/builtin/revert.c b/builtin/revert.c
index 237f2f18d4c..6165bb10143 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, "rebase-preserve-merges-mode", &opts->rebase_preserve_merges_mode,
+				   N_("use for git-rebase--preserve-merges backend"), 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..ca97d9b6539 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" --rebase-preserve-merges-mode "$@" ||
 				die_with_patch $sha1 "$(eval_gettext "Could not pick \$sha1")"
 			;;
 		esac
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..ceaf73a34df 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -397,13 +397,11 @@  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 char *check_need_delete_cherry_pick_head(struct repository *r)
 {
 	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
@@ -411,18 +409,22 @@  static void print_advice(struct repository *r, int show_hint,
 		 */
 		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
 				NULL, 0);
-		return;
+		return msg;
 	}
+	return NULL;
+}
 
-	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>'"));
-		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'"));
-	}
+static void print_advice(struct replay_opts *opts, const char *help_msgs)
+{
+	if (help_msgs)
+		advise("%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'"));
 }
 
 static int write_message(const void *buf, size_t len, const char *filename,
@@ -2261,11 +2263,17 @@  static int do_pick_commit(struct repository *r,
 		res = -1;
 
 	if (res) {
+		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);
 		repo_rerere(r, opts->allow_rerere_auto);
 		goto leave;
 	}
diff --git a/sequencer.h b/sequencer.h
index d57d8ea23d7..5a40b6d8bdc 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 rebase_preserve_merges_mode;
 
 	int mainline;
 
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 &&