Git commit path vs rebase path

Git commit path vs rebase path

[Steven Penny]

I have noticed

git commit uses this path

.git/COMMIT_EDITMSG

git rebase uses this path

/home/Steven/jquery/.git/rebase-merge/git-rebase-todo

So git commit is using a relative path while git rebase is using absolute path.

This causes problem in Windows if your editor does not understand linux paths,
e.g. notepad, Notepad2, Notepad++, etc.

Re: Git commit path vs rebase path

[Junio C Hamano]

Steven Penny <svnpenn@gmail.com> writes:

> I have noticed
>
> git commit uses this path
>
> .git/COMMIT_EDITMSG
>
> git rebase uses this path
>
> /home/Steven/jquery/.git/rebase-merge/git-rebase-todo
>
> So git commit is using a relative path while git rebase is using absolute path.
>
> This causes problem in Windows if your editor does not understand linux paths,
> e.g. notepad, Notepad2, Notepad++, etc.

Hrm, this is not limited to rebase, though.  All shell scripted Porcelain
command use git-sh-setup that gives GIT_DIR as the full path, primarily so
that the implementation of the Porcelain can safely chdir around without
having to worry about relative paths in GIT_DIR.

Most of the time, the commands that use git-sh-setup do cd_to_toplevel
very early.  For these commands, unless you are using GIT_DIR from your
own environment (i.e. where people set the environment point to a place
totally unrelated to the working tree and/or the current directory), it
might look nicer if GIT_DIR given were .git/rebase-merge/git-rebase-todo,
but that will not be the real solution, as sometimes your editor _must_
deal with the full path anyway.  So the issue is _not_ that the path is
absolute, it is that the path is given as a wrong kind of absolute path.

Which suggests that "$(cd "$GIT_DIR" && pwd)" must give a full path that
is suitable for the platform, and your platform wants it to be something
like "c:\home\steven\jquery\..."?  I do not have any Windows environment
to further my speculation, so I'll leave the rest to Windows experts who
may be lurking on this list.

The relevant code snippet in git-sh-setup.sh is this part.

# Make sure we are in a valid repository of a vintage we understand,
# if we require to be in a git repository.
if test -z "$NONGIT_OK"
then
	GIT_DIR=$(git rev-parse --git-dir) || exit
	if [ -z "$SUBDIRECTORY_OK" ]
	then
		test -z "$(git rev-parse --show-cdup)" || {
			exit=$?
			echo >&2 "You need to run this command from the toplevel of the working tree."
			exit $exit
		}
	fi
	test -n "$GIT_DIR" && GIT_DIR=$(cd "$GIT_DIR" && pwd) || {
		echo >&2 "Unable to determine absolute path of git directory"
		exit 1
	}
	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
fi

Re: Git commit path vs rebase path

[Johannes Sixt]

Am 5/7/2012 19:27, schrieb Junio C Hamano:
> Steven Penny <svnpenn@gmail.com> writes:
> 
>> I have noticed
>>
>> git commit uses this path
>>
>> .git/COMMIT_EDITMSG
>>
>> git rebase uses this path
>>
>> /home/Steven/jquery/.git/rebase-merge/git-rebase-todo
>>
>> So git commit is using a relative path while git rebase is using absolute path.
>>
>> This causes problem in Windows if your editor does not understand linux paths,
>> e.g. notepad, Notepad2, Notepad++, etc.
> 
> ... the issue is _not_ that the path is
> absolute, it is that the path is given as a wrong kind of absolute path.
> 
> Which suggests that "$(cd "$GIT_DIR" && pwd)" must give a full path that
> is suitable for the platform, and your platform wants it to be something
> like "c:\home\steven\jquery\..."?

The problem should be mitigated by be39048a7 (git-sh-setup.sh: Add an
pwd() function for MinGW), where the above now returns a Windows-style
absolute path, albeit with forward slashes instead of the backslashes.

I don't know what "/home/..." is in Steven's case, because it should look
more like "/c/home/..." unless it is an MSYS mount point, but even then it
should be reported as Windows-style path with the new pwd function.

IOW, the problem should be fixed in the next release.

-- Hannes

Re: Git commit path vs rebase path

[Steven Penny]

Johannes Sixt wrote:
> The problem should be mitigated by be39048a7 (git-sh-setup.sh: Add an
> pwd() function for MinGW), where the above now returns a Windows-style
> absolute path, albeit with forward slashes instead of the backslashes.

http://github.com/git/git/commit/be390

This looks like a good solution for MinGW, but cygwin will not support that

$ pwd -W
bash: pwd: -W: invalid option
pwd: usage: pwd [-LP]

Cygwin uses cygpath

$ cygpath -m /c/Windows/System32
C:/Windows/System32

Re: Git commit path vs rebase path

[Johannes Sixt]

Am 5/8/2012 8:44, schrieb Steven Penny:
> Johannes Sixt wrote:
>> The problem should be mitigated by be39048a7 (git-sh-setup.sh: Add an
>> pwd() function for MinGW), where the above now returns a Windows-style
>> absolute path, albeit with forward slashes instead of the backslashes.
> 
> http://github.com/git/git/commit/be390
> 
> This looks like a good solution for MinGW, but cygwin will not support that
> 
> $ pwd -W
> bash: pwd: -W: invalid option
> pwd: usage: pwd [-LP]

Are you saying that the new pwd function will also be used on Cygwin? That
would be a bug.

-- Hannes

Re: Git commit path vs rebase path

[Steven Penny]

Johannes Sixt wrote:
> Are you saying that the new pwd function will also be used on Cygwin? That
> would be a bug.

The linked patch should fix the problem for _MinGW_ users.

The problem will persist with _Cygwin_ users.

Re: Git commit path vs rebase path

[Junio C Hamano]

Steven Penny <svnpenn@gmail.com> writes:

> Johannes Sixt wrote:
>> Are you saying that the new pwd function will also be used on Cygwin? That
>> would be a bug.
>
> The linked patch should fix the problem for _MinGW_ users.
>
> The problem will persist with _Cygwin_ users.

