Bug: .gitconfig folder

Bug: .gitconfig folder

[Jorge]

If you have a folder named ~/.gitconfig instead of a file with that 
name, when you try to run some global config editing command it will 
fail with a wrong error message:

     "fatal: Out of memory? mmap failed: No such device"


You can reproduce it:

$rm ~/.gitconfig
$mkdir ~/.gitconfig

$ls -la ~
     ...
     drwxr-xr-x 24 hit  hit       4096 may  4 12:30 .gimp-2.8
     drwxr-xr-x  2 hit  hit       4096 may 27 15:26 .gitconfig
     drwxr-xr-x  6 hit  hit       4096 may 27 14:01 github
     ...

$git config --global user.name foo
     fatal: Out of memory? mmap failed: No such device

$git config --global core.editor "vim"
     fatal: Out of memory? mmap failed: No such device

Re: Bug: .gitconfig folder

[Junio C Hamano]

Jorge <griffin@gmx.es> writes:

> If you have a folder named ~/.gitconfig instead of a file with that
> name, when you try to run some global config editing command it will
> fail with a wrong error message:
>
>     "fatal: Out of memory? mmap failed: No such device"

That indeed is a funny error message.

How about this patch?

-- >8 --
We show that message with die_errno(), but the OS is ought to know
why mmap(2) failed much better than we do.  There is no reason for
us to say "Out of memory?" here.

Note that mmap(2) fails with ENODEV when the file you specify is not
something that can be mmap'ed, so you still need to know that "No
such device" can include cases like having a directory when a
regular file is expected, but we can expect that a user who creates
a directory to a location where a regular file is expected to be
would know what s/he is doing, hopefully ;-)

 sha1_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index ccc6dac..551a9e9 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -720,7 +720,7 @@ void *xmmap(void *start, size_t length,
 		release_pack_memory(length);
 		ret = mmap(start, length, prot, flags, fd, offset);
 		if (ret == MAP_FAILED)
-			die_errno("Out of memory? mmap failed");
+			die_errno("mmap failed");
 	}
 	return ret;
 }

Re: Bug: .gitconfig folder

[Jeff King]

On Wed, May 27, 2015 at 01:30:29PM -0700, Junio C Hamano wrote:

> Jorge <griffin@gmx.es> writes:
> 
> > If you have a folder named ~/.gitconfig instead of a file with that
> > name, when you try to run some global config editing command it will
> > fail with a wrong error message:
> >
> >     "fatal: Out of memory? mmap failed: No such device"
> 
> That indeed is a funny error message.
> 
> How about this patch?
> 
> -- >8 --
> We show that message with die_errno(), but the OS is ought to know
> why mmap(2) failed much better than we do.  There is no reason for
> us to say "Out of memory?" here.
> 
> Note that mmap(2) fails with ENODEV when the file you specify is not
> something that can be mmap'ed, so you still need to know that "No
> such device" can include cases like having a directory when a
> regular file is expected, but we can expect that a user who creates
> a directory to a location where a regular file is expected to be
> would know what s/he is doing, hopefully ;-)
> 
>  sha1_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index ccc6dac..551a9e9 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -720,7 +720,7 @@ void *xmmap(void *start, size_t length,
>  		release_pack_memory(length);
>  		ret = mmap(start, length, prot, flags, fd, offset);
>  		if (ret == MAP_FAILED)
> -			die_errno("Out of memory? mmap failed");
> +			die_errno("mmap failed");
>  	}

This is definitely an improvement, but the real failing of that error
message is that it does not tell us that "~/.gitconfig" is the culprit.
I don't think we can do much from xmmap, though; it does not have the
filename. It would be nice if we got EISDIR from open() in the first
place, but I don't think we can implement that efficiently (if we added
an "xopen" that checked that, it would have to stat() every file we
opened).

-Peff

Re: Bug: .gitconfig folder

[Stefan Beller]

