[PATCH] disable post-checkout test on Cygwin

[PATCH] disable post-checkout test on Cygwin

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

It is broken because of the tricks we have to play with
lstat to get the bearable perfomance out of the call.
Sadly, it disables access to Cygwin's executable attribute,
which Windows filesystems do not have at all.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 t/t5403-post-checkout-hook.sh |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

2009/3/3 Jeff King <peff@peff.net>:
> +mkdir -p templates/hooks
> +cat >templates/hooks/post-checkout <<'EOF'
> +#!/bin/sh
> +echo $@ > $GIT_DIR/post-checkout.args
> +EOF
> +chmod +x templates/hooks/post-checkout
> +
> +test_expect_success 'post-checkout hook is triggered by clone' '
> +       git clone --template=templates . clone3 &&
> +       test -f clone3/.git/post-checkout.args
> +'

This is broken on cygwin: the hook script won't be not marked executable
by copy_file, because the native Win32 stat(2) routines are used and
report the mode of source file as 0666.

[-- Attachment #2: 0001-disable-post-checkout-test-on-Cygwin.diff --]
[-- Type: application/octet-stream, Size: 1235 bytes --]

From e5394ee710460e25369b4755798930a3f19085c5 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 17 Mar 2009 17:22:53 +0100
Subject: [PATCH] disable post-checkout test on Cygwin

It is broken because of the tricks we have to play with
lstat to get the bearable perfomance out of the call.
Sadly, it disables access to Cygwin's executable attribute,
which Windows filesystems do not have at all.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 t/t5403-post-checkout-hook.sh |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index 4fdb418..5858b86 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -71,6 +71,7 @@ test_expect_success 'post-checkout receives the right args when not switching br
         test $old = $new -a $flag = 0
 '
 
+if test "$(git config --bool core.filemode)" = true; then
 mkdir -p templates/hooks
 cat >templates/hooks/post-checkout <<'EOF'
 #!/bin/sh
@@ -82,5 +83,6 @@ test_expect_success 'post-checkout hook is triggered by clone' '
 	git clone --template=templates . clone3 &&
 	test -f clone3/.git/post-checkout.args
 '
+fi
 
 test_done
-- 
1.6.2.142.gaf8db

Re: [PATCH] disable post-checkout test on Cygwin

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> It is broken because of the tricks we have to play with
> lstat to get the bearable perfomance out of the call.
> Sadly, it disables access to Cygwin's executable attribute,
> which Windows filesystems do not have at all.

Hmm, perhaps when checking hooks to see if they are executable, Cygwin
port should avoid using the "tricks"?  Compared to paths inside the
worktree the number of hooks is a lot smaller, no?

Re: [PATCH] disable post-checkout test on Cygwin

[Johannes Sixt]

Junio C Hamano schrieb:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
>> It is broken because of the tricks we have to play with
>> lstat to get the bearable perfomance out of the call.
>> Sadly, it disables access to Cygwin's executable attribute,
>> which Windows filesystems do not have at all.
> 
> Hmm, perhaps when checking hooks to see if they are executable, Cygwin
> port should avoid using the "tricks"?  Compared to paths inside the
> worktree the number of hooks is a lot smaller, no?

Hmm. Nowadays, we run hooks through run_hook() in run_command.c. It uses
access(..., X_OK), not lstat(). We don't play games with access(), do we?

-- Hannes

Re: [PATCH] disable post-checkout test on Cygwin

[Alex Riesen]

Johannes Sixt, Tue, Mar 17, 2009 17:59:09 +0100:
> Junio C Hamano schrieb:
> > Alex Riesen <raa.lkml@gmail.com> writes:
> > 
> >> It is broken because of the tricks we have to play with
> >> lstat to get the bearable perfomance out of the call.
> >> Sadly, it disables access to Cygwin's executable attribute,
> >> which Windows filesystems do not have at all.
> > 
> > Hmm, perhaps when checking hooks to see if they are executable, Cygwin
> > port should avoid using the "tricks"?  Compared to paths inside the
> > worktree the number of hooks is a lot smaller, no?
> 
> Hmm. Nowadays, we run hooks through run_hook() in run_command.c. It uses

The problem is that copy_templates_1 does an lstat on the files in
templates directory and gets 0666 mode (regular file, non-exec) for
executable file under current Cygwin port. The st_mode of that lstat
is passed to copy_file mentioned, which is useless now as we use the
Win32 version of lstat, which doesn't do x-bit.

> access(..., X_OK), not lstat(). We don't play games with access(), do we?
> 

access(..., X_OK) will return -1.

Re: [PATCH] disable post-checkout test on Cygwin

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Johannes Sixt, Tue, Mar 17, 2009 17:59:09 +0100:
>> Junio C Hamano schrieb:
>> > Alex Riesen <raa.lkml@gmail.com> writes:
>> > 
>> >> It is broken because of the tricks we have to play with
>> >> lstat to get the bearable perfomance out of the call.
>> >> Sadly, it disables access to Cygwin's executable attribute,
>> >> which Windows filesystems do not have at all.
>> > 
>> > Hmm, perhaps when checking hooks to see if they are executable, Cygwin
>> > port should avoid using the "tricks"?  Compared to paths inside the
>> > worktree the number of hooks is a lot smaller, no?
>> 
>> Hmm. Nowadays, we run hooks through run_hook() in run_command.c. It uses
>
> The problem is that copy_templates_1 does an lstat on the files in
> templates directory and gets 0666 mode (regular file, non-exec) for
> executable file under current Cygwin port. The st_mode of that lstat
> is passed to copy_file mentioned, which is useless now as we use the
> Win32 version of lstat, which doesn't do x-bit.

Ahhh.

I do not mind the patch as a band-aid to make the testsuite pass, so I'll
apply your patch as-is.  Thanks.

But isn't this something shops that do deploy Cygwin version of git want
to see fixed, so that they can have a site-wide policy implemented in the
hooks copied from templates?  I think we could pass mode 0 to copy_files()
and have the function special case it (and allow a platform specific
copy_files() implementated by Cygwin).  lstat() in the copy_templates_1()
codepath is primarily done to see if we need to descend into a directory
or symlink() and our use of st.st_mode to pass to copy_files() is a no
cost side effect on platforms with x-bit support.

>> access(..., X_OK), not lstat(). We don't play games with access(), do we?
>
> access(..., X_OK) will return -1.

That codepath would also need to be fixed if Cygwin wants to use hooks, I
would guess.

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

[Alex Riesen]

So that Cygwin port can continue work around its supporting
library and get access to its faked file attributes.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Junio C Hamano, Tue, Mar 17, 2009 21:42:31 +0100:
> But isn't this something shops that do deploy Cygwin version of git want
> to see fixed, so that they can have a site-wide policy implemented in the

Frankly, I doubt they know or care.

> hooks copied from templates?  I think we could pass mode 0 to copy_files()
> and have the function special case it (and allow a platform specific
> copy_files() implementated by Cygwin).  lstat() in the copy_templates_1()
> codepath is primarily done to see if we need to descend into a directory
> or symlink() and our use of st.st_mode to pass to copy_files() is a no
> cost side effect on platforms with x-bit support.

And I don't think that the platform broken in so many ways deserves
that kind of treatement. Maybe this patch is enough. Will test it
tomorrow, when I get to mine so much hated Windows system.

> >> access(..., X_OK), not lstat(). We don't play games with access(), do we?
> > access(..., X_OK) will return -1.
> 
> That codepath would also need to be fixed if Cygwin wants to use hooks, I
> would guess.

I hope not. In the reply to Johannes' example I was referring to the
copied file, the one which was created with open(..., O_CREAT..., 0666),
0666 being there because of our lstat stub.

 builtin-init-db.c |    2 +-
 git-compat-util.h |    5 +++++
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index ee3911f..f3f781b 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -66,7 +66,7 @@ static void copy_templates_1(char *path, int baselen,
 		else
 			exists = 1;
 
-		if (lstat(template, &st_template))
+		if (lstat_for_copy(template, &st_template))
 			die("cannot stat template %s", template);
 
 		if (S_ISDIR(st_template.st_mode)) {
diff --git a/git-compat-util.h b/git-compat-util.h
index 878d83d..4c23478 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -85,11 +85,16 @@
 #undef _XOPEN_SOURCE
 #include <grp.h>
 #define _XOPEN_SOURCE 600
+static inline int lstat_for_copy(const char *file_name, struct stat *buf)
+{
+	return lstat(file_name, buf);
+}
 #include "compat/cygwin.h"
 #else
 #undef _ALL_SOURCE /* AIX 5.3L defines a struct list with _ALL_SOURCE. */
 #include <grp.h>
 #define _ALL_SOURCE 1
+#define lstat_for_copy lstat
 #endif
 #else 	/* __MINGW32__ */
 /* pull in Windows compatibility stuff */
-- 
1.6.2.1.171.g3422

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

[Mark Levedahl]

Alex Riesen wrote:

> So that Cygwin port can continue work around its supporting
> library and get access to its faked file attributes.
> 
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
> Junio C Hamano, Tue, Mar 17, 2009 21:42:31 +0100:
>> But isn't this something shops that do deploy Cygwin version of git want
>> to see fixed, so that they can have a site-wide policy implemented in the
> 
> Frankly, I doubt they know or care.

Please don't presume to speak for the world here. I have a not insignificant 
group of users, many on Cygwin, and we depend upon identical behavior 
between Linux and Cygwin versions of git. I maintain my own local build of 
git for my group, among other reasons is to disable the non-Posix lstat 
hack.  Using the Win32 lstat does not speed up git that much (maybe 20-30% 
in my experience) and this trade-off of compatibility vs being not quite so 
dreadfully slow is certainly not worth it for me. (Obviously, others have a 
different view, or this feature would not exist).

So, I strongly urge keeping the differences between POSIX/Linux git and 
Cygwin git as minimal as possible.

Mark Levedahl

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

[Alex Riesen]

2009/3/17 Alex Riesen <raa.lkml@gmail.com>:
>> hooks copied from templates?  I think we could pass mode 0 to copy_files()
>> and have the function special case it (and allow a platform specific
>> copy_files() implemented by Cygwin).  lstat() in the copy_templates_1()
>> codepath is primarily done to see if we need to descend into a directory
>> or symlink() and our use of st.st_mode to pass to copy_files() is a no
>> cost side effect on platforms with x-bit support.
>
> And I don't think that the platform broken in so many ways deserves
> that kind of treatment. Maybe this patch is enough. Will test it
> tomorrow, when I get to mine so much hated Windows system.
>

Ok, I just tested it and it works. Junio, this patch can replace the previous
change which disabled the test.

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

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> So that Cygwin port can continue work around its supporting
> library and get access to its faked file attributes.
>
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ...
> diff --git a/builtin-init-db.c b/builtin-init-db.c
> index ee3911f..f3f781b 100644
> --- a/builtin-init-db.c
> +++ b/builtin-init-db.c
> @@ -66,7 +66,7 @@ static void copy_templates_1(char *path, int baselen,
>  		else
>  			exists = 1;
>  
> -		if (lstat(template, &st_template))
> +		if (lstat_for_copy(template, &st_template))
>  			die("cannot stat template %s", template);
>  
>  		if (S_ISDIR(st_template.st_mode)) {

Yuck; that's a bit too ugly for generic code.  Will there be other places
that this needs to be used?  If so, we'd probably need to encourage its
use where appropriate, which is even uglier but we cannot avoid it...

Also when the underlying system does not know the executable bit, how
would this help?  I thought that earlier you said the part that checks if
it wants to execute hooks with access(X_OK) will fail, so...

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

[Johannes Sixt]

Junio C Hamano schrieb:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
>> So that Cygwin port can continue work around its supporting
>> library and get access to its faked file attributes.
>>
>> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
>> ...
>> diff --git a/builtin-init-db.c b/builtin-init-db.c
>> index ee3911f..f3f781b 100644
>> --- a/builtin-init-db.c
>> +++ b/builtin-init-db.c
>> @@ -66,7 +66,7 @@ static void copy_templates_1(char *path, int baselen,
>>  		else
>>  			exists = 1;
>>  
>> -		if (lstat(template, &st_template))
>> +		if (lstat_for_copy(template, &st_template))
>>  			die("cannot stat template %s", template);
>>  
>>  		if (S_ISDIR(st_template.st_mode)) {
> 
> Yuck; that's a bit too ugly for generic code.  Will there be other places
> that this needs to be used?  If so, we'd probably need to encourage its
> use where appropriate, which is even uglier but we cannot avoid it...
> 
> Also when the underlying system does not know the executable bit, how
> would this help?  I thought that earlier you said the part that checks if
> it wants to execute hooks with access(X_OK) will fail, so...

The "underlying system" in this case is Cygwin, and it *does* have an
executable bit.

But the FS gymnastics that implement it are slow and affect all lstat()
calls, so we have replaced lstat() with a simpler and faster
implementation. Only that the replacement doesn't know about the X bit
anymore; it always returns mode 0666.

Therefore, if a file is created whose mode is influenced by the fast
lstat(), then it will always be non-X. The access(, X_OK) call on the hook
script would do the right thing if only the script were created with the
correct mode. access(, X_OK) fails because the file was created with non-X
permissions.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> The "underlying system" in this case is Cygwin, and it *does* have an
> executable bit.
>
> But the FS gymnastics that implement it are slow and affect all lstat()
> calls, so we have replaced lstat() with a simpler and faster
> implementation. Only that the replacement doesn't know about the X bit
> anymore; it always returns mode 0666.
>
> Therefore, if a file is created whose mode is influenced by the fast
> lstat(), then it will always be non-X. The access(, X_OK) call on the hook
> script would do the right thing if only the script were created with the
> correct mode. access(, X_OK) fails because the file was created with non-X
> permissions.

OK.  To make sure that I understand the situation better, allow me to
paraphrase what you said.

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?

 (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?

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;

 * 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:

	int lstat_fast(const char *path, struct stat *st);
        void lstat_remainder(const char *path, struct stat *st);

On POSIX systems, we implement it as

        #define lstat_fast(path,st) lstat((path),(st))
        #define lstat_remainder(path,st) do { ; /* nothing */ } while (0)
 
but on Windows and Cygwin, we might implement the format to get
good-enough information for comparison purposes and implement latter to
fill the x-bit and some other parts that are expensive to learn.

Most of the callers that are only interested in seeing if a path has
changed can use the lstat_fast(), while the codepath that actually does
use the information for modified path can augment the information a
previous call to "fast" variant obtained with an additional call to the
"slow" lstat_remainder().

The attached patch to convert a part of read-cache.c is only for
illustration purposes.

The refresh_cache_ent() function first calls lstat_fast() to get cheap
information that is good enough, gives it to ie_match_stat(), to see if
anything has changed, and let fill_stat_cache_info() to update the cached
information for later comparison.

We do not have to touch ce_match_stat_basic() that does care about x-bit
but assuming that the answer to question (2) is "yes", we would not miss
MODE_CHANGED report from it; other information such as timestamps would
have changed in such a case as well, and ie_match_stat() that called
ce_match_stat_basic() will still say "changed" for such an entry.

The add_file_to_index() function checks if the path exists with the "fast"
one, gives the result to add_to_index() that wants the full mode bits
hence invokes the "slow" one to fill in the remainder.

If you have to pass information between "fast" and "slow" variant other
than what you can put in "struct stat" in order for "slow" one to take
advantage of what "fast" one already has done, we would need to introduce
something like

	struct gitstat {
        	struct stat st;
		some other info for platform;
	};

and pass that around instead, which would become a larger change, though.
I am hoping it won't be the case.

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.



diff --git a/read-cache.c b/read-cache.c
index 3f58711..d12d11b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -562,7 +562,7 @@ static void record_intent_to_add(struct cache_entry *ce)
 int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
 {
 	int size, namelen, was_same;
-	mode_t st_mode = st->st_mode;
+	mode_t st_mode;
 	struct cache_entry *ce, *alias;
 	unsigned ce_option = CE_MATCH_IGNORE_VALID|CE_MATCH_RACY_IS_DIRTY;
 	int verbose = flags & (ADD_CACHE_VERBOSE | ADD_CACHE_PRETEND);
@@ -571,6 +571,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	int add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|
 			  (intent_only ? ADD_CACHE_NEW_ONLY : 0));
 
+	lstat_remainder(path, st);
+	st_mode = st->st_mode;
 	if (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))
 		return error("%s: can only add regular files, symbolic links or git-directories", path);
 
@@ -637,7 +639,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 int add_file_to_index(struct index_state *istate, const char *path, int flags)
 {
 	struct stat st;
-	if (lstat(path, &st))
+	if (lstat_fast(path, &st))
 		die("%s: unable to stat (%s)", path, strerror(errno));
 	return add_to_index(istate, path, &st, flags);
 }
@@ -1013,7 +1015,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 		return ce;
 	}
 
-	if (lstat(ce->name, &st) < 0) {
+	if (lstat_fast(ce->name, &st) < 0) {
 		if (err)
 			*err = errno;
 		return NULL;

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

[Johannes Sixt]

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

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

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

That's Ok.  The whole speculation depended on your answer to (2) being
"yes", and because it is not the case, you can safely skip it.

I just wanted to see if we can sprinkle a handful of lstat_remainder()
calls that disappear on POSIX side without changing anything else, which
would be the least impact solution from my point of view.

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

Hm, I sort of agree.

If you imagine somebody with POSIX background who is new to git's codebase
and is trying understand the resulting code, I thought lstat() followed by
lstat_remainder() is conceptually slightly easier to explain ("'remainder'
is needed on platforms whose lstat() do not get the x-bit right, and we
use it only where it matters"), but I do not think the difference is that
great.

You need to explain when to add the "remainder" one after an existing
lstat() call in this API, or you need to explain when to replace an
existing lstat() with "for copy" in the Alex's API.  Either way you need
to contaminate the generic codepath with the distasteful concept that "On
some platforms, you lstat() may not do what you asked it to do".

And that was what I was unhappy about Alex's patch to begin with.  Not the
"patch" nor "Alex", but the fact that we have to deal with the "issue".

And lstat() followed by lstat_remainder() does not solve that issue at
all.  So let's scrap this lstat_fast()/lstat_remainder().

Re: [PATCH] disable post-checkout test on Cygwin

[Alex Riesen]

Junio C Hamano, Tue, Mar 17, 2009 17:52:09 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > It is broken because of the tricks we have to play with
> > lstat to get the bearable perfomance out of the call.
> > Sadly, it disables access to Cygwin's executable attribute,
> > which Windows filesystems do not have at all.
> 
> Hmm, perhaps when checking hooks to see if they are executable, Cygwin
> port should avoid using the "tricks"?  Compared to paths inside the
> worktree the number of hooks is a lot smaller, no?

Yes, the damn thing is just hard to disable without ifdef in
builtin-db.c or builtin-clone.c