What does a full pathname, fed as a parameter when invoking Windows native
binaries like notepad, look like in a Cygwin environment?  That is, if you
are writing a bash script that is meant to run in a Cygwin environment,
and if the script takes the name of a file in the current directory, but
it needs to chdir around for its own reasons before spawning notepad on
the file, i.e.

	#!/bin/bash
	file="$(pwd)/$1"
        ...
        cd ..some..where..else..you..have..no..control..over
        notepad "$file"

what is the right incantation to replace `pwd` in 'file="$(pwd)/$1"'
above?

Whatever that is, using that instead of `pwd` in git-sh-setup.sh here:

	test -n "$GIT_DIR" && GIT_DIR=$(cd "$GIT_DIR" && pwd) || {

would be the solution.

Re: Git commit path vs rebase path

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Steven Penny <svnpenn@gmail.com> writes:
>
>> Johannes Sixt wrote:
>>> Are you saying that the new pwd function will also be used on Cygwin? That
>>> would be a bug.
>>
>> The linked patch should fix the problem for _MinGW_ users.
>>
>> The problem will persist with _Cygwin_ users.
>
> What does a full pathname, fed as a parameter when invoking Windows native
> binaries like notepad, look like in a Cygwin environment?  That is, if you
> are writing a bash script that is meant to run in a Cygwin environment,
> and if the script takes the name of a file in the current directory, but
> it needs to chdir around for its own reasons before spawning notepad on
> the file, i.e.
>
> 	#!/bin/bash
> 	file="$(pwd)/$1"
>         ...
>         cd ..some..where..else..you..have..no..control..over
>         notepad "$file"
>
> what is the right incantation to replace `pwd` in 'file="$(pwd)/$1"'
> above?
>
> Whatever that is, using that instead of `pwd` in git-sh-setup.sh here:
>
> 	test -n "$GIT_DIR" && GIT_DIR=$(cd "$GIT_DIR" && pwd) || {
>
> would be the solution.

Actually, the above is stated rather poorly.  The path that ends up in
$file must be usable by both Windows native and Cygwin programs, as the
user may be using "vi" from Cygwin, or "notepad" like your example.

If there is no way to formulate $file to be acceptable for both, I do not
see any sane solution other than one side compromising for the rest,
perhaps by having

	$ cat >notepad.sh <<\EOF
        file=$(cygpath -m "$1") && exec notepad.exe "$file"
        EOF

on the end-user side, or something like that.

Re: Git commit path vs rebase path

[Steven Penny]

Junio C Hamano wrote:
> Actually, the above is stated rather poorly.  The path that ends up in
> $file must be usable by both Windows native and Cygwin programs, as the
> user may be using "vi" from Cygwin, or "notepad" like your example.

Excellent point.

I ran some test and this is what I came up with

# VI
/cygdrive/c/test/hello.sh # works
C:\test\hello.sh # works
C:/test/hello/sh # works

# NOTEPAD
/cygdrive/c/test/hello.sh # does not work
C:\test\hello.sh # works
C:/test/hello.sh # works

so the best compromise would be "C:/test/hello.sh" which can be created with

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7b3ae75..ba198d2 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -260,6 +260,11 @@ case $(uname -s) in
 		return 1
 	}
 	;;
+*CYGWIN*)
+    pwd () {
+        builtin cygpath -m
+    }
+    ;;
 *)
 	is_absolute_path () {
 		case "$1" in

http://github.com/svnpenn/git/commit/692bc

Re: Git commit path vs rebase path

[Junio C Hamano]

Steven Penny <svnpenn@gmail.com> writes:

> so the best compromise would be "C:/test/hello.sh" which can be created with
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 7b3ae75..ba198d2 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -260,6 +260,11 @@ case $(uname -s) in
>  		return 1
>  	}
>  	;;
> +*CYGWIN*)
> +    pwd () {
> +        builtin cygpath -m
> +    }
> +    ;;

What does "uname -s" say on cygwin exactly?  Our existing system detection
code I found was in the Makefile and it looks out "uname -o" output and
compares it with "Cygwin".

I am not suggesting to change this part of the code to use "uname -o" at
all; I just want to double check that the above *CYGWIN* matches it.

Re: Git commit path vs rebase path

[Steven Penny]

Junio C Hamano wrote:
> What does "uname -s" say on cygwin exactly?  Our existing system detection
> code I found was in the Makefile and it looks out "uname -o" output and
> compares it with "Cygwin".

This is what I get

$ uname -s
CYGWIN_NT-6.1-WOW64

$ uname -o
Cygwin

Re: Git commit path vs rebase path

[Ramsay Jones]

Steven Penny wrote:
> Junio C Hamano wrote:
>> Actually, the above is stated rather poorly.  The path that ends up in
>> $file must be usable by both Windows native and Cygwin programs, as the
>> user may be using "vi" from Cygwin, or "notepad" like your example.
> 
> Excellent point.
> 
> I ran some test and this is what I came up with
> 
> # VI
> /cygdrive/c/test/hello.sh # works
> C:\test\hello.sh # works
> C:/test/hello/sh # works
> 
> # NOTEPAD
> /cygdrive/c/test/hello.sh # does not work
> C:\test\hello.sh # works
> C:/test/hello.sh # works
> 
> so the best compromise would be "C:/test/hello.sh" which can be created with
> 
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 7b3ae75..ba198d2 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -260,6 +260,11 @@ case $(uname -s) in
>  		return 1
>  	}
>  	;;
> +*CYGWIN*)
> +    pwd () {
> +        builtin cygpath -m
> +    }
> +    ;;
>  *)
>  	is_absolute_path () {
>  		case "$1" in
> 
> http://github.com/svnpenn/git/commit/692bc

I would rather define a script; it can then be used independently of git.

Personally, I don't have this specific problem because I use (the cygwin
version of) vim. (does anybody actually use notepad?)

