Re: [PATCH] Define a version of lstat(2) specially for copy operation
- From
Johannes Sixt <j.sixt@viscovery.net>
- Date
- Mar 18, 2009, 10:14 UTC
- Message-ID
- <49C0C983.5060602@viscovery.net>
- In-Reply-To
- <7v8wn3i3ee.fsf@gitster.siamese.dyndns.org>
Junio C Hamano schrieb:
Show 14 quoted lines
> I've always thought core.filemode was about "On FAT the filesystem does > not have x-bit and Cygwin fakes it by looking at .exe extension and using > other heuristics", but this lstat() "trick" is not about that. It is "On > Windows you need to issue multiple FS API calls in order to get full > information to fill struct stat, which is expensive. We omit the one to > learn about the x-bit; it won't make a difference because most people run > with core.filemode." > > If that is what is going on, I have a few quick questions: > > (1) How much of "struct stat" information can you get with the easiest > and quickest FS API call on Windows? Does it give you big enough > subset of what we store in the cache entry to detect modification > reliably?
Yes.
> (2) When you do an equivalent of "chmod +x path" on Windows, does it > change one of the fields in your answer to (1)? In other words, can > you detect such a change with the fastest and reliable-enough FS API > call?
Unfortunately, no. That's why we have this discussion.
Show 7 quoted lines
> If answers to both questions are "yes", then I suspect we might be able to > improve the situation without sacrificing performance too much. > > In the generic part, we use lstat(2) for various purposes: > > * To learn existence and type of a FS entity, in order to decide if we > need to descend into a directory or treat it as a blob;
The fast version does not detect Cygwin's symbolic links, hence, not all types of FS entity that we are interested in are covered.
Show 18 quoted lines
> * To learn the current "status" that can be compared with what is stored > in the cache entry, in order to decide if the FS entity hasn't been > modified; > > * To learn the last-modified timestamp, in order to implement the > racy-git avoidance logic; > > * To learn x-bit, so that we can update it in the cache entry, and give > the appropriate x-bit to a file that we create (Alex's > lstat_for_copy() is about this usage). > > If the first three can be learned with a fast-and-reliable-enough FS API > call, and the x-bit and perhaps something else that do not fall into the > first three need a lot more work to figure out, we could split lstat() > into two. The result from a "fast" variant of lstat() is used for the > first three and allow platform implementation of it to be incomplete > (i.e. the Cygwin "trick" to omit x-bit is OK for that purpose), and add > another "slow" variant to complement it:
Sorry that I skip your elaboration on this. First, I barely understand read-cache.c; and second, I have the feeling that this is over-engineering: It introduces a genericity that we don't need in practice. We don't need it because Cygwin by default is built with NO_TRUSTABLE_FILEMODE, and the X-bit is uninteresting in this case anyway [*]. And there is no other platform that would make use of it. Not even the MinGW port, because it wouldn't have a "slow" part, either.
[*] Ok, Cygwin's slow part would also recognize symbolic links, but we haven't heard complaints about the fast lstat() implementation in this respect. Perhaps because we have an escape door for people who need it (core.ignorecygwinfstricks=false).
Show 6 quoted lines
> And if we do not need such an extra "struct gitstat", then there is no > reason for us to introduce lstat_fast(). We can just use lstat(), keep > the "fast and incomplete" Cygwin one as lstat(), but insert calls to > lstat_remainder() at strategic places. Immediately before copy_file() is > called in builtin-init-db.c::copy_templates_1() would be one of those > strategic places.
But that wouldn't be different in principle from Alex's patch that introduced lstat_for_copy(), would it? Since it would be used in exactly the same strategic places.
-- Hannes