git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [RFC] test-lib.sh: No POSIXPERM for cygwin
@ 2013-03-19 19:49 Torsten Bögershausen
  2013-03-19 21:03 ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Torsten Bögershausen @ 2013-03-19 19:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramsay Jones, git, j6t, tboegi

On Friday 08 February 2013 07:08:14 Torsten Bögershausen wrote:
> On 07.02.13 20:35, Junio C Hamano wrote:
> > Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> >
> >> Torsten Bögershausen wrote:
> >>> t0070 and t1301 fail when running the test suite under cygwin.
> >>> Skip the failing tests by unsetting POSIXPERM.
> >> t1301 does not fail for me. (WIN XP (SP3) on NTFS)
> > Others run Cygwin with vfat or some other filesystem, and some of
> > them do not cope will with POSIXPERM, perhaps?
> >
> > Not having POSIXPERM by default for Cygwin may be a saner default
> > than having one, if we have to pick one.
> >
> > It may be debatable to have this default as platform attribute,
> > though.
> >
> Yes, 1301 passes on cygwin 1.5, but not on 1.7.
> And it breaks on VFAT, for all kind of OS.
> 
> Thanks for comments, I'll put more investigations on my todo stack.
> /Torsten

It turns out that cygwin 1.7 handles the read only attribute different then
cygwin 1.5.
Under cygwin 1.5 the "read only" attribute is synchronized with the
"user write" bit from the permissions.

tb@PC /cygdrive/c/temp/pt
$ echo x >x
$ chmod 444 x
$ attrib x
A    R     C:\temp\pt\x
Cygwin 1.7 does not do the synchronization, which means that some files
seem to be writable when do_stat() is used, but the permissions are r--r--r--

We can improve Git to work for cygwin 1.7, a suggestion for a patch
may look like this:

-- >8 --
Subject: [PATCH] Make core.sharedRepository work under cygwin 1.7

When core.sharedRepository is used, set_shared_perm() in path.c
needs lstat() to return the correct POSIX permissions.

The default for cygwin is core.ignoreCygwinFSTricks = false, which
means that a simplified and fast implementation of lstat() is used.

Especially the file permission bits are wrong in cygwin_lstat_fn():
The read-only attribute of a file is used to calculate
the permissions, resulting in either rw-r--r-- or r--r--r--

Use a compile switch IGNORECYGWINFSTRICKS to disable the usage
of cygwin_lstat_fn() only in path.c

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 compat/cygwin.h | 2 ++
 path.c          | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/compat/cygwin.h b/compat/cygwin.h
index a3229f5..984efbe 100644
--- a/compat/cygwin.h
+++ b/compat/cygwin.h
@@ -5,5 +5,7 @@ typedef int (*stat_fn_t)(const char*, struct stat*);
 extern stat_fn_t cygwin_stat_fn;
 extern stat_fn_t cygwin_lstat_fn;
 
+#ifndef IGNORECYGWINFSTRICKS
 #define stat(path, buf) (*cygwin_stat_fn)(path, buf)
 #define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
+#endif
diff --git a/path.c b/path.c
index d3d3f8b..0acfabf 100644
--- a/path.c
+++ b/path.c
@@ -10,6 +10,8 @@
  *
  * which is what it's designed for.
  */
+#define IGNORECYGWINFSTRICKS
+
 #include "cache.h"
 #include "strbuf.h"
 #include "string-list.h"