I mostly, but not exclusively, use cygwin tools on cygwin. For example I
use win32 versions of doxygen, ghostscript, tex (MikTex 2.7), graphviz etc.
However, the makefiles which drive those tools use relative paths ...

In fact the only problem I have encountered is with firefox, which I
resolved by hacking up a script. Indeed I have already posted to the
list on this issue before (back in January 2009) and, to save you the
effort of trawling the archives, I append that mail below.

HTH

ATB,
Ramsay Jones

-- >8 --
Date: Sat, 24 Jan 2009 18:51:23 +0000
From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
To: =?ISO-8859-1?Q?Bj=F6rn_Steinbrink?= <B.Steinbrink@gmx.de>
CC: Junio C Hamano <gitster@pobox.com>,  jaeckel@stzedn.de, 
 git@vger.kernel.org
Subject: Re: [PATCH] cygwin: Convert paths for html help from posix to windows

Björn Steinbrink wrote:
> When using "git help --web" with cygwin, we used to pass the posix path
> to the browser, but a native windows browser will expect a windows path
> and is unable to make use of the given path.
> 
> So the cygwin port gets its own open_html implementation that handles
> the path conversion.
> 
> Reported-by: Steffen Jaeckel <jaeckel@stzedn.de>
> Tested-by: Steffen Jaeckel <jaeckel@stzedn.de>
> 
> Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
> ---
> OK, I don't really know if this is the right way to do it. Maybe when
> the browser was built for cygwin this breaks? I have no clue, it's
> admittedly just the result of a quick glance at the code and some
> googling to find the "right" cygwin function... :-/
> 

Hi Björn,

I had the same problem. However, rather than modifying git, I created a
firefox wrapper script (in ~/bin) which used the cygpath command line
tool to do the path conversion. Also, if you use "git instaweb", you
also need to filter out http URLs and pass them through un-molested
by cygpath (it turns http://localhost into http:\localhost).

My script is clearly a "quick hack" just to get something working for
me, but you may find it useful as a starting point for your own
("proper" ;-) script, so I've included it below.

HTH,
Ramsay Jones

-->8--
#!/bin/sh
#

while test $# != 0
do
    case "$1" in
	-version)
	    echo "firefox 1.5.0.2"
	    exit 0
	    ;;
	-new-tab)
	    echo "-new-tab not supported"
	    exit 1
	    ;;
	--)
	    shift
	    break
	    ;;
	-*)
	    echo "option '$1' not supported"
	    exit 1
	    ;;
	*)
	    break
	    ;;
    esac
    shift
done

if test "$1" = ""
then
	p=
else
	case "$1" in
	    http*)
		p="$1"
		;;
	    *)
		p="$(cygpath -w "$1")"
		;;
	esac
fi


"/cygdrive/c/Program Files/Mozilla Firefox/firefox.exe" "$p"

exit 0



-- 8< --

Re: Git commit path vs rebase path

[Steven Penny]

Ramsay Jones wrote:
> I would rather define a script; it can then be used independently of git.

So your suggestion is to have git-sh-setup.sh account for MinGW, which is its
current state, but not account for Cygwin?

> Personally, I don't have this specific problem because I use (the cygwin
> version of) vim. (does anybody actually use notepad?)

If you had read carefully, you would have noticed that I mentioned more than
notepad. As well Notepad2, and Notepad++, etc.


> I mostly, but not exclusively, use cygwin tools on cygwin. For example I
> use win32 versions of doxygen, ghostscript, tex (MikTex 2.7), graphviz etc.
> However, the makefiles which drive those tools use relative paths ...

This convo is not about what tools _you_ use, but about the current
incompatibility with several native windows text editors.

Re: Git commit path vs rebase path

[Ramsay Jones]

Steven Penny wrote:
> Ramsay Jones wrote:
>> I would rather define a script; it can then be used independently of git.
> 
> So your suggestion is to have git-sh-setup.sh account for MinGW, which is its
> current state, but not account for Cygwin?

I wasn't specifically suggesting that no; I was suggesting that I prefer to
fix the problem with a script on cygwin. (Again, the script can be used
independently of git)

BTW, Johannes, earlier you said commit be39048 ("git-sh-setup.sh: Add an pwd()
function for MinGW", 17-04-2012) would fix the problem on MinGW; I'm not so
sure it will. I haven't actually tested it, so don't take my word for it. ;-P
(See below for an explanation of my doubts)

>> Personally, I don't have this specific problem because I use (the cygwin
>> version of) vim. (does anybody actually use notepad?)
> 
> If you had read carefully, you would have noticed that I mentioned more than
> notepad. As well Notepad2, and Notepad++, etc.

Yes, I did notice that you mentioned more than notepad ...

>> I mostly, but not exclusively, use cygwin tools on cygwin. For example I
>> use win32 versions of doxygen, ghostscript, tex (MikTex 2.7), graphviz etc.
>> However, the makefiles which drive those tools use relative paths ...
> 
> This convo is not about what tools _you_ use, but about the current
> incompatibility with several native windows text editors.

OK.

So, yes, I didn't give your patch a look; sorry about that. Let's take a look
now (quoting from earlier email):

> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> > index 7b3ae75..ba198d2 100644
> > --- a/git-sh-setup.sh
> > +++ b/git-sh-setup.sh
> > @@ -260,6 +260,11 @@ case $(uname -s) in
> >  		return 1
> >  	}
> >  	;;
> > +*CYGWIN*)
> > +    pwd () {
> > +        builtin cygpath -m
> > +    }
> > +    ;;
> >  *)
> >  	is_absolute_path () {
> >  		case "$1" in
> > 
> > http://github.com/svnpenn/git/commit/692bc

I haven't actually tried to apply or test your patch, so take the
following with a pinch of salt ...

I don't think this will work because:

    - cygpath is not a bash builtin, so bash *should* simply issue
      an error something along the lines of "not a shell builtin".

    - cygpath requires an input path

So, I would have expected the body of the pwd function to be something like:

    cygpath -m "$PWD"

or maybe

    cygpath -m "$(builtin pwd)"

(Again, I'm just typing into my mail client, so not tested ...)

Also note that the MinGW pwd() uses a shell builtin and so, unlike the above,
does not suffer any fork+exec overhead.

If we fix the above, then another problem (*which MinGW shares*) is that the
pwd() function is defined *after* the code that sets $GIT_DIR from which the
rebase state directory name is derived (see git-sh-setup.sh lines 223-239).

Note that cygwin git will create the various inputs (commit template say) with
lf only line endings; so the windows text editor you use must be able to cope
with such an input. (I think the PSEdit editor will cope just fine).

Similar comments apply to all other external programs launched by git (for
example, external diff/merge tools, clean/smudge filters ...).

HTH

ATB,
Ramsay Jones

Re: Git commit path vs rebase path

[Steven Penny]

Ramsay Jones wrote:
> I haven't actually tried to apply or test your patch, so take the
> following with a pinch of salt ...
>
> I don't think this will work because:
>
>    - cygpath is not a bash builtin, so bash *should* simply issue
>      an error something along the lines of "not a shell builtin".
>
>    - cygpath requires an input path
>
> So, I would have expected the body of the pwd function to be something like:
>
>    cygpath -m "$PWD"

If you would actually look at the file, you would see that pretty much
everything you just wrote is wrong

SNIPPET

*MINGW*)
	pwd () {
		builtin pwd -W
	}

http://github.com/git/git/blob/master/git-sh-setup.sh

Re: Git commit path vs rebase path

[Johannes Sixt]

Am 5/14/2012 0:58, schrieb Ramsay Jones:
> BTW, Johannes, earlier you said commit be39048 ("git-sh-setup.sh: Add an pwd()
> function for MinGW", 17-04-2012) would fix the problem on MinGW; I'm not so
> sure it will. 

> [...] problem (*which MinGW shares*) is that the
> pwd() function is defined *after* the code that sets $GIT_DIR from which the
> rebase state directory name is derived (see git-sh-setup.sh lines 223-239).

Good catch!

Nevertheless, if I set GIT_EDITOR to "notepad", 'git rebase -i' works to
some degree. Leaving aside that it does not understand LF line endings, it
opens the git-rebase-todo file; I can edit and save it. That is, it looks
like a usable Windows style path was passed to Notepad. (This is with
MinGW git, of course.)

But I can't use "Save As": Notepad just responds with "Error #3002". I
would attribute this to the forward slashes in the absolute path name, and
that the file dialog can't cope with them. Personally, I don't think this
is a big deal: We have heard of people who set GIT_EDITOR to something
Windows-y, but there were no complaints of the kind I mention in this
paragraph. (Why should there be any? There is no reason to use "Save As"
when git invokes an editor.)

-- Hannes

Re: Git commit path vs rebase path

[Ramsay Jones]

Johannes Sixt wrote:
> Am 5/14/2012 0:58, schrieb Ramsay Jones:
>> BTW, Johannes, earlier you said commit be39048 ("git-sh-setup.sh: Add an pwd()
>> function for MinGW", 17-04-2012) would fix the problem on MinGW; I'm not so
>> sure it will. 
> 
>> [...] problem (*which MinGW shares*) is that the
>> pwd() function is defined *after* the code that sets $GIT_DIR from which the
>> rebase state directory name is derived (see git-sh-setup.sh lines 223-239).
> 
> Good catch!
> 
> Nevertheless, if I set GIT_EDITOR to "notepad", 'git rebase -i' works to
> some degree. Leaving aside that it does not understand LF line endings, it
> opens the git-rebase-todo file; I can edit and save it. That is, it looks
> like a usable Windows style path was passed to Notepad. (This is with
> MinGW git, of course.)
> 

I was about to, confidently, proclaim that it should fail when
you run that rebase from a subdirectory ... Unfortunately, it
seems to work! *ahem*

The reason I was confident, is simply that I have seen this
pattern:

    p= ...some path...
    p=$(cd "$p" && pwd)

many times while debugging scripts on MinGW, so I 'know' that if
p is initially a relative path, then the result is an absolute
windows path. For example, while cwd is /home/ramsay/git then:

    .git => C:/msysgit/msysgit/home/ramsay/git/.git

Whereas if p is initially an absolute windows path, then the result
is a POSIX-like path, viz:

    C:/msysgit/msysgit/home/ramsay/git/.git => /usr/home/ramsay/git/.git

If you combine that with the behaviour of 'git rev-parse', thus:

    $ cd /home/ramsay/git
    $ git rev-parse --git-dir
    .git
    $ cd xdiff
    $ git rev-parse --git-dir
    C:/msysgit/msysgit/home/ramsay/git/.git
    $

you should see "git rebase -i" invoking the editor with a POSIX
path.

[BTW, I have noticed that I sometimes get different behaviour when
I type this stuff into an interactive shell. :(]

Anyway, a bit of debugging shows that I was right, kinda! :-D

I placed a "set -x" in git-sh-setup (line 222) just before the code
to set up GIT_DIR and ran git-rebase like so:

    $ cd /home/ramsay/git
    $ GIT_EDITOR=false ./bin-wrappers/git rebase -i master uname >ttt 2>&1
    $ cd xdiff
    $ GIT_EDITOR=false ../bin-wrappers/git rebase -i master uname >sss 2>&1
    $ diff ../ttt sss | more

which showed that in the subdirectory case, git was indeed invoking
the editor with a POSIX-like path. (The output files, ttt and sss, show
some other interesting/odd behaviour).

['uname' is an old branch of mine that, currently, is based on v1.7.10]

The reason it works, despite the above, is clear if you now run:

    $ GIT_EDITOR=args ../bin-wrappers/git rebase -i master uname

    ...

    +++ git var GIT_EDITOR
    ++ GIT_SEQUENCE_EDITOR=args
    ++ eval args '"$@"'
    +++ args /usr/home/ramsay/git/.git/rebase-merge/git-rebase-todo
    argv[0] = 'C:\msysgit\msysgit\home\ramsay\bin\args.exe'
    argv[1] = 'C:/msysgit/msysgit/home/ramsay/git/.git/rebase-merge/git-rebase-todo'

    ++ die_abort 'Could not execute editor'
    ++ rm -rf /usr/home/ramsay/git/.git/rebase-merge
    ++ die 'Could not execute editor'
    ++ die_with_status 1 'Could not execute editor'
    ++ status=1
    ++ shift
    ++ echo 'Could not execute editor'
    Could not execute editor
    ++ exit 1

[I'm sure you can guess what the args program looks like!]

So, the msys "path munging" of program arguments saves the day!

HTH

ATB,
Ramsay Jones

Re: Git commit path vs rebase path

[Johannes Sixt]

Am 5/15/2012 19:32, schrieb Ramsay Jones:
>     ++ GIT_SEQUENCE_EDITOR=args
>     ++ eval args '"$@"'
>     +++ args /usr/home/ramsay/git/.git/rebase-merge/git-rebase-todo
>     argv[0] = 'C:\msysgit\msysgit\home\ramsay\bin\args.exe'
>     argv[1] = 'C:/msysgit/msysgit/home/ramsay/git/.git/rebase-merge/git-rebase-todo'
...
> So, the msys "path munging" of program arguments saves the day!

Absolutely. This path munging is an essential detail in the process.

I don't know whether Cygwin has a similar feature, but I suppose not,
otherwise we wouldn't have received this issue report.

-- Hannes

[PATCH 0/2] Re: Git commit path vs rebase path

[Junio C Hamano]

I do not want to see a discussion hanging around waiting for somebody to
bring it to conclusion, especially when the topic is not something I can
give a tested definitive solution.  It is a sure way to lose the effort so
far that brought us almost there but not quite by not following through
and forgetting about it.

Here are two patches I *think* represent the conclusion of the discussion,
but it needs eyeballing from stakeholders.  I may have misunderstood the
issue entirely.

Also Steven's patch needs a sign-off.


Junio C Hamano (1):
  git-sh-setup: define workaround wrappers before they are used

Steven Penny (1):
  git-sh-setup: work around Cygwin path handling gotchas

 git-sh-setup.sh | 46 ++++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

-- 
1.7.10.2.537.g0ac6509

[PATCH 1/2] git-sh-setup: define workaround wrappers before they are used

[Junio C Hamano]

Recently we tweaked this scriptlet to let mingw port redefine "pwd" to
always return Windows-style path, but the code to do so came after the
first use of "pwd" to set up $GIT_DIR shell variable.

Move the block to define these workaround wrappers, so that everything
everything that executes when the scriptlet is dot-sourced uses the
replacements.

Noticed-by: Ramsay Jones
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-sh-setup.sh | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7b3ae75..770a86e 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -218,27 +218,8 @@ clear_local_git_env() {
 	unset $(git rev-parse --local-env-vars)
 }
 
-# Make sure we are in a valid repository of a vintage we understand,
-# if we require to be in a git repository.
-if test -z "$NONGIT_OK"
-then
-	GIT_DIR=$(git rev-parse --git-dir) || exit
-	if [ -z "$SUBDIRECTORY_OK" ]
-	then
-		test -z "$(git rev-parse --show-cdup)" || {
-			exit=$?
-			echo >&2 "You need to run this command from the toplevel of the working tree."
-			exit $exit
-		}
-	fi
-	test -n "$GIT_DIR" && GIT_DIR=$(cd "$GIT_DIR" && pwd) || {
-		echo >&2 "Unable to determine absolute path of git directory"
-		exit 1
-	}
-	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
-fi
 
-# Fix some commands on Windows
+# Platform specific tweaks to work around some commands
 case $(uname -s) in
 *MINGW*)
 	# Windows has its own (incompatible) sort and find
@@ -269,3 +250,23 @@ case $(uname -s) in
 		return 1
 	}
 esac
+
+# Make sure we are in a valid repository of a vintage we understand,
+# if we require to be in a git repository.
+if test -z "$NONGIT_OK"
+then
+	GIT_DIR=$(git rev-parse --git-dir) || exit
+	if [ -z "$SUBDIRECTORY_OK" ]
+	then
+		test -z "$(git rev-parse --show-cdup)" || {
+			exit=$?
+			echo >&2 "You need to run this command from the toplevel of the working tree."
+			exit $exit
+		}
+	fi
+	test -n "$GIT_DIR" && GIT_DIR=$(cd "$GIT_DIR" && pwd) || {
+		echo >&2 "Unable to determine absolute path of git directory"
+		exit 1
+	}
+	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
+fi
-- 
1.7.10.2.537.g0ac6509

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

[Junio C Hamano]

From: Steven Penny <svnpenn@gmail.com>

From: Steven Penny <svnpenn@gmail.com>

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..f0f941d 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -241,6 +241,11 @@ case $(uname -s) in
 		return 1
 	}
 	;;
+*CYGWIN*)
+	pwd () {
+		builtin cygpath -m
+	}
+	;;
 *)
 	is_absolute_path () {
 		case "$1" in
-- 
1.7.10.2.537.g0ac6509

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

[Steven Penny]

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

diff -u a/git-sh-setup.sh b/git-sh-setup.sh
--- a/git-sh-setup.sh	2012-05-16 13:48:43.160399300 -0500
+++ b/git-sh-setup.sh	2012-05-16 13:50:29.465479600 -0500
@@ -218,26 +218,6 @@
 	unset $(git rev-parse --local-env-vars)
 }

