Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

Johannes Sixt said the following on 04.09.2007 12:53:

Marius Storm-Olsen schrieb:

quoted

Johannes Sixt said the following on 04.09.2007 09:41:

quoted

Thanks a lot! I've pushed it out in mingw.git's master.

Ops, already in master branch?

Yes, it looked so polished ;)

quoted

http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127 

Looks good, although you should now handle INVALID_HANDLE_VALUE at the 
beginning of git_fstat() like this:

	HANDLE fh = (HANDLE)_get_osfhandle(fd);
	if (fh == INVALID_HANDLE_VALUE)
		return -1;	/* errno has been set */

	if (GetFileInformationByHandle(...

Actually, that's already handled.
GetFileType will report FILE_TYPE_UNKNOWN (0), and GetLastError() will 
return a result != NO_ERROR (<-- so you can follow the codepath, but 
it actually returns ERROR_INVALID_HANDLE (6))
So, it will fall all the way through the function, and end up 
returning -1, with errno = EBADF.

(I use the
     case FILE_TYPE_UNKNOWN:
         if (GetLastError() != NO_ERROR)
             break;
construct, since the documentation says that an FILE_TYPE_UNKNOWN is 
still a _valid_ handle iff GetLastError() returns NO_ERROR. So, then 
we pass it on to the normal fstat function to let that figure out what 
we're dealing with)

quoted

Ok, I can give it a performance test, but I tend to agree with David 
Kastrup there. It would be better if we rather fix the places where we 
check with the local timestamp instead; depending of course on how many 
places we actually do this.
We'll see how much the timezone conversion in the custom stat functions 
actually hurt us performance wise.

I'd make the decision on the grounds of a perfomance test. If it turns out 
that the penalty is bearable, we should keep this stuff private to the MinGW 
build. Otherwise, we would need MinGW specific code at the call sites 
(unless we can hide the opposite conversion in some other wrapper function).

... time passes ...

Ok, I just tested FileTimeToLocalFileTime() in a tight loop, and I can run 
it 100,000,000 times per second. So I'm confident that there won't be any 
noticable degradation with my proposed change.

Ok. I haven't done the performance test with Git yet, but we'll see. 
If it's not noticeable, I'll add it to all the timestamps we have.

-- 
.marius