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

Fix a bug on windows #156

Merged
merged 7 commits into from Jul 18, 2018

Conversation

Projects
None yet
2 participants
@wi1dcard
Copy link

wi1dcard commented Jul 3, 2018

On windows, git binary paths are usually at: "C:\Program Files...".

When I use:

GitWorkingCopy::getStatus();

It cause an error:
image

I found getStatus() use GitWrapper::git to run a raw command, so I changed it to use GitWorkingCopy::run firstly, but GitWrapper::git is still not working if git binary path including spaces.

I reviewed the code of command line formating, found a bug of concat gitBinary and gitCommandLine, and it's now fixed.

My English is not professional, but I'm trying my best. Thanks.

@TomasVotruba

This comment has been minimized.

Copy link
Collaborator

TomasVotruba commented Jul 4, 2018

Hey, thanks for the PR.

I'll look on it more deeply, but first please fix the tests: https://travis-ci.org/cpliakas/git-wrapper/jobs/399933252#L625

wi1dcard added some commits Jul 4, 2018

@wi1dcard

This comment has been minimized.

Copy link

wi1dcard commented Jul 4, 2018

All fixed. Thanks.

Please review.

@TomasVotruba

This comment has been minimized.

Copy link
Collaborator

TomasVotruba commented Jul 4, 2018

The PR still fails

@wi1dcard

This comment has been minimized.

Copy link

wi1dcard commented Jul 4, 2018

Sorry, I have no idea why coveralls coverage decreased. Combining ifs and removing an unused var was the only thing I did at the last commit.

image

@TomasVotruba

This comment has been minimized.

Copy link
Collaborator

TomasVotruba commented Jul 4, 2018

Oh, I see. I thought the Travis fails, I'll check it

}
if ($cwd === null && ($directory = $gitCommand->getDirectory()) !== null &&
! $cwd = realpath($directory)) {
throw new GitException('Path to working directory could not be resolved: ' . $directory);

This comment has been minimized.

@TomasVotruba

TomasVotruba Jul 4, 2018

Collaborator

I guess complexity sniff is behind this change, right?

If so, please decouple this to own method. It would be more readable than this super long condition

// Support for executing an arbitrary git command.
if (is_string($gitCommandLine)) {
$commandLine = implode(' ', $commandLine);
$commandLine = $gitCommand->getCommandLine();

This comment has been minimized.

@TomasVotruba

TomasVotruba Jul 4, 2018

Collaborator

Why removing the "git" prefix?
The varible naming is incosistent now

@wi1dcard

This comment has been minimized.

Copy link

wi1dcard commented Jul 4, 2018

See:

It caused a code-standard warning, so I try to remove an variable and combine a nested-if condition. Should I ignore it and revert to this commit?

Thanks for helping :D

@TomasVotruba

This comment has been minimized.

Copy link
Collaborator

TomasVotruba commented Jul 4, 2018

Nono, just decouple this to own method as I wrote :) that will do

@wi1dcard

This comment has been minimized.

Copy link

wi1dcard commented Jul 4, 2018

I'm not good at Unit Test / CI. Thanks for your correction! It's all passed finnnnnally. 👍

@wi1dcard

This comment has been minimized.

Copy link

wi1dcard commented Jul 18, 2018

@TomasVotruba Would you please merge this request?

{
if ($cwd === null) {
$directory = $gitCommand->getDirectory();
if ($directory !== null) {

This comment has been minimized.

@TomasVotruba

TomasVotruba Jul 18, 2018

Collaborator

Great job!

Here you can use early return

if ($cwd) {
   return $cwd;
}

$directory = $gitCommand->getDirectory();
if ($directory === null) {
    return $cwd;
}

// etc
@TomasVotruba

This comment has been minimized.

Copy link
Collaborator

TomasVotruba commented Jul 18, 2018

I'm sorry. There was one comment that was not sent (GitHub review double sending).

After that I'll merge it

@TomasVotruba

This comment has been minimized.

Copy link
Collaborator

TomasVotruba commented Jul 18, 2018

Thank you 👍

@TomasVotruba TomasVotruba merged commit 0ea6f3c into cpliakas:master Jul 18, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage decreased (-0.2%) to 91.233%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment