[PATCH/RFC v1 1/1] cygwin: Allow pushing to UNC paths

[PATCH/RFC v1 1/1] cygwin: Allow pushing to UNC paths

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

cygwin can use an UNC path like //server/share/repo
$ cd //server/share/dir
$ mkdir test
$ cd test
$ git init --bare

However, when we try to push from a local Git repository to this repo,
there are 2 problems:
- Git converts the leading "//" into a single "/".
- The remote repo is not accepted because setup.c calls
  access(getenv(DB_ENVIRONMENT), X_OK)
  and this call fails. In other words, checking the executable bit
  of a directory mounted on a SAMBA share is not reliable (and not needed).

As cygwin handles an UNC path so well, Git can support them better.
- Introduce cygwin_offset_1st_component() which keeps the leading "//",
  similar to what Git for Windows does.
- Move CYGWIN out of the POSIX in the tests for path normalization in t0060.
- Use cygwin_access() with a relaxed test for the executable bit on
  a directory pointed out by an UNC path.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 compat/cygwin.c       | 29 +++++++++++++++++++++++++++++
 compat/cygwin.h       |  7 +++++++
 config.mak.uname      |  1 +
 git-compat-util.h     |  3 +++
 t/t0060-path-utils.sh |  2 ++
 5 files changed, 42 insertions(+)
 create mode 100644 compat/cygwin.c
 create mode 100644 compat/cygwin.h

diff --git a/compat/cygwin.c b/compat/cygwin.c
new file mode 100644
index 0000000..d98e877
--- /dev/null
+++ b/compat/cygwin.c
@@ -0,0 +1,29 @@
+#include "../git-compat-util.h"
+#include "../cache.h"
+
+int cygwin_offset_1st_component(const char *path)
+{
+	const char *pos = path;
+	/* unc paths */
+	if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
+		/* skip server name */
+		pos = strchr(pos + 2, '/');
+		if (!pos)
+			return 0; /* Error: malformed unc path */
+
+		do {
+			pos++;
+		} while (*pos && pos[0] != '/');
+	}
+	return pos + is_dir_sep(*pos) - path;
+}
+
+#undef access
+int cygwin_access(const char *filename, int mode)
+{
+	/* the execute bit does not work on SAMBA drives */
+	if (filename[0] == '/' && filename[1] == '/' )
+		return access(filename, mode & ~X_OK);
+	else
+		return access(filename, mode);
+}
diff --git a/compat/cygwin.h b/compat/cygwin.h
new file mode 100644
index 0000000..efa12ad
--- /dev/null
+++ b/compat/cygwin.h
@@ -0,0 +1,7 @@
+int cygwin_access(const char *filename, int mode);
+#undef access
+#define access cygwin_access
+
+
+int cygwin_offset_1st_component(const char *path);
+#define offset_1st_component cygwin_offset_1st_component
diff --git a/config.mak.uname b/config.mak.uname
index adfb90b..551e465 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -184,6 +184,7 @@ ifeq ($(uname_O),Cygwin)
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
+	COMPAT_OBJS += compat/cygwin.o
 endif
 ifeq ($(uname_S),FreeBSD)
 	NEEDS_LIBICONV = YesPlease
diff --git a/git-compat-util.h b/git-compat-util.h
index 047172d..db9c22d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -189,6 +189,9 @@
 #include <sys/sysctl.h>
 #endif
 
+#if defined(__CYGWIN__)
+#include "compat/cygwin.h"
+#endif
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
 #include "compat/mingw.h"
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 444b5a4..7ea2bb5 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -70,6 +70,8 @@ ancestor() {
 case $(uname -s) in
 *MINGW*)
 	;;
+*CYGWIN*)
+	;;
 *)
 	test_set_prereq POSIX
 	;;
-- 
2.10.0

Re: [PATCH/RFC v1 1/1] cygwin: Allow pushing to UNC paths

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> cygwin can use an UNC path like //server/share/repo
> $ cd //server/share/dir
> $ mkdir test
> $ cd test
> $ git init --bare
>
> However, when we try to push from a local Git repository to this repo,
> there are 2 problems:
> - Git converts the leading "//" into a single "/".
> - The remote repo is not accepted because setup.c calls
>   access(getenv(DB_ENVIRONMENT), X_OK)
>   and this call fails. In other words, checking the executable bit
>   of a directory mounted on a SAMBA share is not reliable (and not needed).
>
> As cygwin handles an UNC path so well, Git can support them better.
> - Introduce cygwin_offset_1st_component() which keeps the leading "//",
>   similar to what Git for Windows does.
> - Move CYGWIN out of the POSIX in the tests for path normalization in t0060.
> - Use cygwin_access() with a relaxed test for the executable bit on
>   a directory pointed out by an UNC path.

Thanks.

The offset-1st-component thing looks like a right thing to do.

