[PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin

[PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin

[Ramsay Jones]

The 'forced modes' test fails on cygwin because the post-update
hook loses it's executable bit when copied from the templates
directory by git-init. The template loses it's executable bit
because the lstat() function resolves to the "native Win32 API"
implementation.

This call to lstat() happens after git-init has set the "git_dir"
(so has_git_dir() returns true), but before the configuration has
been fully initialised. At this point git_config() does not find
any config files to parse and returns 0. Unfortunately, the code
used to determine the cygwin l/stat() function bindings did not
check the return from git_config() and assumed that the config
was complete and accessible once "git_dir" was set.

In order to fix the test, we simply change the binding code to
test the return value from git_config(), to ensure that it actually
had config values to read, before determining the requested binding.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 compat/cygwin.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/compat/cygwin.c b/compat/cygwin.c
index b4a51b9..b38dbd7 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -114,8 +114,7 @@ static int git_cygwin_config(const char *var, const char *value, void *cb)
 
 static int init_stat(void)
 {
-	if (have_git_dir()) {
-		git_config(git_cygwin_config, NULL);
+	if (have_git_dir() && git_config(git_cygwin_config,NULL)) {
 		if (!core_filemode && native_stat) {
 			cygwin_stat_fn = cygwin_stat;
 			cygwin_lstat_fn = cygwin_lstat;
-- 
1.7.5

Re: [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> The 'forced modes' test fails on cygwin because the post-update
> hook loses it's executable bit when copied from the templates
> directory by git-init. The template loses it's executable bit
> because the lstat() function resolves to the "native Win32 API"
> implementation.

> This call to lstat() happens after git-init has set the "git_dir"
> (so has_git_dir() returns true), but before the configuration has
> been fully initialised. At this point git_config() does not find
> any config files to parse and returns 0. Unfortunately, the code
> used to determine the cygwin l/stat() function bindings did not
> check the return from git_config() and assumed that the config
> was complete and accessible once "git_dir" was set.

Ok, so this is not really about "a test fails so we will sweep the issue
under rag", but "we try to optimize too early, before we have enough
information, so let the code take slow path before we know what is in the
configuration file".

> In order to fix the test, we simply change the binding code to
> test the return value from git_config(), to ensure that it actually
> had config values to read, before determining the requested binding.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  compat/cygwin.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/compat/cygwin.c b/compat/cygwin.c
> index b4a51b9..b38dbd7 100644
> --- a/compat/cygwin.c
> +++ b/compat/cygwin.c
> @@ -114,8 +114,7 @@ static int git_cygwin_config(const char *var, const char *value, void *cb)
>  
>  static int init_stat(void)
>  {
> -	if (have_git_dir()) {
> -		git_config(git_cygwin_config, NULL);
> +	if (have_git_dir() && git_config(git_cygwin_config,NULL)) {
>  		if (!core_filemode && native_stat) {
>  			cygwin_stat_fn = cygwin_stat;
>  			cygwin_lstat_fn = cygwin_lstat;

Re: [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin

[Ramsay Jones]

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
[...]
>> This call to lstat() happens after git-init has set the "git_dir"
>> (so has_git_dir() returns true), but before the configuration has
>> been fully initialised. At this point git_config() does not find
>> any config files to parse and returns 0. Unfortunately, the code
>> used to determine the cygwin l/stat() function bindings did not
>> check the return from git_config() and assumed that the config
>> was complete and accessible once "git_dir" was set.
> 
> Ok, so this is not really about "a test fails so we will sweep the issue
> under rag", 

Er ... dunno! I don't quite understand what you mean by this. :(

> ... but "we try to optimize too early, before we have enough
> information, so let the code take slow path before we know what is in the
> configuration file".

Yes. While debugging this test failure, I noticed this behaviour, which
I consider to be incorrect (ie a bug), and so I determined to fix it up.
Of course, I knew that this would have the effect of delaying the binding
of l/stat to the WIN32 implementation, which in turn would have the
side-effect of fixing this test case!

So, yes, this is a "drive-by" bug-fix for this test; it could be broken
again by future patches which change the timing of various setup/config
function calls (I *don't* think it will actually, but don't quote me).

You could argue that, because of commit adbc0b6 et. seq. and commit
c869753 (which means that the test-suite is run with core.filemode as
false and core.ignorecygwinfstricks true) that the POSIXPERM prerequiste
should not be set (because the WIN32 l/stat implementation does not
support it). In that case, this test would not be run, and the whole
issue would be moot!  However, on NTFS at least, cygwin *does* support
POSIXPERM.

[Hmm, has anybody tried running the test-suite on a FAT32 filesystem
on Linux! *just joking*]

I *always* set core.filemode true in my cygwin repo(s) so that I don't
have to deal with these problems. :-P  (I would happily revert adbc0b6,
but then I don't have very large repos ...)

BTW, as far as I know, the only remaining problem with the test-suite
on cygwin is an intermittent failure of t4130-apply-criss-cross-rename.sh.
This would also not fail at all if the WIN32 l/stat were not used (this
time because of the inode emulation; just as on Linux, it forces git to
notice the file-change despite the timestamps). Note that t4130-*.sh
also fails intermittently on MinGW, for the same reason, but the frequency
of failure is about 3 times greater on cygwin.

ATB,
Ramsay Jones

Re: [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin

[Johannes Sixt]

Am 16.06.2011 22:23, schrieb Ramsay Jones:
> 
> The 'forced modes' test fails on cygwin because the post-update
> hook loses it's executable bit when copied from the templates
> directory by git-init. The template loses it's executable bit
> because the lstat() function resolves to the "native Win32 API"
> implementation.
> 
> This call to lstat() happens after git-init has set the "git_dir"
> (so has_git_dir() returns true), but before the configuration has
> been fully initialised. At this point git_config() does not find
> any config files to parse and returns 0. Unfortunately, the code
> used to determine the cygwin l/stat() function bindings did not
> check the return from git_config() and assumed that the config
> was complete and accessible once "git_dir" was set.
> 
> In order to fix the test, we simply change the binding code to
> test the return value from git_config(), to ensure that it actually
> had config values to read, before determining the requested binding.
> 
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>  compat/cygwin.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/compat/cygwin.c b/compat/cygwin.c
> index b4a51b9..b38dbd7 100644
> --- a/compat/cygwin.c
> +++ b/compat/cygwin.c
> @@ -114,8 +114,7 @@ static int git_cygwin_config(const char *var, const char *value, void *cb)
>  
>  static int init_stat(void)
>  {
> -	if (have_git_dir()) {
> -		git_config(git_cygwin_config, NULL);
> +	if (have_git_dir() && git_config(git_cygwin_config,NULL)) {
>  		if (!core_filemode && native_stat) {
>  			cygwin_stat_fn = cygwin_stat;
>  			cygwin_lstat_fn = cygwin_lstat;

So, this means that if neither core.filemode nor
core.ignorecygwinfstricks is assigned a value, then regular (Cygwin's)
l/stat is used. Ok, that's what we need: the default value of
core.filemode is true, which means we need Cygwin's l/stat; it trumps
the default value of core.ignorecygwinfstricks, which is also true. Good!

BTW, it seems the patch fixes a bug when the two config parameters are
not assigned a value: the initialization looks like this[*]:

static int native_stat = 1;
static int core_filemode;

i.e., the default value of core.filemode seen by compat/cygwin.c is
actually false, and the fast native l/stat would be used, contrary to
the documentation. Am I missing something?

[*] Note to bystanders: compat/cygwin.c keeps its own copy of
core.filemode; see the comments near these variables.

-- Hannes

Re: [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>>  static int init_stat(void)
>>  {
>> -	if (have_git_dir()) {
>> -		git_config(git_cygwin_config, NULL);
>> +	if (have_git_dir() && git_config(git_cygwin_config,NULL)) {
>>  		if (!core_filemode && native_stat) {
>>  			cygwin_stat_fn = cygwin_stat;
>>  			cygwin_lstat_fn = cygwin_lstat;
>
> So, this means that if neither core.filemode nor
> core.ignorecygwinfstricks is assigned a value, then regular (Cygwin's)
> l/stat is used. Ok, that's what we need: the default value of
> core.filemode is true, which means we need Cygwin's l/stat; it trumps
> the default value of core.ignorecygwinfstricks, which is also
> true. Good!
>
> BTW, it seems the patch fixes a bug when the two config parameters are
> not assigned a value: the initialization looks like this[*]:
>
> static int native_stat = 1;
> static int core_filemode;
>
> i.e., the default value of core.filemode seen by compat/cygwin.c is
> actually false, and the fast native l/stat would be used, contrary to
> the documentation. Am I missing something?

Probably you are not missing anything.  It is a regression introduced by
7974843 (compat/cygwin.c: make runtime detection of lstat/stat lessor
impact, 2008-10-23).

What the added "&& git_config()" is doing is that until we actually open
and read .git/config and the like, we do not take that if (), meaning we
leave the cygwin_stat_fn pointing at the cygwin_stat_stub (same for
lstat), using the "unoptimized" cygwin version.  Once we do read from
config we are likely to have core.filemode defined (prepared by git init),
so the "default" value here would probably not matter in practice.

Re: [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin

[Ramsay Jones]

Johannes Sixt wrote:
> BTW, it seems the patch fixes a bug when the two config parameters are
> not assigned a value: the initialization looks like this[*]:
> 
> static int native_stat = 1;
> static int core_filemode;
> 
> i.e., the default value of core.filemode seen by compat/cygwin.c is
> actually false, and the fast native l/stat would be used, contrary to
> the documentation. Am I missing something?

No, that is indeed a bug. See commit 7974843 (compat/cygwin.c: make
runtime detection of lstat/stat lessor impact, 23-10-2008).

That commit "taught" me to always change the core.filemode key set up
by git-init by changing the value ("false" -> "true"), *not* by simply
deleting that line in .git/config. Otherwise, you end up with the
"trust_executable_bit" (aka core.filemode) set to 1 and core_filemode
set to 0. This leads to yet more schizophrenic behaviour; for example,
in my cygwin git repo:

    $ git diff-files -p
    $ vim .git/config # remove the core.filemode key
    $ git diff-files -p
    diff --git a/.gitattributes b/.gitattributes
    diff --git a/.gitignore b/.gitignore
    diff --git a/.mailmap b/.mailmap
    diff --git a/COPYING b/COPYING
    diff --git a/Documentation/.gitattributes b/Documentation/.gitattributes
    diff --git a/Documentation/.gitignore b/Documentation/.gitignore
    diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
    ...
    diff --git a/check-builtins.sh b/check-builtins.sh
    old mode 100755
    new mode 100644
    diff --git a/check-racy.c b/check-racy.c
    diff --git a/check_bindir b/check_bindir
    old mode 100755
    new mode 100644
    diff --git a/color.c b/color.c
    ...
    diff --git a/xdiff/xutils.c b/xdiff/xutils.c
    diff --git a/xdiff/xutils.h b/xdiff/xutils.h
    diff --git a/zlib.c b/zlib.c
    $ git diff-files -p | wc -l
    3438
    $ git diff-files -p | grep '^old mode' | wc -l
    641
    $ vim .git/config # put "core.filemode true" back in
    $ git diff-files -p
    $

I had a patch in my cygwin repo which initialized core_filemode to 1
for ages, but never remembered to submit it. (I can't find it anymore,
so I must have deleted that branch). Not that I actually ran a git
with that patch applied.

ATB,
Ramsay Jones

Re* [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> No, that is indeed a bug. See commit 7974843 (compat/cygwin.c: make
> runtime detection of lstat/stat lessor impact, 23-10-2008).

Let's fix it while the bug has our attention, then.

-- >8 --
Subject: cygwin: trust executable bit by default

Earlier 7974843 (compat/cygwin.c: make runtime detection of lstat/stat
lessor impact, 2008-10-23) fixed the low-level "do we use cygwin specific
hacks for stat/lstat?" logic not to call into git_default_config() from
random codepaths that are typically very late in the program, to prevent
the call from potentially overwriting other variables that are initialized
from the configuration.

However, it forgot that on Cygwin, trust-executable-bit should default to
true.

Noticed by J6t, confirmed by Ramsay Jones, and the brown paper bag is on
Gitster's head.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 compat/cygwin.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/compat/cygwin.c b/compat/cygwin.c
index b4a51b9..ba3327f 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -101,7 +101,7 @@ static int cygwin_stat(const char *path, struct stat *buf)
  * and calling git_default_config() from here would break such variables.
  */
 static int native_stat = 1;
-static int core_filemode;
+static int core_filemode = 1; /* matches trust_executable_bit default */
 
 static int git_cygwin_config(const char *var, const char *value, void *cb)
 {