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

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Jun 17, 2011, 22:26 UTC
Message-ID: <4DFBD4B0.6020109@ramsay1.demon.co.uk>
In-Reply-To: <7v8vt1h1g5.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

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

[...]

Show 10 quoted lines

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