-# Make sure we are in a valid repository of a vintage we understand,
-# if we require to be in a git repository.
-if test -z "$NONGIT_OK"
-then
-	GIT_DIR=$(git rev-parse --git-dir) || exit
-	if [ -z "$SUBDIRECTORY_OK" ]
-	then
-		test -z "$(git rev-parse --show-cdup)" || {
-			exit=$?
-			echo >&2 "You need to run this command from the toplevel of the
working tree."
-			exit $exit
-		}
-	fi
-	test -n "$GIT_DIR" && GIT_DIR=$(cd "$GIT_DIR" && pwd) || {
-		echo >&2 "Unable to determine absolute path of git directory"
-		exit 1
-	}
-	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
-fi
-
 # Fix some commands on Windows
 case $(uname -s) in
 *MINGW*)
@@ -260,6 +240,11 @@
 		return 1
 	}
 	;;
+*CYGWIN*)
+	pwd () {
+		cygpath -m "$PWD"
+	}
+	;;
 *)
 	is_absolute_path () {
 		case "$1" in
@@ -269,3 +254,23 @@
 		return 1
 	}
 esac
+
+# Make sure we are in a valid repository of a vintage we understand,
+# if we require to be in a git repository.
+if test -z "$NONGIT_OK"
+then
+	GIT_DIR=$(git rev-parse --git-dir) || exit
+	if [ -z "$SUBDIRECTORY_OK" ]
+	then
+		test -z "$(git rev-parse --show-cdup)" || {
+			exit=$?
+			echo >&2 "You need to run this command from the toplevel of the
working tree."
+			exit $exit
+		}
+	fi
+	test -n "$GIT_DIR" && GIT_DIR=$(cd "$GIT_DIR" && pwd) || {
+		echo >&2 "Unable to determine absolute path of git directory"
+		exit 1
+	}
+	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
+fi

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

