Added check for Windows environment so that we can fix paths #161

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants
@mhart

mhart commented Oct 16, 2011

After a recent installation of msysGit (Git-1.7.7-preview20111014.exe), I've found that the path seems to be getting mangled when invoking git-flow, and the following error occurs:

$ git flow init
C:\Program Files (x86)\Git\bin\git-flow: line 68: ./gitflow-common: No such file or directory
C:\Program Files (x86)\Git\bin\git-flow: line 76: ./gitflow-shFlags: No such file or directory

This turns out to be because the GITFLOW_DIR variable is created by using dirname $0 and $0 in this case happens to be a Windows path with backslashes, and so the command just returns '.' instead of the actual path to Git.

This patch checks to see if we're in a Windows environment and if so converts the path accordingly.

@mhart mhart referenced this pull request Nov 6, 2011

Closed

Useable with MSysGit? #25

@sjaeckel

This comment has been minimized.

Show comment
Hide comment
@sjaeckel

sjaeckel Nov 8, 2011

Contributor

+1 to be integrated soon for msysgit users!

Contributor

sjaeckel commented Nov 8, 2011

+1 to be integrated soon for msysgit users!

git-flow
@@ -42,7 +42,12 @@ if [ "$DEBUG" = "yes" ]; then
set -x
fi
-export GITFLOW_DIR=$(dirname "$0")
+if [ -n "$MSYSTEM" ]; then

This comment has been minimized.

@NiklasHansen

NiklasHansen Nov 17, 2011

This doesn't detect my system being Windows. That might be because I, when installed MsysGit, chose to only use Windows Command Prompt, and not using the "normal" git bash. So if there is another way of detecting Windows, that detects Windows in both situations, that would probably be better.

@NiklasHansen

NiklasHansen Nov 17, 2011

This doesn't detect my system being Windows. That might be because I, when installed MsysGit, chose to only use Windows Command Prompt, and not using the "normal" git bash. So if there is another way of detecting Windows, that detects Windows in both situations, that would probably be better.

This comment has been minimized.

@nvie

nvie Nov 23, 2011

Owner

What exactly does the $MSYSTEM variable come from here? Is it guaranteed to be set inside all MsysGit shells? Is this documented behaviour?

@nvie

nvie Nov 23, 2011

Owner

What exactly does the $MSYSTEM variable come from here? Is it guaranteed to be set inside all MsysGit shells? Is this documented behaviour?

This comment has been minimized.

@nvie

nvie Nov 23, 2011

Owner

...because I agree with @NiklasHansen's comment on this one.

@nvie

nvie Nov 23, 2011

Owner

...because I agree with @NiklasHansen's comment on this one.

@mhart

This comment has been minimized.

Show comment
Hide comment
@mhart

mhart Nov 28, 2011

OK - didn't realise it was occurring in all shells - I've changed the check to look at the OS environment variable instead - let me know if that fixes the problem.

mhart commented Nov 28, 2011

OK - didn't realise it was occurring in all shells - I've changed the check to look at the OS environment variable instead - let me know if that fixes the problem.

@dvide

This comment has been minimized.

Show comment
Hide comment
@dvide

dvide Nov 30, 2011

Your latest commit seems to work for me @mhart, on both the normal Windows cmd and on Git Bash. Thanks!

dvide commented Nov 30, 2011

Your latest commit seems to work for me @mhart, on both the normal Windows cmd and on Git Bash. Thanks!

@nvie

This comment has been minimized.

Show comment
Hide comment
@nvie

nvie Nov 30, 2011

Owner

Is the uname command available? Because this is the standard way of detecting OSes on most systems. Could you guys run that command (and uname -a) and see what happens?

Owner

nvie commented Nov 30, 2011

Is the uname command available? Because this is the standard way of detecting OSes on most systems. Could you guys run that command (and uname -a) and see what happens?

@mhart

This comment has been minimized.

Show comment
Hide comment
@mhart

mhart Nov 30, 2011

It's available in Git Bash - shows up as "MINGW32_NT-6.1 etc, etc" for me - not sure about other environments

mhart commented Nov 30, 2011

It's available in Git Bash - shows up as "MINGW32_NT-6.1 etc, etc" for me - not sure about other environments

@dvide

This comment has been minimized.

Show comment
Hide comment
@dvide

dvide Nov 30, 2011

@nvie It's not available on windows. It's there in Git Bash like @mhart says, but unfortunately not if you're just using the normal cmd.exe

dvide commented Nov 30, 2011

@nvie It's not available on windows. It's there in Git Bash like @mhart says, but unfortunately not if you're just using the normal cmd.exe

@mhart

This comment has been minimized.

Show comment
Hide comment
@mhart

mhart Nov 30, 2011

Happy to change to 'uname | grep -c ^MINGW' (checking $OSTYPE would be the other portable option) - do we need to consider Cygwin, etc though?

