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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 5, 2017, 21:14 UTC
Message-ID: <xmqqinj6h9i2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170703144137.27176-1-tboegi@web.de>

tboegi@web.de writes:

Show 25 quoted lines

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

Show 8 quoted lines

> 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.

Show 14 quoted lines

> 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.