Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't just prepend the git path. #994

Closed
rakete opened this issue Jun 12, 2016 · 5 comments
Closed

Don't just prepend the git path. #994

rakete opened this issue Jun 12, 2016 · 5 comments

Comments

@rakete
Copy link

rakete commented Jun 12, 2016

In cmder/vendor/init.bat, you first check if %ProgramFiles%\Git exists, and then prepend its /bin, /usr/bin and /usr/share/vim/vim74 to PATH. This approach has two downsides:

  1. Git explicitly asks the user if he wants to include its /bin and /usr/bin in PATH when it is installed, it does so because including the path shadows Windows own find.exe and makes it unusable. I think it even defaults to not including those paths and only includes %ProgramFiles%\Git\cmd.
  2. Users may order their path so that the executables in %ProgramFiles%\Git\usr\bin are shadowed by other executables on purpose, e.g. by something like https://github.com/bmatzelle/gow. Well, at least I do that, and prepending those directories then creates an issue where I can't launch the vim I have installed under %ProgramFiles%\Vim, but only the one supplied by Git.

Personally I've just worked around my issue by commenting out the line that prepends to PATH. But I think it would be better if you'd only prepend %ProgramFiles%\Git\cmd, if anything at all. /bin, /usr/bin and /vim74 should be included by the user in his PATH when he wants them and not inserted by his terminal emulator.

Maybe there is some easy check to test if git.exe is already in PATH? Then you could check that and then only prepend %ProgramFiles%\Git\cmd if really necessary, instead of the whole git environment.

@jankatins
Copy link
Contributor

jankatins commented Jun 12, 2016

Just FYI: the development version now appends the additional commands https://github.com/cmderdev/cmder/blob/development/vendor/init.bat#L46-L93 The additional commands are still included but e.g. find is not shadowed.

@Stanzilla
Copy link
Member

^ this. Does that fix all your complaints, @rakete?

@rakete
Copy link
Author

rakete commented Jun 13, 2016

Yes, appending the path would probably solve my problem.

Although personally I would still say that it would be better to try to minimize any kind of modification of the users environment. There will always be the assumption on the users side that whatever is his PATH outside of cmder, will be the PATH inside of cmder.

There is also a maximum length of PATH that you might run into with this approach (varies between Windows versions, Windows 10 seems to finally address this problem), see: http://stackoverflow.com/questions/1880321/why-does-the-260-character-path-length-limit-exist-in-windows

@jankatins
Copy link
Contributor

@rakete: At least I came to cmder with the opposite assumption: finally getting proper unix commands :-)

@DamnedScholar
Copy link

There will always be the assumption on the users side that whatever is his PATH outside of cmder, will be the PATH inside of cmder.

If this assumption exists, it should probably be fixed with clearer communication, maybe via documentation or an on-install or on-first-run message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants