{"thread":{"id":"33270","subject":"Re: [RFC] test-lib.sh: No POSIXPERM for cygwin","startedAt":"2013-03-23T12:40:29Z","lastAt":"2013-03-25T15:53:04Z","messageCount":3,"participants":["Torsten Bögershausen","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"212047","messageId":"201303231340.29687.tboegi@web.de","threadId":"33270","inReplyTo":null,"subject":"Re: [RFC] test-lib.sh: No POSIXPERM for cygwin","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-03-23T12:40:29Z","receivedAt":"2013-03-23T12:40:29Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 19.03.13 22:03, Junio C Hamano wrote:\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> +}\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> \nThanks for review, I Changed the function name into get_st_mode_bits(),\nbut that is only a suggestion.\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 the fast implementation in do_stat() is used instead of lstat().\n\nlstat() under cygwin uses the Windows security model to implement\nPOSIX-like permissions.\nThe user, group or everyone bits can be set individually.\n\ndo_stat() simplifes the file permission bits, and may return a wrong value:\nThe read-only attribute of a file is used to calculate\nthe permissions, resulting in either rw-r--r-- or r--r--r--\n\nOne effect of the simplified do_stat() is that t1301 failes.\n\nAdd a function cygwin_get_st_mode_bits() which returns the POSIX permissions.\nWhen not compiling for cygwin, true_mode_bits() in path.c is used.\n\nSide note:\nt1301 passes under cygwin 1.5.\nThe \"user write\" bit is synchronized with the \"read only\" attribute of a file:\n\n$ chmod 444 x\n$ attrib x\nA    R     C:\\temp\\pt\\x\n\ncygwin 1.7 would show\nA          C:\\temp\\pt\\x\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n compat/cygwin.c   | 13 +++++++++++++\n compat/cygwin.h   |  5 +++++\n git-compat-util.h |  1 +\n path.c            | 20 +++++++++++++++++---\n 4 files changed, 36 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/cygwin.c b/compat/cygwin.c\nindex 5428858..871b41d 100644\n--- a/compat/cygwin.c\n+++ b/compat/cygwin.c\n@@ -1,3 +1,4 @@\n+#define CYGWIN_C\n #define WIN32_LEAN_AND_MEAN\n #ifdef CYGWIN_V15_WIN32API\n #include \"../git-compat-util.h\"\n@@ -10,6 +11,18 @@\n #endif\n #include \"../cache.h\" /* to read configuration */\n \n+/*\n+ * Return POSIX permission bits, regardless of core.ignorecygwinfstricks\n+ */\n+int cygwin_get_st_mode_bits(const char *path, int *mode)\n+{\n+\tstruct stat st;\n+\tif (lstat(path, &st) < 0)\n+\t\treturn -1;\n+\t*mode = st.st_mode;\n+\treturn 0;\n+}\n+\n static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)\n {\n \tlong long winTime = ((long long)ft->dwHighDateTime << 32) +\ndiff --git a/compat/cygwin.h b/compat/cygwin.h\nindex a3229f5..c04965a 100644\n--- a/compat/cygwin.h\n+++ b/compat/cygwin.h\n@@ -4,6 +4,11 @@\n 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+int cygwin_get_st_mode_bits(const char *path, int *mode);\n \n+#define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m))\n+#ifndef CYGWIN_C\n+/* cygwin.c needs the original lstat() */\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/git-compat-util.h b/git-compat-util.h\nindex b7eaaa9..04a67ad 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -160,6 +160,7 @@\n typedef long intptr_t;\n typedef unsigned long uintptr_t;\n #endif\n+int get_st_mode_bits(const char *path, int *mode);\n #if defined(__CYGWIN__)\n #undef _XOPEN_SOURCE\n #include <grp.h>\ndiff --git a/path.c b/path.c\nindex d3d3f8b..2fdccc2 100644\n--- a/path.c\n+++ b/path.c\n@@ -14,6 +14,22 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n \n+#ifndef get_st_mode_bits\n+/*\n+ * The replacement lstat(2) we use on Cygwin is incomplete and\n+ * may return wrong permission bits. Most of the time we do not care,\n+ * but the callsites of this wrapper do care.\n+ */\n+int get_st_mode_bits(const char *path, int *mode)\n+{\n+\tstruct stat st;\n+\tif (lstat(path, &st) < 0)\n+\t\treturn -1;\n+\t*mode = st.st_mode;\n+\treturn 0;\n+}\n+#endif\n+\n static char bad_path[] = \"/bad-path/\";\n \n static char *get_pathname(void)\n@@ -391,7 +407,6 @@ const char *enter_repo(const char *path, int strict)\n \n int set_shared_perm(const char *path, int mode)\n {\n-\tstruct stat st;\n \tint tweak, shared, orig_mode;\n \n \tif (!shared_repository) {\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 (get_st_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-- \n1.8.2.341.g543621f\n"},{"id":"212067","messageId":"CAPig+cR0HdQq4P=MWg7xkFLMzNimitty_Bj7mVx+w3eoi9bZ3g@mail.gmail.com","threadId":"33270","inReplyTo":"201303231340.29687.tboegi@web.de","subject":"Re: [RFC] test-lib.sh: No POSIXPERM for cygwin","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-03-24T02:49:17Z","receivedAt":"2013-03-24T02:49:17Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 23, 2013 at 8:40 AM, Torsten Bögershausen <tboegi@web.de> wrote:\n> Subject: [PATCH] Make core.sharedRepository work under cygwin 1.7\n>\n> When core.sharedRepository is used, set_shared_perm() in path.c\n> needs lstat() to return the correct POSIX permissions.\n>\n> The default for cygwin is core.ignoreCygwinFSTricks = false, which\n> means that the fast implementation in do_stat() is used instead of lstat().\n>\n> lstat() under cygwin uses the Windows security model to implement\n> POSIX-like permissions.\n> The user, group or everyone bits can be set individually.\n>\n> do_stat() simplifes the file permission bits, and may return a wrong value:\n> The read-only attribute of a file is used to calculate\n> the permissions, resulting in either rw-r--r-- or r--r--r--\n>\n> One effect of the simplified do_stat() is that t1301 failes.\n\ns/failes/fails/\n\n> Add a function cygwin_get_st_mode_bits() which returns the POSIX permissions.\n> When not compiling for cygwin, true_mode_bits() in path.c is used.\n"},{"id":"212169","messageId":"515072E0.4030103@web.de","threadId":"33270","inReplyTo":"CAPig+cR0HdQq4P=MWg7xkFLMzNimitty_Bj7mVx+w3eoi9bZ3g@mail.gmail.com","subject":"Re: [RFC] test-lib.sh: No POSIXPERM for cygwin","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-03-25T15:53:04Z","receivedAt":"2013-03-25T15:53:04Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 24.03.13 03:49, Eric Sunshine wrote:\n> On Sat, Mar 23, 2013 at 8:40 AM, Torsten Bögershausen <tboegi@web.de> wrote:\n>> Subject: [PATCH] Make core.sharedRepository work under cygwin 1.7\n[..]\n> \n> s/failes/fails/\n> \nThanks for review,\nI will send a new patch in a minute.\n\nIt is actually 2 patches,\n- The fix as discussed here\n- A small refactoring of set_shared_perm() in path.c,\n  which I found while digging in the code.\n"}]}