From: Johannes Sixt <j6t@kdbg.org>
To: Marius Storm-Olsen <marius@storm-olsen.com>
Cc: marius@trolltech.com, git@vger.kernel.org, msysgit@googlegroups.com
Subject: Re: [PATCH] MinGW readdir reimplementation to support d_type
Date: Sat, 11 Apr 2009 23:44:58 +0200 [thread overview]
Message-ID: <49E10F5A.9010400@kdbg.org> (raw)
In-Reply-To: <49DEFA30.1000101@gmail.com>
Marius Storm-Olsen schrieb:
> Johannes Sixt said the following on 09.04.2009 22:34:
>> Marius Storm-Olsen schrieb:
>>> +struct mingw_dirent
>>> +{
>>> + long d_ino; /* Always zero. */
>>> + union {
>>> + unsigned short d_reclen; /* Always zero. */
>>> + unsigned char d_type; /* Reimplementation adds this */
>>> + };
>>
>> VERY sneaky! I was wondering why you could get away without replacing
>> opendir and closedir, and why you still defined a replacement
>> mingw_DIR that contains the replacement mingw_dirent, until I noticed
>> this unnamed union.
>>
>> Since we don't use d_reclen anywhere in the code, wouldn't you get
>> away with
>>
>> #define d_type d_reclen
>>
>> unless the type (short vs. char) makes a difference. Or would you say
>> that doing that would be even more sneaky?
>
> I'm sure it could be done just with a define. However, given the
> remaining unused variables, I was wondering about also packing in
> permission bits and file modification time in there, to optimize the
> status checking even further. That way, on Windows, we would only need
> one 'readdir' pass to check the whole repository, with no lstats
> whatsoever. So, this was patch was a 'primer' for that, hence the union
> with a proper uchar for the d_type.
>
> However, that would also mean a significant change in the status
> checking code, as it first lstat's ever file in the index, then uses
> read_directory + lstat's for others. I guess that'll be too big of a
> change in core code, so the vision is moot?
>
> I'd be ok to just use the define, provided that it compiles cleanly of
> course, if the above seems too ambitious. :-) I kinda feel like the
> current code is more clean though :)
With a comment in the commit message, it would have been clear, perhaps.
I'll carry this in my (private) tree for a while with the below squashed
in to avoid a lot of warnings.
-- Hannes
diff --git a/compat/mingw.h b/compat/mingw.h
index 104b310..16ec76b 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -260,4 +260,5 @@ struct mingw_dirent
};
#define dirent mingw_dirent
#define readdir(x) mingw_readdir(x)
+struct dirent *mingw_readdir(DIR *dir);
#endif // !NO_MINGW_REPLACE_READDIR
next prev parent reply other threads:[~2009-04-11 21:46 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-04-08 21:01 [PATCH] MinGW readdir reimplementation to support d_type Marius Storm-Olsen
2009-04-09 20:34 ` [msysGit] " Johannes Sixt
2009-04-10 7:50 ` Marius Storm-Olsen
2009-04-11 21:44 ` Johannes Sixt [this message]
2009-05-07 21:26 ` Heiko Voigt
2009-05-08 5:45 ` Marius Storm-Olsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=49E10F5A.9010400@kdbg.org \
--to=j6t@kdbg.org \
--cc=git@vger.kernel.org \
--cc=marius@storm-olsen.com \
--cc=marius@trolltech.com \
--cc=msysgit@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).