{"thread":{"id":"53154","subject":"[PATCH] sequencer: honor GIT_REFLOG_ACTION","startedAt":"2020-04-01T20:31:41Z","lastAt":"2020-04-07T23:05:08Z","messageCount":14,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Ian Jackson","Elijah Newren","Phillip Wood","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"394519","messageId":"pull.746.git.git.1585773096145.gitgitgadget@gmail.com","threadId":"53154","inReplyTo":null,"subject":"[PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-01T20:31:35Z","receivedAt":"2020-04-01T20:31:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThere is a lot of code to honor GIT_REFLOG_ACTION throughout git,\nincluding some in sequencer.c; unfortunately, reflog_message() and its\ncallers ignored it. Instruct reflog_message() to check the existing\nenvironment variable, and use it when present as an override to\naction_name().\n\nAlso restructure pick_commits() to only temporarily modify\nGIT_REFLOG_ACTION for a short duration and then restore the old value,\nso that when we do this setting within a loop we do not keep adding \"\n(pick)\" substrings and end up with a reflog message of the form\n rebase (pick) (pick) (pick) (finish): returning to refs/heads/master\n\nSigned-off-by: Elijah Newren \n---\n sequencer: honor GIT_REFLOG_ACTION\n \n I'm not the best with getenv/setenv. The xstrdup() wrapping is\n apparently necessary on mac and bsd. The xstrdup seems like it leaves us\n with a memory leak, but since setenv(3) says to not alter or free it, I\n think it's right. Anyone have any alternative suggestions?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-746%2Fnewren%2Fhonor-reflog-action-in-sequencer-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-746/newren/honor-reflog-action-in-sequencer-v1\nPull-Request: https://github.com/git/git/pull/746\n\n sequencer.c | 9 +++++++--\n t/t3406-rebase-message.sh | 16 ++++++++--------\n 2 files changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e528225e787..5837fdaabbe 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3708,10 +3708,11 @@ static const char *reflog_message(struct replay_opts *opts,\n {\n \tva_list ap;\n \tstatic struct strbuf buf = STRBUF_INIT;\n+\tchar *reflog_action = getenv(\"GIT_REFLOG_ACTION\");\n \n \tva_start(ap, fmt);\n \tstrbuf_reset(&buf);\n-\tstrbuf_addstr(&buf, action_name(opts));\n+\tstrbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n \tif (sub_action)\n \t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n \tif (fmt) {\n@@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,\n \t\t\tstruct replay_opts *opts)\n {\n \tint res = 0, reschedule = 0;\n+\tchar *prev_reflog_action;\n \n \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n+\tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t\topts->record_origin || opts->edit));\n@@ -3845,12 +3848,14 @@ static int pick_commits(struct repository *r,\n \t\t}\n \t\tif (item->command <= TODO_SQUASH) {\n \t\t\tif (is_rebase_i(opts))\n-\t\t\t\tsetenv(\"GIT_REFLOG_ACTION\", reflog_message(opts,\n+\t\t\t\tsetenv(GIT_REFLOG_ACTION, reflog_message(opts,\n \t\t\t\t\tcommand_to_string(item->command), NULL),\n \t\t\t\t\t1);\n \t\t\tres = do_pick_commit(r, item->command, item->commit,\n \t\t\t\t\t opts, is_final_fixup(todo_list),\n \t\t\t\t\t &check_todo);\n+\t\t\tif (is_rebase_i(opts))\n+\t\t\t\tsetenv(GIT_REFLOG_ACTION, prev_reflog_action, 1);\n \t\t\tif (is_rebase_i(opts) && res < 0) {\n \t\t\t\t/* Reschedule */\n \t\t\t\tadvise(_(rescheduled_advice),\ndiff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\nindex 61b76f33019..927a4f4a4e4 100755\n--- a/t/t3406-rebase-message.sh\n+++ b/t/t3406-rebase-message.sh\n@@ -89,22 +89,22 @@ test_expect_success 'GIT_REFLOG_ACTION' '\n \tgit checkout -b reflog-topic start &&\n \ttest_commit reflog-to-rebase &&\n \n-\tgit rebase --apply reflog-onto &&\n+\tgit rebase reflog-onto &&\n \tgit log -g --format=%gs -3 >actual &&\n \tcat >expect <<-\\EOF &&\n-\trebase finished: returning to refs/heads/reflog-topic\n-\trebase: reflog-to-rebase\n-\trebase: checkout reflog-onto\n+\trebase (finish): returning to refs/heads/reflog-topic\n+\trebase (pick): reflog-to-rebase\n+\trebase (start): checkout reflog-onto\n \tEOF\n \ttest_cmp expect actual &&\n \n \tgit checkout -b reflog-prefix reflog-to-rebase &&\n-\tGIT_REFLOG_ACTION=change-the-reflog git rebase --apply reflog-onto &&\n+\tGIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&\n \tgit log -g --format=%gs -3 >actual &&\n \tcat >expect <<-\\EOF &&\n-\trebase finished: returning to refs/heads/reflog-prefix\n-\tchange-the-reflog: reflog-to-rebase\n-\tchange-the-reflog: checkout reflog-onto\n+\tchange-the-reflog (finish): returning to refs/heads/reflog-prefix\n+\tchange-the-reflog (pick): reflog-to-rebase\n+\tchange-the-reflog (start): checkout reflog-onto\n \tEOF\n \ttest_cmp expect actual\n '\n\nbase-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925\n-- \ngitgitgadget\n"},{"id":"394521","messageId":"xmqqftdn53z6.fsf@gitster.c.googlers.com","threadId":"53154","inReplyTo":"pull.746.git.git.1585773096145.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-01T20:46:21Z","receivedAt":"2020-04-01T20:46:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> There is a lot of code to honor GIT_REFLOG_ACTION throughout git,\n> including some in sequencer.c; unfortunately, reflog_message() and its\n> callers ignored it. Instruct reflog_message() to check the existing\n> environment variable, and use it when present as an override to\n> action_name().\n>\n> Also restructure pick_commits() to only temporarily modify\n> GIT_REFLOG_ACTION for a short duration and then restore the old value,\n\nYeah, I was wondering what you'd be doing about that setenv(). The\ncode around there looks good. I briefly wondered what would happen\nwhen the environment variable is totally unset upon entry, but then\nwe'd have the fallback value of action_name(opts) in there, so we\nwon't have a risk of running xstrdup(NULL).\n\n"},{"id":"394533","messageId":"24197.9157.362143.972556@chiark.greenend.org.uk","threadId":"53154","inReplyTo":"pull.746.git.git.1585773096145.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2020-04-01T23:29:09Z","receivedAt":"2020-04-01T23:29:13Z","isPatch":true,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"Hi. Thanks for looking at this.\n\nElijah Newren via GitGitGadget writes (\"[PATCH] sequencer: honor GIT_REFLOG_ACTION\"):\n> I'm not the best with getenv/setenv. The xstrdup() wrapping is\n> apparently necessary on mac and bsd. The xstrdup seems like it leaves us\n> with a memory leak, but since setenv(3) says to not alter or free it, I\n> think it's right. Anyone have any alternative suggestions?\n\nI can try to help. It's not entirely trivial.\n\nThe setenv interface is a wrapper around putenv. putenv has had a\nvariety of different semantics. Some of these sets of semantics\ncannot be used to re-set the same environment variable without a\nmemory leak - and even figuring out what semantics you have would be\ncomplex and tend to produce code which would fail in bad ways.\nThere's a short summary of the situation in Linux's putenv(3).\n\nWould it be possible for git to arrange to set GIT_REFLOG_ACTION only\nwhen it is invoking subprocesses ? Otherwise it would update, and\nlook at, a global variable of its own. (Or a parameter to relevant\nfunctions if one doesn't like the action-at-a-distance effect of a\nglobal.)\n\nAnd, it seems to me that the reflog handling should be centralised.\n\n> +\tchar *reflog_action = getenv(\"GIT_REFLOG_ACTION\");\n> \n> \tva_start(ap, fmt);\n> \tstrbuf_reset(&buf);\n> -\tstrbuf_addstr(&buf, action_name(opts));\n> +\tstrbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n\nOpen coding this kind of thing at every site which needs to think\nabout the reflog actions will surely result in some of the instances\nhaving bugs.\n\nWriting a single function that contans this (or most of it) would\nhappily decouple all of its call sites from literally asking about\ngetenv(\"GIT_REFLOG_ACTION\") thereby making it easier to do the\nindirection-through-program-variables I suggest.\n\nHaving said that,\n\n> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\n> index 61b76f33019..927a4f4a4e4 100755\n> --- a/t/t3406-rebase-message.sh\n> +++ b/t/t3406-rebase-message.sh\n\nThis test case convinces me that the patch has the right behaviour for\nat least the case I care about :-).\n\nThanks,\nIan.\n\n-- \nIan Jackson These opinions are my own.\n\nIf I emailed you from an address @fyvzl.net or @evade.org.uk, that is\na private address which bypasses my fierce spamfilter.\n"},{"id":"394543","messageId":"CABPp-BGo=6W5wfba7us8ca3eAfz04v8WxyOQ96DkoXn2fV=J1Q@mail.gmail.com","threadId":"53154","inReplyTo":"24197.9157.362143.972556@chiark.greenend.org.uk","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-04-02T05:15:37Z","receivedAt":"2020-04-02T05:15:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 1, 2020 at 4:29 PM Ian Jackson\n wrote:\n>\n> Hi. Thanks for looking at this.\n>\n> Elijah Newren via GitGitGadget writes (\"[PATCH] sequencer: honor GIT_REFLOG_ACTION\"):\n> > I'm not the best with getenv/setenv. The xstrdup() wrapping is\n> > apparently necessary on mac and bsd. The xstrdup seems like it leaves us\n> > with a memory leak, but since setenv(3) says to not alter or free it, I\n> > think it's right. Anyone have any alternative suggestions?\n>\n> I can try to help. It's not entirely trivial.\n>\n> The setenv interface is a wrapper around putenv. putenv has had a\n> variety of different semantics. Some of these sets of semantics\n> cannot be used to re-set the same environment variable without a\n> memory leak - and even figuring out what semantics you have would be\n> complex and tend to produce code which would fail in bad ways.\n> There's a short summary of the situation in Linux's putenv(3).\n>\n> Would it be possible for git to arrange to set GIT_REFLOG_ACTION only\n> when it is invoking subprocesses ? Otherwise it would update, and\n> look at, a global variable of its own. (Or a parameter to relevant\n> functions if one doesn't like the action-at-a-distance effect of a\n> global.)\n>\n> And, it seems to me that the reflog handling should be centralised.\n>\n> > + char *reflog_action = getenv(\"GIT_REFLOG_ACTION\");\n> >\n> > va_start(ap, fmt);\n> > strbuf_reset(&buf);\n> > - strbuf_addstr(&buf, action_name(opts));\n> > + strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n>\n> Open coding this kind of thing at every site which needs to think\n> about the reflog actions will surely result in some of the instances\n> having bugs.\n>\n> Writing a single function that contans this (or most of it) would\n> happily decouple all of its call sites from literally asking about\n> getenv(\"GIT_REFLOG_ACTION\") thereby making it easier to do the\n> indirection-through-program-variables I suggest.\n\nThat sounds great, but I'm not sure that \"only when invoking\nsubprocesses\" will limit the places where we set the environment\nvariable all that much; it might actually expand it. I wasn't there\nfor the whole history, but my understanding is the rebase code has\nslowly transformed from the original all-shell rebase\nimplementation(s), to being a helper program that the shell could call\ninto for parts of its operations and passing control back and forth\nbetween shell and C, to being a reimplementation of just invoking the\nsame commands that the shell script would have, to slowly transforming\ninto an actual library where invocations of other git subprocesses are\nbeing replaced with relevant function calls. It's a long cleanup\nprocess that is still ongoing. I'd like to get to the point where we\nonly invoke subprocesses if the user specifies --exec or a special\nmerge strategy, but that's a goal with a longer term timeframe than\nfixing a 2.26 regression.\n\n> Having said that,\n>\n> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\n> > index 61b76f33019..927a4f4a4e4 100755\n> > --- a/t/t3406-rebase-message.sh\n> > +++ b/t/t3406-rebase-message.sh\n>\n> This test case convinces me that the patch has the right behaviour for\n> at least the case I care about :-).\n\nCool, sounds like it's a good immediate fix for the 2.26 regression,\nand then longer term as we continue refactoring we can hopefully\nisolate subprocess handling and writing of state.\n\nAs a heads up, though, my personal plans for rebase (subject to buy-in\nfrom other stakeholders) is to make it do a lot more in-memory work.\nIn particular, this means for common cases there will be no subprocess\ninvocations, no writing of any state unless/until you hit a conflict,\nno updating of any files in the working tree until all commits have\nbeen created (or a conflict is hit), and no updating of the branch\nuntil after all the commits have been created. Thus, for the common\ncases with no conflicts, there would only be 1 entry in the reflog of\nHEAD the entire operation, rather than approximately 1 per commit. I\nhave a proof-of-concept showing these ideas work for basic cases. So,\nI hope your tests don't depend on the number of entries added to\nHEAD's reflog.\n"},{"id":"394556","messageId":"b187cb5f-a6c8-2908-e3fd-e1210e6970e0@gmail.com","threadId":"53154","inReplyTo":"pull.746.git.git.1585773096145.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-04-02T09:25:29Z","receivedAt":"2020-04-02T09:25:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nThanks for fixing this\n\nOn 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> There is a lot of code to honor GIT_REFLOG_ACTION throughout git,\n> including some in sequencer.c; unfortunately, reflog_message() and its\n> callers ignored it. Instruct reflog_message() to check the existing\n> environment variable, and use it when present as an override to\n> action_name().\n> \n> Also restructure pick_commits() to only temporarily modify\n> GIT_REFLOG_ACTION for a short duration and then restore the old value,\n> so that when we do this setting within a loop we do not keep adding \"\n> (pick)\" substrings and end up with a reflog message of the form\n> rebase (pick) (pick) (pick) (finish): returning to refs/heads/master\n> \n> Signed-off-by: Elijah Newren \n> ---\n> sequencer: honor GIT_REFLOG_ACTION\n> \n> I'm not the best with getenv/setenv. The xstrdup() wrapping is\n> apparently necessary on mac and bsd. The xstrdup seems like it leaves us\n> with a memory leak, but since setenv(3) says to not alter or free it, I\n> think it's right. Anyone have any alternative suggestions?\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-746%2Fnewren%2Fhonor-reflog-action-in-sequencer-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-746/newren/honor-reflog-action-in-sequencer-v1\n> Pull-Request: https://github.com/git/git/pull/746\n> \n> sequencer.c | 9 +++++++--\n> t/t3406-rebase-message.sh | 16 ++++++++--------\n> 2 files changed, 15 insertions(+), 10 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index e528225e787..5837fdaabbe 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3708,10 +3708,11 @@ static const char *reflog_message(struct replay_opts *opts,\n> {\n> \tva_list ap;\n> \tstatic struct strbuf buf = STRBUF_INIT;\n> +\tchar *reflog_action = getenv(\"GIT_REFLOG_ACTION\");\n\nMinor nit - you're using a string here rather that the pre-processor \nconstant that is used below\n\n> \tva_start(ap, fmt);\n> \tstrbuf_reset(&buf);\n> -\tstrbuf_addstr(&buf, action_name(opts));\n> +\tstrbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n> \tif (sub_action)\n> \t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n> \tif (fmt) {\n> @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,\n> \t\t\tstruct replay_opts *opts)\n> {\n> \tint res = 0, reschedule = 0;\n> +\tchar *prev_reflog_action;\n> \n> \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n> +\tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n\nI'm confused as to why saving the environment variable immediately after \nsetting it works but the test shows it does - why doesn't this clobber \nthe value of GIT_REFLOG_ACTION set by the user?\n\nBest Wishes\n\nPhillip\n\n> \tif (opts->allow_ff)\n> \t\tassert(!(opts->signoff || opts->no_commit ||\n> \t\t\t\topts->record_origin || opts->edit));\n> @@ -3845,12 +3848,14 @@ static int pick_commits(struct repository *r,\n> \t\t}\n> \t\tif (item->command <= TODO_SQUASH) {\n> \t\t\tif (is_rebase_i(opts))\n> -\t\t\t\tsetenv(\"GIT_REFLOG_ACTION\", reflog_message(opts,\n> +\t\t\t\tsetenv(GIT_REFLOG_ACTION, reflog_message(opts,\n> \t\t\t\t\tcommand_to_string(item->command), NULL),\n> \t\t\t\t\t1);\n> \t\t\tres = do_pick_commit(r, item->command, item->commit,\n> \t\t\t\t\t opts, is_final_fixup(todo_list),\n> \t\t\t\t\t &check_todo);\n> +\t\t\tif (is_rebase_i(opts))\n> +\t\t\t\tsetenv(GIT_REFLOG_ACTION, prev_reflog_action, 1);\n> \t\t\tif (is_rebase_i(opts) && res < 0) {\n> \t\t\t\t/* Reschedule */\n> \t\t\t\tadvise(_(rescheduled_advice),\n> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\n> index 61b76f33019..927a4f4a4e4 100755\n> --- a/t/t3406-rebase-message.sh\n> +++ b/t/t3406-rebase-message.sh\n> @@ -89,22 +89,22 @@ test_expect_success 'GIT_REFLOG_ACTION' '\n> \tgit checkout -b reflog-topic start &&\n> \ttest_commit reflog-to-rebase &&\n> \n> -\tgit rebase --apply reflog-onto &&\n> +\tgit rebase reflog-onto &&\n> \tgit log -g --format=%gs -3 >actual &&\n> \tcat >expect <<-\\EOF &&\n> -\trebase finished: returning to refs/heads/reflog-topic\n> -\trebase: reflog-to-rebase\n> -\trebase: checkout reflog-onto\n> +\trebase (finish): returning to refs/heads/reflog-topic\n> +\trebase (pick): reflog-to-rebase\n> +\trebase (start): checkout reflog-onto\n> \tEOF\n> \ttest_cmp expect actual &&\n> \n> \tgit checkout -b reflog-prefix reflog-to-rebase &&\n> -\tGIT_REFLOG_ACTION=change-the-reflog git rebase --apply reflog-onto &&\n> +\tGIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&\n> \tgit log -g --format=%gs -3 >actual &&\n> \tcat >expect <<-\\EOF &&\n> -\trebase finished: returning to refs/heads/reflog-prefix\n> -\tchange-the-reflog: reflog-to-rebase\n> -\tchange-the-reflog: checkout reflog-onto\n> +\tchange-the-reflog (finish): returning to refs/heads/reflog-prefix\n> +\tchange-the-reflog (pick): reflog-to-rebase\n> +\tchange-the-reflog (start): checkout reflog-onto\n> \tEOF\n> \ttest_cmp expect actual\n> '\n> \n> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925\n> \n"},{"id":"394557","messageId":"5c5532ac-7df5-e8ef-9122-f015783427c2@gmail.com","threadId":"53154","inReplyTo":"CABPp-BGo=6W5wfba7us8ca3eAfz04v8WxyOQ96DkoXn2fV=J1Q@mail.gmail.com","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-04-02T09:39:34Z","receivedAt":"2020-04-02T09:39:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 02/04/2020 06:15, Elijah Newren wrote:\n> On Wed, Apr 1, 2020 at 4:29 PM Ian Jackson\n> wrote:\n>>\n>> Hi. Thanks for looking at this.\n>>\n>> Elijah Newren via GitGitGadget writes (\"[PATCH] sequencer: honor GIT_REFLOG_ACTION\"):\n>>> I'm not the best with getenv/setenv. The xstrdup() wrapping is\n>>> apparently necessary on mac and bsd. The xstrdup seems like it leaves us\n>>> with a memory leak, but since setenv(3) says to not alter or free it, I\n>>> think it's right. Anyone have any alternative suggestions?\n>>\n>> I can try to help. It's not entirely trivial.\n>>\n>> The setenv interface is a wrapper around putenv. putenv has had a\n>> variety of different semantics. Some of these sets of semantics\n>> cannot be used to re-set the same environment variable without a\n>> memory leak - and even figuring out what semantics you have would be\n>> complex and tend to produce code which would fail in bad ways.\n>> There's a short summary of the situation in Linux's putenv(3).\n>>\n>> Would it be possible for git to arrange to set GIT_REFLOG_ACTION only\n>> when it is invoking subprocesses ? Otherwise it would update, and >> look at, a global variable of its own. (Or a parameter to relevant\n>> functions if one doesn't like the action-at-a-distance effect of a\n>> global.)\n>>\n>> And, it seems to me that the reflog handling should be centralised.\n>>\n>>> + char *reflog_action = getenv(\"GIT_REFLOG_ACTION\");\n>>>\n>>> va_start(ap, fmt);\n>>> strbuf_reset(&buf);\n>>> - strbuf_addstr(&buf, action_name(opts));\n>>> + strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n>>\n>> Open coding this kind of thing at every site which needs to think\n>> about the reflog actions will surely result in some of the instances\n>> having bugs.\n>>\n>> Writing a single function that contans this (or most of it) would\n>> happily decouple all of its call sites from literally asking about\n>> getenv(\"GIT_REFLOG_ACTION\") thereby making it easier to do the\n>> indirection-through-program-variables I suggest.\n> \n> That sounds great, but I'm not sure that \"only when invoking\n> subprocesses\" will limit the places where we set the environment\n> variable all that much; it might actually expand it.\n\nOff hand I think we'd need to change run_git_checkout(), \nrun_git_commit() and do_merge() to set the environment variable and fix \ntry_to_commit() to use a proper variable for the reflog message.\n\n> I wasn't there\n> for the whole history, but my understanding is the rebase code has\n> slowly transformed from the original all-shell rebase\n> implementation(s), to being a helper program that the shell could call\n> into for parts of its operations and passing control back and forth\n> between shell and C, to being a reimplementation of just invoking the\n> same commands that the shell script would have, to slowly transforming\n> into an actual library where invocations of other git subprocesses are\n> being replaced with relevant function calls. It's a long cleanup\n> process that is still ongoing. I'd like to get to the point where we\n> only invoke subprocesses if the user specifies --exec or a special\n> merge strategy, but that's a goal with a longer term timeframe than\n> fixing a 2.26 regression.\n> \n>> Having said that,\n>>\n>>> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\n>>> index 61b76f33019..927a4f4a4e4 100755\n>>> --- a/t/t3406-rebase-message.sh\n>>> +++ b/t/t3406-rebase-message.sh\n>>\n>> This test case convinces me that the patch has the right behaviour for\n>> at least the case I care about :-).\n> \n> Cool, sounds like it's a good immediate fix for the 2.26 regression,\n> and then longer term as we continue refactoring we can hopefully\n> isolate subprocess handling and writing of state.\n> \n> As a heads up, though, my personal plans for rebase (subject to buy-in\n> from other stakeholders) is to make it do a lot more in-memory work.\n> In particular, this means for common cases there will be no subprocess\n> invocations, no writing of any state unless/until you hit a conflict,\n> no updating of any files in the working tree until all commits have\n> been created (or a conflict is hit), \n\nI'm with you up to here - it sounds fantastic\n\n> and no updating of the branch\n> until after all the commits have been created.\n\nWe only update the branch reflog at the end of the rebase now.\n\n> Thus, for the common\n> cases with no conflicts, there would only be 1 entry in the reflog of\n> HEAD the entire operation, rather than approximately 1 per commit. \n\nThis I'm not so sure about. In the past where I've messed up a rebase \nand not noticed until after a subsequent rebase it has been really \nuseful to be able to go through HEAD's reflog and find out where exactly \nI messed up by looking at the sequence of picks for the first rebase. \nSpecifically it shows which commits where squashed together which you \ncannot get by running git log on the result of the rebase.\n\n> I\n> have a proof-of-concept showing these ideas work for basic cases. \n\nSounds exciting\n\nBest Wishes\n\nPhillip\n\n So,\n> I hope your tests don't depend on the number of entries added to\n> HEAD's reflog.\n> \n"},{"id":"394606","messageId":"CABPp-BE_mimSRg5wf0Yzn2s-dX=64ZS1jGszqwHzr3aju0bj=A@mail.gmail.com","threadId":"53154","inReplyTo":"b187cb5f-a6c8-2908-e3fd-e1210e6970e0@gmail.com","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-04-02T17:01:34Z","receivedAt":"2020-04-02T17:01:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Phillip,\n\nOn Thu, Apr 2, 2020 at 2:25 AM Phillip Wood wrote:\n>\n> Hi Elijah\n>\n> Thanks for fixing this\n>\n> On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > There is a lot of code to honor GIT_REFLOG_ACTION throughout git,\n> > including some in sequencer.c; unfortunately, reflog_message() and its\n> > callers ignored it. Instruct reflog_message() to check the existing\n> > environment variable, and use it when present as an override to\n> > action_name().\n> >\n> > Also restructure pick_commits() to only temporarily modify\n> > GIT_REFLOG_ACTION for a short duration and then restore the old value,\n> > so that when we do this setting within a loop we do not keep adding \"\n> > (pick)\" substrings and end up with a reflog message of the form\n> > rebase (pick) (pick) (pick) (finish): returning to refs/heads/master\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > sequencer: honor GIT_REFLOG_ACTION\n> >\n> > I'm not the best with getenv/setenv. The xstrdup() wrapping is\n> > apparently necessary on mac and bsd. The xstrdup seems like it leaves us\n> > with a memory leak, but since setenv(3) says to not alter or free it, I\n> > think it's right. Anyone have any alternative suggestions?\n> >\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-746%2Fnewren%2Fhonor-reflog-action-in-sequencer-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-746/newren/honor-reflog-action-in-sequencer-v1\n> > Pull-Request: https://github.com/git/git/pull/746\n> >\n> > sequencer.c | 9 +++++++--\n> > t/t3406-rebase-message.sh | 16 ++++++++--------\n> > 2 files changed, 15 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index e528225e787..5837fdaabbe 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -3708,10 +3708,11 @@ static const char *reflog_message(struct replay_opts *opts,\n> > {\n> > va_list ap;\n> > static struct strbuf buf = STRBUF_INIT;\n> > + char *reflog_action = getenv(\"GIT_REFLOG_ACTION\");\n>\n> Minor nit - you're using a string here rather that the pre-processor\n> constant that is used below\n\nYeah, true. However, using a mixture of both styles is consistent\nwith the current code's inconsistency about which one should be used.\n:-)\n\n> > va_start(ap, fmt);\n> > strbuf_reset(&buf);\n> > - strbuf_addstr(&buf, action_name(opts));\n> > + strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n> > if (sub_action)\n> > strbuf_addf(&buf, \" (%s)\", sub_action);\n> > if (fmt) {\n> > @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,\n> > struct replay_opts *opts)\n> > {\n> > int res = 0, reschedule = 0;\n> > + char *prev_reflog_action;\n> >\n> > setenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n> > + prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n>\n> I'm confused as to why saving the environment variable immediately after\n> setting it works but the test shows it does - why doesn't this clobber\n> the value of GIT_REFLOG_ACTION set by the user?\n\nThe third parameter, 0, means only set the environment variable if\nit's not already set.\n"},{"id":"394609","messageId":"CABPp-BGPFTOX75CfWwJxcoBMs_DC3xBXOBDeqFE5439v0fqBZw@mail.gmail.com","threadId":"53154","inReplyTo":"5c5532ac-7df5-e8ef-9122-f015783427c2@gmail.com","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-04-02T17:40:22Z","receivedAt":"2020-04-02T17:40:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Phillip,\n\nOn Thu, Apr 2, 2020 at 2:39 AM Phillip Wood wrote:\n>\n> On 02/04/2020 06:15, Elijah Newren wrote:\n\n> > As a heads up, though, my personal plans for rebase (subject to buy-in\n> > from other stakeholders) is to make it do a lot more in-memory work.\n> > In particular, this means for common cases there will be no subprocess\n> > invocations, no writing of any state unless/until you hit a conflict,\n> > no updating of any files in the working tree until all commits have\n> > been created (or a conflict is hit),\n>\n> I'm with you up to here - it sounds fantastic\n>\n> > and no updating of the branch\n> > until after all the commits have been created.\n>\n> We only update the branch reflog at the end of the rebase now.\n>\n> > Thus, for the common\n> > cases with no conflicts, there would only be 1 entry in the reflog of\n> > HEAD the entire operation, rather than approximately 1 per commit.\n>\n> This I'm not so sure about. In the past where I've messed up a rebase\n> and not noticed until after a subsequent rebase it has been really\n> useful to be able to go through HEAD's reflog and find out where exactly\n> I messed up by looking at the sequence of picks for the first rebase.\n> Specifically it shows which commits where squashed together which you\n> cannot get by running git log on the result of the rebase.\n\nInteresting. Hmm....\n\n> > I\n> > have a proof-of-concept showing these ideas work for basic cases.\n>\n> Sounds exciting\n\nFeel free to take a look: See\nhttps://github.com/newren/git/blob/git-merge-2020-demo/README.md and\nhttps://github.com/newren/git/blob/git-merge-2020-demo/builtin/fast-rebase.c\n\nSadly, between sparse-checkout, expoential slowdown in dir.c, and\nvarious reports about rebase for 2.26, I haven't been able to work on\nthat stuff since the morning of March 4. And I've been ignoring some\nnon-git stuff that I really need to work on. But hopefully I'll get\nto start cleaning things up soon and sending them on to the list for\nreview. I keep hoping...\n"},{"id":"394620","messageId":"09397e37-a22b-5159-b760-bae238ae3ed6@gmail.com","threadId":"53154","inReplyTo":"CABPp-BE_mimSRg5wf0Yzn2s-dX=64ZS1jGszqwHzr3aju0bj=A@mail.gmail.com","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-04-02T19:05:17Z","receivedAt":"2020-04-02T19:05:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 02/04/2020 18:01, Elijah Newren wrote:\n> Hi Phillip,\n> \n> On Thu, Apr 2, 2020 at 2:25 AM Phillip Wood wrote:\n>>\n>> Hi Elijah\n>>\n>> Thanks for fixing this\n>>\n>> On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:\n>>> From: Elijah Newren \n>>>\n>>> There is a lot of code to honor GIT_REFLOG_ACTION throughout git,\n>>> including some in sequencer.c; unfortunately, reflog_message() and its\n>>> callers ignored it. Instruct reflog_message() to check the existing\n>>> environment variable, and use it when present as an override to\n>>> action_name().\n>>>\n>>> Also restructure pick_commits() to only temporarily modify\n>>> GIT_REFLOG_ACTION for a short duration and then restore the old value,\n>>> so that when we do this setting within a loop we do not keep adding \"\n>>> (pick)\" substrings and end up with a reflog message of the form\n>>> rebase (pick) (pick) (pick) (finish): returning to refs/heads/master\n>>>\n>>> Signed-off-by: Elijah Newren \n>>> ---\n>>> sequencer: honor GIT_REFLOG_ACTION\n>>>\n>>> I'm not the best with getenv/setenv. The xstrdup() wrapping is\n>>> apparently necessary on mac and bsd. The xstrdup seems like it leaves us\n>>> with a memory leak, but since setenv(3) says to not alter or free it, I\n>>> think it's right. Anyone have any alternative suggestions?\n>>>\n>>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-746%2Fnewren%2Fhonor-reflog-action-in-sequencer-v1\n>>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-746/newren/honor-reflog-action-in-sequencer-v1\n>>> Pull-Request: https://github.com/git/git/pull/746\n>>>\n>>> sequencer.c | 9 +++++++--\n>>> t/t3406-rebase-message.sh | 16 ++++++++--------\n>>> 2 files changed, 15 insertions(+), 10 deletions(-)\n>>>\n>>> diff --git a/sequencer.c b/sequencer.c\n>>> index e528225e787..5837fdaabbe 100644\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> @@ -3708,10 +3708,11 @@ static const char *reflog_message(struct replay_opts *opts,\n>>> {\n>>> va_list ap;\n>>> static struct strbuf buf = STRBUF_INIT;\n>>> + char *reflog_action = getenv(\"GIT_REFLOG_ACTION\");\n>>\n>> Minor nit - you're using a string here rather that the pre-processor\n>> constant that is used below\n> \n> Yeah, true. However, using a mixture of both styles is consistent\n> with the current code's inconsistency about which one should be used.\n> :-)\n\nNice!\n\n>>> va_start(ap, fmt);\n>>> strbuf_reset(&buf);\n>>> - strbuf_addstr(&buf, action_name(opts));\n>>> + strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n>>> if (sub_action)\n>>> strbuf_addf(&buf, \" (%s)\", sub_action);\n>>> if (fmt) {\n>>> @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,\n>>> struct replay_opts *opts)\n>>> {\n>>> int res = 0, reschedule = 0;\n>>> + char *prev_reflog_action;\n>>>\n>>> setenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n>>> + prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n>>\n>> I'm confused as to why saving the environment variable immediately after\n>> setting it works but the test shows it does - why doesn't this clobber\n>> the value of GIT_REFLOG_ACTION set by the user?\n> \n> The third parameter, 0, means only set the environment variable if\n> it's not already set.\n\nAh thanks, I thought I must be missing something fairly obvious but \ncouldn't see what it was\n\nBest Wishes\n\nPhillip\n\n"},{"id":"394956","messageId":"nycvar.QRO.7.76.6.2004071649190.46@tvgsbejvaqbjf.bet","threadId":"53154","inReplyTo":"09397e37-a22b-5159-b760-bae238ae3ed6@gmail.com","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-07T14:50:17Z","receivedAt":"2020-04-07T14:50:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 Apr 2020, Phillip Wood wrote:\n\n> On 02/04/2020 18:01, Elijah Newren wrote:\n> >\n> > On Thu, Apr 2, 2020 at 2:25 AM Phillip Wood \n> > wrote:\n> > >\n> > > On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:\n> > >\n> > > > va_start(ap, fmt);\n> > > > strbuf_reset(&buf);\n> > > > - strbuf_addstr(&buf, action_name(opts));\n> > > > + strbuf_addstr(&buf, reflog_action ? reflog_action :\n> > > > action_name(opts));\n> > > > if (sub_action)\n> > > > strbuf_addf(&buf, \" (%s)\", sub_action);\n> > > > if (fmt) {\n> > > > @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,\n> > > > struct replay_opts *opts)\n> > > > {\n> > > > int res = 0, reschedule = 0;\n> > > > + char *prev_reflog_action;\n> > > >\n> > > > setenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n> > > > + prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n> > >\n> > > I'm confused as to why saving the environment variable immediately after\n> > > setting it works but the test shows it does - why doesn't this clobber\n> > > the value of GIT_REFLOG_ACTION set by the user?\n> >\n> > The third parameter, 0, means only set the environment variable if\n> > it's not already set.\n>\n> Ah thanks, I thought I must be missing something fairly obvious but couldn't\n> see what it was\n\nFWIW I was also about to comment on that. Maybe that warrants even a code\ncomment above the `prev_reflog_action`?\n\nCiao,\nDscho\n"},{"id":"394967","messageId":"CABPp-BFXT1QkTLUFSAju2TwzVdSRjKSyLQYp2KaoW2+S2U8KJw@mail.gmail.com","threadId":"53154","inReplyTo":"nycvar.QRO.7.76.6.2004071649190.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-04-07T15:18:33Z","receivedAt":"2020-04-07T15:18:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Apr 7, 2020 at 7:50 AM Johannes Schindelin\n wrote:\n>\n> Hi,\n>\n> On Thu, 2 Apr 2020, Phillip Wood wrote:\n>\n> > On 02/04/2020 18:01, Elijah Newren wrote:\n> > >\n> > > On Thu, Apr 2, 2020 at 2:25 AM Phillip Wood \n> > > wrote:\n> > > >\n> > > > On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:\n> > > >\n> > > > > va_start(ap, fmt);\n> > > > > strbuf_reset(&buf);\n> > > > > - strbuf_addstr(&buf, action_name(opts));\n> > > > > + strbuf_addstr(&buf, reflog_action ? reflog_action :\n> > > > > action_name(opts));\n> > > > > if (sub_action)\n> > > > > strbuf_addf(&buf, \" (%s)\", sub_action);\n> > > > > if (fmt) {\n> > > > > @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,\n> > > > > struct replay_opts *opts)\n> > > > > {\n> > > > > int res = 0, reschedule = 0;\n> > > > > + char *prev_reflog_action;\n> > > > >\n> > > > > setenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n> > > > > + prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n> > > >\n> > > > I'm confused as to why saving the environment variable immediately after\n> > > > setting it works but the test shows it does - why doesn't this clobber\n> > > > the value of GIT_REFLOG_ACTION set by the user?\n> > >\n> > > The third parameter, 0, means only set the environment variable if\n> > > it's not already set.\n> >\n> > Ah thanks, I thought I must be missing something fairly obvious but couldn't\n> > see what it was\n>\n> FWIW I was also about to comment on that. Maybe that warrants even a code\n> comment above the `prev_reflog_action`?\n\nYeah, if it tripped you both up, I'll add such a comment to the code\nto help explain it.\n"},{"id":"394976","messageId":"pull.746.v2.git.git.1586278763462.gitgitgadget@gmail.com","threadId":"53154","inReplyTo":"pull.746.git.git.1585773096145.gitgitgadget@gmail.com","subject":"[PATCH v2] sequencer: honor GIT_REFLOG_ACTION","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-07T16:59:23Z","receivedAt":"2020-04-07T16:59:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThere is a lot of code to honor GIT_REFLOG_ACTION throughout git,\nincluding some in sequencer.c; unfortunately, reflog_message() and its\ncallers ignored it. Instruct reflog_message() to check the existing\nenvironment variable, and use it when present as an override to\naction_name().\n\nAlso restructure pick_commits() to only temporarily modify\nGIT_REFLOG_ACTION for a short duration and then restore the old value,\nso that when we do this setting within a loop we do not keep adding \"\n(pick)\" substrings and end up with a reflog message of the form\n rebase (pick) (pick) (pick) (finish): returning to refs/heads/master\n\nSigned-off-by: Elijah Newren \n---\n sequencer: honor GIT_REFLOG_ACTION\n \n Changes since v1:\n \n * Just use preprocessor constant (instead of mix of it and string\n constant)\n * Add a comment next to code that surprised both Phillip and Dscho\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-746%2Fnewren%2Fhonor-reflog-action-in-sequencer-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-746/newren/honor-reflog-action-in-sequencer-v2\nPull-Request: https://github.com/git/git/pull/746\n\nRange-diff vs v1:\n\n 1: 5c6d74af194 ! 1: a12cc2d2c3a sequencer: honor GIT_REFLOG_ACTION\n @@ sequencer.c: static const char *reflog_message(struct replay_opts *opts,\n {\n \tva_list ap;\n \tstatic struct strbuf buf = STRBUF_INIT;\n -+\tchar *reflog_action = getenv(\"GIT_REFLOG_ACTION\");\n ++\tchar *reflog_action = getenv(GIT_REFLOG_ACTION);\n \n \tva_start(ap, fmt);\n \tstrbuf_reset(&buf);\n @@ sequencer.c: static int pick_commits(struct repository *r,\n \tint res = 0, reschedule = 0;\n +\tchar *prev_reflog_action;\n \n ++\t/* Note that 0 for 3rd parameter of setenv means set only if not set */\n \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n +\tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n \tif (opts->allow_ff)\n\n\n sequencer.c | 10 ++++++++--\n t/t3406-rebase-message.sh | 16 ++++++++--------\n 2 files changed, 16 insertions(+), 10 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e528225e787..24a62d5aa5d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3708,10 +3708,11 @@ static const char *reflog_message(struct replay_opts *opts,\n {\n \tva_list ap;\n \tstatic struct strbuf buf = STRBUF_INIT;\n+\tchar *reflog_action = getenv(GIT_REFLOG_ACTION);\n \n \tva_start(ap, fmt);\n \tstrbuf_reset(&buf);\n-\tstrbuf_addstr(&buf, action_name(opts));\n+\tstrbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n \tif (sub_action)\n \t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n \tif (fmt) {\n@@ -3799,8 +3800,11 @@ static int pick_commits(struct repository *r,\n \t\t\tstruct replay_opts *opts)\n {\n \tint res = 0, reschedule = 0;\n+\tchar *prev_reflog_action;\n \n+\t/* Note that 0 for 3rd parameter of setenv means set only if not set */\n \tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n+\tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t\topts->record_origin || opts->edit));\n@@ -3845,12 +3849,14 @@ static int pick_commits(struct repository *r,\n \t\t}\n \t\tif (item->command <= TODO_SQUASH) {\n \t\t\tif (is_rebase_i(opts))\n-\t\t\t\tsetenv(\"GIT_REFLOG_ACTION\", reflog_message(opts,\n+\t\t\t\tsetenv(GIT_REFLOG_ACTION, reflog_message(opts,\n \t\t\t\t\tcommand_to_string(item->command), NULL),\n \t\t\t\t\t1);\n \t\t\tres = do_pick_commit(r, item->command, item->commit,\n \t\t\t\t\t opts, is_final_fixup(todo_list),\n \t\t\t\t\t &check_todo);\n+\t\t\tif (is_rebase_i(opts))\n+\t\t\t\tsetenv(GIT_REFLOG_ACTION, prev_reflog_action, 1);\n \t\t\tif (is_rebase_i(opts) && res < 0) {\n \t\t\t\t/* Reschedule */\n \t\t\t\tadvise(_(rescheduled_advice),\ndiff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\nindex 61b76f33019..927a4f4a4e4 100755\n--- a/t/t3406-rebase-message.sh\n+++ b/t/t3406-rebase-message.sh\n@@ -89,22 +89,22 @@ test_expect_success 'GIT_REFLOG_ACTION' '\n \tgit checkout -b reflog-topic start &&\n \ttest_commit reflog-to-rebase &&\n \n-\tgit rebase --apply reflog-onto &&\n+\tgit rebase reflog-onto &&\n \tgit log -g --format=%gs -3 >actual &&\n \tcat >expect <<-\\EOF &&\n-\trebase finished: returning to refs/heads/reflog-topic\n-\trebase: reflog-to-rebase\n-\trebase: checkout reflog-onto\n+\trebase (finish): returning to refs/heads/reflog-topic\n+\trebase (pick): reflog-to-rebase\n+\trebase (start): checkout reflog-onto\n \tEOF\n \ttest_cmp expect actual &&\n \n \tgit checkout -b reflog-prefix reflog-to-rebase &&\n-\tGIT_REFLOG_ACTION=change-the-reflog git rebase --apply reflog-onto &&\n+\tGIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&\n \tgit log -g --format=%gs -3 >actual &&\n \tcat >expect <<-\\EOF &&\n-\trebase finished: returning to refs/heads/reflog-prefix\n-\tchange-the-reflog: reflog-to-rebase\n-\tchange-the-reflog: checkout reflog-onto\n+\tchange-the-reflog (finish): returning to refs/heads/reflog-prefix\n+\tchange-the-reflog (pick): reflog-to-rebase\n+\tchange-the-reflog (start): checkout reflog-onto\n \tEOF\n \ttest_cmp expect actual\n '\n\nbase-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925\n-- \ngitgitgadget\n"},{"id":"395020","messageId":"nycvar.QRO.7.76.6.2004080037080.46@tvgsbejvaqbjf.bet","threadId":"53154","inReplyTo":"CABPp-BFXT1QkTLUFSAju2TwzVdSRjKSyLQYp2KaoW2+S2U8KJw@mail.gmail.com","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-07T22:37:24Z","receivedAt":"2020-04-07T22:37:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Tue, 7 Apr 2020, Elijah Newren wrote:\n\n> On Tue, Apr 7, 2020 at 7:50 AM Johannes Schindelin\n> wrote:\n> >\n> > On Thu, 2 Apr 2020, Phillip Wood wrote:\n> >\n> > > On 02/04/2020 18:01, Elijah Newren wrote:\n> > > >\n> > > > On Thu, Apr 2, 2020 at 2:25 AM Phillip Wood \n> > > > wrote:\n> > > > >\n> > > > > On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:\n> > > > >\n> > > > > > va_start(ap, fmt);\n> > > > > > strbuf_reset(&buf);\n> > > > > > - strbuf_addstr(&buf, action_name(opts));\n> > > > > > + strbuf_addstr(&buf, reflog_action ? reflog_action :\n> > > > > > action_name(opts));\n> > > > > > if (sub_action)\n> > > > > > strbuf_addf(&buf, \" (%s)\", sub_action);\n> > > > > > if (fmt) {\n> > > > > > @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,\n> > > > > > struct replay_opts *opts)\n> > > > > > {\n> > > > > > int res = 0, reschedule = 0;\n> > > > > > + char *prev_reflog_action;\n> > > > > >\n> > > > > > setenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n> > > > > > + prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n> > > > >\n> > > > > I'm confused as to why saving the environment variable immediately after\n> > > > > setting it works but the test shows it does - why doesn't this clobber\n> > > > > the value of GIT_REFLOG_ACTION set by the user?\n> > > >\n> > > > The third parameter, 0, means only set the environment variable if\n> > > > it's not already set.\n> > >\n> > > Ah thanks, I thought I must be missing something fairly obvious but couldn't\n> > > see what it was\n> >\n> > FWIW I was also about to comment on that. Maybe that warrants even a code\n> > comment above the `prev_reflog_action`?\n>\n> Yeah, if it tripped you both up, I'll add such a comment to the code\n> to help explain it.\n\nThank you!\nDscho\n"},{"id":"395025","messageId":"xmqqwo6qrj6r.fsf@gitster.c.googlers.com","threadId":"53154","inReplyTo":"nycvar.QRO.7.76.6.2004080037080.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-07T23:05:00Z","receivedAt":"2020-04-07T23:05:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > FWIW I was also about to comment on that. Maybe that warrants even a code\n>> > comment above the `prev_reflog_action`?\n>>\n>> Yeah, if it tripped you both up, I'll add such a comment to the code\n>> to help explain it.\n>\n> Thank you!\n\nThanks, all.\n"}]}