On Wed, May 27, 2015 at 3:18 PM, Jeff King <peff@peff.net> wrote:
> On Wed, May 27, 2015 at 01:30:29PM -0700, Junio C Hamano wrote:
>
>> Jorge <griffin@gmx.es> writes:
>>
>> > If you have a folder named ~/.gitconfig instead of a file with that
>> > name, when you try to run some global config editing command it will
>> > fail with a wrong error message:
>> >
>> >     "fatal: Out of memory? mmap failed: No such device"
>>
>> That indeed is a funny error message.
>>
>> How about this patch?
>>
>> -- >8 --
>> We show that message with die_errno(), but the OS is ought to know
>> why mmap(2) failed much better than we do.  There is no reason for
>> us to say "Out of memory?" here.
>>
>> Note that mmap(2) fails with ENODEV when the file you specify is not
>> something that can be mmap'ed, so you still need to know that "No
>> such device" can include cases like having a directory when a
>> regular file is expected, but we can expect that a user who creates
>> a directory to a location where a regular file is expected to be
>> would know what s/he is doing, hopefully ;-)
>>
>>  sha1_file.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sha1_file.c b/sha1_file.c
>> index ccc6dac..551a9e9 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -720,7 +720,7 @@ void *xmmap(void *start, size_t length,
>>               release_pack_memory(length);
>>               ret = mmap(start, length, prot, flags, fd, offset);
>>               if (ret == MAP_FAILED)
>> -                     die_errno("Out of memory? mmap failed");
>> +                     die_errno("mmap failed");
>>       }
>
> This is definitely an improvement, but the real failing of that error
> message is that it does not tell us that "~/.gitconfig" is the culprit.
> I don't think we can do much from xmmap, though; it does not have the
> filename. It would be nice if we got EISDIR from open() in the first
> place, but I don't think we can implement that efficiently (if we added
> an "xopen" that checked that, it would have to stat() every file we
> opened).

What is our thinking for after-fact recovery attempts?
Like we try the mmap first, if that fails we just use open to get the
contents of
the file. And when open fails, we can still print a nice error message?

>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug: .gitconfig folder

[Jeff King]

On Wed, May 27, 2015 at 03:24:47PM -0700, Stefan Beller wrote:

> What is our thinking for after-fact recovery attempts?
> Like we try the mmap first, if that fails we just use open to get the
> contents of
> the file. And when open fails, we can still print a nice error message?

For config, I think we could just open and read the file in the first
place. The data is not typically very big (and if you have a 3G config
file and git barfs with "out of memory", I can live with that).

-Peff

Re: Bug: .gitconfig folder

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> -			die_errno("Out of memory? mmap failed");
>> +			die_errno("mmap failed");
>
> This is definitely an improvement, but the real failing of that error
> message is that it does not tell us that "~/.gitconfig" is the culprit.
> I don't think we can do much from xmmap, though; it does not have the
> filename. It would be nice if we got EISDIR from open() in the first
> place, but I don't think we can implement that efficiently (if we added
> an "xopen" that checked that, it would have to stat() every file we
> opened).

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?

Re: Bug: .gitconfig folder

[Jeff King]

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

[PATCH 1/4] read-cache.c: drop PROT_WRITE from mmap of index

[Jeff King]

Once upon a time, git's in-memory representation of a cache
entry actually pointed to the mmap'd on-disk data. So in
520fc24 (Allow writing to the private index file mapping.,
2005-04-26), we specified PROT_WRITE so that we could tweak
the entries while we run (in our own MAP_PRIVATE copy-on-write
version, of course).

Later, 7a51ed6 (Make on-disk index representation separate
from in-core one, 2008-01-14) stopped doing this; we copy
the data into our in-core representation, and then drop the
mmap immediately. We can therefore drop the PROT_WRITE flag.
It's probably not hurting anything as it is, but it's
potentially confusing.

Note that we could also mark the mapping as "const" to
verify that we never write to it. However, we don't
typically do that for our other maps, as it then requires
casting to munmap() it.

