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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html