RFC patchcygwin: Allow pushing to UNC paths

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

tboegi@web.de writes:

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.

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(-)

tboegi@web.de writes:

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.

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

tboegi@web.de writes:

OK.  

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

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.

Thanks.

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(+)