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

From: Jeff King <peff@peff.net>
Date: Oct 27, 2020, 07:23 UTC
Message-ID: <20201027072309.GE3005508@coredump.intra.peff.net>
In-Reply-To: <nycvar.QRO.7.76.6.2010261722230.56@tvgsbejvaqbjf.bet>

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

Show 20 quoted lines

> 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