patchsequencer: honor GIT_REFLOG_ACTION

> From: Elijah Newren <newren@gmail.com>
>
> There is a lot of code to honor GIT_REFLOG_ACTION throughout git,
> including some in sequencer.c; unfortunately, reflog_message() and its
> callers ignored it.  Instruct reflog_message() to check the existing
> environment variable, and use it when present as an override to
> action_name().
>
> Also restructure pick_commits() to only temporarily modify
> GIT_REFLOG_ACTION for a short duration and then restore the old value,

> +	char *reflog_action = getenv("GIT_REFLOG_ACTION");
>  
>  	va_start(ap, fmt);
>  	strbuf_reset(&buf);
> -	strbuf_addstr(&buf, action_name(opts));
> +	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));

>
> Hi.  Thanks for looking at this.
>
> Elijah Newren via GitGitGadget writes ("[PATCH] sequencer: honor GIT_REFLOG_ACTION"):
> >     I'm not the best with getenv/setenv. The xstrdup() wrapping is
> >     apparently necessary on mac and bsd. The xstrdup seems like it leaves us
> >     with a memory leak, but since setenv(3) says to not alter or free it, I
> >     think it's right. Anyone have any alternative suggestions?
>
> I can try to help.  It's not entirely trivial.
>
> The setenv interface is a wrapper around putenv.  putenv has had a
> variety of different semantics.  Some of these sets of semantics
> cannot be used to re-set the same environment variable without a
> memory leak - and even figuring out what semantics you have would be
> complex and tend to produce code which would fail in bad ways.
> There's a short summary of the situation in Linux's putenv(3).
>
> Would it be possible for git to arrange to set GIT_REFLOG_ACTION only
> when it is invoking subprocesses ?  Otherwise it would update, and
> look at, a global variable of its own.  (Or a parameter to relevant
> functions if one doesn't like the action-at-a-distance effect of a
> global.)
>
> And, it seems to me that the reflog handling should be centralised.
>
> > +     char *reflog_action = getenv("GIT_REFLOG_ACTION");
> >
> >       va_start(ap, fmt);
> >       strbuf_reset(&buf);
> > -     strbuf_addstr(&buf, action_name(opts));
> > +     strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
>
> Open coding this kind of thing at every site which needs to think
> about the reflog actions will surely result in some of the instances
> having bugs.
>
> Writing a single function that contans this (or most of it) would
> happily decouple all of its call sites from literally asking about
> getenv("GIT_REFLOG_ACTION") thereby making it easier to do the
> indirection-through-program-variables I suggest.

> Having said that,
>
> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> > index 61b76f33019..927a4f4a4e4 100755
> > --- a/t/t3406-rebase-message.sh
> > +++ b/t/t3406-rebase-message.sh
>
> This test case convinces me that the patch has the right behaviour for
> at least the case I care about :-).

> On Wed, Apr 1, 2020 at 4:29 PM Ian Jackson
> <ijackson@chiark.greenend.org.uk> wrote:
>>
>> Hi.  Thanks for looking at this.
>>
>> Elijah Newren via GitGitGadget writes ("[PATCH] sequencer: honor GIT_REFLOG_ACTION"):
>>>      I'm not the best with getenv/setenv. The xstrdup() wrapping is
>>>      apparently necessary on mac and bsd. The xstrdup seems like it leaves us
>>>      with a memory leak, but since setenv(3) says to not alter or free it, I
>>>      think it's right. Anyone have any alternative suggestions?
>>
>> I can try to help.  It's not entirely trivial.
>>
>> The setenv interface is a wrapper around putenv.  putenv has had a
>> variety of different semantics.  Some of these sets of semantics
>> cannot be used to re-set the same environment variable without a
>> memory leak - and even figuring out what semantics you have would be
>> complex and tend to produce code which would fail in bad ways.
>> There's a short summary of the situation in Linux's putenv(3).
>>
>> Would it be possible for git to arrange to set GIT_REFLOG_ACTION only
>> when it is invoking subprocesses ? Otherwise it would update, and >> look at, a global variable of its own.  (Or a parameter to relevant
>> functions if one doesn't like the action-at-a-distance effect of a
>> global.)
>>
>> And, it seems to me that the reflog handling should be centralised.
>>
>>> +     char *reflog_action = getenv("GIT_REFLOG_ACTION");
>>>
>>>        va_start(ap, fmt);
>>>        strbuf_reset(&buf);
>>> -     strbuf_addstr(&buf, action_name(opts));
>>> +     strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
>>
>> Open coding this kind of thing at every site which needs to think
>> about the reflog actions will surely result in some of the instances
>> having bugs.
>>
>> Writing a single function that contans this (or most of it) would
>> happily decouple all of its call sites from literally asking about
>> getenv("GIT_REFLOG_ACTION") thereby making it easier to do the
>> indirection-through-program-variables I suggest.
> 
> That sounds great, but I'm not sure that "only when invoking
> subprocesses" will limit the places where we set the environment
> variable all that much; it might actually expand it.

>  I wasn't there
> for the whole history, but my understanding is the rebase code has
> slowly transformed from the original all-shell rebase
> implementation(s), to being a helper program that the shell could call
> into for parts of its operations and passing control back and forth
> between shell and C, to being a reimplementation of just invoking the
> same commands that the shell script would have, to slowly transforming
> into an actual library where invocations of other git subprocesses are
> being replaced with relevant function calls.  It's a long cleanup
> process that is still ongoing.  I'd like to get to the point where we
> only invoke subprocesses if the user specifies --exec or a special
> merge strategy, but that's a goal with a longer term timeframe than
> fixing a 2.26 regression.
> 
>> Having said that,
>>
>>> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
>>> index 61b76f33019..927a4f4a4e4 100755
>>> --- a/t/t3406-rebase-message.sh
>>> +++ b/t/t3406-rebase-message.sh
>>
>> This test case convinces me that the patch has the right behaviour for
>> at least the case I care about :-).
> 
> Cool, sounds like it's a good immediate fix for the 2.26 regression,
> and then longer term as we continue refactoring we can hopefully
> isolate subprocess handling and writing of state.
> 
> As a heads up, though, my personal plans for rebase (subject to buy-in
> from other stakeholders) is to make it do a lot more in-memory work.
> In particular, this means for common cases there will be no subprocess
> invocations, no writing of any state unless/until you hit a conflict,
> no updating of any files in the working tree until all commits have
> been created (or a conflict is hit), 

> > As a heads up, though, my personal plans for rebase (subject to buy-in
> > from other stakeholders) is to make it do a lot more in-memory work.
> > In particular, this means for common cases there will be no subprocess
> > invocations, no writing of any state unless/until you hit a conflict,
> > no updating of any files in the working tree until all commits have
> > been created (or a conflict is hit),
>
> I'm with you up to here - it sounds fantastic
>
> > and no updating of the branch
> > until after all the commits have been created.
>
> We only update the branch reflog at the end of the rebase now.
>
> > Thus, for the common
> > cases with no conflicts, there would only be 1 entry in the reflog of
> > HEAD the entire operation, rather than approximately 1 per commit.
>
> This I'm not so sure about. In the past where I've messed up a rebase
> and not noticed until after a subsequent rebase it has been really
> useful to be able to go through HEAD's reflog and find out where exactly
> I messed up by looking at the sequence of picks for the first rebase.
> Specifically it shows which commits where squashed together which you
> cannot get by running git log on the result of the rebase.

