{"thread":{"id":"25582","subject":"[PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE","startedAt":"2010-10-28T20:53:29Z","lastAt":"2010-11-18T20:27:46Z","messageCount":10,"participants":["Junio C Hamano","Johannes Sixt","Erik Faye-Lund","Jonathan Nieder","Casey Dahlin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"154676","messageId":"1288299210-27092-1-git-send-email-gitster@pobox.com","threadId":"25582","inReplyTo":null,"subject":"[PATCH 1/2] read_sha1_file(): report correct name of packfile with a corrupt object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-28T20:53:29Z","receivedAt":"2010-10-28T20:53:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clarify the error reporting logic by moving the normal codepath (i.e. we\nread the object we wanted to read correctly) up and return early.\n\nThe logic to report the name of the packfile with a corrupt object,\nintroduced by e8b15e6 (sha1_file: Show the the type and path to corrupt\nobjects, 2010-06-10), was totally bogus. The function that knows which\nbad object came from what packfile is has_packed_and_bad(); make it report\nwhich packfile the problem was found.\n\n\"Corrupt\" is already an adjective, e.g. an object is \"corrupt\"; we do not\nhave to say \"corrupted\".\n\nSigned-off-by: Junio C Hamano \n---\n sha1_file.c | 41 ++++++++++++++++++++++++-----------------\n 1 files changed, 24 insertions(+), 17 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 0cd9435..5ed5497 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1003,7 +1003,7 @@ static void mark_bad_packed_object(struct packed_git *p,\n \tp->num_bad_objects++;\n }\n \n-static int has_packed_and_bad(const unsigned char *sha1)\n+static const struct packed_git *has_packed_and_bad(const unsigned char *sha1)\n {\n \tstruct packed_git *p;\n \tunsigned i;\n@@ -1011,8 +1011,8 @@ static int has_packed_and_bad(const unsigned char *sha1)\n \tfor (p = packed_git; p; p = p->next)\n \t\tfor (i = 0; i < p->num_bad_objects; i++)\n \t\t\tif (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))\n-\t\t\t\treturn 1;\n-\treturn 0;\n+\t\t\t\treturn p;\n+\treturn NULL;\n }\n \n int check_sha1_signature(const unsigned char *sha1, void *map, unsigned long size, const char *type)\n@@ -2079,6 +2079,11 @@ static void *read_object(const unsigned char *sha1, enum object_type *type,\n \treturn read_packed_sha1(sha1, type, size);\n }\n \n+/*\n+ * This function dies on corrupted objects; the callers who want to\n+ * deal with them should arrange to call read_object() and give error\n+ * messages themselves.\n+ */\n void *read_sha1_file_repl(const unsigned char *sha1,\n \t\t\t enum object_type *type,\n \t\t\t unsigned long *size,\n@@ -2087,28 +2092,30 @@ void *read_sha1_file_repl(const unsigned char *sha1,\n \tconst unsigned char *repl = lookup_replace_object(sha1);\n \tvoid *data = read_object(repl, type, size);\n \tchar *path;\n+\tconst struct packed_git *p;\n+\n+\tif (data) {\n+\t\tif (replacement)\n+\t\t\t*replacement = repl;\n+\t\treturn data;\n+\t}\n \n \t/* die if we replaced an object with one that does not exist */\n-\tif (!data && repl != sha1)\n+\tif (repl != sha1)\n \t\tdie(\"replacement %s not found for %s\",\n \t\t sha1_to_hex(repl), sha1_to_hex(sha1));\n \n-\t/* legacy behavior is to die on corrupted objects */\n-\tif (!data) {\n-\t\tif (has_loose_object(repl)) {\n-\t\t\tpath = sha1_file_name(sha1);\n-\t\t\tdie(\"loose object %s (stored in %s) is corrupted\", sha1_to_hex(repl), path);\n-\t\t}\n-\t\tif (has_packed_and_bad(repl)) {\n-\t\t\tpath = sha1_pack_name(sha1);\n-\t\t\tdie(\"packed object %s (stored in %s) is corrupted\", sha1_to_hex(repl), path);\n-\t\t}\n+\tif (has_loose_object(repl)) {\n+\t\tpath = sha1_file_name(sha1);\n+\t\tdie(\"loose object %s (stored in %s) is corrupt\",\n+\t\t sha1_to_hex(repl), path);\n \t}\n \n-\tif (replacement)\n-\t\t*replacement = repl;\n+\tif ((p = has_packed_and_bad(repl)) != NULL)\n+\t\tdie(\"packed object %s (stored in %s) is corrupt\",\n+\t\t sha1_to_hex(repl), p->pack_name);\n \n-\treturn data;\n+\treturn NULL;\n }\n \n void *read_object_with_reference(const unsigned char *sha1,\n-- \n1.7.3.2.191.g2d0e57\n"},{"id":"154675","messageId":"1288299210-27092-2-git-send-email-gitster@pobox.com","threadId":"25582","inReplyTo":"1288299210-27092-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-28T20:53:30Z","receivedAt":"2010-10-28T20:53:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git fsck\" bails out with a claim that a loose object that cannot be\nread but exists on the filesystem to be corrupt, which is wrong when\nread_object() failed due to e.g. EMFILE.\n\nSigned-off-by: Junio C Hamano \n---\n sha1_file.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5ed5497..f709ed6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2090,16 +2090,21 @@ void *read_sha1_file_repl(const unsigned char *sha1,\n \t\t\t const unsigned char **replacement)\n {\n \tconst unsigned char *repl = lookup_replace_object(sha1);\n-\tvoid *data = read_object(repl, type, size);\n+\tvoid *data;\n \tchar *path;\n \tconst struct packed_git *p;\n \n+\terrno = 0;\n+\tdata = read_object(repl, type, size);\n \tif (data) {\n \t\tif (replacement)\n \t\t\t*replacement = repl;\n \t\treturn data;\n \t}\n \n+\tif (errno != ENOENT)\n+\t\tdie_errno(\"failed to read object %s\", sha1_to_hex(sha1));\n+\n \t/* die if we replaced an object with one that does not exist */\n \tif (repl != sha1)\n \t\tdie(\"replacement %s not found for %s\",\n-- \n1.7.3.2.191.g2d0e57\n"},{"id":"154767","messageId":"4CCA810C.5090202@viscovery.net","threadId":"25582","inReplyTo":"1288299210-27092-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/2] read_sha1_file(): report correct name of packfile with a corrupt object","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-10-29T08:08:44Z","receivedAt":"2010-10-29T08:08:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/28/2010 22:53, schrieb Junio C Hamano:\n> \"Corrupt\" is already an adjective, ... we do not\n> have to say \"corrupted\".\n\n> + * This function dies on corrupted objects; the callers who want to\n\nExcept sometimes. ;)\n\n-- Hannes\n"},{"id":"156097","messageId":"AANLkTi=L1Z6kxubMf3yPUfpY9ugd+Qan+yCxi3dp4oR4@mail.gmail.com","threadId":"25582","inReplyTo":"1288299210-27092-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-11-18T14:19:03Z","receivedAt":"2010-11-18T14:19:03Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Oct 28, 2010 at 10:53 PM, Junio C Hamano wrote:\n> \"git fsck\" bails out with a claim that a loose object that cannot be\n> read but exists on the filesystem to be corrupt, which is wrong when\n> read_object() failed due to e.g. EMFILE.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>  sha1_file.c |    7 ++++++-\n>  1 files changed, 6 insertions(+), 1 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 5ed5497..f709ed6 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2090,16 +2090,21 @@ void *read_sha1_file_repl(const unsigned char *sha1,\n>                          const unsigned char **replacement)\n>  {\n>        const unsigned char *repl = lookup_replace_object(sha1);\n> -       void *data = read_object(repl, type, size);\n> +       void *data;\n>        char *path;\n>        const struct packed_git *p;\n>\n> +       errno = 0;\n> +       data = read_object(repl, type, size);\n>        if (data) {\n>                if (replacement)\n>                        *replacement = repl;\n>                return data;\n>        }\n>\n> +       if (errno != ENOENT)\n> +               die_errno(\"failed to read object %s\", sha1_to_hex(sha1));\n> +\n>        /* die if we replaced an object with one that does not exist */\n>        if (repl != sha1)\n>                die(\"replacement %s not found for %s\",\n> --\n> 1.7.3.2.191.g2d0e57\n>\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>\n\nThis patch seems to break clone/fetch on Windows:\n\n$ GIT_TRACE=1 git clone http://github.com/kusma/rocket.git clone-test\ntrace: built-in: git 'clone' 'http://github.com/kusma/rocket.git' 'clone-test'\nCloning into clone-test...\ntrace: run_command: 'git-remote-http' 'origin'\n'http://github.com/kusma/rocket.git'\ntrace: run_command: 'fetch-pack' '--stateless-rpc' '--lock-pack'\n'--thin' 'http://github.com/kusma/rocket.git/' 'refs/heads/master'\n'refs/heads/work/icon' 'refs/heads/work/io-callback'\n'refs/heads/work/sdl-example' 'refs/tags/v0.8'\ntrace: built-in: git 'fetch-pack' '--stateless-rpc' '--lock-pack'\n'--thin' 'http://github.com/kusma/rocket.git/' 'refs/heads/master'\n'refs/heads/work/icon' 'refs/heads/work/io-callback'\n'refs/heads/work/sdl-example' 'refs/tags/v0.8'\nfatal: failed to read object 7ea7cabd967429886c26e78b3ec573d54a95fa9d: No error\nfatal: The remote end hung up unexpectedly\n\nWhat happens, is that read_object returns NULL, but errno is 0.\nFurther, it looks to me like read_object can only return NULL through\nthe unpack_sha1_file (problem with the compressed data) or\nread_packed_sha1 (find_pack_entry() failure) code-paths.\n\nerrno is set to ENOENT by open_sha1_file (through map_sha1_file)\nbefore any possible error-points. I guess this makes the \"errno = 0\"\nredundant, but I think it improves readability of the code. I'm\nguessing that errno gets overwritten by some other call, losing the\nENOENT. Perhaps some unintended side-effect of one of the\ncompat/mingw.[ch]-wrappers?\n\nI did find such a case, but fixing it doesn't do the trick for me:\n\n---8<---\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 29f4036..fdbf093 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -198,9 +198,10 @@ static inline time_t filetime_to_time_t(const FILETIME *ft)\n */\n static int do_lstat(int follow, const char *file_name, struct stat *buf)\n {\n+\tint err;\n \tWIN32_FILE_ATTRIBUTE_DATA fdata;\n\n-\tif (!(errno = get_file_attr(file_name, &fdata))) {\n+\tif (!(err = get_file_attr(file_name, &fdata))) {\n \t\tbuf->st_ino = 0;\n \t\tbuf->st_gid = 0;\n \t\tbuf->st_uid = 0;\n@@ -233,6 +234,7 @@ static int do_lstat(int follow, const char\n*file_name, struct stat *buf)\n \t\t}\n \t\treturn 0;\n \t}\n+\terrno = err;\n \treturn -1;\n }\n\n---8<---\n\nThe following patch makes clone complete again, but I doubt it's the\nright solution. However, I'm out of ideas for now. Anyone? :)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 25f6965..9d4cb9c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2102,7 +2102,7 @@ void *read_sha1_file_repl(const unsigned char *sha1,\n \t\treturn data;\n \t}\n\n-\tif (errno != ENOENT)\n+\tif (errno && errno != ENOENT)\n \t\tdie_errno(\"failed to read object %s\", sha1_to_hex(sha1));\n\n \t/* die if we replaced an object with one that does not exist */\n"},{"id":"156111","messageId":"4CE55794.7050201@viscovery.net","threadId":"25582","inReplyTo":"AANLkTi=L1Z6kxubMf3yPUfpY9ugd+Qan+yCxi3dp4oR4@mail.gmail.com","subject":"Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-11-18T16:43:00Z","receivedAt":"2010-11-18T16:43:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11/18/2010 15:19, schrieb Erik Faye-Lund:\n> What happens, is that read_object returns NULL, but errno is 0.\n> Further, it looks to me like read_object can only return NULL through\n> the unpack_sha1_file (problem with the compressed data) or\n> read_packed_sha1 (find_pack_entry() failure) code-paths.\n> \n> errno is set to ENOENT by open_sha1_file (through map_sha1_file)\n> before any possible error-points. I guess this makes the \"errno = 0\"\n> redundant, but I think it improves readability of the code. I'm\n> guessing that errno gets overwritten by some other call, losing the\n> ENOENT. Perhaps some unintended side-effect of one of the\n> compat/mingw.[ch]-wrappers?\n\nThe problem is in opendir() called via prepare_packed_git_one() via\nprepare_packed_git(). It resets errno to 0 on success.\n\nYou can test this easily by inserting test_done after the 3rd test of\nt5530 and run it with --debug; in the trash-directory you can run\n\n ../../git-pack-objects --revs --all --stdout >/dev/null wrote:\n> Am 11/18/2010 15:19, schrieb Erik Faye-Lund:\n>> What happens, is that read_object returns NULL, but errno is 0.\n>> Further, it looks to me like read_object can only return NULL through\n>> the unpack_sha1_file (problem with the compressed data) or\n>> read_packed_sha1 (find_pack_entry() failure) code-paths.\n>>\n>> errno is set to ENOENT by open_sha1_file (through map_sha1_file)\n>> before any possible error-points. I guess this makes the \"errno = 0\"\n>> redundant, but I think it improves readability of the code. I'm\n>> guessing that errno gets overwritten by some other call, losing the\n>> ENOENT. Perhaps some unintended side-effect of one of the\n>> compat/mingw.[ch]-wrappers?\n>\n> The problem is in opendir() called via prepare_packed_git_one() via\n> prepare_packed_git(). It resets errno to 0 on success.\n>\n\nHmm, this is not one of our wrappings, but a bug in the mingw-runtine,\nright? Perhaps we should report it? I see that \"errno = 0;\" is the\nfirst line of code of _topendir() in the mingw-runtime (see\nhttp://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/mingw/mingwex/dirent.c?rev=1.9&cvsroot=src),\nand from the looks of it it should simply be removed. The same goes\nfor closedir() and readdir().\n\nThat being said, we should probably work around this problem with\nopendir() ourselves. This patch fixes the problem for me:\n\n---8<---\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 35d9813..b5f16fa 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -133,6 +133,29 @@ static inline int mingw_unlink(const char *pathname)\n }\n #define unlink mingw_unlink\n\n+static inline DIR *mingw_opendir(const char *path)\n+{\n+\tDIR *ret;\n+\tint saved_errno = errno;\n+\n+\tif (!(ret = opendir(path)))\n+\t\treturn NULL;\n+\n+\terrno = saved_errno;\n+\treturn ret;\n+}\n+#define opendir mingw_opendir\n+\n+static inline int mingw_closedir(DIR *dir)\n+{\n+\tint saved_errno = errno;\n+\tif (closedir(dir) == -1)\n+\t\treturn -1;\n+\terrno = saved_errno;\n+\treturn 0;\n+}\n+#define closedir mingw_closedir\n+\n #define WNOHANG 1\n pid_t waitpid(pid_t pid, int *status, unsigned options);\n\n---8<---\n\nI didn't patch readdir(), since we already have our own\nimplementation, but perhaps the code should work together with\nNO_MINGW_REPLACE_READDIR also... Can somebody remind me why we have an\nopt-out for the readdir()-replacement? Debugging?\n\n> You can test this easily by inserting test_done after the 3rd test of\n> t5530 and run it with --debug; in the trash-directory you can run\n>\n>  ../../git-pack-objects --revs --all --stdout >/dev/null \n> and observe the different failure modes on Windows and Linux.\n>\n\nThanks.\n\n> This makes me question whether the approach of Junio's fix is sane. It\n> depends on errno being set *way* before it is checked and after *a*lot* of\n> potentially failing system and library calls have been called. Which\n> function is it that is expected to fail with ENOENT? git_open_noatime()?\n>\n\nI was wondering about the same thing, I don't think this approach is\nvery easy to follow. But either way I think we should make sure\nopendir/closedir/readdir doesn't mess with errno.\n"},{"id":"156117","messageId":"20101118172920.GC16610@burratino","threadId":"25582","inReplyTo":"AANLkTikns0Rm5znC7FOa72qZXKpQzazKiYsPe-oDNMAk@mail.gmail.com","subject":"Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-18T17:29:20Z","receivedAt":"2010-11-18T17:29:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Erik Faye-Lund wrote:\n\n> I was wondering about the same thing, I don't think this approach is\n> very easy to follow. But either way I think we should make sure\n> opendir/closedir/readdir doesn't mess with errno.\n\nOther platforms and are allowed to (and do) clobber errno in many\nnon-error situations, so the main effect would be to work around/hide\nsome git bugs.\n\nFor mingw-runtime, the balance might be different. Making buggy,\nnon-portable application code easier to port does not sound like a bad\nthing.\n\nJust my two cents,\nJonathan\n"},{"id":"156126","messageId":"AANLkTi=rrcc5tia03K-Rtqs3vyjOv9P1NArnmbYhCQ27@mail.gmail.com","threadId":"25582","inReplyTo":"20101118172920.GC16610@burratino","subject":"Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-11-18T18:18:37Z","receivedAt":"2010-11-18T18:18:37Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Nov 18, 2010 at 6:29 PM, Jonathan Nieder wrote:\n> Erik Faye-Lund wrote:\n>\n>> I was wondering about the same thing, I don't think this approach is\n>> very easy to follow. But either way I think we should make sure\n>> opendir/closedir/readdir doesn't mess with errno.\n>\n> Other platforms and are allowed to (and do) clobber errno in many\n> non-error situations, so the main effect would be to work around/hide\n> some git bugs.\n\nI don't think that's the case; K&R (section B1.7) says \"In addition,\nthe integer expression errno (declared in ) may contain an\nerror number that contains information about the most recent error.\".\nA non-error condition would not be information about the most recent\nerror. I interpret the use of the word \"may\" to mean \"some functions\ndoes not update errno, so errno could also have information about\nolder errors\" not as \"some functions can clear errno\", but other might\ndisagree with me.\n\nPOSIX (which defines opendir) says \"No function in this volume of IEEE\nStd 1003.1-2001 shall set errno to 0\", so I think it's clear cut in\nthis case. (see\nhttp://www.opengroup.org/onlinepubs/009695399/functions/errno.html for\nthe full text)\n\nWe also seem to depend quite heavily on errno being preserved on\nnon-errors some places around in the code.\n"},{"id":"156127","messageId":"20101118182358.GQ355@fearengine.rdu.redhat.com","threadId":"25582","inReplyTo":"AANLkTi=rrcc5tia03K-Rtqs3vyjOv9P1NArnmbYhCQ27@mail.gmail.com","subject":"Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE","fromName":"Casey Dahlin","fromEmail":"cdahlin@redhat.com","sentAt":"2010-11-18T18:23:59Z","receivedAt":"2010-11-18T18:23:59Z","isPatch":true,"sender":{"key":"cdahlin@redhat.com","avatar":null},"body":"On Thu, Nov 18, 2010 at 07:18:37PM +0100, Erik Faye-Lund wrote:\n> On Thu, Nov 18, 2010 at 6:29 PM, Jonathan Nieder wrote:\n> > Erik Faye-Lund wrote:\n> >\n> >> I was wondering about the same thing, I don't think this approach is\n> >> very easy to follow. But either way I think we should make sure\n> >> opendir/closedir/readdir doesn't mess with errno.\n> >\n> > Other platforms and are allowed to (and do) clobber errno in many\n> > non-error situations, so the main effect would be to work around/hide\n> > some git bugs.\n> \n> I don't think that's the case; K&R (section B1.7) says \"In addition,\n> the integer expression errno (declared in ) may contain an\n> error number that contains information about the most recent error.\".\n> A non-error condition would not be information about the most recent\n> error. I interpret the use of the word \"may\" to mean \"some functions\n> does not update errno, so errno could also have information about\n> older errors\" not as \"some functions can clear errno\", but other might\n> disagree with me.\n> \n> POSIX (which defines opendir) says \"No function in this volume of IEEE\n> Std 1003.1-2001 shall set errno to 0\", so I think it's clear cut in\n> this case. (see\n> http://www.opengroup.org/onlinepubs/009695399/functions/errno.html for\n> the full text)\n> \n> We also seem to depend quite heavily on errno being preserved on\n> non-errors some places around in the code.\n\nErrno can be clobbered if some subset of the operation in question\nfailed but the operation itself succeeded. For example suppose opendir\ndid a memory allocation that failed, but was able to recover from it and\nreturn successfully. Errno might contain ENOMEM at the exit.\n\ntl;dr - errno can be clobbered when functions that succeed call\nfunctions that fail while progressing.\n\n--CJD\n"},{"id":"156136","messageId":"20101118202746.GG16610@burratino","threadId":"25582","inReplyTo":"AANLkTi=rrcc5tia03K-Rtqs3vyjOv9P1NArnmbYhCQ27@mail.gmail.com","subject":"Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-18T20:27:46Z","receivedAt":"2010-11-18T20:27:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Erik Faye-Lund wrote:\n\n> We also seem to depend quite heavily on errno being preserved on\n> non-errors some places around in the code.\n\nHmm, sounds worrying.\n\n- builtin/fetch.c::s_update_ref appears to have some of this kind of\n bogosity.\n- safe_create_leading_directories_const and git_config_set_multivar let\n free() touch errno. Probably not a problem.\n- lock_file ought to preserve or set errno on failure.\n- daemon::run_service sometimes sets errno on failure and sometimes not.\n This is confusing even if callers don't seem to care.\n- fast-import::dump_marks_helper ought to do its own error reporting.\n- resolve_ref seems to rely on get_packed_refs not to clobber errno.\n- sha1_file::open_packed_git_1 seems to rely on unuse_one_window not to\n clobber errno.\n\nAside from that:\n\nThe following does not take into account correctness of error\nmessages. (That is probably a good reason to fix compat/mingw's\nopendir. Sorry, I forgot to take that into account before.)\n\nBuiltins\n--------\napply.c:\n\tchecks errno following lstat() failure.\n\tchecks errno following try_create_file() return from\n\t mkdir(), symlink(), or open() failure.\nclone.c:\tchecks errno following mkdir() and unlink() failure.\ncommit.c:\tsaves errno following strbuf_read_file() failure.\nfetch.c:\n\tchecks errno following write_in_full(), close(), fopen() failure.\n\tsee above for the s_update_ref() checks.\nfor-each-ref.c:\tchecks errno following strtol() failure.\ngrep.c:\tchecks errno following open() and lstat() failure.\nindex-pack.c:\n\tchecks errno following open() failure.\n\tchecks errno following odb_pack_keep() [= open()] failure.\ninit-db.c:\tchecks errno following mkdir(), lstat() failure.\nlog.c:\tchecks errno following mkdir() failure.\nmailsplit.c:\tchecks errno following opendir() failure.\npack-objects.c:\tchecks errno following fgets() failure.\nrm.c:\tchecks errno following lstat() failure.\nupdate-index.c:\n\tchecks errno following lstat() failure.\n\trelies on strtoul() not to clobber errno on success.\n\tchecks errno following hold_locked_index() return from\n\t lock_file() failure (see above).\nupload-archive.c:\n\tchecks errno following read(), pipe(), fork(), and poll() failure.\n\nCompatibility routines\n----------------------\ncygwin.c, inet_ntop.c, mingw.c, and mmap.c use errno in\nnoncosmetic ways. I haven't checked them.\n\ngit wrapper\n-----------\ngit.c:\tchecks errno following run_command_v_opt() failure.\n\nlibgit\n------\nconfig.c:\tchecks errno following open() failure, with intervening free().\nconnect.c:\tchecks errno following connect(), socket() failure.\ncopy.c:\tsaves errno following xread(), xwrite() failure.\ndaemon.c:\tchecks errno following poll(), accept() failure.\ndiff-lib.c:\tchecks errno following lstat() failure.\ndiff.c:\tchecks errno following lstat() failure.\ndir.c:\tchecks errno following unlink() failure.\nentry.c:\tchecks errno following mkdir() failure.\nfast-import.c:\n\tsaves errno following fdopen(), fprintf(), fclose() failure.\n\tsaves errno following commit_lock_file [=close() or rename()] failure.\n\trelies on strtoul() not to clobber errno on success.\ngit-compat-util.h:\n\trelies on strtoul(), strtol() not to clobber errno on success.\nhttp.c:\tchecks errno following open() failure.\nlockfile.c:\tchecks errno following open() failure.\nmerge-recursive.c:\n\tchecks errno following write_in_full(), unlink() failure.\npath.c:\tchecks errno following snprintf() failure.\nread-cache.c:\tchecks errno following lstat(), open() failure.\nrefs.c:\tsee above for resolve_ref() worries.\nrerere.c:\tchecks errno following fwrite(), unlink() failure.\nrun-command.c:\n\tchecks errno following waitpid, pipe, fork, execv, failure.\n\twait_or_whine() does not set errno when child died by signal.\nsetup.c:\tchecks errno following lstat() failure.\nsha1_file.c:\n\tchecks errno following open(), opendir() failure.\n\tsee above re unuse_one_window() failure.\nsha1_name.c:\tchecks errno following lstat() failure.\nstrbuf.c:\tchecks errno following readlink() failure.\nsymlinks.c:\tchecks errno following stat(), lstat() failure.\ntransport-helper.c:\tchecks errno following start_command() failure.\nunpack-trees.c:\tchecks errno following lstat() failure.\nupload-pack.c:\tchecks errno following poll() failure.\nutf8.c:\tchecks errno following iconv() failure.\nwrapper.c:\n\tchecks errno following read(), write(), unlink(), rmdir() failure.\nwrite_or_die.c:\tchecks errno following fflush(), write() failures.\n\nTest helpers\n------------\ntest-run-command.c:\tchecks errno following start_command() failure.\n"}]}