{"thread":{"id":"61015","subject":"[PATCH] rebase -i: stop setting GIT_CHERRY_PICK_HELP","startedAt":"2024-02-27T14:06:27Z","lastAt":"2024-02-28T14:45:43Z","messageCount":3,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"489498","messageId":"pull.1678.git.1709042783847.gitgitgadget@gmail.com","threadId":"61015","inReplyTo":null,"subject":"[PATCH] rebase -i: stop setting GIT_CHERRY_PICK_HELP","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-27T14:06:23Z","receivedAt":"2024-02-27T14:06:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nSetting this environment variable causes the sequencer to display a\ncustom message when it stops for the user to resolve conflicts and\nremove CHERRY_PICK_HEAD. Setting it in \"git rebase\" is a vestige of\nthe scripted implementation, now that it is a builtin command we do\nnot need to communicate with the sequencer machinery via environment\nvariables.\n\nMove the conflicts advice to use when rebasing into\nsequencer.c so we do not need to pass it via the environment.\n\nNote that we retain the changes in e4301f73fff (sequencer: unset\nGIT_CHERRY_PICK_HELP for 'exec' commands, 2024-02-02) just in case\nGIT_CHERRY_PICK_HELP is set in the environment when \"git rebase\" is\nrun.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    rebase -i: stop setting GIT_CHERRY_PICK_HELP\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1678%2Fphillipwood%2Frebase-stop-setting-GIT_CHERRY_PICK_HELP-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1678/phillipwood/rebase-stop-setting-GIT_CHERRY_PICK_HELP-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1678\n\n builtin/rebase.c | 14 +++-----------\n sequencer.c      | 14 +++++++++++++-\n sequencer.h      |  2 ++\n 3 files changed, 18 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 5b086f651a6..d0cc518c931 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -567,13 +567,6 @@ static int move_to_original_branch(struct rebase_options *opts)\n \treturn ret;\n }\n \n-static const char *resolvemsg =\n-N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n-\"\\\"git add/rm <conflicted_files>\\\", then run \\\"git rebase --continue\\\".\\n\"\n-\"You can instead skip this commit: run \\\"git rebase --skip\\\".\\n\"\n-\"To abort and get back to the state before \\\"git rebase\\\", run \"\n-\"\\\"git rebase --abort\\\".\");\n-\n static int run_am(struct rebase_options *opts)\n {\n \tstruct child_process am = CHILD_PROCESS_INIT;\n@@ -587,7 +580,7 @@ static int run_am(struct rebase_options *opts)\n \t\t     opts->reflog_action);\n \tif (opts->action == ACTION_CONTINUE) {\n \t\tstrvec_push(&am.args, \"--resolved\");\n-\t\tstrvec_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n+\t\tstrvec_pushf(&am.args, \"--resolvemsg=%s\", rebase_resolvemsg);\n \t\tif (opts->gpg_sign_opt)\n \t\t\tstrvec_push(&am.args, opts->gpg_sign_opt);\n \t\tstatus = run_command(&am);\n@@ -598,7 +591,7 @@ static int run_am(struct rebase_options *opts)\n \t}\n \tif (opts->action == ACTION_SKIP) {\n \t\tstrvec_push(&am.args, \"--skip\");\n-\t\tstrvec_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n+\t\tstrvec_pushf(&am.args, \"--resolvemsg=%s\", rebase_resolvemsg);\n \t\tstatus = run_command(&am);\n \t\tif (status)\n \t\t\treturn status;\n@@ -672,7 +665,7 @@ static int run_am(struct rebase_options *opts)\n \n \tstrvec_pushv(&am.args, opts->git_am_opts.v);\n \tstrvec_push(&am.args, \"--rebasing\");\n-\tstrvec_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n+\tstrvec_pushf(&am.args, \"--resolvemsg=%s\", rebase_resolvemsg);\n \tstrvec_push(&am.args, \"--patch-format=mboxrd\");\n \tif (opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE)\n \t\tstrvec_push(&am.args, \"--rerere-autoupdate\");\n@@ -700,7 +693,6 @@ static int run_specific_rebase(struct rebase_options *opts)\n \n \tif (opts->type == REBASE_MERGE) {\n \t\t/* Run sequencer-based rebase */\n-\t\tsetenv(\"GIT_CHERRY_PICK_HELP\", resolvemsg, 1);\n \t\tif (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT))\n \t\t\tsetenv(\"GIT_SEQUENCE_EDITOR\", \":\", 1);\n \t\tif (opts->gpg_sign_opt) {\ndiff --git a/sequencer.c b/sequencer.c\nindex f49a871ac06..76027ad5f5c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -461,10 +461,22 @@ static void free_message(struct commit *commit, struct commit_message *msg)\n \trepo_unuse_commit_buffer(the_repository, commit, msg->message);\n }\n \n+const char *rebase_resolvemsg =\n+N_(\"Resolve all conflicts manually, mark them as resolved with\\n\"\n+\"\\\"git add/rm <conflicted_files>\\\", then run \\\"git rebase --continue\\\".\\n\"\n+\"You can instead skip this commit: run \\\"git rebase --skip\\\".\\n\"\n+\"To abort and get back to the state before \\\"git rebase\\\", run \"\n+\"\\\"git rebase --abort\\\".\");\n+\n static void print_advice(struct repository *r, int show_hint,\n \t\t\t struct replay_opts *opts)\n {\n-\tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n+\tconst char *msg;\n+\n+\tif (is_rebase_i(opts))\n+\t\tmsg = rebase_resolvemsg;\n+\telse\n+\t\tmsg = getenv(\"GIT_CHERRY_PICK_HELP\");\n \n \tif (msg) {\n \t\tadvise(\"%s\\n\", msg);\ndiff --git a/sequencer.h b/sequencer.h\nindex dcef7bb99c0..437eabd38af 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -14,6 +14,8 @@ const char *rebase_path_todo(void);\n const char *rebase_path_todo_backup(void);\n const char *rebase_path_dropped(void);\n \n+extern const char *rebase_resolvemsg;\n+\n #define APPEND_SIGNOFF_DEDUP (1u << 0)\n \n enum replay_action {\n\nbase-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0\n-- \ngitgitgadget\n"},{"id":"489560","messageId":"xmqqil29sqfp.fsf@gitster.g","threadId":"61015","inReplyTo":"pull.1678.git.1709042783847.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase -i: stop setting GIT_CHERRY_PICK_HELP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T18:38:34Z","receivedAt":"2024-02-27T18:38:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Note that we retain the changes in e4301f73fff (sequencer: unset\n> GIT_CHERRY_PICK_HELP for 'exec' commands, 2024-02-02) just in case\n> GIT_CHERRY_PICK_HELP is set in the environment when \"git rebase\" is\n> run.\n\nIs this comment about this part of the code?\n\n> +\tconst char *msg;\n> +\n> +\tif (is_rebase_i(opts))\n> +\t\tmsg = rebase_resolvemsg;\n> +\telse\n> +\t\tmsg = getenv(\"GIT_CHERRY_PICK_HELP\");\n\nTesting is_rebase_i() first means we ignore the environment\nunconditionally and use our own message always in \"rebase -i\", no?\n\nNot that I think we should honor the environment variable and let it\noverride our message.  I just found the description a bit confusing.\n\n> diff --git a/sequencer.h b/sequencer.h\n> index dcef7bb99c0..437eabd38af 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -14,6 +14,8 @@ const char *rebase_path_todo(void);\n>  const char *rebase_path_todo_backup(void);\n>  const char *rebase_path_dropped(void);\n>  \n> +extern const char *rebase_resolvemsg;\n\nThis is more library-ish part of the system than a random file in\nthe builtin/ directory.  This place as the final location for the\nstring makes sense to me.\n\nThanks.\n"},{"id":"489615","messageId":"a11e3e0a-23a7-422f-9f3b-ab673860f1f8@gmail.com","threadId":"61015","inReplyTo":"xmqqil29sqfp.fsf@gitster.g","subject":"Re: [PATCH] rebase -i: stop setting GIT_CHERRY_PICK_HELP","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-28T14:45:36Z","receivedAt":"2024-02-28T14:45:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 27/02/2024 18:38, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> Note that we retain the changes in e4301f73fff (sequencer: unset\n>> GIT_CHERRY_PICK_HELP for 'exec' commands, 2024-02-02) just in case\n>> GIT_CHERRY_PICK_HELP is set in the environment when \"git rebase\" is\n>> run.\n> \n> Is this comment about this part of the code?\n\nNo, it is about\n\n         strvec_push(&cmd.env, \"GIT_CHERRY_PICK_HELP\");\n\nin do_exec() which clears GIT_CHERRY_PICK_HELP in the child environment \nwhen running an exec command so that \"exec git cherry-pick ...\" retains \nthe correct author information.\n\n>> +\tconst char *msg;\n>> +\n>> +\tif (is_rebase_i(opts))\n>> +\t\tmsg = rebase_resolvemsg;\n>> +\telse\n>> +\t\tmsg = getenv(\"GIT_CHERRY_PICK_HELP\");\n> \n> Testing is_rebase_i() first means we ignore the environment\n> unconditionally and use our own message always in \"rebase -i\", no?\n\nYes, this matches the existing behavior in builtin/rebase.c where we call\n\n\tsetenv(\"GIT_CHERRY_PICK_HELP\", resolvemsg, 1);\n\nto set GIT_CHERRY_PICK_HELP even if it is already set in the environment.\n\n> Not that I think we should honor the environment variable and let it\n> override our message.  I just found the description a bit confusing.\n\nI should have been clearer what that it was talking about - i.e. it is \nstill worth clearing GIT_CHERRY_PICK_HELP in the environment when \nrunning exec commands.\n\nBest Wishes\n\nPhillip\n\n\n>> diff --git a/sequencer.h b/sequencer.h\n>> index dcef7bb99c0..437eabd38af 100644\n>> --- a/sequencer.h\n>> +++ b/sequencer.h\n>> @@ -14,6 +14,8 @@ const char *rebase_path_todo(void);\n>>   const char *rebase_path_todo_backup(void);\n>>   const char *rebase_path_dropped(void);\n>>   \n>> +extern const char *rebase_resolvemsg;\n> \n> This is more library-ish part of the system than a random file in\n> the builtin/ directory.  This place as the final location for the\n> string makes sense to me.\n> \n> Thanks.\n"}]}