git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: Bug: .gitconfig folder

From
Jeff King <peff@peff.net>
Date
May 28, 2015, 07:51 UTC
Message-ID
<20150528075142.GB3688@peff.net>
In-Reply-To
<xmqq1ti1k5nv.fsf@gitster.dls.corp.google.com>
On Wed, May 27, 2015 at 03:38:12PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> 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
Previous: Junio C HamanoNext: Jeff King
Message 7 of 20 in “Bug: .gitconfig folder”
  1. JorgeMay 27, 2015
  2. Junio C HamanoMay 27, 2015
  3. Jeff KingMay 27, 2015
  4. Stefan BellerMay 27, 2015
  5. Jeff KingMay 28, 2015
  6. Junio C HamanoMay 27, 2015
  7. Jeff KingMay 28, 2015
  8. 1/4 read-cache.c: drop PROT_WRITE from mmap of indexJeff King, May 28, 2015
  9. 2/4 config.c: fix mmap leak when writing configJeff King, May 28, 2015
  10. config.c: fix writing config files on Windows network sharesKarsten Blees, Jun 30, 2015
  11. Torsten BögershausenJun 30, 2015
  12. Jeff KingJun 30, 2015
  13. Johannes SchindelinJun 30, 2015
  14. Jeff KingJun 30, 2015
  15. 3/4 config.c: avoid xmmap error messagesJeff King, May 28, 2015
  16. 4/4 config.c: rewrite ENODEV into EISDIR when mmap failsJeff King, May 28, 2015
  17. Junio C HamanoMay 28, 2015
  18. Jeff KingMay 28, 2015
  19. Junio C HamanoMay 28, 2015
  20. Junio C HamanoMay 28, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.