On Thu, Nov 18, 2010 at 5:43 PM, Johannes Sixt <j.sixt@xxxxxxxxxxxxx> wrote: > Am 11/18/2010 15:19, schrieb Erik Faye-Lund: >> What happens, is that read_object returns NULL, but errno is 0. >> Further, it looks to me like read_object can only return NULL through >> the unpack_sha1_file (problem with the compressed data) or >> read_packed_sha1 (find_pack_entry() failure) code-paths. >> >> errno is set to ENOENT by open_sha1_file (through map_sha1_file) >> before any possible error-points. I guess this makes the "errno = 0" >> redundant, but I think it improves readability of the code. I'm >> guessing that errno gets overwritten by some other call, losing the >> ENOENT. Perhaps some unintended side-effect of one of the >> compat/mingw.[ch]-wrappers? > > The problem is in opendir() called via prepare_packed_git_one() via > prepare_packed_git(). It resets errno to 0 on success. > Hmm, this is not one of our wrappings, but a bug in the mingw-runtine, right? Perhaps we should report it? I see that "errno = 0;" is the first line of code of _topendir() in the mingw-runtime (see http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/mingw/mingwex/dirent.c?rev=1.9&cvsroot=src), and from the looks of it it should simply be removed. The same goes for closedir() and readdir(). That being said, we should probably work around this problem with opendir() ourselves. This patch fixes the problem for me: ---8<--- diff --git a/compat/mingw.h b/compat/mingw.h index 35d9813..b5f16fa 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -133,6 +133,29 @@ static inline int mingw_unlink(const char *pathname) } #define unlink mingw_unlink +static inline DIR *mingw_opendir(const char *path) +{ + DIR *ret; + int saved_errno = errno; + + if (!(ret = opendir(path))) + return NULL; + + errno = saved_errno; + return ret; +} +#define opendir mingw_opendir + +static inline int mingw_closedir(DIR *dir) +{ + int saved_errno = errno; + if (closedir(dir) == -1) + return -1; + errno = saved_errno; + return 0; +} +#define closedir mingw_closedir + #define WNOHANG 1 pid_t waitpid(pid_t pid, int *status, unsigned options); ---8<--- I didn't patch readdir(), since we already have our own implementation, but perhaps the code should work together with NO_MINGW_REPLACE_READDIR also... Can somebody remind me why we have an opt-out for the readdir()-replacement? Debugging? > You can test this easily by inserting test_done after the 3rd test of > t5530 and run it with --debug; in the trash-directory you can run > > ../../git-pack-objects --revs --all --stdout >/dev/null </dev/null > > and observe the different failure modes on Windows and Linux. > Thanks. > This makes me question whether the approach of Junio's fix is sane. It > depends on errno being set *way* before it is checked and after *a*lot* of > potentially failing system and library calls have been called. Which > function is it that is expected to fail with ENOENT? git_open_noatime()? > I was wondering about the same thing, I don't think this approach is very easy to follow. But either way I think we should make sure opendir/closedir/readdir doesn't mess with errno. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html