[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH] add GIT_FAST_STAT mode for Cygwin
From:       Dmitry Potapov <dpotapov () gmail ! com>
Date:       2008-09-23 19:48:02
Message-ID: 20080923194802.GQ21650 () dpotapov ! dyndns ! org
[Download RAW message or body]

On Tue, Sep 23, 2008 at 09:03:08PM +0200, Johannes Sixt wrote:
> On Dienstag, 23. September 2008, Dmitry Potapov wrote:
> > +static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
> > +{ 
> > +	long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;
> > +	winTime -= 116444736000000000LL; /* Windows to Unix  Epoch conversion */
> > +	ts->tv_sec = (time_t)(winTime/10000000); /* 100-nanosecond interval to seconds */
> > +	ts->tv_nsec = (long)(winTime - ts->tv_sec) * 100; /* nanoseconds */ +}
> 
> +	ts->tv_nsec = (long)(winTime - ts->tv_sec*10000000LL) * 100;

Thanks.... What was I thought about when wrote this....

> 
> > +static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)
> > +{ 
> > +	WIN32_FILE_ATTRIBUTE_DATA fdata;
> > +
> > +	if (file_name[0] == '/')
> > +		return cygstat (file_name, buf);
> 
> You should do this in the caller; it would make this function's
> semantics much clearer.

IMHO, the semantic of this function is clear: do_stat performs stat/lstat
using Windows API with falling back on Cygwin implementation in those
rare cases that it cannot handle correctly. Absolute path is just one of
those cases. So, I am not sure what you win by moving this two lines out.


> > +	if (GetFileAttributesExA(file_name, GetFileExInfoStandard, &fdata)) {
> > +		int fMode = S_IREAD;
> > +		/*
> > +		 * If the system attribute is set and it is not a directory then
> > +		 * it could be a symbol link created in the nowinsymlinks mode.
> > +		 * Normally, Cygwin works in the winsymlinks mode, so this situation
> > +		 * is very unlikely. For the sake of simplicity of our code, let's
> > +		 * Cygwin to handle it.
> > +		 */
> > +		if ((fdata.dwFileAttributes & FILE_ATTRIBUTE_SYSTEM) &&
> > +		    !(fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY))
> > +			return cygstat (file_name, buf);

This is specific to cygwin.

> > +
> > +		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
> > +			fMode |= S_IFDIR;
> > +		else
> > +			fMode |= S_IFREG;
> > +		if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
> > +			fMode |= S_IWRITE;

These lines the same as mingw

> > +
> > +		/* st_dev, st_rdev are not used by Git */
> > +		buf->st_dev = buf->st_rdev = 0;

I set this to 0, while MinGW Git uses _getdrive(). I have no idea why
it does so. Git does not use this field, and if it did, adding the
_current_ drive number is useless at best when we are trying to
determine whether the file is changed or not.

> > +		/* it is difficult to obtain the inode number on Windows,
> > +		 * so let's set it to zero as MinGW Git does. */
> > +		buf->st_ino = 0;
> > +		buf->st_mode = fMode;
> > +		buf->st_nlink = 1;
> > +		buf->st_uid = buf->st_gid = 0;

This is the same as for MinGW

> > +#ifdef __CYGWIN_USE_BIG_TYPES__
> > +		buf->st_size = ((_off64_t)fdata.nFileSizeHigh << 32) +
> > +			fdata.nFileSizeLow;
> > +#else
> > +		buf->st_size = (off_t)fdata.nFileSizeLow;
> > +#endif
> > +		buf->st_blocks = size_to_blocks(buf->st_size);
> > +		filetime_to_timespec(&fdata.ftLastAccessTime, &buf->st_atim);
> > +		filetime_to_timespec(&fdata.ftLastWriteTime, &buf->st_mtim);
> > +		filetime_to_timespec(&fdata.ftCreationTime, &buf->st_ctim);

This is different: using 64-bit version for st_size, st_blocks does not
exist in MinGW, and finally filetime_to_timespec instead of filetime_to_time_t,
as well as the name of fields is different (st_ctim instead of st_ctime, etc).

> > +		errno = 0;
> > +		return 0;
> > +	}
> > +
> > +	switch (GetLastError()) {
> > +	case ERROR_ACCESS_DENIED:
> > +	case ERROR_SHARING_VIOLATION:
> > +	case ERROR_LOCK_VIOLATION:
> > +	case ERROR_SHARING_BUFFER_EXCEEDED:
> > +		errno = EACCES;
> > +		break;
> > +	case ERROR_BUFFER_OVERFLOW:
> > +		errno = ENAMETOOLONG;
> > +		break;
> > +	case ERROR_NOT_ENOUGH_MEMORY:
> > +		errno = ENOMEM;
> > +		break;
> > +	default:
> > +		/* In the winsymlinks mode (which is the default), Cygwin
> > +		 * emulates symbol links using Windows shortcut files. These
> > +		 * files are formed by adding .lnk extension. So, if we have
> > +		 * not found the specified file name, it could be that it is
> > +		 * a symbol link. Let's Cygwin to deal with that.
> > +		 */
> > +		return cygstat (file_name, buf);
> > +	}

This is the same as in MinGW, except the default case, where MinGW
returns error immediately while this version calls the fallback
function.


> > +	return -1;
> 
> You do duplicate a lot of code here. Any chances to factor out the
> common parts?

I don't see much common code here. Initialization of 5 variables where
four of them are just constants? Perhaps, the biggest common part here
is conversion of dwFileAttributes to st_mode, but it is still 5 lines of
trivial code.

Dmitry
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]