Re: [PATCH] Define a version of lstat(2) specially for copy operation

Junio C Hamano schrieb:

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.

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.

 * 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).

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