[Junio C Hamano]

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

Re: Git commit path vs rebase path

[Ramsay Jones]

Johannes Sixt wrote:
> Am 5/15/2012 19:32, schrieb Ramsay Jones:
>>     ++ GIT_SEQUENCE_EDITOR=args
>>     ++ eval args '"$@"'
>>     +++ args /usr/home/ramsay/git/.git/rebase-merge/git-rebase-todo
>>     argv[0] = 'C:\msysgit\msysgit\home\ramsay\bin\args.exe'
>>     argv[1] = 'C:/msysgit/msysgit/home/ramsay/git/.git/rebase-merge/git-rebase-todo'
> ...
>> So, the msys "path munging" of program arguments saves the day!
> 
> Absolutely. This path munging is an essential detail in the process.
> 
> I don't know whether Cygwin has a similar feature, but I suppose not,
> otherwise we wouldn't have received this issue report.

Yes, but I keep forgetting about this msys feature (I don't know why!).
I've had this "slap forehead" moment 3 or 4 times already (why do you
think I have the args program close to hand? :-D ).

As you surmised, cygwin does not have this feature. On cygwin you are
encouraged to use the cygpath utility in scripts. (Also, cygwin does
provide an API to convert to/from POSIX/win32 paths from your own
programs. eg. cygwin_conv_to_[full]_win32_path() and cygwin_conv_to_\
[full]_posix_path().)

>From the cygwin user guide, in a section titled "Using Cygwin effectively
with Windows", we find this:

    "Windows programs do not understand POSIX pathnames, so any arguments
    that reference the filesystem must be in Windows (or DOS) format or
    translated. Cygwin provides the cygpath utility for converting
    between Windows and POSIX paths. A complete description of its options
    and examples of its usage are in Section 3.7.2, including a shell script
    for starting Windows Explorer in any directory. The same format works
    for most Windows programs, for example

        notepad.exe "$(cygpath -aw "Desktop/Phone Numbers.txt")"

    A few programs require a Windows-style, semicolon-delimited path list,
    which cygpath can translate from a POSIX path with the -p option. For
    example, a Java compilation from bash might look like this:

        javac -cp "$(cygpath -pw "$CLASSPATH")" hello.java

    Since using quoting and subshells is somewhat awkward, it is often
    preferable to use cygpath in shell scripts."

Just as an exercise, I created a script to use the windows PSPad editor
(included below), using it to create a commit and also run this rebase:

    GIT_EDITOR=pspad git rebase -i master uname

Both git commands launched the editor (and completed their task) just fine.

Note that the script can be improved greatly, but it only took ten minutes
to create and is sufficient to this task. (PSPad supports more than one file
on the command line, despite what it's help file says, although all options
apply to all files. You should be able to "bundle" the options ...).


ATB,
Ramsay Jones

-- >8 --
#!/bin/sh

file=
opts=

while test $# != 0
do
    case "$1" in
	-[hH])
	    # open file in hex editor
	    opts="$opts /H"
	    ;;
	-[rR])
	    # open file in read-only mode
	    opts="$opts /R"
	    ;;
	-[0-9]*)
	    # open file and goto line n
	    opts="$opts /${1:1}"
	    ;;
	-*)
	    echo "option '$1' not supported"
	    exit 1
	    ;;
	*)
	    if test -n "$file"; then
		    echo "only one filename allowed"
		    exit 1
	    fi
	    file="$1"
	    ;;
    esac
    shift
done

if test -n "$file"; then
	file="$(cygpath -aw "$file")"
fi

"C:/Program Files/PSPad editor/PSPad.exe" "$file" $opts

-- 8< --

Re: Git commit path vs rebase path

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> As you surmised, cygwin does not have this feature. On cygwin you are
> encouraged to use the cygpath utility in scripts. (Also, cygwin does
> provide an API to convert to/from POSIX/win32 paths from your own
> programs. eg. cygwin_conv_to_[full]_win32_path() and cygwin_conv_to_\
> [full]_posix_path().)
> ...
> Just as an exercise, I created a script to use the windows PSPad editor
> (included below), using it to create a commit and also run this rebase:
>
>     GIT_EDITOR=pspad git rebase -i master uname
>
> Both git commands launched the editor (and completed their task) just fine.

So where does the above leave us?