> From: Elijah Newren <newren@gmail.com>
> 
> There is a lot of code to honor GIT_REFLOG_ACTION throughout git,
> including some in sequencer.c; unfortunately, reflog_message() and its
> callers ignored it.  Instruct reflog_message() to check the existing
> environment variable, and use it when present as an override to
> action_name().
> 
> Also restructure pick_commits() to only temporarily modify
> GIT_REFLOG_ACTION for a short duration and then restore the old value,
> so that when we do this setting within a loop we do not keep adding "
> (pick)" substrings and end up with a reflog message of the form
>      rebase (pick) (pick) (pick) (finish): returning to refs/heads/master
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>      sequencer: honor GIT_REFLOG_ACTION
>      
>      I'm not the best with getenv/setenv. The xstrdup() wrapping is
>      apparently necessary on mac and bsd. The xstrdup seems like it leaves us
>      with a memory leak, but since setenv(3) says to not alter or free it, I
>      think it's right. Anyone have any alternative suggestions?
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-746%2Fnewren%2Fhonor-reflog-action-in-sequencer-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-746/newren/honor-reflog-action-in-sequencer-v1
> Pull-Request: https://github.com/git/git/pull/746
> 
>   sequencer.c               |  9 +++++++--
>   t/t3406-rebase-message.sh | 16 ++++++++--------
>   2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index e528225e787..5837fdaabbe 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3708,10 +3708,11 @@ static const char *reflog_message(struct replay_opts *opts,
>   {
>   	va_list ap;
>   	static struct strbuf buf = STRBUF_INIT;
> +	char *reflog_action = getenv("GIT_REFLOG_ACTION");

>   	va_start(ap, fmt);
>   	strbuf_reset(&buf);
> -	strbuf_addstr(&buf, action_name(opts));
> +	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
>   	if (sub_action)
>   		strbuf_addf(&buf, " (%s)", sub_action);
>   	if (fmt) {
> @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,
>   			struct replay_opts *opts)
>   {
>   	int res = 0, reschedule = 0;
> +	char *prev_reflog_action;
>   
>   	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> +	prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));

>   	if (opts->allow_ff)
>   		assert(!(opts->signoff || opts->no_commit ||
>   				opts->record_origin || opts->edit));
> @@ -3845,12 +3848,14 @@ static int pick_commits(struct repository *r,
>   		}
>   		if (item->command <= TODO_SQUASH) {
>   			if (is_rebase_i(opts))
> -				setenv("GIT_REFLOG_ACTION", reflog_message(opts,
> +				setenv(GIT_REFLOG_ACTION, reflog_message(opts,
>   					command_to_string(item->command), NULL),
>   					1);
>   			res = do_pick_commit(r, item->command, item->commit,
>   					     opts, is_final_fixup(todo_list),
>   					     &check_todo);
> +			if (is_rebase_i(opts))
> +				setenv(GIT_REFLOG_ACTION, prev_reflog_action, 1);
>   			if (is_rebase_i(opts) && res < 0) {
>   				/* Reschedule */
>   				advise(_(rescheduled_advice),
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 61b76f33019..927a4f4a4e4 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -89,22 +89,22 @@ test_expect_success 'GIT_REFLOG_ACTION' '
>   	git checkout -b reflog-topic start &&
>   	test_commit reflog-to-rebase &&
>   
> -	git rebase --apply reflog-onto &&
> +	git rebase reflog-onto &&
>   	git log -g --format=%gs -3 >actual &&
>   	cat >expect <<-\EOF &&
> -	rebase finished: returning to refs/heads/reflog-topic
> -	rebase: reflog-to-rebase
> -	rebase: checkout reflog-onto
> +	rebase (finish): returning to refs/heads/reflog-topic
> +	rebase (pick): reflog-to-rebase
> +	rebase (start): checkout reflog-onto
>   	EOF
>   	test_cmp expect actual &&
>   
>   	git checkout -b reflog-prefix reflog-to-rebase &&
> -	GIT_REFLOG_ACTION=change-the-reflog git rebase --apply reflog-onto &&
> +	GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&
>   	git log -g --format=%gs -3 >actual &&
>   	cat >expect <<-\EOF &&
> -	rebase finished: returning to refs/heads/reflog-prefix
> -	change-the-reflog: reflog-to-rebase
> -	change-the-reflog: checkout reflog-onto
> +	change-the-reflog (finish): returning to refs/heads/reflog-prefix
> +	change-the-reflog (pick): reflog-to-rebase
> +	change-the-reflog (start): checkout reflog-onto
>   	EOF
>   	test_cmp expect actual
>   '
> 
> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
> 

>
> Hi Elijah
>
> Thanks for fixing this
>
> On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > There is a lot of code to honor GIT_REFLOG_ACTION throughout git,
> > including some in sequencer.c; unfortunately, reflog_message() and its
> > callers ignored it.  Instruct reflog_message() to check the existing
> > environment variable, and use it when present as an override to
> > action_name().
> >
> > Also restructure pick_commits() to only temporarily modify
> > GIT_REFLOG_ACTION for a short duration and then restore the old value,
> > so that when we do this setting within a loop we do not keep adding "
> > (pick)" substrings and end up with a reflog message of the form
> >      rebase (pick) (pick) (pick) (finish): returning to refs/heads/master
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >      sequencer: honor GIT_REFLOG_ACTION
> >
> >      I'm not the best with getenv/setenv. The xstrdup() wrapping is
> >      apparently necessary on mac and bsd. The xstrdup seems like it leaves us
> >      with a memory leak, but since setenv(3) says to not alter or free it, I
> >      think it's right. Anyone have any alternative suggestions?
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-746%2Fnewren%2Fhonor-reflog-action-in-sequencer-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-746/newren/honor-reflog-action-in-sequencer-v1
> > Pull-Request: https://github.com/git/git/pull/746
> >
> >   sequencer.c               |  9 +++++++--
> >   t/t3406-rebase-message.sh | 16 ++++++++--------
> >   2 files changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index e528225e787..5837fdaabbe 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3708,10 +3708,11 @@ static const char *reflog_message(struct replay_opts *opts,
> >   {
> >       va_list ap;
> >       static struct strbuf buf = STRBUF_INIT;
> > +     char *reflog_action = getenv("GIT_REFLOG_ACTION");
>
> Minor nit - you're using a string here rather that the pre-processor
> constant that is used below

> >       va_start(ap, fmt);
> >       strbuf_reset(&buf);
> > -     strbuf_addstr(&buf, action_name(opts));
> > +     strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
> >       if (sub_action)
> >               strbuf_addf(&buf, " (%s)", sub_action);
> >       if (fmt) {
> > @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,
> >                       struct replay_opts *opts)
> >   {
> >       int res = 0, reschedule = 0;
> > +     char *prev_reflog_action;
> >
> >       setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> > +     prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
>
> I'm confused as to why saving the environment variable immediately after
> setting it works but the test shows it does - why doesn't this clobber
> the value of GIT_REFLOG_ACTION set by the user?

> Hi Phillip,
> 
> On Thu, Apr 2, 2020 at 2:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Elijah
>>
>> Thanks for fixing this
>>
>> On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>>
>>> There is a lot of code to honor GIT_REFLOG_ACTION throughout git,
>>> including some in sequencer.c; unfortunately, reflog_message() and its
>>> callers ignored it.  Instruct reflog_message() to check the existing
>>> environment variable, and use it when present as an override to
>>> action_name().
>>>
>>> Also restructure pick_commits() to only temporarily modify
>>> GIT_REFLOG_ACTION for a short duration and then restore the old value,
>>> so that when we do this setting within a loop we do not keep adding "
>>> (pick)" substrings and end up with a reflog message of the form
>>>       rebase (pick) (pick) (pick) (finish): returning to refs/heads/master
>>>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>> ---
>>>       sequencer: honor GIT_REFLOG_ACTION
>>>
>>>       I'm not the best with getenv/setenv. The xstrdup() wrapping is
>>>       apparently necessary on mac and bsd. The xstrdup seems like it leaves us
>>>       with a memory leak, but since setenv(3) says to not alter or free it, I
>>>       think it's right. Anyone have any alternative suggestions?
>>>
>>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-746%2Fnewren%2Fhonor-reflog-action-in-sequencer-v1
>>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-746/newren/honor-reflog-action-in-sequencer-v1
>>> Pull-Request: https://github.com/git/git/pull/746
>>>
>>>    sequencer.c               |  9 +++++++--
>>>    t/t3406-rebase-message.sh | 16 ++++++++--------
>>>    2 files changed, 15 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index e528225e787..5837fdaabbe 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -3708,10 +3708,11 @@ static const char *reflog_message(struct replay_opts *opts,
>>>    {
>>>        va_list ap;
>>>        static struct strbuf buf = STRBUF_INIT;
>>> +     char *reflog_action = getenv("GIT_REFLOG_ACTION");
>>
>> Minor nit - you're using a string here rather that the pre-processor
>> constant that is used below
> 
> Yeah, true.  However, using a mixture of both styles is consistent
> with the current code's inconsistency about which one should be used.
> :-)

>>>        va_start(ap, fmt);
>>>        strbuf_reset(&buf);
>>> -     strbuf_addstr(&buf, action_name(opts));
>>> +     strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
>>>        if (sub_action)
>>>                strbuf_addf(&buf, " (%s)", sub_action);
>>>        if (fmt) {
>>> @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,
>>>                        struct replay_opts *opts)
>>>    {
>>>        int res = 0, reschedule = 0;
>>> +     char *prev_reflog_action;
>>>
>>>        setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
>>> +     prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
>>
>> I'm confused as to why saving the environment variable immediately after
>> setting it works but the test shows it does - why doesn't this clobber
>> the value of GIT_REFLOG_ACTION set by the user?
> 
> The third parameter, 0, means only set the environment variable if
> it's not already set.

> On 02/04/2020 18:01, Elijah Newren wrote:
> >
> > On Thu, Apr 2, 2020 at 2:25 AM Phillip Wood <phillip.wood123@gmail.com>
> > wrote:
> > >
> > > On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:
> > >
> > > >        va_start(ap, fmt);
> > > >        strbuf_reset(&buf);
> > > > -     strbuf_addstr(&buf, action_name(opts));
> > > > +     strbuf_addstr(&buf, reflog_action ? reflog_action :
> > > > action_name(opts));
> > > >        if (sub_action)
> > > >                strbuf_addf(&buf, " (%s)", sub_action);
> > > >        if (fmt) {
> > > > @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,
> > > >                        struct replay_opts *opts)
> > > >    {
> > > >        int res = 0, reschedule = 0;
> > > > +     char *prev_reflog_action;
> > > >
> > > >        setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> > > > +     prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
> > >
> > > I'm confused as to why saving the environment variable immediately after
> > > setting it works but the test shows it does - why doesn't this clobber
> > > the value of GIT_REFLOG_ACTION set by the user?
> >
> > The third parameter, 0, means only set the environment variable if
> > it's not already set.
>
> Ah thanks, I thought I must be missing something fairly obvious but couldn't
> see what it was

>
> Hi,
>
> On Thu, 2 Apr 2020, Phillip Wood wrote:
>
> > On 02/04/2020 18:01, Elijah Newren wrote:
> > >
> > > On Thu, Apr 2, 2020 at 2:25 AM Phillip Wood <phillip.wood123@gmail.com>
> > > wrote:
> > > >
> > > > On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:
> > > >
> > > > >        va_start(ap, fmt);
> > > > >        strbuf_reset(&buf);
> > > > > -     strbuf_addstr(&buf, action_name(opts));
> > > > > +     strbuf_addstr(&buf, reflog_action ? reflog_action :
> > > > > action_name(opts));
> > > > >        if (sub_action)
> > > > >                strbuf_addf(&buf, " (%s)", sub_action);
> > > > >        if (fmt) {
> > > > > @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,
> > > > >                        struct replay_opts *opts)
> > > > >    {
> > > > >        int res = 0, reschedule = 0;
> > > > > +     char *prev_reflog_action;
> > > > >
> > > > >        setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> > > > > +     prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
> > > >
> > > > I'm confused as to why saving the environment variable immediately after
> > > > setting it works but the test shows it does - why doesn't this clobber
> > > > the value of GIT_REFLOG_ACTION set by the user?
> > >
> > > The third parameter, 0, means only set the environment variable if
> > > it's not already set.
> >
> > Ah thanks, I thought I must be missing something fairly obvious but couldn't
> > see what it was
>
> FWIW I was also about to comment on that. Maybe that warrants even a code
> comment above the `prev_reflog_action`?

> On Tue, Apr 7, 2020 at 7:50 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Thu, 2 Apr 2020, Phillip Wood wrote:
> >
> > > On 02/04/2020 18:01, Elijah Newren wrote:
> > > >
> > > > On Thu, Apr 2, 2020 at 2:25 AM Phillip Wood <phillip.wood123@gmail.com>
> > > > wrote:
> > > > >
> > > > > On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:
> > > > >
> > > > > >        va_start(ap, fmt);
> > > > > >        strbuf_reset(&buf);
> > > > > > -     strbuf_addstr(&buf, action_name(opts));
> > > > > > +     strbuf_addstr(&buf, reflog_action ? reflog_action :
> > > > > > action_name(opts));
> > > > > >        if (sub_action)
> > > > > >                strbuf_addf(&buf, " (%s)", sub_action);
> > > > > >        if (fmt) {
> > > > > > @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,
> > > > > >                        struct replay_opts *opts)
> > > > > >    {
> > > > > >        int res = 0, reschedule = 0;
> > > > > > +     char *prev_reflog_action;
> > > > > >
> > > > > >        setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> > > > > > +     prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
> > > > >
> > > > > I'm confused as to why saving the environment variable immediately after
> > > > > setting it works but the test shows it does - why doesn't this clobber
> > > > > the value of GIT_REFLOG_ACTION set by the user?
> > > >
> > > > The third parameter, 0, means only set the environment variable if
> > > > it's not already set.
> > >
> > > Ah thanks, I thought I must be missing something fairly obvious but couldn't
> > > see what it was
> >
> > FWIW I was also about to comment on that. Maybe that warrants even a code
> > comment above the `prev_reflog_action`?
>
> Yeah, if it tripped you both up, I'll add such a comment to the code
> to help explain it.

>> > FWIW I was also about to comment on that. Maybe that warrants even a code
>> > comment above the `prev_reflog_action`?
>>
>> Yeah, if it tripped you both up, I'll add such a comment to the code
>> to help explain it.
>
> Thank you!