rebase -i: stop setting GIT_CHERRY_PICK_HELP

PATCH In review Phillip Wood via GitGitGadget 3 participants 3 messages Feb 27, 2024 – Feb 28, 2024

lore

Share & tools

lore

Phillip Wood via GitGitGadget

author

2024-02-27 14:06 UTC #0 lore raw

PATCH 1/1 rebase -i: stop setting GIT_CHERRY_PICK_HELP

From: Phillip Wood <[email protected]>

Setting this environment variable causes the sequencer to display a custom message when it stops for the user to resolve conflicts and remove CHERRY_PICK_HEAD. Setting it in "git rebase" is a vestige of the scripted implementation, now that it is a builtin command we do not need to communicate with the sequencer machinery via environment variables.

Move the conflicts advice to use when rebasing into sequencer.c so we do not need to pass it via the environment.

Note that we retain the changes in e4301f73fff (sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands, 2024-02-02) just in case GIT_CHERRY_PICK_HELP is set in the environment when "git rebase" is run.

Signed-off-by Phillip Wood <[email protected]>

---
rebase -i: stop setting GIT_CHERRY_PICK_HELP

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1678%2Fphillipwood%2Frebase-stop-setting-GIT_CHERRY_PICK_HELP-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1678/phillipwood/rebase-stop-setting-GIT_CHERRY_PICK_HELP-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1678

 builtin/rebase.c | 14 +++-----------
 sequencer.c      | 14 +++++++++++++-
 sequencer.h      |  2 ++
 3 files changed, 18 insertions(+), 12 deletions(-)

builtin/rebase.c +3 ~~-11~~

index 5b086f651a6..d0cc518c931 100644
@@ -567,13 +567,6 @@ static int move_to_original_branch(struct rebase_options *opts)
 	return ret;
 }
 
-static const char *resolvemsg =
-N_("Resolve all conflicts manually, mark them as resolved with\n"
-"\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"
-"You can instead skip this commit: run \"git rebase --skip\".\n"
-"To abort and get back to the state before \"git rebase\", run "
-"\"git rebase --abort\".");
-
 static int run_am(struct rebase_options *opts)
 {
 	struct child_process am = CHILD_PROCESS_INIT;

@@ -587,7 +580,7 @@ static int run_am(struct rebase_options *opts)
 		     opts->reflog_action);
 	if (opts->action == ACTION_CONTINUE) {
 		strvec_push(&am.args, "--resolved");
-		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
+		strvec_pushf(&am.args, "--resolvemsg=%s", rebase_resolvemsg);
 		if (opts->gpg_sign_opt)
 			strvec_push(&am.args, opts->gpg_sign_opt);
 		status = run_command(&am);

@@ -598,7 +591,7 @@ static int run_am(struct rebase_options *opts)
 	}
 	if (opts->action == ACTION_SKIP) {
 		strvec_push(&am.args, "--skip");
-		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
+		strvec_pushf(&am.args, "--resolvemsg=%s", rebase_resolvemsg);
 		status = run_command(&am);
 		if (status)
 			return status;

@@ -672,7 +665,7 @@ static int run_am(struct rebase_options *opts)
 
 	strvec_pushv(&am.args, opts->git_am_opts.v);
 	strvec_push(&am.args, "--rebasing");
-	strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
+	strvec_pushf(&am.args, "--resolvemsg=%s", rebase_resolvemsg);
 	strvec_push(&am.args, "--patch-format=mboxrd");
 	if (opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE)
 		strvec_push(&am.args, "--rerere-autoupdate");

@@ -700,7 +693,6 @@ static int run_specific_rebase(struct rebase_options *opts)
 
 	if (opts->type == REBASE_MERGE) {
 		/* Run sequencer-based rebase */
-		setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
 		if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT))
 			setenv("GIT_SEQUENCE_EDITOR", ":", 1);
 		if (opts->gpg_sign_opt) {

sequencer.c +13 -1

index f49a871ac06..76027ad5f5c 100644
@@ -461,10 +461,22 @@ static void free_message(struct commit *commit, struct commit_message *msg)
 	repo_unuse_commit_buffer(the_repository, commit, msg->message);
 }
 
