Message ID | f2ce9bdc01892b514f75c6c25c3393765593b1ca.1540290197.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Use nanosecond-precision file times on Windows | expand |
On Tue, Oct 23, 2018 at 03:23:21AM -0700, Karsten Blees via GitGitGadget wrote: > - if (!get_file_info_by_handle(fh, buf)) > + case FILE_TYPE_CHAR: > + case FILE_TYPE_PIPE: > + /* initialize stat fields */ > + memset(buf, 0, sizeof(*buf)); > + buf->st_nlink = 1; > + > + if (type == FILE_TYPE_CHAR) { > + buf->st_mode = _S_IFCHR; > + } else { > + buf->st_mode = _S_IFIFO; > + if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL)) > + buf->st_size = avail; These lines strike me as a bit odd. As far as I'm aware, Unix systems don't return anything useful in this field when calling fstat on a pipe. Is there a reason we fill this in on Windows? If so, could the commit message explain what that is?
Hi brian, On Wed, 24 Oct 2018, brian m. carlson wrote: > On Tue, Oct 23, 2018 at 03:23:21AM -0700, Karsten Blees via GitGitGadget wrote: > > - if (!get_file_info_by_handle(fh, buf)) > > + case FILE_TYPE_CHAR: > > + case FILE_TYPE_PIPE: > > + /* initialize stat fields */ > > + memset(buf, 0, sizeof(*buf)); > > + buf->st_nlink = 1; > > + > > + if (type == FILE_TYPE_CHAR) { > > + buf->st_mode = _S_IFCHR; > > + } else { > > + buf->st_mode = _S_IFIFO; > > + if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL)) > > + buf->st_size = avail; > > These lines strike me as a bit odd. As far as I'm aware, Unix systems > don't return anything useful in this field when calling fstat on a pipe. > Is there a reason we fill this in on Windows? If so, could the commit > message explain what that is? AFAICT the idea was to imitate MSVCRT's fstat() in these cases. But a quick web search suggests that you are right: https://bugzilla.redhat.com/show_bug.cgi?id=58768#c4 (I could not find any official documentation talking about fstat() and pipes, but I trust Alan to know their stuff). Do note, please, that according to the issue described in that link, at least *some* glibc/Linux combinations behave in exactly the way this patch implements it. At this point, I am wary of changing this, too, as the code in question has been in production (read: tested thoroughly) in the current form for *years*, and I am really loathe to introduce a bug where even Windows-specific code in compat/ might rely on this behavior. (And no, I do not trust our test suite to find all of those use cases.) Ciao, Dscho
On Wed, Oct 24, 2018 at 09:37:43AM +0200, Johannes Schindelin wrote: > Hi brian, > > On Wed, 24 Oct 2018, brian m. carlson wrote: > > These lines strike me as a bit odd. As far as I'm aware, Unix systems > > don't return anything useful in this field when calling fstat on a pipe. > > Is there a reason we fill this in on Windows? If so, could the commit > > message explain what that is? > > AFAICT the idea was to imitate MSVCRT's fstat() in these cases. > > But a quick web search suggests that you are right: > https://bugzilla.redhat.com/show_bug.cgi?id=58768#c4 (I could not find any > official documentation talking about fstat() and pipes, but I trust Alan > to know their stuff). Yeah, that behavior is quite old. I'm surprised that Linux ever did that. > Do note, please, that according to the issue described in that link, at > least *some* glibc/Linux combinations behave in exactly the way this patch > implements it. > > At this point, I am wary of changing this, too, as the code in question > has been in production (read: tested thoroughly) in the current form for > *years*, and I am really loathe to introduce a bug where even > Windows-specific code in compat/ might rely on this behavior. (And no, I > do not trust our test suite to find all of those use cases.) I don't feel strongly either way. I feel confident the rest of Git doesn't use that field, so I don't see any downsides to keeping it other than the slight overhead of populating it. I just thought I'd ask in case there was something important I was missing.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > Yeah, that behavior is quite old. I'm surprised that Linux ever did > that. > ... > I don't feel strongly either way. I feel confident the rest of Git > doesn't use that field, so I don't see any downsides to keeping it other > than the slight overhead of populating it. I just thought I'd ask in > case there was something important I was missing. OK, I'd consider that this part of the review settled for taking the patch as-is. Let's mark the topic for merging to 'next' soonish in the what's cooking report. Thanks.
diff --git a/compat/mingw.c b/compat/mingw.c index d2e7d86db..07fc0b79a 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -771,20 +771,31 @@ int mingw_stat(const char *file_name, struct stat *buf) int mingw_fstat(int fd, struct stat *buf) { HANDLE fh = (HANDLE)_get_osfhandle(fd); + DWORD avail, type = GetFileType(fh) & ~FILE_TYPE_REMOTE; - if (fh == INVALID_HANDLE_VALUE) { - errno = EBADF; - return -1; - } - /* direct non-file handles to MS's fstat() */ - if (GetFileType(fh) != FILE_TYPE_DISK) - return _fstati64(fd, buf); + switch (type) { + case FILE_TYPE_DISK: + return get_file_info_by_handle(fh, buf); - if (!get_file_info_by_handle(fh, buf)) + case FILE_TYPE_CHAR: + case FILE_TYPE_PIPE: + /* initialize stat fields */ + memset(buf, 0, sizeof(*buf)); + buf->st_nlink = 1; + + if (type == FILE_TYPE_CHAR) { + buf->st_mode = _S_IFCHR; + } else { + buf->st_mode = _S_IFIFO; + if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL)) + buf->st_size = avail; + } return 0; - errno = EBADF; - return -1; + default: + errno = EBADF; + return -1; + } } static inline void time_t_to_filetime(time_t t, FILETIME *ft)