Re: [PATCH 8/9] git_config_set: use do_config_from_file() directly

From: Jeff King <peff@peff.net>
Date: Mar 30, 2018, 13:14 UTC
Message-ID: <20180330131456.GB29568@sigill.intra.peff.net>
In-Reply-To: <nycvar.QRO.7.76.6.1803301500560.5026@qfpub.tvgsbejvaqbjf.bet>

On Fri, Mar 30, 2018 at 03:02:00PM +0200, Johannes Schindelin wrote:

Show 16 quoted lines

> > I'm not sure I understand that last paragraph. What does flockfile() have
> > to do with stdin/stdout?
> > 
> > The point of those calls is that we're locking the FILE handle, so that
> > it's safe for the lower-level config code to run getc_unlocked(), which
> > is faster.
> > 
> > So without those, we're calling getc_unlocked() without holding the
> > lock. I think it probably works in practice because we know that we're
> > single-threaded, but it seems a bit sketchy.
> 
> Oops. I misunderstood the purpose of flockfile(), then. I thought it was
> only about multiple users of stdin/stdout.
> 
> Will have a look whether flockfile()/funlockfile() can be moved into
> do_config_from_file() instead.

In a sense stdin/stdout are much more susceptible to this because they're global variables, and any thread may touch them. For the config code, we open our own handle that we don't expose elsewhere. So probably it would be fine just to use the unlocked variants even without locking.

But IMHO it's good practice to always flockfile() before using the unlocked variants. My reading of POSIX is that it's OK to use the unlocked variants without holding the lock (if you know there won't be contention), but if it's not hard to err on the side of safety, I'd prefer it.

-Peff