-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
Enhancements, improvements... #15
base: master
Are you sure you want to change the base?
Conversation
Fix an error with SSL in some instances causing failures. Add methods to determine if there are local or remote changes only, in addition to checking for all changes. Add method to get the current revision of local git. Add method to read commit changes between local and remote. Update iGit documentation. Change fetch, run methods to return command output, versus throwing an error.
Merge piping to sdout.
Thanks for PR. I will review it during weekend, I don't have much time at the moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I added some comments, can you look at them? Thanks.
@@ -25,6 +25,8 @@ public function __construct($repository) | |||
if(basename($repository) === '.git') | |||
{ | |||
$repository = dirname($repository); | |||
// Fix ssl errors. | |||
exec('git config --global http.sslVerify false'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bad practise, SSL verify is usefull. Right way is fix SSL issue on server. Alternatively you can extends class GitRepository
and change constructor in your code base:
class OwnGitRepository extends \Cz\Git\GitRepository
{
public function __construct($repository) {
parent::__construct($repository);
exec('git config --global http.sslVerify false');
}
}
@@ -360,19 +362,92 @@ public function hasChanges() | |||
$this->begin(); | |||
$lastLine = exec('git status'); | |||
$this->end(); | |||
return (strpos($lastLine, 'nothing to commit')) === FALSE; // FALSE => changes | |||
return ((strpos($lastLine, 'nothing to commit')) || (strpos($lastLine, 'branch is behind'))) === FALSE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is BC break. Method hasChanges()
means "has changes to commit" (I will update phpdoc).
What is your use-case? In what case is last line "branch is behind"? Can you give a example? Probably better will be new method, something like your suggested hasRemoteChanges()
or isRemoteSynced()
.
$this->end(); | ||
return $commits; | ||
} | ||
public function getRev() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better is getCurrentRevision()
? It's consistent with getCurrentBranch()
.
$this->begin(); | ||
$result = $this->run("git pull $remote", $params); | ||
$this->end(); | ||
return $result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's BC break. Why you need git pull
output?
$this->begin(); | ||
$result = $this->run("git fetch $remote", $params); | ||
$this->end(); | ||
return $result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's BC break. Why you need git fetch
output?
$shortHead = exec($command.'"%h"'); | ||
$subject = exec($command.'"%s"'); | ||
exec($command.'"%b"',$body); | ||
$body = implode('<br>',$body); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use \n
instead of <br>
.
$author = exec($command.'"%aN"'); | ||
$date = exec($command.'"%aD"'); | ||
$commit = [ | ||
'head'=>$head, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe hash
is better than head
.
$date = exec($command.'"%aD"'); | ||
$commit = [ | ||
'head'=>$head, | ||
'shortHead'=>$shortHead, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is shortHead
really needed? It can be obtained from hash
.
$this->begin(); | ||
$commits = []; | ||
if (!is_numeric($limit)) { | ||
$command = "git log $limit..$branch --oneline"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Values of $limit
and $branch
must be escaped - use self::processCommand()
and maybe add ' 2>&1'
(it redirects error output to STDOUT).
'subject'=>$subject, | ||
'body'=>$body, | ||
'author'=>$author, | ||
'date'=>$date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think better is date in format like 2013-08-18T15:34:40+00:00
(--date=iso-strict
) or 2013-09-12 09:43:52 +0000
(--date=iso
) than default Sun Aug 18 14:49:42 2013 +0000
.
$commits = []; | ||
if (!is_numeric($limit)) { | ||
$command = "git log $limit..$branch --oneline"; | ||
exec($command,$shorts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if exec
fails (for example if git
is not installed)?
No description provided.