mhart commented Nov 30, 2011

Happy to change to 'uname | grep -c ^MINGW' (checking $OSTYPE would be the other portable option) - do we need to consider Cygwin, etc though?

@mhart

This comment has been minimized.

Show comment
Hide comment
@mhart

mhart Nov 30, 2011

Ah, sorry @dvide - didn't see your comment.

mhart commented Nov 30, 2011

Ah, sorry @dvide - didn't see your comment.

@dvide

This comment has been minimized.

Show comment
Hide comment
@dvide

dvide Nov 30, 2011

Yeah, the problem is that the msysgit installer adds %ProgramFiles%\Git\cmd to your system PATH, not %ProgramFiles%\Git\bin where the uname.exe resides. It seems like it uses some scripts in the %ProgramFiles%\Git\cmd directory for invoking git and gitk. Otherwise uname would probably work OK.

dvide commented Nov 30, 2011

Yeah, the problem is that the msysgit installer adds %ProgramFiles%\Git\cmd to your system PATH, not %ProgramFiles%\Git\bin where the uname.exe resides. It seems like it uses some scripts in the %ProgramFiles%\Git\cmd directory for invoking git and gitk. Otherwise uname would probably work OK.

@dvide

This comment has been minimized.

Show comment
Hide comment
@dvide

dvide Nov 30, 2011

Tested commit 3a52112 on Cygwin and it doesn't seem to cause any problems; though it works on Cygwin without the change anyway. echo $OS on Cygwin still outputs Windows_NT, so it must be running your new code, but I guess it doesn't matter because there aren't any backslashes in the paths. GITFLOW_DIR ends up as /usr/local/bin in my tests regardless. Thinking about it, is there any need for an if at all in then? We don't need to detect OS; it will just replace any backslashes IF they exist. If they don't exist, no harm done. So might just be OK to use export GITFLOW_DIR=$(dirname "$(echo $0 | sed -e 's,\\,/,g')") for all cases?

Also OSTYPE isn't available on the standard Windows terminal either.

dvide commented Nov 30, 2011

Tested commit 3a52112 on Cygwin and it doesn't seem to cause any problems; though it works on Cygwin without the change anyway. echo $OS on Cygwin still outputs Windows_NT, so it must be running your new code, but I guess it doesn't matter because there aren't any backslashes in the paths. GITFLOW_DIR ends up as /usr/local/bin in my tests regardless. Thinking about it, is there any need for an if at all in then? We don't need to detect OS; it will just replace any backslashes IF they exist. If they don't exist, no harm done. So might just be OK to use export GITFLOW_DIR=$(dirname "$(echo $0 | sed -e 's,\\,/,g')") for all cases?

Also OSTYPE isn't available on the standard Windows terminal either.

@mhart

This comment has been minimized.

Show comment
Hide comment
@mhart

mhart Nov 30, 2011

Considering that you can't create a file with a backslash in the name on Windows anyway (even Cygwin won't deal with it: http://www.cygwin.com/cygwin-ug-net/using-specialnames.html), it should be fine... no?

mhart commented Nov 30, 2011

Considering that you can't create a file with a backslash in the name on Windows anyway (even Cygwin won't deal with it: http://www.cygwin.com/cygwin-ug-net/using-specialnames.html), it should be fine... no?

@dvide

This comment has been minimized.

Show comment
Hide comment
@dvide

dvide Nov 30, 2011

@mhart Are you allowed backslashes in names on normal non-cygwin unix? If so, I think it's probably best to keep the windows OS detection then.

dvide commented Nov 30, 2011

@mhart Are you allowed backslashes in names on normal non-cygwin unix? If so, I think it's probably best to keep the windows OS detection then.

@mhart

This comment has been minimized.

Show comment
Hide comment
@mhart

mhart Nov 30, 2011

Yeah - I wasn't suggesting getting rid of the detection - just that the slash replace should work on any Windows shell regardless, without messing with filenames any more so than the shell itself would.

mhart commented Nov 30, 2011

Yeah - I wasn't suggesting getting rid of the detection - just that the slash replace should work on any Windows shell regardless, without messing with filenames any more so than the shell itself would.

@dvide

This comment has been minimized.

Show comment
Hide comment
@dvide

dvide Nov 30, 2011

@mhart That's correct yes. Sorry for the confusion. I was the one suggesting to get rid of the detection, but I didn't realize you could have backslashes in names on other OSes so scratch that. But you're right that on Cygwin, even with your new code it doesn't cause any issues there anyway, so that's nice to know.

dvide commented Nov 30, 2011

@mhart That's correct yes. Sorry for the confusion. I was the one suggesting to get rid of the detection, but I didn't realize you could have backslashes in names on other OSes so scratch that. But you're right that on Cygwin, even with your new code it doesn't cause any issues there anyway, so that's nice to know.

@mhart

