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

1.3.16-pre: 1.3.15 Fixes #2339

Merged
merged 19 commits into from
Jul 29, 2020
Merged

Conversation

daxgames
Copy link
Member

@daxgames daxgames commented Jun 27, 2020

Fixes

Adds

@daxgames daxgames changed the title fix vscode terminal errors when setting cmder fast_init=1 outside cmder 1.3.16-pre: 1.3.15 Fixes Jun 29, 2020
Copy link

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sooo, the vendor/init.bat in the cmder_mini.zip artifact is not using the version from this PR for some reason 🤔
I updated it manually however, and the error is gone.

I do have one review comment though.

vendor/init.bat Outdated
Comment on lines 204 to 211
if not defined GIT_INSTALL_ROOT (
:: get the absolute path to the user provided git binary
%lib_git% is_git_shim "%%~dpF"
%lib_git% get_user_git_version
%lib_git% compare_git_versions

if defined GIT_INSTALL_ROOT (
goto :FOUND_GIT
goto :FOUND_GIT
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the script always go to FOUND_GIT which does not seem intended, e.g. %lib_git% compare_git_versions does not define GIT_INSTALL_ROOT when the checked git is older than the vendored git.

So this should probably be checked at the end of each iteration instead:

Suggested change
if not defined GIT_INSTALL_ROOT (
:: get the absolute path to the user provided git binary
%lib_git% is_git_shim "%%~dpF"
%lib_git% get_user_git_version
%lib_git% compare_git_versions
if defined GIT_INSTALL_ROOT (
goto :FOUND_GIT
goto :FOUND_GIT
)
:: get the absolute path to the user provided git binary
%lib_git% is_git_shim "%%~dpF"
%lib_git% get_user_git_version
%lib_git% compare_git_versions
if defined GIT_INSTALL_ROOT (
goto :FOUND_GIT
)

@daxgames
Copy link
Member Author

daxgames commented Jul 1, 2020

@jack1142 you are correct. Fixed

@daxgames
Copy link
Member Author

daxgames commented Jul 1, 2020

Copy link

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me and code looks fine (with the low knowledge of batch I have at least) 👍
I don't think the if not defined GIT_INSTALL_ROOT is needed but it's not gonna break anything so it's just nitpicking :P

@daxgames
Copy link
Member Author

daxgames commented Jul 1, 2020

I need to look at one other thing and when I do I will merge this in.

vendor/init.bat Outdated
:: See vendor\git-for-windows\README.portable for why we do this
:: A
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@daxgames daxgames merged commit 906bb5d into cmderdev:master Jul 29, 2020
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

Successfully merging this pull request may close these issues.

3 participants