marzog00 / mu.mu Goto Github PK

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

Thanks,
Ian.

-- 
Ian Jackson <[email protected]>   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.