I think the reason why you marked this as RFC is because you found
the "access" bit a bit iffy?  If so, I share the feeling.  If it
were called only from the codepath in setup.c::is_git_directory(),
it may be OK, but I suspect that there are other places that do care
about access() for other reasons in the codebase, and I am not sure
if it is safe to change the behaviour of access() like this.

Stepping back a bit.

The implementation of is_git_directory() wants to ensure that the
top level of the object database (i.e.  $GIT_OBJECT_DIRECTORY or
$GIT_DIR/objects) and the reference store (i.e. $GIT_DIR/refs) can
be "executed".  But what it really wants to see is that it is a
directory we can search.  If we had a regular file that is executable,
it would happily say "Yes!", even though that is clearly bogus and
not a Git repository.

So perhaps we would want a bit higher-level abstraction API
implemented as:

	int is_searchable_directory(const char *path)
	{
		struct stat st;

	        return (!stat(path, &st) && S_ISDIR(st.st_mode));
	}

on Cygwin (as SMB share may not give you correct access(2)), and

	int is_searchable_directory(const char *path)
	{
		struct stat st;

	        return (!stat(path, &st) && 
			S_ISDIR(st.st_mode) &&
			!access(path, X_OK));
	}

elsewhere, or something like that, and use that in the
implementation of is_git_directory()?

I dunno.  I see compat/mingw.c discards X_OK the same way you did,
so perhaps your version is a right solution at least in the shorter
term anyway.  

Regardless, I think that we would want to make sure that the thing
is a directory where is_git_directory() uses access(2).  But that
could be an orthogonal issue.

> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  compat/cygwin.c       | 29 +++++++++++++++++++++++++++++
>  compat/cygwin.h       |  7 +++++++
>  config.mak.uname      |  1 +
>  git-compat-util.h     |  3 +++
>  t/t0060-path-utils.sh |  2 ++
>  5 files changed, 42 insertions(+)
>  create mode 100644 compat/cygwin.c
>  create mode 100644 compat/cygwin.h
>
> diff --git a/compat/cygwin.c b/compat/cygwin.c
> new file mode 100644
> index 0000000..d98e877
> --- /dev/null
> +++ b/compat/cygwin.c
> @@ -0,0 +1,29 @@
> +#include "../git-compat-util.h"
> +#include "../cache.h"
> +
> +int cygwin_offset_1st_component(const char *path)
> +{
> +	const char *pos = path;
> +	/* unc paths */
> +	if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
> +		/* skip server name */
> +		pos = strchr(pos + 2, '/');
> +		if (!pos)
> +			return 0; /* Error: malformed unc path */
> +
> +		do {
> +			pos++;
> +		} while (*pos && pos[0] != '/');
> +	}
> +	return pos + is_dir_sep(*pos) - path;
> +}
> +
> +#undef access
> +int cygwin_access(const char *filename, int mode)
> +{
> +	/* the execute bit does not work on SAMBA drives */
> +	if (filename[0] == '/' && filename[1] == '/' )
> +		return access(filename, mode & ~X_OK);
> +	else
> +		return access(filename, mode);
> +}
> diff --git a/compat/cygwin.h b/compat/cygwin.h
> new file mode 100644
> index 0000000..efa12ad
> --- /dev/null
> +++ b/compat/cygwin.h
> @@ -0,0 +1,7 @@
> +int cygwin_access(const char *filename, int mode);
> +#undef access
> +#define access cygwin_access
> +
> +
> +int cygwin_offset_1st_component(const char *path);
> +#define offset_1st_component cygwin_offset_1st_component
> diff --git a/config.mak.uname b/config.mak.uname
> index adfb90b..551e465 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -184,6 +184,7 @@ ifeq ($(uname_O),Cygwin)
>  	UNRELIABLE_FSTAT = UnfortunatelyYes
>  	SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
>  	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
> +	COMPAT_OBJS += compat/cygwin.o
>  endif
>  ifeq ($(uname_S),FreeBSD)
>  	NEEDS_LIBICONV = YesPlease
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 047172d..db9c22d 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -189,6 +189,9 @@
>  #include <sys/sysctl.h>
>  #endif
>  
> +#if defined(__CYGWIN__)
> +#include "compat/cygwin.h"
> +#endif
>  #if defined(__MINGW32__)
>  /* pull in Windows compatibility stuff */
>  #include "compat/mingw.h"
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index 444b5a4..7ea2bb5 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -70,6 +70,8 @@ ancestor() {
>  case $(uname -s) in
>  *MINGW*)
>  	;;
> +*CYGWIN*)
> +	;;
>  *)
>  	test_set_prereq POSIX
>  	;;

[PATCH v2 1/2] Check DB_ENVIRONMENT using is_directory()

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

