Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION

From: Ian Jackson <ijackson@chiark.greenend.org.uk>
Date: Apr 1, 2020, 23:29 UTC
Message-ID: <24197.9157.362143.972556@chiark.greenend.org.uk>
In-Reply-To: <pull.746.git.git.1585773096145.gitgitgadget@gmail.com>

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.

Show 6 quoted lines

> +	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 :-).

Thanks, Ian.

-- 
Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.