Your message showed a way to "solve" it without changing git, but the
presense of workaround alone is not a good reason not to change git, if
the change makes things better without breaking other things.  I'd like
to know if Steven's patch breaks Cygwin users in other ways.

Steven's message showed that spawning Cygwin programs and feeding paths
in $GIT_DIR to them would not be broken if we start giving GIT_DIR as a
Windows style path inside shell scripted Porcelains.  But will there be
other things that get broken by the change?  An example of what would
worry me is that we define has_drive_prefix() only for compat/mingw.h,
so if somebody exports GIT_DIR back to call the binaries from scripted
Porcelains, it is likely that it would break git running on Cygwin,
where it expects only posix-style paths.  Upon seeing C:/foo/bar.txt, it
will start saying "Ah, that is a relative path" and may end up adding
the $(pwd) in front when it needs to know its absolute path.

Re: [PATCH 1/2] git-sh-setup: define workaround wrappers before they are used

[Ramsay Jones]

Junio C Hamano wrote:
> Recently we tweaked this scriptlet to let mingw port redefine "pwd" to
> always return Windows-style path, but the code to do so came after the
> first use of "pwd" to set up $GIT_DIR shell variable.
> 
> Move the block to define these workaround wrappers, so that everything
> everything that executes when the scriptlet is dot-sourced uses the
> replacements.
> 
> Noticed-by: Ramsay Jones
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  git-sh-setup.sh | 41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 7b3ae75..770a86e 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -218,27 +218,8 @@ clear_local_git_env() {
>  	unset $(git rev-parse --local-env-vars)
>  }
>  
> -# Make sure we are in a valid repository of a vintage we understand,
> -# if we require to be in a git repository.
> -if test -z "$NONGIT_OK"
> -then
> -	GIT_DIR=$(git rev-parse --git-dir) || exit
> -	if [ -z "$SUBDIRECTORY_OK" ]
> -	then
> -		test -z "$(git rev-parse --show-cdup)" || {
> -			exit=$?
> -			echo >&2 "You need to run this command from the toplevel of the working tree."
> -			exit $exit
> -		}
> -	fi
> -	test -n "$GIT_DIR" && GIT_DIR=$(cd "$GIT_DIR" && pwd) || {
> -		echo >&2 "Unable to determine absolute path of git directory"
> -		exit 1
> -	}
> -	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
> -fi
>  
> -# Fix some commands on Windows
> +# Platform specific tweaks to work around some commands
>  case $(uname -s) in
>  *MINGW*)
>  	# Windows has its own (incompatible) sort and find
> @@ -269,3 +250,23 @@ case $(uname -s) in
>  		return 1
>  	}
>  esac
> +
> +# Make sure we are in a valid repository of a vintage we understand,
> +# if we require to be in a git repository.
> +if test -z "$NONGIT_OK"
> +then
> +	GIT_DIR=$(git rev-parse --git-dir) || exit
> +	if [ -z "$SUBDIRECTORY_OK" ]
> +	then
> +		test -z "$(git rev-parse --show-cdup)" || {
> +			exit=$?
> +			echo >&2 "You need to run this command from the toplevel of the working tree."
> +			exit $exit
> +		}
> +	fi
> +	test -n "$GIT_DIR" && GIT_DIR=$(cd "$GIT_DIR" && pwd) || {
> +		echo >&2 "Unable to determine absolute path of git directory"
> +		exit 1
> +	}
> +	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
> +fi

Thanks for doing this. (I would have got around to it, honest! However, it does
solve a minor problem for me, since I kinda promised not to post anymore MinGW
specific patches. :-D ).

I have not done a full test on this patch; I have only run the following tests
on MinGW and cygwin:

    $ git grep -l -e 'git *submodule' -- t
    t/t5526-fetch-submodules.sh
    t/t6008-rev-list-submodule.sh
    t/t7003-filter-branch.sh
    t/t7400-submodule-basic.sh
    t/t7401-submodule-summary.sh
    t/t7403-submodule-sync.sh
    t/t7405-submodule-merge.sh
    t/t7406-submodule-update.sh
    t/t7407-submodule-foreach.sh
    t/t7408-submodule-reference.sh
    t/t7506-status-submodule.sh
    t/t7610-mergetool.sh
    t/t9300-fast-import.sh
    t/t9350-fast-export.sh

(On MinGW i have to use a "GIT_TEST_CMP='diff -ub'" prefix, otherwise there are
some failures caused by "lf/crlf line ending" problems).

All of these tests pass. I don't anticipate any problems (especially on cygwin,
since it is essentially a noop there), but I have not done a complete test.

I probably won't get to it soon, so I'm hoping somebody can beat me to it!

ATB,
Ramsay Jones

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

[Ramsay Jones]

Junio C Hamano wrote:
> 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"
#               )
#
$

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

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

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

That is more or less irrelevant, not in the sense that what you say is
irrelevant, but in the sense that something can be worked around in a
different way alone is not a good reason to reject a patch, if its benefit
outweigh its costs.

The more important message for you to respond to is this one:

    Subject: Re: Git commit path vs rebase path
    To: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
    Cc: Johannes Sixt <j.sixt@viscovery.net>,
            Steven Penny <svnpenn@gmail.com>,
            Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
    Date: Thu, 17 May 2012 12:19:33 -0700
    Message-ID: <xmqqfwayobsq.fsf@junio.mtv.corp.google.com>

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

If I speculated in the other message is correct (in short, "In Cygwin
world, Git is compiled to use POSIX paths and would not work with Windows
paths."), I think this "problem" is fundamentally un"fix"able.

And from Cygwin Git, your programs (like $EDITOR and hooks) will get POSIX
paths.  It is your programs' responsibility to turn them into Windows
paths if/as necessary.

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

Of course.  That is one typical symptom that suggests my speculation was
correct.

So "I don't think this patch is necessary" is irrelevant, but "This patch
is harmful; Git on Cygwin is never supposed to use Windows paths" is very
relevant ;-)

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

[Steven Penny]