In setup.c is_git_directory() checks a Git directory using access(X_OK).
This does not check, if path is a file or a directory.
Check path with is_directory() instead.
---
After all the discussions (and lots of tests) I found that this patch
works for my setup.
All in all could the error reporting be improvved for is_git_directory(),
as there may be "access denied", or "not a directory" or others, but
that is for another day.

setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 358fbc2..5a7ee2e 100644
--- a/setup.c
+++ b/setup.c
@@ -321,7 +321,7 @@ int is_git_directory(const char *suspect)
 
 	/* Check non-worktree-related signatures */
 	if (getenv(DB_ENVIRONMENT)) {
-		if (access(getenv(DB_ENVIRONMENT), X_OK))
+		if (!is_directory(getenv(DB_ENVIRONMENT)))
 			goto done;
 	}
 	else {
-- 
2.10.0

[PATCH v2 2/2] cygwin: Allow pushing to UNC paths

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

 cygwin can use an UNC path like //server/share/repo
 $ cd //server/share/dir
 $ mkdir test
 $ cd test
 $ git init --bare

 However, when we try to push from a local Git repository to this repo,
 there is a problem: Git converts the leading "//" into a single "/".

 As cygwin handles an UNC path so well, Git can support them better:
 - Introduce cygwin_offset_1st_component() which keeps the leading "//",
   similar to what Git for Windows does.
 - Move CYGWIN out of the POSIX in the tests for path normalization in t0060.
---
 config.mak.uname      | 1 +
 git-compat-util.h     | 3 +++
 t/t0060-path-utils.sh | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index adfb90b..551e465 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -184,6 +184,7 @@ ifeq ($(uname_O),Cygwin)
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
+	COMPAT_OBJS += compat/cygwin.o
 endif
 ifeq ($(uname_S),FreeBSD)
 	NEEDS_LIBICONV = YesPlease
diff --git a/git-compat-util.h b/git-compat-util.h
index 047172d..db9c22d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -189,6 +189,9 @@
 #include <sys/sysctl.h>
 #endif
 
+#if defined(__CYGWIN__)
+#include "compat/cygwin.h"
+#endif
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
 #include "compat/mingw.h"
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 444b5a4..7ea2bb5 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -70,6 +70,8 @@ ancestor() {
 case $(uname -s) in
 *MINGW*)
 	;;
+*CYGWIN*)
+	;;
 *)
 	test_set_prereq POSIX
 	;;
-- 
2.10.0

Re: [PATCH v2 1/2] Check DB_ENVIRONMENT using is_directory()

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> In setup.c is_git_directory() checks a Git directory using access(X_OK).
> This does not check, if path is a file or a directory.
> Check path with is_directory() instead.
> ---
> After all the discussions (and lots of tests) I found that this patch
> works for my setup.
> All in all could the error reporting be improvved for is_git_directory(),
> as there may be "access denied", or "not a directory" or others, but
> that is for another day.

Wouldn't this be a slight regression, though?  

We used to ignore an unsearchable directory but now we blindly say
"ah, it is a directory".

Checking is_directory() in addition to the existing access() would
be making a progress by fixing one bug (i.e. we no longer are
confused by an executable file there); skipping that access() based
on the filesystem quirks can be left for another day, of course.

> setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/setup.c b/setup.c
> index 358fbc2..5a7ee2e 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -321,7 +321,7 @@ int is_git_directory(const char *suspect)
>  
>  	/* Check non-worktree-related signatures */
>  	if (getenv(DB_ENVIRONMENT)) {
> -		if (access(getenv(DB_ENVIRONMENT), X_OK))
> +		if (!is_directory(getenv(DB_ENVIRONMENT)))
>  			goto done;
>  	}
>  	else {

[PATCH v3 1/1] cygwin: Allow pushing to UNC paths

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

 cygwin can use an UNC path like //server/share/repo
 $ cd //server/share/dir
 $ mkdir test
 $ cd test
 $ git init --bare

 However, when we try to push from a local Git repository to this repo,
 there is a problem: Git converts the leading "//" into a single "/".

 As cygwin handles an UNC path so well, Git can support them better:
 - Introduce cygwin_offset_1st_component() which keeps the leading "//",
   similar to what Git for Windows does.
 - Move CYGWIN out of the POSIX in the tests for path normalization in t0060

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---

I think I skip all the changing in setup.c and cygwin_access() for the
moment:
- It is not clear, what is a regression and what is an improvement
- It may be a problem that could be solved in cygwin itself
- I was able to push a an UNC path on a Windows server
  when the domain controller was reachable.


compat/cygwin.c       | 19 +++++++++++++++++++
 compat/cygwin.h       |  2 ++
 config.mak.uname      |  1 +
 git-compat-util.h     |  3 +++
 t/t0060-path-utils.sh |  2 ++
 5 files changed, 27 insertions(+)
 create mode 100644 compat/cygwin.c
 create mode 100644 compat/cygwin.h

diff --git a/compat/cygwin.c b/compat/cygwin.c
new file mode 100644
index 0000000..b9862d6
--- /dev/null
+++ b/compat/cygwin.c
@@ -0,0 +1,19 @@
+#include "../git-compat-util.h"
+#include "../cache.h"
+
+int cygwin_offset_1st_component(const char *path)
+{
+	const char *pos = path;
+	/* unc paths */
+	if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
+		/* skip server name */
+		pos = strchr(pos + 2, '/');
+		if (!pos)
+			return 0; /* Error: malformed unc path */
+
+		do {
+			pos++;
+		} while (*pos && pos[0] != '/');
+	}
+	return pos + is_dir_sep(*pos) - path;
+}
diff --git a/compat/cygwin.h b/compat/cygwin.h
new file mode 100644
index 0000000..8e52de4
--- /dev/null
+++ b/compat/cygwin.h
@@ -0,0 +1,2 @@
+int cygwin_offset_1st_component(const char *path);
+#define offset_1st_component cygwin_offset_1st_component
diff --git a/config.mak.uname b/config.mak.uname
index adfb90b..551e465 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -184,6 +184,7 @@ ifeq ($(uname_O),Cygwin)
 	UNRELIABLE_FSTAT = UnfortunatelyYes
 	SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
 	OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
