Re: Bug: .gitconfig folder

[Jeff King <peff@peff.net>]

On Wed, May 27, 2015 at 03:38:12PM -0700, Junio C Hamano wrote:

> The patch was meant to be a tongue-in-cheek tangent that is a vast
> improvement for cases where we absolutely need to use mmap but does
> not help the OP at all ;-)  I do not think there is any need for the
> config reader to read the existing file via mmap interface; just
> open it, strbuf_read() the whole thing (and complain when it cannot)
> and we should be ok.
> 
> Or do we write back through the mmaped region or something?

No, I think we must never do that in our code because our compat mmap
implementation uses pread(). So all maps must be MAP_PRIVATE (and our
compat mmap barfs if it is not).

I started to go the strbuf_read() route, but it just felt so dirty to
change the way the code works only to try to get a better error message.
So here's my attempt at making it better while still using mmap. The end
result is:

  $ mkdir foo
  $ git config --file=foo some.key value
  error: unable to mmap 'foo': Is a directory

Having looked through the code, I think the _ideal_ way to implement it
would actually be with read() and seek(). We read through the config
once (with the normal parser, which wraps stdio) and mark the offsets of
chunks we want to copy to the output. Then we mmap the original (under
lock, at least, so it shouldn't be racy) and output the existing chunks
and any new content in the appropriate order.

So ideally writing each chunk would just be seek() and copy_fd(). But
our offsets aren't quite perfect. In some cases we read backwards in our
mmap to find the right cutoff point. I'm sure this is fixable given
sufficient refactoring, but the config-writing code is such a tangled
mess that I don't want to spend the time or risk the regressions.

  [1/4]: read-cache.c: drop PROT_WRITE from mmap of index
  [2/4]: config.c: fix mmap leak when writing config
  [3/4]: config.c: avoid xmmap error messages
  [4/4]: config.c: rewrite ENODEV into EISDIR when mmap fails

-Peff