Re: [PATCH 4/3] am, sequencer: stop parsing our own committer ident

[Jeff King <peff@peff.net>]

On Mon, Oct 26, 2020 at 05:25:01PM +0100, Johannes Schindelin wrote:

> On Fri, 23 Oct 2020, Jeff King wrote:
> 
> > diff --git a/ident.c b/ident.c
> > index 6aba4b5cb6..7743c1ed05 100644
> > --- a/ident.c
> > +++ b/ident.c
> > @@ -384,6 +384,12 @@ const char *fmt_ident(const char *name, const char *email,
> >  	struct strbuf *ident = &ident_pool[index];
> >  	index = (index + 1) % ARRAY_SIZE(ident_pool);
> >
> > +	if (!email) {
> > +		if (whose_ident == WANT_AUTHOR_IDENT)
> > +			email = getenv("GIT_AUTHOR_EMAIL");
> > +		else if (whose_ident == WANT_COMMITTER_IDENT)
> > +			email = getenv("GIT_COMMITTER_EMAIL");
> 
> I *guess* that this is a strict improvement, calling `getenv()` much
> closer to the time the value is actually used (and hence avoiding the
> problem where pointers returned by `getenv()` get stale due to environment
> changes).

I don't think it changes much in practice. Most of the callers are
passing the values directly in to this function, and there's not much
that happens between the function starting and these calls.

The more worrisome stretch is that we likely call strbuf functions while
holding on to a getenv() pointer. And those potentially do things like
xmalloc(), which looks at GIT_ALLOC_LIMIT, etc. But though POSIX
promises only one getenv() result at a time, we definitely don't adhere
to that (after all, we routinely pass the results of three separate
getenv() calls to fmt_ident!). As you well know, because mingw_getenv()
has a circular buffer hack to deal with this. :)

So it certainly isn't making anything worse, but I'd be surprised if it
actually helped at all.

-Peff