+	COMPAT_OBJS += compat/cygwin.o
 endif
 ifeq ($(uname_S),FreeBSD)
 	NEEDS_LIBICONV = YesPlease
diff --git a/git-compat-util.h b/git-compat-util.h
index 047172d..db9c22d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -189,6 +189,9 @@
 #include <sys/sysctl.h>
 #endif
 
+#if defined(__CYGWIN__)
+#include "compat/cygwin.h"
+#endif
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
 #include "compat/mingw.h"
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 444b5a4..7ea2bb5 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -70,6 +70,8 @@ ancestor() {
 case $(uname -s) in
 *MINGW*)
 	;;
+*CYGWIN*)
+	;;
 *)
 	test_set_prereq POSIX
 	;;
-- 
2.10.0

Re: [PATCH v3 1/1] cygwin: Allow pushing to UNC paths

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
>  cygwin can use an UNC path like //server/share/repo
>  $ cd //server/share/dir
>  $ mkdir test
>  $ cd test
>  $ git init --bare
>
>  However, when we try to push from a local Git repository to this repo,
>  there is a problem: Git converts the leading "//" into a single "/".
>
>  As cygwin handles an UNC path so well, Git can support them better:
>  - Introduce cygwin_offset_1st_component() which keeps the leading "//",
>    similar to what Git for Windows does.
>  - Move CYGWIN out of the POSIX in the tests for path normalization in t0060
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>
> I think I skip all the changing in setup.c and cygwin_access() for the
> moment:
> - It is not clear, what is a regression and what is an improvement
> - It may be a problem that could be solved in cygwin itself
> - I was able to push a an UNC path on a Windows server
>   when the domain controller was reachable.

OK.  

It certainly makes it simpler to improve things one at a time ;-)

> diff --git a/compat/cygwin.h b/compat/cygwin.h
> new file mode 100644
> index 0000000..8e52de4
> --- /dev/null
> +++ b/compat/cygwin.h
> @@ -0,0 +1,2 @@
> +int cygwin_offset_1st_component(const char *path);
> +#define offset_1st_component cygwin_offset_1st_component

I originally found this somewhat iffy, but decided to take it as-is.
But we may want to fix it (and the original sin that brought this
file into this shape) later.

The reason of "iffy"-ness I felt were twofold:

 - This header file is only two lines long and is designed to be
   included at a single place (git-compat-util.h).  It might be
   better to inline its contents directly there, if we do not expect
   it to grow aggressively in the future.

 - If it is to be a header file on its own, it should have the
   standard double-inclusion guard

    #ifndef COMPAT_CYGWIN_H
    #define COMPAT_CYGWIN_H
	...
    #endif /* COMPAT_CYGWIN_H */

  around it.

I see this was modeled after existing compat/mingw.h, which is
sufficiently large that it deserves to be its own header, but then
it should have the double-inclusion guard around it.  

I am neutral between inlining the contents of cygwin.h to where it
is included and keeping the organization this patch proposes.  If we
anticipate futher development on Cygwin, perhaps having the header
as a separate file, even if it starts small, makes sense, so I do
not mind it.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 047172d..db9c22d 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -189,6 +189,9 @@
>  #include <sys/sysctl.h>
>  #endif
>  
> +#if defined(__CYGWIN__)
> +#include "compat/cygwin.h"
> +#endif
>  #if defined(__MINGW32__)
>  /* pull in Windows compatibility stuff */
>  #include "compat/mingw.h"


Thanks.