-- 
1.8.2.341.g543621f

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [RFC] test-lib.sh: No POSIXPERM for cygwin
@ 2013-03-23 12:40 Torsten Bögershausen
  2013-03-24  2:49 ` Eric Sunshine
  0 siblings, 1 reply; 12+ messages in thread
From: Torsten Bögershausen @ 2013-03-23 12:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: j6t, git, ramsay, tboegi

On 19.03.13 22:03, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> Use a compile switch IGNORECYGWINFSTRICKS to disable the usage
>> of cygwin_lstat_fn() only in path.c
> 
> The analysis of the problem and the basic idea to disable the
> fast-but-lying fstricks in the code that matters may be good, but
> the implementation is questionable.
> 
> What if we later need to move functions around, etc., so that some
> other calls in path.c still do want to use the fstricks bit while
> the existing ones in the file want the real lstat() information?
> 
> Actually, that already is the case.  The call to lstat() in
> validate_headref() only cares about the S_ISXXX() type and can
> afford to use the fast-and-lying one, no?
> 
> How about doing something like this in the generic codepath, and
> implement your own cygwin_true_mode_bits() function at the cygwin
> compatibility layer, and add
> 
>     #define true_mode_bits cygwin_true_mode_bits
> 
> in the compat/cygwin.h file?  The change has the documentation value
> to clarify what each lstat() is used for, too.
> 
> Ideally, the "#ifndef true_mode_bits" part may want to become a
> generic helper function if there are lstat() calls in other files
> that cygwin wants to use the real lstat() not the fast-but-lying
> one, but one step at a time.
> 
> Hrm?
> 
>  path.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/path.c b/path.c
> index d3d3f8b..d0b31e5 100644
> --- a/path.c
> +++ b/path.c
> @@ -14,6 +14,21 @@
>  #include "strbuf.h"
>  #include "string-list.h"
>  
> +#ifndef true_mode_bits
> +/*
> + * The replacement lstat(2) we use on Cygwin is incomplete and
> + * lies about permission bits; most of the time we do not care,
> + * but the callsites of this wrpper do care.
> + */
> +static int true_mode_bits(const char *path, int *mode)
> +{
> +	struct stat st;
> +	if (lstat(path, &st) < 0)
> +		return -1;
> +	return st.st_mode;
> +}
> +#endif
> +
>  static char bad_path[] = "/bad-path/";
>  
>  static char *get_pathname(void)
> @@ -400,9 +415,8 @@ int set_shared_perm(const char *path, int mode)
>  		return 0;
>  	}
>  	if (!mode) {
> -		if (lstat(path, &st) < 0)
> +		if (true_mode_bits(path, &mode) < 0)
>  			return -1;
> -		mode = st.st_mode;
>  		orig_mode = mode;
>  	} else
>  		orig_mode = 0;
> 
Thanks for review, I Changed the function name into get_st_mode_bits(),
but that is only a suggestion.

-- >8 --
Subject: [PATCH] Make core.sharedRepository work under cygwin 1.7

When core.sharedRepository is used, set_shared_perm() in path.c
needs lstat() to return the correct POSIX permissions.

The default for cygwin is core.ignoreCygwinFSTricks = false, which
means that the fast implementation in do_stat() is used instead of lstat().

lstat() under cygwin uses the Windows security model to implement
POSIX-like permissions.
The user, group or everyone bits can be set individually.

do_stat() simplifes the file permission bits, and may return a wrong value:
The read-only attribute of a file is used to calculate
the permissions, resulting in either rw-r--r-- or r--r--r--

One effect of the simplified do_stat() is that t1301 failes.

Add a function cygwin_get_st_mode_bits() which returns the POSIX permissions.
When not compiling for cygwin, true_mode_bits() in path.c is used.

Side note:
t1301 passes under cygwin 1.5.
The "user write" bit is synchronized with the "read only" attribute of a file:

$ chmod 444 x
$ attrib x
A    R     C:\temp\pt\x

cygwin 1.7 would show
A          C:\temp\pt\x

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 compat/cygwin.c   | 13 +++++++++++++
 compat/cygwin.h   |  5 +++++
 git-compat-util.h |  1 +
 path.c            | 20 +++++++++++++++++---
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/compat/cygwin.c b/compat/cygwin.c
index 5428858..871b41d 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -1,3 +1,4 @@
+#define CYGWIN_C
 #define WIN32_LEAN_AND_MEAN
 #ifdef CYGWIN_V15_WIN32API
 #include "../git-compat-util.h"
@@ -10,6 +11,18 @@
 #endif
 #include "../cache.h" /* to read configuration */
 
+/*
+ * Return POSIX permission bits, regardless of core.ignorecygwinfstricks
+ */
+int cygwin_get_st_mode_bits(const char *path, int *mode)
+{
+	struct stat st;
+	if (lstat(path, &st) < 0)
+		return -1;
+	*mode = st.st_mode;
+	return 0;
+}
+
 static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
 {
 	long long winTime = ((long long)ft->dwHighDateTime << 32) +
diff --git a/compat/cygwin.h b/compat/cygwin.h
index a3229f5..c04965a 100644
--- a/compat/cygwin.h
+++ b/compat/cygwin.h
@@ -4,6 +4,11 @@
 typedef int (*stat_fn_t)(const char*, struct stat*);
 extern stat_fn_t cygwin_stat_fn;
 extern stat_fn_t cygwin_lstat_fn;
+int cygwin_get_st_mode_bits(const char *path, int *mode);
 
+#define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m))
+#ifndef CYGWIN_C
+/* cygwin.c needs the original lstat() */
 #define stat(path, buf) (*cygwin_stat_fn)(path, buf)
 #define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
+#endif
diff --git a/git-compat-util.h b/git-compat-util.h
index b7eaaa9..04a67ad 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -160,6 +160,7 @@
 typedef long intptr_t;
 typedef unsigned long uintptr_t;
 #endif
+int get_st_mode_bits(const char *path, int *mode);
 #if defined(__CYGWIN__)
 #undef _XOPEN_SOURCE
 #include <grp.h>
diff --git a/path.c b/path.c
index d3d3f8b..2fdccc2 100644
--- a/path.c
+++ b/path.c
@@ -14,6 +14,22 @@
 #include "strbuf.h"
 #include "string-list.h"
 
+#ifndef get_st_mode_bits
+/*
+ * The replacement lstat(2) we use on Cygwin is incomplete and
+ * may return wrong permission bits. Most of the time we do not care,
+ * but the callsites of this wrapper do care.
+ */
+int get_st_mode_bits(const char *path, int *mode)
+{
+	struct stat st;
+	if (lstat(path, &st) < 0)
+		return -1;
+	*mode = st.st_mode;
+	return 0;
+}
+#endif
+
 static char bad_path[] = "/bad-path/";
 
 static char *get_pathname(void)
@@ -391,7 +407,6 @@ const char *enter_repo(const char *path, int strict)
 
 int set_shared_perm(const char *path, int mode)
 {
-	struct stat st;
 	int tweak, shared, orig_mode;
 
 	if (!shared_repository) {
@@ -400,9 +415,8 @@ int set_shared_perm(const char *path, int mode)
 		return 0;
 	}
 	if (!mode) {
-		if (lstat(path, &st) < 0)
+		if (get_st_mode_bits(path, &mode) < 0)
 			return -1;
-		mode = st.st_mode;
 		orig_mode = mode;
 	} else
 		orig_mode = 0;
-- 
1.8.2.341.g543621f

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [RFC] test-lib.sh: No POSIXPERM for cygwin
@ 2013-01-27 14:57 Torsten Bögershausen
  2013-02-06  9:34 ` Erik Faye-Lund
  2013-02-07 18:25 ` Ramsay Jones
  0 siblings, 2 replies; 12+ messages in thread
From: Torsten Bögershausen @ 2013-01-27 14:57 UTC (permalink / raw)
  To: ramsay, git; +Cc: j6t, tboegi

t0070 and t1301 fail when running the test suite under cygwin.
Skip the failing tests by unsetting POSIXPERM.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 t/test-lib.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1a6c4ab..94b097e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -669,7 +669,6 @@ case $(uname -s) in
 	test_set_prereq SED_STRIPS_CR
 	;;
 *CYGWIN*)
-	test_set_prereq POSIXPERM
 	test_set_prereq EXECKEEPSPID
 	test_set_prereq NOT_MINGW
 	test_set_prereq SED_STRIPS_CR
-- 
1.8.1.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-03-25 15:53 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-19 19:49 [RFC] test-lib.sh: No POSIXPERM for cygwin Torsten Bögershausen
2013-03-19 21:03 ` Junio C Hamano
2013-03-19 21:10   ` Junio C Hamano
  -- strict thread matches above, loose matches on Subject: below --
2013-03-23 12:40 Torsten Bögershausen
2013-03-24  2:49 ` Eric Sunshine
2013-03-25 15:53   ` Torsten Bögershausen
2013-01-27 14:57 Torsten Bögershausen
2013-02-06  9:34 ` Erik Faye-Lund
2013-02-06 20:16   ` Torsten Bögershausen
2013-02-07 18:25 ` Ramsay Jones
2013-02-07 19:35   ` Junio C Hamano
2013-02-08  6:08     ` Torsten Bögershausen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).