{"thread":{"id":"39437","subject":"Bug: .gitconfig folder","startedAt":"2015-05-27T13:29:11Z","lastAt":"2015-06-30T16:01:25Z","messageCount":20,"participants":["Jorge","Junio C Hamano","Jeff King","Stefan Beller","Karsten Blees","Torsten Bögershausen","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"262228","messageId":"5565C6A7.60007@gmx.es","threadId":"39437","inReplyTo":null,"subject":"Bug: .gitconfig folder","fromName":"Jorge","fromEmail":"griffin@gmx.es","sentAt":"2015-05-27T13:29:11Z","receivedAt":"2015-05-27T13:29:11Z","isPatch":false,"sender":{"key":"griffin@gmx.es","avatar":null},"body":"If you have a folder named ~/.gitconfig instead of a file with that \nname, when you try to run some global config editing command it will \nfail with a wrong error message:\n\n \"fatal: Out of memory? mmap failed: No such device\"\n\n\nYou can reproduce it:\n\n$rm ~/.gitconfig\n$mkdir ~/.gitconfig\n\n$ls -la ~\n ...\n drwxr-xr-x 24 hit hit 4096 may 4 12:30 .gimp-2.8\n drwxr-xr-x 2 hit hit 4096 may 27 15:26 .gitconfig\n drwxr-xr-x 6 hit hit 4096 may 27 14:01 github\n ...\n\n$git config --global user.name foo\n fatal: Out of memory? mmap failed: No such device\n\n$git config --global core.editor \"vim\"\n fatal: Out of memory? mmap failed: No such device\n"},{"id":"262264","messageId":"xmqq7frtlq56.fsf@gitster.dls.corp.google.com","threadId":"39437","inReplyTo":"5565C6A7.60007@gmx.es","subject":"Re: Bug: .gitconfig folder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-27T20:30:29Z","receivedAt":"2015-05-27T20:30:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jorge writes:\n\n> If you have a folder named ~/.gitconfig instead of a file with that\n> name, when you try to run some global config editing command it will\n> fail with a wrong error message:\n>\n> \"fatal: Out of memory? mmap failed: No such device\"\n\nThat indeed is a funny error message.\n\nHow about this patch?\n\n-- >8 --\nWe show that message with die_errno(), but the OS is ought to know\nwhy mmap(2) failed much better than we do. There is no reason for\nus to say \"Out of memory?\" here.\n\nNote that mmap(2) fails with ENODEV when the file you specify is not\nsomething that can be mmap'ed, so you still need to know that \"No\nsuch device\" can include cases like having a directory when a\nregular file is expected, but we can expect that a user who creates\na directory to a location where a regular file is expected to be\nwould know what s/he is doing, hopefully ;-)\n\n sha1_file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ccc6dac..551a9e9 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -720,7 +720,7 @@ void *xmmap(void *start, size_t length,\n \t\trelease_pack_memory(length);\n \t\tret = mmap(start, length, prot, flags, fd, offset);\n \t\tif (ret == MAP_FAILED)\n-\t\t\tdie_errno(\"Out of memory? mmap failed\");\n+\t\t\tdie_errno(\"mmap failed\");\n \t}\n \treturn ret;\n }\n"},{"id":"262292","messageId":"20150527221813.GF23259@peff.net","threadId":"39437","inReplyTo":"xmqq7frtlq56.fsf@gitster.dls.corp.google.com","subject":"Re: Bug: .gitconfig folder","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T22:18:13Z","receivedAt":"2015-05-27T22:18:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 01:30:29PM -0700, Junio C Hamano wrote:\n\n> Jorge writes:\n> \n> > If you have a folder named ~/.gitconfig instead of a file with that\n> > name, when you try to run some global config editing command it will\n> > fail with a wrong error message:\n> >\n> > \"fatal: Out of memory? mmap failed: No such device\"\n> \n> That indeed is a funny error message.\n> \n> How about this patch?\n> \n> -- >8 --\n> We show that message with die_errno(), but the OS is ought to know\n> why mmap(2) failed much better than we do. There is no reason for\n> us to say \"Out of memory?\" here.\n> \n> Note that mmap(2) fails with ENODEV when the file you specify is not\n> something that can be mmap'ed, so you still need to know that \"No\n> such device\" can include cases like having a directory when a\n> regular file is expected, but we can expect that a user who creates\n> a directory to a location where a regular file is expected to be\n> would know what s/he is doing, hopefully ;-)\n> \n> sha1_file.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index ccc6dac..551a9e9 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -720,7 +720,7 @@ void *xmmap(void *start, size_t length,\n> \t\trelease_pack_memory(length);\n> \t\tret = mmap(start, length, prot, flags, fd, offset);\n> \t\tif (ret == MAP_FAILED)\n> -\t\t\tdie_errno(\"Out of memory? mmap failed\");\n> +\t\t\tdie_errno(\"mmap failed\");\n> \t}\n\nThis is definitely an improvement, but the real failing of that error\nmessage is that it does not tell us that \"~/.gitconfig\" is the culprit.\nI don't think we can do much from xmmap, though; it does not have the\nfilename. It would be nice if we got EISDIR from open() in the first\nplace, but I don't think we can implement that efficiently (if we added\nan \"xopen\" that checked that, it would have to stat() every file we\nopened).\n\n-Peff\n"},{"id":"262294","messageId":"CAGZ79kbGyZLx=UYCi5JeSLj7keMhcX_eH3qtWs3O+PidRjye4w@mail.gmail.com","threadId":"39437","inReplyTo":"20150527221813.GF23259@peff.net","subject":"Re: Bug: .gitconfig folder","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-27T22:24:47Z","receivedAt":"2015-05-27T22:24:47Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 27, 2015 at 3:18 PM, Jeff King wrote:\n> On Wed, May 27, 2015 at 01:30:29PM -0700, Junio C Hamano wrote:\n>\n>> Jorge writes:\n>>\n>> > If you have a folder named ~/.gitconfig instead of a file with that\n>> > name, when you try to run some global config editing command it will\n>> > fail with a wrong error message:\n>> >\n>> > \"fatal: Out of memory? mmap failed: No such device\"\n>>\n>> That indeed is a funny error message.\n>>\n>> How about this patch?\n>>\n>> -- >8 --\n>> We show that message with die_errno(), but the OS is ought to know\n>> why mmap(2) failed much better than we do. There is no reason for\n>> us to say \"Out of memory?\" here.\n>>\n>> Note that mmap(2) fails with ENODEV when the file you specify is not\n>> something that can be mmap'ed, so you still need to know that \"No\n>> such device\" can include cases like having a directory when a\n>> regular file is expected, but we can expect that a user who creates\n>> a directory to a location where a regular file is expected to be\n>> would know what s/he is doing, hopefully ;-)\n>>\n>> sha1_file.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index ccc6dac..551a9e9 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -720,7 +720,7 @@ void *xmmap(void *start, size_t length,\n>> release_pack_memory(length);\n>> ret = mmap(start, length, prot, flags, fd, offset);\n>> if (ret == MAP_FAILED)\n>> - die_errno(\"Out of memory? mmap failed\");\n>> + die_errno(\"mmap failed\");\n>> }\n>\n> This is definitely an improvement, but the real failing of that error\n> message is that it does not tell us that \"~/.gitconfig\" is the culprit.\n> I don't think we can do much from xmmap, though; it does not have the\n> filename. It would be nice if we got EISDIR from open() in the first\n> place, but I don't think we can implement that efficiently (if we added\n> an \"xopen\" that checked that, it would have to stat() every file we\n> opened).\n\nWhat is our thinking for after-fact recovery attempts?\nLike we try the mmap first, if that fails we just use open to get the\ncontents of\nthe file. And when open fails, we can still print a nice error message?\n\n>\n> -Peff\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"262296","messageId":"xmqq1ti1k5nv.fsf@gitster.dls.corp.google.com","threadId":"39437","inReplyTo":"20150527221813.GF23259@peff.net","subject":"Re: Bug: .gitconfig folder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-27T22:38:12Z","receivedAt":"2015-05-27T22:38:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> -\t\t\tdie_errno(\"Out of memory? mmap failed\");\n>> +\t\t\tdie_errno(\"mmap failed\");\n>\n> This is definitely an improvement, but the real failing of that error\n> message is that it does not tell us that \"~/.gitconfig\" is the culprit.\n> I don't think we can do much from xmmap, though; it does not have the\n> filename. It would be nice if we got EISDIR from open() in the first\n> place, but I don't think we can implement that efficiently (if we added\n> an \"xopen\" that checked that, it would have to stat() every file we\n> opened).\n\nThe patch was meant to be a tongue-in-cheek tangent that is a vast\nimprovement for cases where we absolutely need to use mmap but does\nnot help the OP at all ;-) I do not think there is any need for the\nconfig reader to read the existing file via mmap interface; just\nopen it, strbuf_read() the whole thing (and complain when it cannot)\nand we should be ok.\n\nOr do we write back through the mmaped region or something?\n"},{"id":"262306","messageId":"20150528061327.GA3688@peff.net","threadId":"39437","inReplyTo":"CAGZ79kbGyZLx=UYCi5JeSLj7keMhcX_eH3qtWs3O+PidRjye4w@mail.gmail.com","subject":"Re: Bug: .gitconfig folder","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-28T06:13:27Z","receivedAt":"2015-05-28T06:13:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 03:24:47PM -0700, Stefan Beller wrote:\n\n> What is our thinking for after-fact recovery attempts?\n> Like we try the mmap first, if that fails we just use open to get the\n> contents of\n> the file. And when open fails, we can still print a nice error message?\n\nFor config, I think we could just open and read the file in the first\nplace. The data is not typically very big (and if you have a 3G config\nfile and git barfs with \"out of memory\", I can live with that).\n\n-Peff\n"},{"id":"262309","messageId":"20150528075142.GB3688@peff.net","threadId":"39437","inReplyTo":"xmqq1ti1k5nv.fsf@gitster.dls.corp.google.com","subject":"Re: Bug: .gitconfig folder","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-28T07:51:42Z","receivedAt":"2015-05-28T07:51:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 03:38:12PM -0700, Junio C Hamano wrote:\n\n> The patch was meant to be a tongue-in-cheek tangent that is a vast\n> improvement for cases where we absolutely need to use mmap but does\n> not help the OP at all ;-) I do not think there is any need for the\n> config reader to read the existing file via mmap interface; just\n> open it, strbuf_read() the whole thing (and complain when it cannot)\n> and we should be ok.\n> \n> Or do we write back through the mmaped region or something?\n\nNo, I think we must never do that in our code because our compat mmap\nimplementation uses pread(). So all maps must be MAP_PRIVATE (and our\ncompat mmap barfs if it is not).\n\nI started to go the strbuf_read() route, but it just felt so dirty to\nchange the way the code works only to try to get a better error message.\nSo here's my attempt at making it better while still using mmap. The end\nresult is:\n\n $ mkdir foo\n $ git config --file=foo some.key value\n error: unable to mmap 'foo': Is a directory\n\nHaving looked through the code, I think the _ideal_ way to implement it\nwould actually be with read() and seek(). We read through the config\nonce (with the normal parser, which wraps stdio) and mark the offsets of\nchunks we want to copy to the output. Then we mmap the original (under\nlock, at least, so it shouldn't be racy) and output the existing chunks\nand any new content in the appropriate order.\n\nSo ideally writing each chunk would just be seek() and copy_fd(). But\nour offsets aren't quite perfect. In some cases we read backwards in our\nmmap to find the right cutoff point. I'm sure this is fixable given\nsufficient refactoring, but the config-writing code is such a tangled\nmess that I don't want to spend the time or risk the regressions.\n\n [1/4]: read-cache.c: drop PROT_WRITE from mmap of index\n [2/4]: config.c: fix mmap leak when writing config\n [3/4]: config.c: avoid xmmap error messages\n [4/4]: config.c: rewrite ENODEV into EISDIR when mmap fails\n\n-Peff\n"},{"id":"262310","messageId":"20150528075400.GA23395@peff.net","threadId":"39437","inReplyTo":"20150528075142.GB3688@peff.net","subject":"[PATCH 1/4] read-cache.c: drop PROT_WRITE from mmap of index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-28T07:54:00Z","receivedAt":"2015-05-28T07:54:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time, git's in-memory representation of a cache\nentry actually pointed to the mmap'd on-disk data. So in\n520fc24 (Allow writing to the private index file mapping.,\n2005-04-26), we specified PROT_WRITE so that we could tweak\nthe entries while we run (in our own MAP_PRIVATE copy-on-write\nversion, of course).\n\nLater, 7a51ed6 (Make on-disk index representation separate\nfrom in-core one, 2008-01-14) stopped doing this; we copy\nthe data into our in-core representation, and then drop the\nmmap immediately. We can therefore drop the PROT_WRITE flag.\nIt's probably not hurting anything as it is, but it's\npotentially confusing.\n\nNote that we could also mark the mapping as \"const\" to\nverify that we never write to it. However, we don't\ntypically do that for our other maps, as it then requires\ncasting to munmap() it.\n\nSigned-off-by: Jeff King \n---\nThis one obviously is not necessary for the rest of it, but just\nsomething I noticed while writing my response to you. But read to the\nend of the series; there might be a twist ending that brings it back!\n\n read-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 723d48d..5dee4e2 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1562,7 +1562,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \tif (mmap_size < sizeof(struct cache_header) + 20)\n \t\tdie(\"index file smaller than expected\");\n \n-\tmmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+\tmmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tif (mmap == MAP_FAILED)\n \t\tdie_errno(\"unable to map index file\");\n \tclose(fd);\n-- \n2.4.2.668.gc3b1ade.dirty\n"},{"id":"262311","messageId":"20150528075443.GB23395@peff.net","threadId":"39437","inReplyTo":"20150528075142.GB3688@peff.net","subject":"[PATCH 2/4] config.c: fix mmap leak when writing config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-28T07:54:43Z","receivedAt":"2015-05-28T07:54:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We mmap the existing config file, but fail to unmap it if we\nhit an error. The function already has a shared exit path,\nso we can fix this by moving the mmap pointer to the\nfunction scope and clearing it in the shared exit.\n\nSigned-off-by: Jeff King \n---\n config.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex ab46462..6917100 100644\n--- a/config.c\n+++ b/config.c\n@@ -1939,6 +1939,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \tint ret;\n \tstruct lock_file *lock = NULL;\n \tchar *filename_buf = NULL;\n+\tchar *contents = NULL;\n+\tsize_t contents_sz;\n \n \t/* parse-key returns negative; flip the sign to feed exit(3) */\n \tret = 0 - git_config_parse_key(key, &store.key, &store.baselen);\n@@ -1988,8 +1990,7 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t\tgoto write_err_out;\n \t} else {\n \t\tstruct stat st;\n-\t\tchar *contents;\n-\t\tsize_t contents_sz, copy_begin, copy_end;\n+\t\tsize_t copy_begin, copy_end;\n \t\tint i, new_line = 0;\n \n \t\tif (value_regex == NULL)\n@@ -2108,8 +2109,6 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t\t\t contents_sz - copy_begin) <\n \t\t\t contents_sz - copy_begin)\n \t\t\t\tgoto write_err_out;\n-\n-\t\tmunmap(contents, contents_sz);\n \t}\n \n \tif (commit_lock_file(lock) < 0) {\n@@ -2135,6 +2134,8 @@ out_free:\n \tif (lock)\n \t\trollback_lock_file(lock);\n \tfree(filename_buf);\n+\tif (contents)\n+\t\tmunmap(contents, contents_sz);\n \treturn ret;\n \n write_err_out:\n-- \n2.4.2.668.gc3b1ade.dirty\n"},{"id":"262312","messageId":"20150528075614.GC23395@peff.net","threadId":"39437","inReplyTo":"20150528075142.GB3688@peff.net","subject":"[PATCH 3/4] config.c: avoid xmmap error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-28T07:56:15Z","receivedAt":"2015-05-28T07:56:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The config-writing code uses xmmap to map the existing\nconfig file, which will die if the map fails. This has two\ndownsides:\n\n 1. The error message is not very helpful, as it lacks any\n context about the file we are mapping:\n\n $ mkdir foo\n $ git config --file=foo some.key value\n fatal: Out of memory? mmap failed: No such device\n\n 2. We normally do not die in this code path; instead, we'd\n rather report the error and return an appropriate exit\n status (which is part of the public interface\n documented in git-config.1).\n\nThis patch introduces a \"gentle\" form of xmmap which lets us\nproduce our own error message. We do not want to use mmap\ndirectly, because we would like to use the other\ncompatibility elements of xmmap (e.g., handling 0-length\nmaps portably).\n\nThe end result is:\n\n $ git.compile config --file=foo some.key value\n error: unable to mmap 'foo': No such device\n $ echo $?\n 3\n\nSigned-off-by: Jeff King \n---\n config.c | 11 +++++++++--\n git-compat-util.h | 1 +\n sha1_file.c | 15 +++++++++++----\n 3 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 6917100..e7dc155 100644\n--- a/config.c\n+++ b/config.c\n@@ -2053,8 +2053,15 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \n \t\tfstat(in_fd, &st);\n \t\tcontents_sz = xsize_t(st.st_size);\n-\t\tcontents = xmmap(NULL, contents_sz, PROT_READ,\n-\t\t\tMAP_PRIVATE, in_fd, 0);\n+\t\tcontents = xmmap_gently(NULL, contents_sz, PROT_READ,\n+\t\t\t\t\tMAP_PRIVATE, in_fd, 0);\n+\t\tif (contents == MAP_FAILED) {\n+\t\t\terror(\"unable to mmap '%s': %s\",\n+\t\t\t config_filename, strerror(errno));\n+\t\t\tret = CONFIG_INVALID_FILE;\n+\t\t\tcontents = NULL;\n+\t\t\tgoto out_free;\n+\t\t}\n \t\tclose(in_fd);\n \n \t\tif (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 17584ad..0cc7ae8 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -718,6 +718,7 @@ extern char *xstrndup(const char *str, size_t len);\n extern void *xrealloc(void *ptr, size_t size);\n extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n+extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ccc6dac..73e0bc0 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -707,8 +707,8 @@ static void mmap_limit_check(size_t length)\n \t\t (uintmax_t)length, (uintmax_t)limit);\n }\n \n-void *xmmap(void *start, size_t length,\n-\tint prot, int flags, int fd, off_t offset)\n+void *xmmap_gently(void *start, size_t length,\n+\t\t int prot, int flags, int fd, off_t offset)\n {\n \tvoid *ret;\n \n@@ -719,12 +719,19 @@ void *xmmap(void *start, size_t length,\n \t\t\treturn NULL;\n \t\trelease_pack_memory(length);\n \t\tret = mmap(start, length, prot, flags, fd, offset);\n-\t\tif (ret == MAP_FAILED)\n-\t\t\tdie_errno(\"Out of memory? mmap failed\");\n \t}\n \treturn ret;\n }\n \n+void *xmmap(void *start, size_t length,\n+\tint prot, int flags, int fd, off_t offset)\n+{\n+\tvoid *ret = xmmap_gently(start, length, prot, flags, fd, offset);\n+\tif (ret == MAP_FAILED)\n+\t\t\tdie_errno(\"Out of memory? mmap failed\");\n+\treturn ret;\n+}\n+\n void close_pack_windows(struct packed_git *p)\n {\n \twhile (p->windows) {\n-- \n2.4.2.668.gc3b1ade.dirty\n"},{"id":"262313","messageId":"20150528080300.GD23395@peff.net","threadId":"39437","inReplyTo":"20150528075142.GB3688@peff.net","subject":"[PATCH 4/4] config.c: rewrite ENODEV into EISDIR when mmap fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-28T08:03:01Z","receivedAt":"2015-05-28T08:03:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we try to mmap a directory, we'll get ENODEV. This\ntranslates to \"no such device\" for the user, which is not\nvery helpful. Since we've just fstat()'d the file, we can\neasily check whether the problem was a directory to give a\nbetter message.\n\nSigned-off-by: Jeff King \n---\nIt feels a bit wrong to put this magic conversion here, and not in\nxmmap. But of course xmmap does not have the stat information.\n\nWhich makes me wonder if we should provide an interface that will take\nthe whole \"struct stat\" rather than just the size. That's less flexible,\nbut in most cases, we're mapping the whole file (the packfiles are the\nbig exception, where we use a window).\n\nWe could also potentially drop some of the useless options. As of patch\n1, all of our calls are PROT_READ. They must all be MAP_PRIVATE, or our\npread compatibility wrapper will fail, and we never use other flags. We\nnever request a specific address. And in a whole-file remap, the offset\nwill always be 0. So something like:\n\n void *xmmap_file(int fd, struct stat *st);\n\nwould probably work. We could even do the fstat() on behalf of the\ncaller, though they need to know the length themselves. Maybe:\n\n void *xmmap_file(int fd, size_t *len);\n\nI dunno if it is worth it or not.\n\n config.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex e7dc155..29fa012 100644\n--- a/config.c\n+++ b/config.c\n@@ -2056,6 +2056,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\tcontents = xmmap_gently(NULL, contents_sz, PROT_READ,\n \t\t\t\t\tMAP_PRIVATE, in_fd, 0);\n \t\tif (contents == MAP_FAILED) {\n+\t\t\tif (errno == ENODEV && S_ISDIR(st.st_mode))\n+\t\t\t\terrno = EISDIR;\n \t\t\terror(\"unable to mmap '%s': %s\",\n \t\t\t config_filename, strerror(errno));\n \t\t\tret = CONFIG_INVALID_FILE;\n-- \n2.4.2.668.gc3b1ade.dirty\n"},{"id":"262342","messageId":"xmqqbnh4iqcc.fsf@gitster.dls.corp.google.com","threadId":"39437","inReplyTo":"20150528075142.GB3688@peff.net","subject":"Re: Bug: .gitconfig folder","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-28T17:06:43Z","receivedAt":"2015-05-28T17:06:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, May 27, 2015 at 03:38:12PM -0700, Junio C Hamano wrote:\n>\n>> The patch was meant to be a tongue-in-cheek tangent that is a vast\n>> improvement for cases where we absolutely need to use mmap but does\n>> not help the OP at all ;-) I do not think there is any need for the\n>> config reader to read the existing file via mmap interface; just\n>> open it, strbuf_read() the whole thing (and complain when it cannot)\n>> and we should be ok.\n>> \n>> Or do we write back through the mmaped region or something?\n>\n> No, I think we must never do that in our code because our compat mmap\n> implementation uses pread(). So all maps must be MAP_PRIVATE (and our\n> compat mmap barfs if it is not).\n>\n> I started to go the strbuf_read() route, but it just felt so dirty to\n> change the way the code works only to try to get a better error message.\n\nHmm. I actually thought that we long time ago updated the system to\nread small loose object files via read(2) instead of mmap(2) purely\nas an optimization, as mmap(2) is a bad match if you are going to\nread the whole thing from the beginning to the end anyway, and the\n\"why not strbuf_read() the whole configuration file\" was a suggestion\nalong that line.\n\nBut apparently we do not have such an optimization in read_object()\ncodepath, perhaps I was hallucinating X-<.\n\n> ... but the config-writing code is such a tangled\n> mess that I don't want to spend the time or risk the regressions.\n\nThat part I agree with. I was kinda hoping that the previous GSoC\nwould clean it up, but that did not happen.\n"},{"id":"262343","messageId":"xmqq7frsiq3o.fsf@gitster.dls.corp.google.com","threadId":"39437","inReplyTo":"20150528080300.GD23395@peff.net","subject":"Re: [PATCH 4/4] config.c: rewrite ENODEV into EISDIR when mmap fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-28T17:11:55Z","receivedAt":"2015-05-28T17:11:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If we try to mmap a directory, we'll get ENODEV. This\n> translates to \"no such device\" for the user, which is not\n> very helpful. Since we've just fstat()'d the file, we can\n> easily check whether the problem was a directory to give a\n> better message.\n>\n> Signed-off-by: Jeff King \n> ---\n> It feels a bit wrong to put this magic conversion here, and not in\n> xmmap. But of course xmmap does not have the stat information.\n> ...\n> diff --git a/config.c b/config.c\n> index e7dc155..29fa012 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2056,6 +2056,8 @@ int git_config_set_multivar_in_file(const char *config_filename,\n> \t\tcontents = xmmap_gently(NULL, contents_sz, PROT_READ,\n> \t\t\t\t\tMAP_PRIVATE, in_fd, 0);\n> \t\tif (contents == MAP_FAILED) {\n> +\t\t\tif (errno == ENODEV && S_ISDIR(st.st_mode))\n> +\t\t\t\terrno = EISDIR;\n> \t\t\terror(\"unable to mmap '%s': %s\",\n> \t\t\t config_filename, strerror(errno));\n> \t\t\tret = CONFIG_INVALID_FILE;\n\nI think this patch places the \"magic\" at the right place, but I\nwould have preferred to see something more like this:\n\n\tif (contents == MAP_FAILED) {\n \tif (errno == ENODEV && S_ISDIR(st.st_mode))\n\t\t\terror(\"unable to mmap a directory '%s',\n \tconfig_filename);\n\t\telse\n \terror(\"unable to mmap '%s': %s\",\n \tconfig_filename, strerror(errno));\n\t\tret = CONFIG_INVALID_FILE;\n\nBut that is a very minor preference. I am OK with relying on our\nknowledge that strerror(EISDIR) would give something that says \"the\nthing is a directory which is not appropriate for the operation\", as\nnobody after that strerror() refers to 'errno' in this codepath.\n\nThanks. The patches were a pleasant read.\n"},{"id":"262378","messageId":"20150528204436.GB29148@peff.net","threadId":"39437","inReplyTo":"xmqq7frsiq3o.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/4] config.c: rewrite ENODEV into EISDIR when mmap fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-28T20:44:37Z","receivedAt":"2015-05-28T20:44:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 28, 2015 at 10:11:55AM -0700, Junio C Hamano wrote:\n\n> > \t\tif (contents == MAP_FAILED) {\n> > +\t\t\tif (errno == ENODEV && S_ISDIR(st.st_mode))\n> > +\t\t\t\terrno = EISDIR;\n> > \t\t\terror(\"unable to mmap '%s': %s\",\n> > \t\t\t config_filename, strerror(errno));\n> > \t\t\tret = CONFIG_INVALID_FILE;\n> \n> I think this patch places the \"magic\" at the right place, but I\n> would have preferred to see something more like this:\n> \n> \tif (contents == MAP_FAILED) {\n> \tif (errno == ENODEV && S_ISDIR(st.st_mode))\n> \t\t\terror(\"unable to mmap a directory '%s',\n> \tconfig_filename);\n> \t\telse\n> \terror(\"unable to mmap '%s': %s\",\n> \tconfig_filename, strerror(errno));\n> \t\tret = CONFIG_INVALID_FILE;\n> \n> But that is a very minor preference. I am OK with relying on our\n> knowledge that strerror(EISDIR) would give something that says \"the\n> thing is a directory which is not appropriate for the operation\", as\n> nobody after that strerror() refers to 'errno' in this codepath.\n\nI am OK if you want to switch it. Certainly EISDIR produces good output\non my system, but I don't know if that is universal.\n\nWe also know S_ISDIR(st.st_mode) _before_ we actually mmap. So I was\ntempted to simply check it beforehand, under the assumption that the\nmmap cannot possibly work if we have a directory. But by doing it in the\nerror code path, then we _know_ we are not affecting the outcome, only\nthe error message. :)\n\n-Peff\n"},{"id":"262380","messageId":"xmqqr3q0flus.fsf@gitster.dls.corp.google.com","threadId":"39437","inReplyTo":"20150528204436.GB29148@peff.net","subject":"Re: [PATCH 4/4] config.c: rewrite ENODEV into EISDIR when mmap fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-28T21:11:55Z","receivedAt":"2015-05-28T21:11:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We also know S_ISDIR(st.st_mode) _before_ we actually mmap. So I was\n> tempted to simply check it beforehand, under the assumption that the\n> mmap cannot possibly work if we have a directory. But by doing it in the\n> error code path, then we _know_ we are not affecting the outcome, only\n> the error message. :)\n\nWell, even if your mmap() worked on a directory, having ~/.gitconfig\nas a directory is wrong in the first place, so...\n\nI think what you sent is good as-is, so let's go with them.\n"},{"id":"265234","messageId":"5592A8E5.2090601@gmail.com","threadId":"39437","inReplyTo":"20150528075443.GB23395@peff.net","subject":"[PATCH] config.c: fix writing config files on Windows network shares","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2015-06-30T14:34:13Z","receivedAt":"2015-06-30T14:34:13Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Renaming to an existing file doesn't work on Windows network shares if the\ntarget file is open.\n\nmunmap() the old config file before commit_lock_file.\n\nSigned-off-by: Karsten Blees \n---\n\nSee https://github.com/git-for-windows/git/issues/226\n\nStrangely, renaming to an open file works fine on local disks...\n\n config.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 07133ef..3a23c11 100644\n--- a/config.c\n+++ b/config.c\n@@ -2153,6 +2153,9 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t\t\t contents_sz - copy_begin) <\n \t\t\t contents_sz - copy_begin)\n \t\t\t\tgoto write_err_out;\n+\n+\t\tmunmap(contents, contents_sz);\n+\t\tcontents = NULL;\n \t}\n \n \tif (commit_lock_file(lock) < 0) {\n-- \n2.4.3.windows.1.1.g87477f9\n"},{"id":"265236","messageId":"5592ABBC.60904@web.de","threadId":"39437","inReplyTo":"5592A8E5.2090601@gmail.com","subject":"Re: [PATCH] config.c: fix writing config files on Windows network shares","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-06-30T14:46:20Z","receivedAt":"2015-06-30T14:46:20Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-06-30 16.34, Karsten Blees wrote:\n> Renaming to an existing file doesn't work on Windows network shares if the\n> target file is open.\n> \n> munmap() the old config file before commit_lock_file.\n> \n> Signed-off-by: Karsten Blees \n> ---\n> \n> See https://github.com/git-for-windows/git/issues/226\n> \n> Strangely, renaming to an open file works fine on local disks...\n> \n> config.c | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/config.c b/config.c\n> index 07133ef..3a23c11 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2153,6 +2153,9 @@ int git_config_set_multivar_in_file(const char *config_filename,\n> \t\t\t\t\t contents_sz - copy_begin) <\n> \t\t\t contents_sz - copy_begin)\n> \t\t\t\tgoto write_err_out;\n> +\n> +\t\tmunmap(contents, contents_sz);\n> +\t\tcontents = NULL;\n> \t}\n> \n> \tif (commit_lock_file(lock) < 0) {\n> \n\nNice catch.\nTalking about network file system,\nsomebody volunteering to fix this issue ?\n\nThe value of fstat() is not checked here:\n(indicated by a compiler warning, that contents_sz may be uninitalized.\n\n config.c:\n int git_config_set_multivar_in_file(\n //around line 2063 (the only call to fstat())\n \t\tfstat(in_fd, &st);\n \t\tcontents_sz = xsize_t(st.st_size);\n\n\n(sorry for hijacking your email thread)\n"},{"id":"265237","messageId":"085261f3bbd5d5a14fb038d9909927f8@www.dscho.org","threadId":"39437","inReplyTo":"5592A8E5.2090601@gmail.com","subject":"Re: [PATCH] config.c: fix writing config files on Windows network shares","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-30T14:52:31Z","receivedAt":"2015-06-30T14:52:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn 2015-06-30 16:34, Karsten Blees wrote:\n> Renaming to an existing file doesn't work on Windows network shares if the\n> target file is open.\n> \n> munmap() the old config file before commit_lock_file.\n> \n> Signed-off-by: Karsten Blees \n\nACK.\n\nThanks,\nDscho\n"},{"id":"265243","messageId":"20150630160014.GA3953@peff.net","threadId":"39437","inReplyTo":"5592A8E5.2090601@gmail.com","subject":"Re: [PATCH] config.c: fix writing config files on Windows network shares","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-30T16:00:15Z","receivedAt":"2015-06-30T16:00:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2015 at 04:34:13PM +0200, Karsten Blees wrote:\n\n> Renaming to an existing file doesn't work on Windows network shares if the\n> target file is open.\n> \n> munmap() the old config file before commit_lock_file.\n> \n> Signed-off-by: Karsten Blees \n\nThanks for fixing this.\n\nAcked-by: Jeff King \n\n-Peff\n"},{"id":"265244","messageId":"20150630160125.GB3953@peff.net","threadId":"39437","inReplyTo":"5592ABBC.60904@web.de","subject":"Re: [PATCH] config.c: fix writing config files on Windows network shares","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-30T16:01:25Z","receivedAt":"2015-06-30T16:01:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 30, 2015 at 04:46:20PM +0200, Torsten Bögershausen wrote:\n\n> The value of fstat() is not checked here:\n> (indicated by a compiler warning, that contents_sz may be uninitalized.\n> \n> config.c:\n> int git_config_set_multivar_in_file(\n> //around line 2063 (the only call to fstat())\n> \t\tfstat(in_fd, &st);\n> \t\tcontents_sz = xsize_t(st.st_size);\n\nThere is a similar case in git_config_rename_section_in_file. It looks\nlike they could both just jump to the error case when fstat() fails.\n\n-Peff\n"}]}