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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