This comment has been minimized.

Show comment
Hide comment
@mhart

mhart Nov 30, 2011

Just cleaned up the check/replace - @dvide, let me know if that still works for you

mhart commented Nov 30, 2011

Just cleaned up the check/replace - @dvide, let me know if that still works for you

@dvide

This comment has been minimized.

Show comment
Hide comment
@dvide

dvide Nov 30, 2011

@mhart, Tested bcf1dfa on Git Bash, normal cmd.exe, and Cygwin. Seems to work fine on all three. I did a git flow init, created a feature branch, merged it back to develop branch. No problems.

Though if I edit git-flow to echo $GITFLOW_DIR, I'm getting this: C:\/Program Files (x86)\/Git\/bin\

Not sure if that was intended, but like I say, it still seems to work for me anyway.

dvide commented Nov 30, 2011

@mhart, Tested bcf1dfa on Git Bash, normal cmd.exe, and Cygwin. Seems to work fine on all three. I did a git flow init, created a feature branch, merged it back to develop branch. No problems.

Though if I edit git-flow to echo $GITFLOW_DIR, I'm getting this: C:\/Program Files (x86)\/Git\/bin\

Not sure if that was intended, but like I say, it still seems to work for me anyway.

@nvie

This comment has been minimized.

Show comment
Hide comment
@nvie

nvie Nov 30, 2011

Owner

If we can safely find a way that works on all platforms, I'm fine to avoid the OS-detection and just always replace backslashes as Unix users are unlikely to run into troubles with this, and at the same time we're helping out all Windows users.

Please take a look a my proposal here: develop...feature/convert-slashes

Will this do the trick for all of you? If it works, it has the best of a few advantages:

  • It solves the current slash issues on Windows
  • It does not bother our Unix users (likely)
  • It does not depend on OS-detection
  • It does not assume any non-sh shell (like bash, zsh, etc.)
  • It's a single line of code
Owner

nvie commented Nov 30, 2011

If we can safely find a way that works on all platforms, I'm fine to avoid the OS-detection and just always replace backslashes as Unix users are unlikely to run into troubles with this, and at the same time we're helping out all Windows users.

Please take a look a my proposal here: develop...feature/convert-slashes

Will this do the trick for all of you? If it works, it has the best of a few advantages:

  • It solves the current slash issues on Windows
  • It does not bother our Unix users (likely)
  • It does not depend on OS-detection
  • It does not assume any non-sh shell (like bash, zsh, etc.)
  • It's a single line of code
@dvide

This comment has been minimized.

Show comment
Hide comment
@dvide

dvide Nov 30, 2011

Hi @nvie, your proposal 4b9545e5 doesn't seem to work compared to 3a52112.

Jack@BAKLAVA ~/projects/test
$ git flow init -d
dirname: too many arguments
Try `dirname --help' for more information.

C:\Program Files (x86)\Git\bin\git-flow: line 70: /gitflow-common: No such file or directory
C:\Program Files (x86)\Git\bin\git-flow: line 78: /gitflow-shFlags: No such file or directory

3a52112 produces the appropriately transformed path: C:/Program Files (x86)/Git/bin

It must be the space in the path the produces two arguments to dirname. That's why the quote marks are where they are in 3a52112.

dvide commented Nov 30, 2011

Hi @nvie, your proposal 4b9545e5 doesn't seem to work compared to 3a52112.

Jack@BAKLAVA ~/projects/test
$ git flow init -d
dirname: too many arguments
Try `dirname --help' for more information.

C:\Program Files (x86)\Git\bin\git-flow: line 70: /gitflow-common: No such file or directory
C:\Program Files (x86)\Git\bin\git-flow: line 78: /gitflow-shFlags: No such file or directory

3a52112 produces the appropriately transformed path: C:/Program Files (x86)/Git/bin

It must be the space in the path the produces two arguments to dirname. That's why the quote marks are where they are in 3a52112.

@nvie

This comment has been minimized.

Show comment
Hide comment
@nvie

nvie Dec 1, 2011

Owner

Hi @dvide, I've updated my patch. Can you test it again?

Owner

nvie commented Dec 1, 2011

Hi @dvide, I've updated my patch. Can you test it again?

@dvide

This comment has been minimized.

Show comment
Hide comment
@dvide

dvide Dec 1, 2011

@nvie That works fine yeah. Tested it again on all three.

dvide commented Dec 1, 2011

@nvie That works fine yeah. Tested it again on all three.

@nvie

This comment has been minimized.

Show comment
Hide comment
@nvie

nvie Dec 1, 2011

Owner

OK, then I'm closing this issue and go for the proposed solution. Thanks for your patience, all ;)

Owner

nvie commented Dec 1, 2011

OK, then I'm closing this issue and go for the proposed solution. Thanks for your patience, all ;)

@nvie nvie closed this Dec 1, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment