{"thread":{"id":"27642","subject":"[PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin","startedAt":"2011-06-16T20:23:14Z","lastAt":"2011-06-20T19:31:59Z","messageCount":7,"participants":["Ramsay Jones","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"170161","messageId":"4DFA6632.40607@ramsay1.demon.co.uk","threadId":"27642","inReplyTo":null,"subject":"[PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-06-16T20:23:14Z","receivedAt":"2011-06-16T20:23:14Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe 'forced modes' test fails on cygwin because the post-update\nhook loses it's executable bit when copied from the templates\ndirectory by git-init. The template loses it's executable bit\nbecause the lstat() function resolves to the \"native Win32 API\"\nimplementation.\n\nThis call to lstat() happens after git-init has set the \"git_dir\"\n(so has_git_dir() returns true), but before the configuration has\nbeen fully initialised. At this point git_config() does not find\nany config files to parse and returns 0. Unfortunately, the code\nused to determine the cygwin l/stat() function bindings did not\ncheck the return from git_config() and assumed that the config\nwas complete and accessible once \"git_dir\" was set.\n\nIn order to fix the test, we simply change the binding code to\ntest the return value from git_config(), to ensure that it actually\nhad config values to read, before determining the requested binding.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n compat/cygwin.c |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/cygwin.c b/compat/cygwin.c\nindex b4a51b9..b38dbd7 100644\n--- a/compat/cygwin.c\n+++ b/compat/cygwin.c\n@@ -114,8 +114,7 @@ static int git_cygwin_config(const char *var, const char *value, void *cb)\n \n static int init_stat(void)\n {\n-\tif (have_git_dir()) {\n-\t\tgit_config(git_cygwin_config, NULL);\n+\tif (have_git_dir() && git_config(git_cygwin_config,NULL)) {\n \t\tif (!core_filemode && native_stat) {\n \t\t\tcygwin_stat_fn = cygwin_stat;\n \t\t\tcygwin_lstat_fn = cygwin_lstat;\n-- \n1.7.5\n"},{"id":"170167","messageId":"7v8vt1h1g5.fsf@alter.siamese.dyndns.org","threadId":"27642","inReplyTo":"4DFA6632.40607@ramsay1.demon.co.uk","subject":"Re: [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-16T22:10:50Z","receivedAt":"2011-06-16T22:10:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> The 'forced modes' test fails on cygwin because the post-update\n> hook loses it's executable bit when copied from the templates\n> directory by git-init. The template loses it's executable bit\n> because the lstat() function resolves to the \"native Win32 API\"\n> implementation.\n\n> This call to lstat() happens after git-init has set the \"git_dir\"\n> (so has_git_dir() returns true), but before the configuration has\n> been fully initialised. At this point git_config() does not find\n> any config files to parse and returns 0. Unfortunately, the code\n> used to determine the cygwin l/stat() function bindings did not\n> check the return from git_config() and assumed that the config\n> was complete and accessible once \"git_dir\" was set.\n\nOk, so this is not really about \"a test fails so we will sweep the issue\nunder rag\", but \"we try to optimize too early, before we have enough\ninformation, so let the code take slow path before we know what is in the\nconfiguration file\".\n\n> In order to fix the test, we simply change the binding code to\n> test the return value from git_config(), to ensure that it actually\n> had config values to read, before determining the requested binding.\n>\n> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> ---\n>  compat/cygwin.c |    3 +--\n>  1 files changed, 1 insertions(+), 2 deletions(-)\n>\n> diff --git a/compat/cygwin.c b/compat/cygwin.c\n> index b4a51b9..b38dbd7 100644\n> --- a/compat/cygwin.c\n> +++ b/compat/cygwin.c\n> @@ -114,8 +114,7 @@ static int git_cygwin_config(const char *var, const char *value, void *cb)\n>  \n>  static int init_stat(void)\n>  {\n> -\tif (have_git_dir()) {\n> -\t\tgit_config(git_cygwin_config, NULL);\n> +\tif (have_git_dir() && git_config(git_cygwin_config,NULL)) {\n>  \t\tif (!core_filemode && native_stat) {\n>  \t\t\tcygwin_stat_fn = cygwin_stat;\n>  \t\t\tcygwin_lstat_fn = cygwin_lstat;\n"},{"id":"170173","messageId":"4DFB0C66.5080904@kdbg.org","threadId":"27642","inReplyTo":"4DFA6632.40607@ramsay1.demon.co.uk","subject":"Re: [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-06-17T08:12:22Z","receivedAt":"2011-06-17T08:12:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 16.06.2011 22:23, schrieb Ramsay Jones:\n> \n> The 'forced modes' test fails on cygwin because the post-update\n> hook loses it's executable bit when copied from the templates\n> directory by git-init. The template loses it's executable bit\n> because the lstat() function resolves to the \"native Win32 API\"\n> implementation.\n> \n> This call to lstat() happens after git-init has set the \"git_dir\"\n> (so has_git_dir() returns true), but before the configuration has\n> been fully initialised. At this point git_config() does not find\n> any config files to parse and returns 0. Unfortunately, the code\n> used to determine the cygwin l/stat() function bindings did not\n> check the return from git_config() and assumed that the config\n> was complete and accessible once \"git_dir\" was set.\n> \n> In order to fix the test, we simply change the binding code to\n> test the return value from git_config(), to ensure that it actually\n> had config values to read, before determining the requested binding.\n> \n> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> ---\n>  compat/cygwin.c |    3 +--\n>  1 files changed, 1 insertions(+), 2 deletions(-)\n> \n> diff --git a/compat/cygwin.c b/compat/cygwin.c\n> index b4a51b9..b38dbd7 100644\n> --- a/compat/cygwin.c\n> +++ b/compat/cygwin.c\n> @@ -114,8 +114,7 @@ static int git_cygwin_config(const char *var, const char *value, void *cb)\n>  \n>  static int init_stat(void)\n>  {\n> -\tif (have_git_dir()) {\n> -\t\tgit_config(git_cygwin_config, NULL);\n> +\tif (have_git_dir() && git_config(git_cygwin_config,NULL)) {\n>  \t\tif (!core_filemode && native_stat) {\n>  \t\t\tcygwin_stat_fn = cygwin_stat;\n>  \t\t\tcygwin_lstat_fn = cygwin_lstat;\n\nSo, this means that if neither core.filemode nor\ncore.ignorecygwinfstricks is assigned a value, then regular (Cygwin's)\nl/stat is used. Ok, that's what we need: the default value of\ncore.filemode is true, which means we need Cygwin's l/stat; it trumps\nthe default value of core.ignorecygwinfstricks, which is also true. Good!\n\nBTW, it seems the patch fixes a bug when the two config parameters are\nnot assigned a value: the initialization looks like this[*]:\n\nstatic int native_stat = 1;\nstatic int core_filemode;\n\ni.e., the default value of core.filemode seen by compat/cygwin.c is\nactually false, and the fast native l/stat would be used, contrary to\nthe documentation. Am I missing something?\n\n[*] Note to bystanders: compat/cygwin.c keeps its own copy of\ncore.filemode; see the comments near these variables.\n\n-- Hannes\n"},{"id":"170190","messageId":"7vsjr8f8sl.fsf@alter.siamese.dyndns.org","threadId":"27642","inReplyTo":"4DFB0C66.5080904@kdbg.org","subject":"Re: [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-17T21:27:22Z","receivedAt":"2011-06-17T21:27:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>>  static int init_stat(void)\n>>  {\n>> -\tif (have_git_dir()) {\n>> -\t\tgit_config(git_cygwin_config, NULL);\n>> +\tif (have_git_dir() && git_config(git_cygwin_config,NULL)) {\n>>  \t\tif (!core_filemode && native_stat) {\n>>  \t\t\tcygwin_stat_fn = cygwin_stat;\n>>  \t\t\tcygwin_lstat_fn = cygwin_lstat;\n>\n> So, this means that if neither core.filemode nor\n> core.ignorecygwinfstricks is assigned a value, then regular (Cygwin's)\n> l/stat is used. Ok, that's what we need: the default value of\n> core.filemode is true, which means we need Cygwin's l/stat; it trumps\n> the default value of core.ignorecygwinfstricks, which is also\n> true. Good!\n>\n> BTW, it seems the patch fixes a bug when the two config parameters are\n> not assigned a value: the initialization looks like this[*]:\n>\n> static int native_stat = 1;\n> static int core_filemode;\n>\n> i.e., the default value of core.filemode seen by compat/cygwin.c is\n> actually false, and the fast native l/stat would be used, contrary to\n> the documentation. Am I missing something?\n\nProbably you are not missing anything.  It is a regression introduced by\n7974843 (compat/cygwin.c: make runtime detection of lstat/stat lessor\nimpact, 2008-10-23).\n\nWhat the added \"&& git_config()\" is doing is that until we actually open\nand read .git/config and the like, we do not take that if (), meaning we\nleave the cygwin_stat_fn pointing at the cygwin_stat_stub (same for\nlstat), using the \"unoptimized\" cygwin version.  Once we do read from\nconfig we are likely to have core.filemode defined (prepared by git init),\nso the \"default\" value here would probably not matter in practice.\n"},{"id":"170210","messageId":"4DFBD4B0.6020109@ramsay1.demon.co.uk","threadId":"27642","inReplyTo":"7v8vt1h1g5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-06-17T22:26:56Z","receivedAt":"2011-06-17T22:26:56Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n[...]\n>> This call to lstat() happens after git-init has set the \"git_dir\"\n>> (so has_git_dir() returns true), but before the configuration has\n>> been fully initialised. At this point git_config() does not find\n>> any config files to parse and returns 0. Unfortunately, the code\n>> used to determine the cygwin l/stat() function bindings did not\n>> check the return from git_config() and assumed that the config\n>> was complete and accessible once \"git_dir\" was set.\n> \n> Ok, so this is not really about \"a test fails so we will sweep the issue\n> under rag\", \n\nEr ... dunno! I don't quite understand what you mean by this. :(\n\n> ... but \"we try to optimize too early, before we have enough\n> information, so let the code take slow path before we know what is in the\n> configuration file\".\n\nYes. While debugging this test failure, I noticed this behaviour, which\nI consider to be incorrect (ie a bug), and so I determined to fix it up.\nOf course, I knew that this would have the effect of delaying the binding\nof l/stat to the WIN32 implementation, which in turn would have the\nside-effect of fixing this test case!\n\nSo, yes, this is a \"drive-by\" bug-fix for this test; it could be broken\nagain by future patches which change the timing of various setup/config\nfunction calls (I *don't* think it will actually, but don't quote me).\n\nYou could argue that, because of commit adbc0b6 et. seq. and commit\nc869753 (which means that the test-suite is run with core.filemode as\nfalse and core.ignorecygwinfstricks true) that the POSIXPERM prerequiste\nshould not be set (because the WIN32 l/stat implementation does not\nsupport it). In that case, this test would not be run, and the whole\nissue would be moot!  However, on NTFS at least, cygwin *does* support\nPOSIXPERM.\n\n[Hmm, has anybody tried running the test-suite on a FAT32 filesystem\non Linux! *just joking*]\n\nI *always* set core.filemode true in my cygwin repo(s) so that I don't\nhave to deal with these problems. :-P  (I would happily revert adbc0b6,\nbut then I don't have very large repos ...)\n\nBTW, as far as I know, the only remaining problem with the test-suite\non cygwin is an intermittent failure of t4130-apply-criss-cross-rename.sh.\nThis would also not fail at all if the WIN32 l/stat were not used (this\ntime because of the inode emulation; just as on Linux, it forces git to\nnotice the file-change despite the timestamps). Note that t4130-*.sh\nalso fails intermittently on MinGW, for the same reason, but the frequency\nof failure is about 3 times greater on cygwin.\n\nATB,\nRamsay Jones\n"},{"id":"170211","messageId":"4DFCF6AF.3060502@ramsay1.demon.co.uk","threadId":"27642","inReplyTo":"4DFB0C66.5080904@kdbg.org","subject":"Re: [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-06-18T19:04:15Z","receivedAt":"2011-06-18T19:04:15Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Johannes Sixt wrote:\n> BTW, it seems the patch fixes a bug when the two config parameters are\n> not assigned a value: the initialization looks like this[*]:\n> \n> static int native_stat = 1;\n> static int core_filemode;\n> \n> i.e., the default value of core.filemode seen by compat/cygwin.c is\n> actually false, and the fast native l/stat would be used, contrary to\n> the documentation. Am I missing something?\n\nNo, that is indeed a bug. See commit 7974843 (compat/cygwin.c: make\nruntime detection of lstat/stat lessor impact, 23-10-2008).\n\nThat commit \"taught\" me to always change the core.filemode key set up\nby git-init by changing the value (\"false\" -> \"true\"), *not* by simply\ndeleting that line in .git/config. Otherwise, you end up with the\n\"trust_executable_bit\" (aka core.filemode) set to 1 and core_filemode\nset to 0. This leads to yet more schizophrenic behaviour; for example,\nin my cygwin git repo:\n\n    $ git diff-files -p\n    $ vim .git/config # remove the core.filemode key\n    $ git diff-files -p\n    diff --git a/.gitattributes b/.gitattributes\n    diff --git a/.gitignore b/.gitignore\n    diff --git a/.mailmap b/.mailmap\n    diff --git a/COPYING b/COPYING\n    diff --git a/Documentation/.gitattributes b/Documentation/.gitattributes\n    diff --git a/Documentation/.gitignore b/Documentation/.gitignore\n    diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n    ...\n    diff --git a/check-builtins.sh b/check-builtins.sh\n    old mode 100755\n    new mode 100644\n    diff --git a/check-racy.c b/check-racy.c\n    diff --git a/check_bindir b/check_bindir\n    old mode 100755\n    new mode 100644\n    diff --git a/color.c b/color.c\n    ...\n    diff --git a/xdiff/xutils.c b/xdiff/xutils.c\n    diff --git a/xdiff/xutils.h b/xdiff/xutils.h\n    diff --git a/zlib.c b/zlib.c\n    $ git diff-files -p | wc -l\n    3438\n    $ git diff-files -p | grep '^old mode' | wc -l\n    641\n    $ vim .git/config # put \"core.filemode true\" back in\n    $ git diff-files -p\n    $\n\nI had a patch in my cygwin repo which initialized core_filemode to 1\nfor ages, but never remembered to submit it. (I can't find it anymore,\nso I must have deleted that branch). Not that I actually ran a git\nwith that patch applied.\n\nATB,\nRamsay Jones\n"},{"id":"170316","messageId":"7v8vswcn9s.fsf_-_@alter.siamese.dyndns.org","threadId":"27642","inReplyTo":"4DFCF6AF.3060502@ramsay1.demon.co.uk","subject":"Re* [PATCH 2/3] t1301-*.sh: Fix the 'forced modes' test on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-20T19:31:59Z","receivedAt":"2011-06-20T19:31:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> No, that is indeed a bug. See commit 7974843 (compat/cygwin.c: make\n> runtime detection of lstat/stat lessor impact, 23-10-2008).\n\nLet's fix it while the bug has our attention, then.\n\n-- >8 --\nSubject: cygwin: trust executable bit by default\n\nEarlier 7974843 (compat/cygwin.c: make runtime detection of lstat/stat\nlessor impact, 2008-10-23) fixed the low-level \"do we use cygwin specific\nhacks for stat/lstat?\" logic not to call into git_default_config() from\nrandom codepaths that are typically very late in the program, to prevent\nthe call from potentially overwriting other variables that are initialized\nfrom the configuration.\n\nHowever, it forgot that on Cygwin, trust-executable-bit should default to\ntrue.\n\nNoticed by J6t, confirmed by Ramsay Jones, and the brown paper bag is on\nGitster's head.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n compat/cygwin.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/compat/cygwin.c b/compat/cygwin.c\nindex b4a51b9..ba3327f 100644\n--- a/compat/cygwin.c\n+++ b/compat/cygwin.c\n@@ -101,7 +101,7 @@ static int cygwin_stat(const char *path, struct stat *buf)\n  * and calling git_default_config() from here would break such variables.\n  */\n static int native_stat = 1;\n-static int core_filemode;\n+static int core_filemode = 1; /* matches trust_executable_bit default */\n \n static int git_cygwin_config(const char *var, const char *value, void *cb)\n {\n"}]}