+const char *rebase_resolvemsg =
+N_("Resolve all conflicts manually, mark them as resolved with\n"
+"\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"
+"You can instead skip this commit: run \"git rebase --skip\".\n"
+"To abort and get back to the state before \"git rebase\", run "
+"\"git rebase --abort\".");
+
 static void print_advice(struct repository *r, int show_hint,
 			 struct replay_opts *opts)
 {
-	char *msg = getenv("GIT_CHERRY_PICK_HELP");
+	const char *msg;
+
+	if (is_rebase_i(opts))
+		msg = rebase_resolvemsg;
+	else
+		msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	if (msg) {
 		advise("%s\n", msg);

sequencer.h +2 -0

index dcef7bb99c0..437eabd38af 100644
@@ -14,6 +14,8 @@ const char *rebase_path_todo(void);
 const char *rebase_path_todo_backup(void);
 const char *rebase_path_dropped(void);
 
+extern const char *rebase_resolvemsg;
+
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
 enum replay_action {

base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-02-27 18:38 UTC #1 lore raw

Quoted 4 lines · Phillip Wood via GitGitGadget

Note that we retain the changes in e4301f73fff (sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands, 2024-02-02) just in case GIT_CHERRY_PICK_HELP is set in the environment when "git rebase" is run.

Is this comment about this part of the code?

sequencer.c static void free_message(struct commit *commit, struct commit_message *msg) · line 479

Quoted 6 lines

sequencer.c +6 -0

+	const char *msg;
+
+	if (is_rebase_i(opts))
+		msg = rebase_resolvemsg;
+	else
+		msg = getenv("GIT_CHERRY_PICK_HELP");

Testing is_rebase_i() first means we ignore the environment unconditionally and use our own message always in "rebase -i", no?

Not that I think we should honor the environment variable and let it override our message. I just found the description a bit confusing.

sequencer.h const char *rebase_path_todo(void); · line 17

Quoted 9 lines

sequencer.h +1 -0

index dcef7bb99c0..437eabd38af 100644
@@ -14,6 +14,8 @@ const char *rebase_path_todo(void);
 const char *rebase_path_todo_backup(void);
 const char *rebase_path_dropped(void);
 
+extern const char *rebase_resolvemsg;

This is more library-ish part of the system than a random file in the builtin/ directory. This place as the final location for the string makes sense to me.

Thanks.

[email protected]

2024-02-28 14:45 UTC #2 lore raw

Quoted 8 lines · Junio C Hamano

Quoted 4 lines · Phillip Wood via GitGitGadget
Note that we retain the changes in e4301f73fff (sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands, 2024-02-02) just in case GIT_CHERRY_PICK_HELP is set in the environment when "git rebase" is run.
Is this comment about this part of the code?

No, it is about

        strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP");

in do_exec() which clears GIT_CHERRY_PICK_HELP in the child environment when running an exec command so that "exec git cherry-pick ..." retains the correct author information.

Quoted 9 lines

sequencer.c static void free_message(struct commit *commit, struct commit_message *msg) · line 479
Quoted 6 lines
sequencer.c +6 -0
+	const char *msg;
+
+	if (is_rebase_i(opts))
+		msg = rebase_resolvemsg;
+	else
+		msg = getenv("GIT_CHERRY_PICK_HELP");
Testing is_rebase_i() first means we ignore the environment unconditionally and use our own message always in "rebase -i", no?

Yes, this matches the existing behavior in builtin/rebase.c where we call

	setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);

to set GIT_CHERRY_PICK_HELP even if it is already set in the environment.

Quoted 2 lines

Not that I think we should honor the environment variable and let it override our message. I just found the description a bit confusing.

I should have been clearer what that it was talking about - i.e. it is still worth clearing GIT_CHERRY_PICK_HELP in the environment when running exec commands.

Best Wishes

Phillip

Quoted 15 lines

sequencer.h const char *rebase_path_todo(void); · line 17
Quoted 9 lines
sequencer.h +1 -0
index dcef7bb99c0..437eabd38af 100644
@@ -14,6 +14,8 @@ const char *rebase_path_todo(void);
  const char *rebase_path_todo_backup(void);
  const char *rebase_path_dropped(void);
 
+extern const char *rebase_resolvemsg;
This is more library-ish part of the system than a random file in the builtin/ directory. This place as the final location for the string makes sense to me.
Thanks.