Signed-off-by: Jeff King <peff@peff.net>
---
This one obviously is not necessary for the rest of it, but just
something I noticed while writing my response to you. But read to the
end of the series; there might be a twist ending that brings it back!

 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index 723d48d..5dee4e2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1562,7 +1562,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	if (mmap_size < sizeof(struct cache_header) + 20)
 		die("index file smaller than expected");
 
-	mmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+	mmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	if (mmap == MAP_FAILED)
 		die_errno("unable to map index file");
 	close(fd);
-- 
2.4.2.668.gc3b1ade.dirty

[PATCH 2/4] config.c: fix mmap leak when writing config

[Jeff King]

We mmap the existing config file, but fail to unmap it if we
hit an error. The function already has a shared exit path,
so we can fix this by moving the mmap pointer to the
function scope and clearing it in the shared exit.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index ab46462..6917100 100644
--- a/config.c
+++ b/config.c
@@ -1939,6 +1939,8 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	int ret;
 	struct lock_file *lock = NULL;
 	char *filename_buf = NULL;
+	char *contents = NULL;
+	size_t contents_sz;
 
 	/* parse-key returns negative; flip the sign to feed exit(3) */
 	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
@@ -1988,8 +1990,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 			goto write_err_out;
 	} else {
 		struct stat st;
-		char *contents;
-		size_t contents_sz, copy_begin, copy_end;
+		size_t copy_begin, copy_end;
 		int i, new_line = 0;
 
 		if (value_regex == NULL)
@@ -2108,8 +2109,6 @@ int git_config_set_multivar_in_file(const char *config_filename,
 					  contents_sz - copy_begin) <
 			    contents_sz - copy_begin)
 				goto write_err_out;
-
-		munmap(contents, contents_sz);
 	}
 
 	if (commit_lock_file(lock) < 0) {
@@ -2135,6 +2134,8 @@ out_free:
 	if (lock)
 		rollback_lock_file(lock);
 	free(filename_buf);
+	if (contents)
+		munmap(contents, contents_sz);
 	return ret;
 
 write_err_out:
-- 
2.4.2.668.gc3b1ade.dirty

[PATCH] config.c: fix writing config files on Windows network shares

[Karsten Blees]

Renaming to an existing file doesn't work on Windows network shares if the
target file is open.

munmap() the old config file before commit_lock_file.

Signed-off-by: Karsten Blees <blees@dcon.de>
---

See https://github.com/git-for-windows/git/issues/226

Strangely, renaming to an open file works fine on local disks...

 config.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/config.c b/config.c
index 07133ef..3a23c11 100644
--- a/config.c
+++ b/config.c
@@ -2153,6 +2153,9 @@ int git_config_set_multivar_in_file(const char *config_filename,
 					  contents_sz - copy_begin) <
 			    contents_sz - copy_begin)
 				goto write_err_out;