On Thu, May 17, 2012 at 9:34 PM, Junio C Hamano wrote:
> And from Cygwin Git, your programs (like $EDITOR and hooks) will get POSIX
> paths.  It is your programs' responsibility to turn them into Windows
> paths if/as necessary.

I think this is correct. These program should be accepting Cygwin paths and are
not for obvious reason, they exist outside the the Cygwin world.

For now I will do something like this

	editor(){
		cygpath -m "$1" | xargs notepad
	}

	export -f editor
	
	git config core.editor editor

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

[Ramsay Jones]

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> I guess you won't be shocked to hear that I don't think this patch is
>> necessary. :-P
> 
> That is more or less irrelevant, not in the sense that what you say is
> irrelevant, but in the sense that something can be worked around in a
> different way alone is not a good reason to reject a patch, if its benefit
> outweigh its costs.

I completely agree.

In case it was not clear, I was not suggesting that the patch be rejected on
the basis that the problem could be worked around in a different way.

> If I speculated in the other message is correct (in short, "In Cygwin
> world, Git is compiled to use POSIX paths and would not work with Windows
> paths."), I think this "problem" is fundamentally un"fix"able.

Yes, Cygwin is essentially just another POSIX target as far as Git is
concerned. Cygwin tries hard to provide a POSIX-like environment, but it is
not possible for it to completely hide the fact that the base OS is not
actually a POSIX system.
 
> And from Cygwin Git, your programs (like $EDITOR and hooks) will get POSIX
> paths.  It is your programs' responsibility to turn them into Windows
> paths if/as necessary.

I would say that this is the only sensible way to proceed.

However, you could imagine adding code to accommodate external windows
programs. If we limit ourselves to the text editor, for example, I could
imagine something like the diff attached below to fix up the C based git
programs. (You would need to make similar changes to the shell and perl
scripts which launch the text editor).

I would not like to see a patch based on this (or any others like it) applied,
since it is going in the wrong direction. (Why do people use Cygwin git rather
than MinGW git and vice versa). Of course, it is not my decision to make ... :-P

>> 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:".
> 
> Of course.  That is one typical symptom that suggests my speculation was
> correct.

Heh, I was surprised that it did as well as it did; after all, it passed 44
out of the 45 tests run (from t7400-submodule-basic.sh). ;-)

> So "I don't think this patch is necessary" is irrelevant, but "This patch
> is harmful; Git on Cygwin is never supposed to use Windows paths" is very
> relevant ;-)

I agree.

ATB,
Ramsay Jones

-- >8 --
diff --git a/editor.c b/editor.c
index d834003..cf36e62 100644
--- a/editor.c
+++ b/editor.c
@@ -1,6 +1,9 @@
 #include "cache.h"
 #include "strbuf.h"
 #include "run-command.h"
+#ifdef __CYGWIN__
+# include <sys/cygwin.h>
+#endif
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -37,6 +40,12 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
+#ifdef __CYGWIN__
+		char win32_path[1024];
+
+		cygwin_conv_to_full_win32_path(path, win32_path);
+		args[1] = win32_path;
+#endif
 
 		if (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))
 			return error("There was a problem with the editor '%s'.",
-- 8< --

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

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> However, you could imagine adding code to accommodate external windows
> programs. If we limit ourselves to the text editor, for example, I could
> imagine something like the diff attached below to fix up the C based git
> programs. (You would need to make similar changes to the shell and perl
> scripts which launch the text editor).

If you _only_ allow editors that understands windows style paths, your
patch may make sense, but doesn't it break editors that wants only POSIX
style paths?

>
> ATB,
> Ramsay Jones
>
> -- >8 --
> diff --git a/editor.c b/editor.c
> index d834003..cf36e62 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -1,6 +1,9 @@
>  #include "cache.h"
>  #include "strbuf.h"
>  #include "run-command.h"
> +#ifdef __CYGWIN__
> +# include <sys/cygwin.h>
> +#endif
>  
>  #ifndef DEFAULT_EDITOR
>  #define DEFAULT_EDITOR "vi"
> @@ -37,6 +40,12 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
>  
>  	if (strcmp(editor, ":")) {
>  		const char *args[] = { editor, path, NULL };
> +#ifdef __CYGWIN__
> +		char win32_path[1024];
> +
> +		cygwin_conv_to_full_win32_path(path, win32_path);
> +		args[1] = win32_path;
> +#endif
>  
>  		if (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))
>  			return error("There was a problem with the editor '%s'.",
> -- 8< --

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

[Ramsay Jones]

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> However, you could imagine adding code to accommodate external windows
>> programs. If we limit ourselves to the text editor, for example, I could
>> imagine something like the diff attached below to fix up the C based git
>> programs. (You would need to make similar changes to the shell and perl
>> scripts which launch the text editor).
> 
> If you _only_ allow editors that understands windows style paths, your
> patch may make sense, but doesn't it break editors that wants only POSIX
> style paths?

Yes.

(If it wasn't clear, I included the code to show what I *didn't* want to see!).

Cygwin built text editors will, of course, work fine with POSIX paths, but
may also work with win32 paths as well. For example, vim supports win32 paths
just fine (it's the *only* editor I tested).

I would not be surprised if vim is not alone in that; I think it mainly depends
on whether the editor attempts to "interpret" the parameter, or simply treats it
as an opaque token to be passed straight to [f]open() [1].

Equally, I suspect some cygwin built editors will fail miserably (maybe they try
to interpret the parameter as an scp-like url, say). I don't know and I'm not
about to test every cygwin text editor to find out!

So, in my opinion, adding code to explicitly support win32 paths, while possible,
is likely to open up a can-o-worms (even if this support were opt-in via config).
I would prefer that we don't go there. :-D


ATB,
Ramsay Jones

[1] Which is why git already supports win32 paths to a certain degree. For example,
the following will work as expected:

  $ git config --global core.excludesfile 'C:/cygwin/home/ramsay/.gitignore'

(ie git will read and use the given .gitignore file without problem)