Re: [PATCH 2/2] git-sh-setup: work around Cygwin path handling gotchas

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: May 17, 2012, 23:15 UTC
Message-ID: <4FB58678.1050009@ramsay1.demon.co.uk>
In-Reply-To: <7vd364c5kt.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 64 quoted lines

> Steven Penny <svnpenn@gmail.com> writes:
> 
>> Junio C Hamano wrote:
>>> +*CYGWIN*)
>>> +       pwd () {
>>> +               builtin cygpath -m
>>> +       }
>>> +       ;;
>> Ok I got it!
>>
>> The problem is twofold
>>
>> 1. Ramsay Jones	was right, it needs to be called like
>>
>> 	cygpath -m "$PWD"
>>
>> 2. The Cygwin "pwd" (and quite possibly MinGW "pwd") needs to be defined
>>    **before** it is called
> 
> OK, I missed the first point, it seems.  But you seem to have missed that
> these two problems are more or less independent---that is why I sent two
> patches, not a single ball of wax like the one I am responding to.
> 
> So the replacement for [PATCH 2/2] would now look like this?
> 
> In addition to "applies fine, tested and works" reports from Windows
> stakeholders, I still prefer to have a sign off from you (see
> Documentation/SubmittingPatches).
> 
> Thanks.
> 
> -- >8 --
> From: Steven Penny <svnpenn@gmail.com>
> Date: Wed, 16 May 2012 10:44:49 -0700
> Subject: [PATCH] git-sh-setup: work around Cygwin path handling gotchas
> 
> On Cygwin, tools built for Cygwin can take both Windows-style paths
> (e.g. C:/dir/file.txt or C:\dir\file.txt) and Cygwin-style paths
> (e.g. /cygdrive/c/dir/file.txt), but Windows-native tools can only take
> Windows-style paths.  Because the paths that are relative to $GIT_DIR,
> e.g. the name of the insn sheet file of the "rebase -i" command, are given
> to the programs with $GIT_DIR prefixed, and $GIT_DIR in turn is computed
> by calling "pwd", wrap "pwd" to call "cygpath -m" to give a Windows-style
> path, in a way similar to how mingw does this.
> ---
>  git-sh-setup.sh | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 770a86e..b8e6327 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -241,6 +241,11 @@ case $(uname -s) in
>  		return 1
>  	}
>  	;;
> +*CYGWIN*)
> +	pwd () {
> +		cygpath -m "$PWD"
> +	}
> +	;;
>  *)
>  	is_absolute_path () {
>  		case "$1" in

I guess you won't be shocked to hear that I don't think this patch is necessary. :-P

However, I can appreciate that some people would rather not have to create a shell script to wrap their text editor, just to use git. So I'm certainly not opposed to finding a solution to this problem that doesn't require the user to do so.

My concerns about this patch include:

    - the additional fork+exec overhead associated with calling cygpath.
      I'm not actually claiming there is any substantial increase; I
      haven't tried it, so I don't know how "hot" the pwd() function is.

    - this is a "big hammer" which will affect much more code that is
      required to fix this problem.

The latter is my main concern. I would rather have the call to cygpath at the point in the code where the editor is launched. This would reduce the scope of the change and any side-effects that go with it.

Anyway, I applied this patch tonight to give it a go. The very first test I tried failed. I've attached the log of the failing test below. Note that it is attempting to use "ssh" to a "host" that ends in ".../C:".

I haven't investigated yet (I've got to get some sleep).

HTH

ATB, Ramsay Jones

expecting success:
        (
                cd addtest &&
                git submodule add ../repo relative &&
                test "$(git config -f .gitmodules submodule.relative.url)" = ../
repo &&
                git submodule sync relative &&
                test "$(git config submodule.relative.url)" = "$submodurl/repo"
        )

Cloning into 'relative'...
ssh: /home/ramsay/git/t/trash directory.t7400-submodule-basic/addtest/C: no addr
ess associated with name
fatal: The remote end hung up unexpectedly
Clone of 'C:/cygwin/home/ramsay/git/t/trash directory.t7400-submodule-basic/repo
' into submodule path 'relative' failed
not ok - 41 use superproject as upstream when path is relative and no url is set
 there
#
#               (
#                       cd addtest &&
#                       git submodule add ../repo relative &&
#                       test "$(git config -f .gitmodules submodule.relative.url
)" = ../repo &&
#                       git submodule sync relative &&
#                       test "$(git config submodule.relative.url)" = "$submodur
l/repo"
#               )
#
$