+
+		munmap(contents, contents_sz);
+		contents = NULL;
 	}
 
 	if (commit_lock_file(lock) < 0) {
-- 
2.4.3.windows.1.1.g87477f9

Re: [PATCH] config.c: fix writing config files on Windows network shares

[Torsten Bögershausen]

On 2015-06-30 16.34, Karsten Blees wrote:
> Renaming to an existing file doesn't work on Windows network shares if the
> target file is open.
> 
> munmap() the old config file before commit_lock_file.
> 
> Signed-off-by: Karsten Blees <blees@dcon.de>
> ---
> 
> See https://github.com/git-for-windows/git/issues/226
> 
> Strangely, renaming to an open file works fine on local disks...
> 
>  config.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/config.c b/config.c
> index 07133ef..3a23c11 100644
> --- a/config.c
> +++ b/config.c
> @@ -2153,6 +2153,9 @@ int git_config_set_multivar_in_file(const char *config_filename,
>  					  contents_sz - copy_begin) <
>  			    contents_sz - copy_begin)
>  				goto write_err_out;
> +
> +		munmap(contents, contents_sz);
> +		contents = NULL;
>  	}
>  
>  	if (commit_lock_file(lock) < 0) {
> 

Nice catch.
Talking about network file system,
somebody volunteering to fix this issue ?

The value of fstat() is not checked here:
(indicated by a compiler warning, that contents_sz may be uninitalized.

 config.c:
 int git_config_set_multivar_in_file(
 //around line 2063 (the only call to fstat())
 		fstat(in_fd, &st);
 		contents_sz = xsize_t(st.st_size);


(sorry for hijacking your email thread)

Re: [PATCH] config.c: fix writing config files on Windows network shares

[Jeff King]

On Tue, Jun 30, 2015 at 04:46:20PM +0200, Torsten Bögershausen wrote:

> The value of fstat() is not checked here:
> (indicated by a compiler warning, that contents_sz may be uninitalized.
> 
>  config.c:
>  int git_config_set_multivar_in_file(
>  //around line 2063 (the only call to fstat())
>  		fstat(in_fd, &st);
>  		contents_sz = xsize_t(st.st_size);

There is a similar case in git_config_rename_section_in_file. It looks
like they could both just jump to the error case when fstat() fails.

-Peff

Re: [PATCH] config.c: fix writing config files on Windows network shares

[Johannes Schindelin]

Hi,

On 2015-06-30 16:34, Karsten Blees wrote:
> Renaming to an existing file doesn't work on Windows network shares if the
> target file is open.
> 
> munmap() the old config file before commit_lock_file.
> 
> Signed-off-by: Karsten Blees <blees@dcon.de>

ACK.

Thanks,
Dscho

Re: [PATCH] config.c: fix writing config files on Windows network shares

[Jeff King]

On Tue, Jun 30, 2015 at 04:34:13PM +0200, Karsten Blees wrote:

> Renaming to an existing file doesn't work on Windows network shares if the
> target file is open.
> 
> munmap() the old config file before commit_lock_file.
> 
> Signed-off-by: Karsten Blees <blees@dcon.de>

Thanks for fixing this.

Acked-by: Jeff King <peff@peff.net>

-Peff

[PATCH 3/4] config.c: avoid xmmap error messages

[Jeff King]

The config-writing code uses xmmap to map the existing
config file, which will die if the map fails. This has two
downsides:

  1. The error message is not very helpful, as it lacks any
     context about the file we are mapping:

       $ mkdir foo
       $ git config --file=foo some.key value
       fatal: Out of memory? mmap failed: No such device

  2. We normally do not die in this code path; instead, we'd
     rather report the error and return an appropriate exit
     status (which is part of the public interface
     documented in git-config.1).

This patch introduces a "gentle" form of xmmap which lets us
produce our own error message. We do not want to use mmap
directly, because we would like to use the other
compatibility elements of xmmap (e.g., handling 0-length
maps portably).

The end result is:

    $ git.compile config --file=foo some.key value
    error: unable to mmap 'foo': No such device
    $ echo $?
    3

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c          | 11 +++++++++--
 git-compat-util.h |  1 +
 sha1_file.c       | 15 +++++++++++----
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/config.c b/config.c
index 6917100..e7dc155 100644
--- a/config.c
+++ b/config.c
@@ -2053,8 +2053,15 @@ int git_config_set_multivar_in_file(const char *config_filename,
 
 		fstat(in_fd, &st);
 		contents_sz = xsize_t(st.st_size);
-		contents = xmmap(NULL, contents_sz, PROT_READ,
-			MAP_PRIVATE, in_fd, 0);
+		contents = xmmap_gently(NULL, contents_sz, PROT_READ,
+					MAP_PRIVATE, in_fd, 0);
+		if (contents == MAP_FAILED) {
+			error("unable to mmap '%s': %s",
+			      config_filename, strerror(errno));
+			ret = CONFIG_INVALID_FILE;
+			contents = NULL;
+			goto out_free;
+		}
 		close(in_fd);
 
 		if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {
diff --git a/git-compat-util.h b/git-compat-util.h
index 17584ad..0cc7ae8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -718,6 +718,7 @@ extern char *xstrndup(const char *str, size_t len);
 extern void *xrealloc(void *ptr, size_t size);
 extern void *xcalloc(size_t nmemb, size_t size);
 extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
+extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);
 extern ssize_t xread(int fd, void *buf, size_t len);
 extern ssize_t xwrite(int fd, const void *buf, size_t len);
 extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
diff --git a/sha1_file.c b/sha1_file.c
index ccc6dac..73e0bc0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -707,8 +707,8 @@ static void mmap_limit_check(size_t length)
 		    (uintmax_t)length, (uintmax_t)limit);
 }
 
-void *xmmap(void *start, size_t length,
-	int prot, int flags, int fd, off_t offset)
+void *xmmap_gently(void *start, size_t length,
+		  int prot, int flags, int fd, off_t offset)
 {
 	void *ret;
 
@@ -719,12 +719,19 @@ void *xmmap(void *start, size_t length,
 			return NULL;
 		release_pack_memory(length);
 		ret = mmap(start, length, prot, flags, fd, offset);
-		if (ret == MAP_FAILED)
-			die_errno("Out of memory? mmap failed");
 	}
 	return ret;
 }
 
+void *xmmap(void *start, size_t length,
+	int prot, int flags, int fd, off_t offset)
+{
+	void *ret = xmmap_gently(start, length, prot, flags, fd, offset);
+	if (ret == MAP_FAILED)
+			die_errno("Out of memory? mmap failed");
+	return ret;
+}
+
 void close_pack_windows(struct packed_git *p)
 {
 	while (p->windows) {
-- 
2.4.2.668.gc3b1ade.dirty

[PATCH 4/4] config.c: rewrite ENODEV into EISDIR when mmap fails

[Jeff King]

If we try to mmap a directory, we'll get ENODEV. This
translates to "no such device" for the user, which is not
very helpful. Since we've just fstat()'d the file, we can
easily check whether the problem was a directory to give a
better message.

Signed-off-by: Jeff King <peff@peff.net>
---
It feels a bit wrong to put this magic conversion here, and not in
xmmap. But of course xmmap does not have the stat information.

Which makes me wonder if we should provide an interface that will take
the whole "struct stat" rather than just the size. That's less flexible,
but in most cases, we're mapping the whole file (the packfiles are the
big exception, where we use a window).

We could also potentially drop some of the useless options. As of patch
1, all of our calls are PROT_READ. They must all be MAP_PRIVATE, or our
pread compatibility wrapper will fail, and we never use other flags. We
never request a specific address. And in a whole-file remap, the offset
will always be 0. So something like:

  void *xmmap_file(int fd, struct stat *st);

would probably work. We could even do the fstat() on behalf of the
caller, though they need to know the length themselves. Maybe:

  void *xmmap_file(int fd, size_t *len);

I dunno if it is worth it or not.

 config.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/config.c b/config.c
index e7dc155..29fa012 100644
--- a/config.c
+++ b/config.c
@@ -2056,6 +2056,8 @@ int git_config_set_multivar_in_file(const char *config_filename,
 		contents = xmmap_gently(NULL, contents_sz, PROT_READ,
 					MAP_PRIVATE, in_fd, 0);
 		if (contents == MAP_FAILED) {
+			if (errno == ENODEV && S_ISDIR(st.st_mode))
+				errno = EISDIR;
 			error("unable to mmap '%s': %s",
 			      config_filename, strerror(errno));
 			ret = CONFIG_INVALID_FILE;
-- 
2.4.2.668.gc3b1ade.dirty

Re: [PATCH 4/4] config.c: rewrite ENODEV into EISDIR when mmap fails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If we try to mmap a directory, we'll get ENODEV. This
> translates to "no such device" for the user, which is not
> very helpful. Since we've just fstat()'d the file, we can
> easily check whether the problem was a directory to give a
> better message.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> It feels a bit wrong to put this magic conversion here, and not in
> xmmap. But of course xmmap does not have the stat information.
> ...
> diff --git a/config.c b/config.c
> index e7dc155..29fa012 100644
> --- a/config.c
> +++ b/config.c
> @@ -2056,6 +2056,8 @@ int git_config_set_multivar_in_file(const char *config_filename,
>  		contents = xmmap_gently(NULL, contents_sz, PROT_READ,
>  					MAP_PRIVATE, in_fd, 0);
>  		if (contents == MAP_FAILED) {
> +			if (errno == ENODEV && S_ISDIR(st.st_mode))
> +				errno = EISDIR;
>  			error("unable to mmap '%s': %s",
>  			      config_filename, strerror(errno));
>  			ret = CONFIG_INVALID_FILE;

I think this patch places the "magic" at the right place, but I
would have preferred to see something more like this:

	if (contents == MAP_FAILED) {
        	if (errno == ENODEV && S_ISDIR(st.st_mode))
			error("unable to mmap a directory '%s',
                        	config_filename);
		else
                	error("unable to mmap '%s': %s",
                        	config_filename, strerror(errno));
		ret = CONFIG_INVALID_FILE;

But that is a very minor preference.  I am OK with relying on our
knowledge that strerror(EISDIR) would give something that says "the
thing is a directory which is not appropriate for the operation", as
nobody after that strerror() refers to 'errno' in this codepath.

Thanks.  The patches were a pleasant read.

Re: [PATCH 4/4] config.c: rewrite ENODEV into EISDIR when mmap fails

[Jeff King]

On Thu, May 28, 2015 at 10:11:55AM -0700, Junio C Hamano wrote:

> >  		if (contents == MAP_FAILED) {
> > +			if (errno == ENODEV && S_ISDIR(st.st_mode))
> > +				errno = EISDIR;
> >  			error("unable to mmap '%s': %s",
> >  			      config_filename, strerror(errno));
> >  			ret = CONFIG_INVALID_FILE;
> 
> I think this patch places the "magic" at the right place, but I
> would have preferred to see something more like this:
> 
> 	if (contents == MAP_FAILED) {
>         	if (errno == ENODEV && S_ISDIR(st.st_mode))
> 			error("unable to mmap a directory '%s',
>                         	config_filename);
> 		else
>                 	error("unable to mmap '%s': %s",
>                         	config_filename, strerror(errno));
> 		ret = CONFIG_INVALID_FILE;
> 
> But that is a very minor preference.  I am OK with relying on our
> knowledge that strerror(EISDIR) would give something that says "the
> thing is a directory which is not appropriate for the operation", as
> nobody after that strerror() refers to 'errno' in this codepath.

I am OK if you want to switch it. Certainly EISDIR produces good output
on my system, but I don't know if that is universal.

We also know S_ISDIR(st.st_mode) _before_ we actually mmap. So I was
tempted to simply check it beforehand, under the assumption that the
mmap cannot possibly work if we have a directory. But by doing it in the
error code path, then we _know_ we are not affecting the outcome, only
the error message. :)

-Peff

Re: [PATCH 4/4] config.c: rewrite ENODEV into EISDIR when mmap fails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We also know S_ISDIR(st.st_mode) _before_ we actually mmap. So I was
> tempted to simply check it beforehand, under the assumption that the
> mmap cannot possibly work if we have a directory. But by doing it in the
> error code path, then we _know_ we are not affecting the outcome, only
> the error message. :)

Well, even if your mmap() worked on a directory, having ~/.gitconfig
as a directory is wrong in the first place, so...

I think what you sent is good as-is, so let's go with them.

Re: Bug: .gitconfig folder

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Hmm.  I actually thought that we long time ago updated the system to
read small loose object files via read(2) instead of mmap(2) purely
as an optimization, as mmap(2) is a bad match if you are going to
read the whole thing from the beginning to the end anyway, and the
"why not strbuf_read() the whole configuration file" was a suggestion
along that line.

But apparently we do not have such an optimization in read_object()
codepath, perhaps I was hallucinating X-<.

> ... but the config-writing code is such a tangled
> mess that I don't want to spend the time or risk the regressions.

That part I agree with.  I was kinda hoping that the previous GSoC
would clean it up, but that did not happen.