{"thread":{"id":"33231","subject":"Re: [RFC] test-lib.sh: No POSIXPERM for cygwin","startedAt":"2013-03-19T19:49:58Z","lastAt":"2013-03-19T21:10:02Z","messageCount":3,"participants":["Torsten Bögershausen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"211705","messageId":"201303192049.58755.tboegi@web.de","threadId":"33231","inReplyTo":null,"subject":"Re: [RFC] test-lib.sh: No POSIXPERM for cygwin","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-03-19T19:49:58Z","receivedAt":"2013-03-19T19:49:58Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Friday 08 February 2013 07:08:14 Torsten Bögershausen wrote:\n> On 07.02.13 20:35, Junio C Hamano wrote:\n> > Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n> >\n> >> Torsten Bögershausen wrote:\n> >>> t0070 and t1301 fail when running the test suite under cygwin.\n> >>> Skip the failing tests by unsetting POSIXPERM.\n> >> t1301 does not fail for me. (WIN XP (SP3) on NTFS)\n> > Others run Cygwin with vfat or some other filesystem, and some of\n> > them do not cope will with POSIXPERM, perhaps?\n> >\n> > Not having POSIXPERM by default for Cygwin may be a saner default\n> > than having one, if we have to pick one.\n> >\n> > It may be debatable to have this default as platform attribute,\n> > though.\n> >\n> Yes, 1301 passes on cygwin 1.5, but not on 1.7.\n> And it breaks on VFAT, for all kind of OS.\n> \n> Thanks for comments, I'll put more investigations on my todo stack.\n> /Torsten\n\nIt turns out that cygwin 1.7 handles the read only attribute different then\ncygwin 1.5.\nUnder cygwin 1.5 the \"read only\" attribute is synchronized with the\n\"user write\" bit from the permissions.\n\ntb@PC /cygdrive/c/temp/pt\n$ echo x >x\n$ chmod 444 x\n$ attrib x\nA    R     C:\\temp\\pt\\x\nCygwin 1.7 does not do the synchronization, which means that some files\nseem to be writable when do_stat() is used, but the permissions are r--r--r--\n\nWe can improve Git to work for cygwin 1.7, a suggestion for a patch\nmay look like this:\n\n-- >8 --\nSubject: [PATCH] Make core.sharedRepository work under cygwin 1.7\n\nWhen core.sharedRepository is used, set_shared_perm() in path.c\nneeds lstat() to return the correct POSIX permissions.\n\nThe default for cygwin is core.ignoreCygwinFSTricks = false, which\nmeans that a simplified and fast implementation of lstat() is used.\n\nEspecially the file permission bits are wrong in cygwin_lstat_fn():\nThe read-only attribute of a file is used to calculate\nthe permissions, resulting in either rw-r--r-- or r--r--r--\n\nUse a compile switch IGNORECYGWINFSTRICKS to disable the usage\nof cygwin_lstat_fn() only in path.c\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n compat/cygwin.h | 2 ++\n path.c          | 2 ++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/compat/cygwin.h b/compat/cygwin.h\nindex a3229f5..984efbe 100644\n--- a/compat/cygwin.h\n+++ b/compat/cygwin.h\n@@ -5,5 +5,7 @@ typedef int (*stat_fn_t)(const char*, struct stat*);\n extern stat_fn_t cygwin_stat_fn;\n extern stat_fn_t cygwin_lstat_fn;\n \n+#ifndef IGNORECYGWINFSTRICKS\n #define stat(path, buf) (*cygwin_stat_fn)(path, buf)\n #define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)\n+#endif\ndiff --git a/path.c b/path.c\nindex d3d3f8b..0acfabf 100644\n--- a/path.c\n+++ b/path.c\n@@ -10,6 +10,8 @@\n  *\n  * which is what it's designed for.\n  */\n+#define IGNORECYGWINFSTRICKS\n+\n #include \"cache.h\"\n #include \"strbuf.h\"\n #include \"string-list.h\"\n-- \n1.8.2.341.g543621f\n"},{"id":"211708","messageId":"7v620nhzle.fsf@alter.siamese.dyndns.org","threadId":"33231","inReplyTo":"201303192049.58755.tboegi@web.de","subject":"Re: [RFC] test-lib.sh: No POSIXPERM for cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-19T21:03:09Z","receivedAt":"2013-03-19T21:03:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> Use a compile switch IGNORECYGWINFSTRICKS to disable the usage\n> of cygwin_lstat_fn() only in path.c\n\nThe analysis of the problem and the basic idea to disable the\nfast-but-lying fstricks in the code that matters may be good, but\nthe implementation is questionable.\n\nWhat if we later need to move functions around, etc., so that some\nother calls in path.c still do want to use the fstricks bit while\nthe existing ones in the file want the real lstat() information?\n\nActually, that already is the case.  The call to lstat() in\nvalidate_headref() only cares about the S_ISXXX() type and can\nafford to use the fast-and-lying one, no?\n\nHow about doing something like this in the generic codepath, and\nimplement your own cygwin_true_mode_bits() function at the cygwin\ncompatibility layer, and add\n\n    #define true_mode_bits cygwin_true_mode_bits\n\nin the compat/cygwin.h file?  The change has the documentation value\nto clarify what each lstat() is used for, too.\n\nIdeally, the \"#ifndef true_mode_bits\" part may want to become a\ngeneric helper function if there are lstat() calls in other files\nthat cygwin wants to use the real lstat() not the fast-but-lying\none, but one step at a time.\n\nHrm?\n\n path.c | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex d3d3f8b..d0b31e5 100644\n--- a/path.c\n+++ b/path.c\n@@ -14,6 +14,21 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n \n+#ifndef true_mode_bits\n+/*\n+ * The replacement lstat(2) we use on Cygwin is incomplete and\n+ * lies about permission bits; most of the time we do not care,\n+ * but the callsites of this wrpper do care.\n+ */\n+static int true_mode_bits(const char *path, int *mode)\n+{\n+\tstruct stat st;\n+\tif (lstat(path, &st) < 0)\n+\t\treturn -1;\n+\treturn st.st_mode;\n+}\n+#endif\n+\n static char bad_path[] = \"/bad-path/\";\n \n static char *get_pathname(void)\n@@ -400,9 +415,8 @@ int set_shared_perm(const char *path, int mode)\n \t\treturn 0;\n \t}\n \tif (!mode) {\n-\t\tif (lstat(path, &st) < 0)\n+\t\tif (true_mode_bits(path, &mode) < 0)\n \t\t\treturn -1;\n-\t\tmode = st.st_mode;\n \t\torig_mode = mode;\n \t} else\n \t\torig_mode = 0;\n"},{"id":"211710","messageId":"7vvc8ngkph.fsf@alter.siamese.dyndns.org","threadId":"33231","inReplyTo":"7v620nhzle.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] test-lib.sh: No POSIXPERM for cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-19T21:10:02Z","receivedAt":"2013-03-19T21:10:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Torsten Bögershausen <tboegi@web.de> writes:\n>\n>> Use a compile switch IGNORECYGWINFSTRICKS to disable the usage\n>> of cygwin_lstat_fn() only in path.c\n>\n> The analysis of the problem and the basic idea to disable the\n> fast-but-lying fstricks in the code that matters may be good, but\n> the implementation is questionable.\n>\n> What if we later need to move functions around, etc., so that some\n> other calls in path.c still do want to use the fstricks bit while\n> the existing ones in the file want the real lstat() information?\n>\n> Actually, that already is the case.  The call to lstat() in\n> validate_headref() only cares about the S_ISXXX() type and can\n> afford to use the fast-and-lying one, no?\n>\n> How about doing something like this in the generic codepath, and\n> implement your own cygwin_true_mode_bits() function at the cygwin\n> compatibility layer, and add\n>\n>     #define true_mode_bits cygwin_true_mode_bits\n>\n> in the compat/cygwin.h file?  The change has the documentation value\n> to clarify what each lstat() is used for, too.\n>\n> Ideally, the \"#ifndef true_mode_bits\" part may want to become a\n> generic helper function if there are lstat() calls in other files\n> that cygwin wants to use the real lstat() not the fast-but-lying\n> one, but one step at a time.\n>\n> Hrm?\n>\n>  path.c | 18 ++++++++++++++++--\n>  1 file changed, 16 insertions(+), 2 deletions(-)\n>\n> diff --git a/path.c b/path.c\n> index d3d3f8b..d0b31e5 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -14,6 +14,21 @@\n>  #include \"strbuf.h\"\n>  #include \"string-list.h\"\n>  \n> +#ifndef true_mode_bits\n> +/*\n> + * The replacement lstat(2) we use on Cygwin is incomplete and\n> + * lies about permission bits; most of the time we do not care,\n> + * but the callsites of this wrpper do care.\n> + */\n> +static int true_mode_bits(const char *path, int *mode)\n> +{\n> +\tstruct stat st;\n> +\tif (lstat(path, &st) < 0)\n> +\t\treturn -1;\n> +\treturn st.st_mode;\n\nOf course this should be more like\n\n\t*mode = st.st_mode;\n        return 0;\n\nbut I think you got the idea ;-)\n\n> +}\n> +#endif\n> +\n>  static char bad_path[] = \"/bad-path/\";\n>  \n>  static char *get_pathname(void)\n> @@ -400,9 +415,8 @@ int set_shared_perm(const char *path, int mode)\n>  \t\treturn 0;\n>  \t}\n>  \tif (!mode) {\n> -\t\tif (lstat(path, &st) < 0)\n> +\t\tif (true_mode_bits(path, &mode) < 0)\n>  \t\t\treturn -1;\n> -\t\tmode = st.st_mode;\n>  \t\torig_mode = mode;\n>  \t} else\n>  \t\torig_mode = 0;\n"}]}