-
Notifications
You must be signed in to change notification settings - Fork 438
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
Pre push testsuites #258
Pre push testsuites #258
Conversation
resources/hooks/local/pre-push
Outdated
COMMIT_MSG_FILE=$1 | ||
|
||
# Fetch the GIT diff and format it as command input: | ||
DIFF=$(git diff -r -p -m -M --full-index --staged | cat) |
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 will mostly result in an empty set of files since on push the staged changes are mostly empty of should not be checked since those aren't the files that you're committing. We'll need to find a way to check which files are transfered to the remote.
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.
git diff --name-only origin/master..HEAD
this show all the file changed in the commit not yet pushed. The problem is that require to specify the branch.
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.
Ok I found the right one GIT_CURRENT_BRANCH=$(git name-rev --name-only HEAD) && git diff --name-only origin/$GIT_CURRENT_BRANCH..HEAD --oneline
in that way get the actual branch
src/GrumPHP/Console/Application.php
Outdated
@@ -116,6 +116,10 @@ protected function getDefaultCommands() | |||
$container->get('config'), | |||
$container->get('locator.changed_files') | |||
); | |||
$commands[] = new Command\Git\PrePushCommand( | |||
$container->get('config'), | |||
$container->get('locator.changed_files') |
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.
Also here we'll need the files that are being pushed to the remote instead of the staged files.
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.
for this one I have no idea what to do XD
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.
You could make a new 'locator.pushed_files' locator?
$files = $this->getCommittedFiles($io); | ||
|
||
$context = new TaskRunnerContext( | ||
new GitPreCommitContext($files), |
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.
Here it's best to create a separate context.
Thanks for looking into it! I think it should run on both pre-commit and pre-flush. If you want to disable one of those, you'll have to create custom testsuites in the grumphp.yml file so that the codecept does not run during pre-commit. I don't think there is an easier way to do this at the moment. Note that there is also a vagrant git hook preset. |
Ok so:
For the other feedback I will look it on next because actually is working and I don't know all the code of grumphp. |
Not only one commit: all the files (in possibly multiple commits) that are being pushed. A possibile solution is indeed to create a new About the testsuites: the documentation can be found here: https://github.com/phpro/grumphp/blob/master/doc/testsuites.md#overriding-git-hook-test-suites |
Ok so for the testsuits it's like in the example of my first comment so the idea is to have something like this:
In that way in tasks we define the task and in the hook specify when execute them. |
Uhm as I can see codeception it is not executed on pre-push the |
Ok the problem is that the content don't have the files , I think that the problem is the other issue mentioned above |
Gitlib don't have a method to get the list of file waiting for a push, so I used git, the problem is how to get the context on ChangeFiles.php for |
Hi @Mte90, |
I fixed few errors, but few of them are based of my no-complete code on |
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.
Hello @Mte90,
I've taken some time to review the changes and I must say: it is looking promising!
Can you also make sure that all the code you added has tested phpspec scenarios?
Currently the pre-push context is applied to a lot of tasks. This means that by default, GrumPHP runs all these tasks both on commit and on push.
In many occasions you want to commit and next push a commit. This means that the same code will be tested twice in a short amount of time. Is this something we would want to do?
Another possibility is to make this hook configurable so that it only runs in pre-push when it is configured to do so. Does that make sense to you?
resources/hooks/local/pre-push
Outdated
GIT_CURRENT_BRANCH=$(git name-rev --name-only HEAD) | ||
|
||
# Fetch the GIT diff and format it as command input: | ||
DIFF=git diff --name-only origin/"$GIT_CURRENT_BRANCH"..HEAD --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.
What if the remote branch does not exist yet?
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.
good point, the problem that we need a point for to the difference.
probably it's better to find another command
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.
ok this one is blocking, after find a better way i will fix that, the vagrant one and also the code inside the changedfiles.php
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.
ok with git rev-parse HEAD
it's possible to get the latest commit in queue but I don't find a way to get the latest commit pushed.
The problem is that all the command require to have a remote reference so in any case that hook will not work without a first commit online.
resources/hooks/local/pre-push
Outdated
@@ -0,0 +1,18 @@ | |||
#!/bin/sh | |||
|
|||
GIT_CURRENT_BRANCH=$(git name-rev --name-only 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 it's better to compare against a remote commit hash instead of a name?
resources/hooks/local/pre-push
Outdated
# Run GrumPHP | ||
(cd "./" && printf "%s\n" "${DIFF}" | exec 'vendor/phpro/grumphp/bin/grumphp' 'git:pre-push') | ||
|
||
# Validate exit code of above command |
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.
The exit checks can be removed. The shell script will return the exit code of the last command executed.
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.
so line 9?
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.
The script can end with the grumphp command. There is no need for the $RC check.
@@ -0,0 +1,18 @@ | |||
#!/bin/sh |
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.
Can you also add a hook for the vagrant preset?
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 was thinking to committed the file but seems not, okay I will do asap.
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.
done.
src/GrumPHP/Console/Application.php
Outdated
@@ -116,6 +116,10 @@ protected function getDefaultCommands() | |||
$container->get('config'), | |||
$container->get('locator.changed_files') | |||
); | |||
$commands[] = new Command\Git\PrePushCommand( | |||
$container->get('config'), | |||
$container->get('locator.changed_files') |
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.
You could make a new 'locator.pushed_files' locator?
/** | ||
* @return FilesCollection | ||
*/ | ||
protected function getCommittedFiles(ConsoleIO $io) |
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.
You could change the name to getPushedFiles()
src/GrumPHP/Locator/ChangedFiles.php
Outdated
@@ -41,8 +42,20 @@ public function __construct(Repository $repository, Filesystem $filesystem) | |||
public function locateFromGitRepository() |
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.
Ad mentioned above, you should create a new pushed files locator
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.
created.
src/GrumPHP/Locator/ChangedFiles.php
Outdated
@@ -41,8 +42,20 @@ public function __construct(Repository $repository, Filesystem $filesystem) | |||
public function locateFromGitRepository() | |||
{ | |||
$diff = $this->repository->getWorkingCopy()->getDiffStaged(); | |||
// if($context instanceof GitPrePushContext ) { | |||
$actualbranch = trim(shell_exec('git name-rev --name-only 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.
Try to avoid shell_exec. This is untestable code.
You can use the gitonomy library to run git commands through the ProcessBuilder:
\Gitonomy\Git\Repository::run()
src/GrumPHP/Locator/ChangedFiles.php
Outdated
@@ -41,8 +42,20 @@ public function __construct(Repository $repository, Filesystem $filesystem) | |||
public function locateFromGitRepository() | |||
{ | |||
$diff = $this->repository->getWorkingCopy()->getDiffStaged(); | |||
// if($context instanceof GitPrePushContext ) { | |||
$actualbranch = trim(shell_exec('git name-rev --name-only HEAD')); | |||
$diff = explode("\n", trim(shell_exec('git diff --name-only origin/' . $actualbranch . '..HEAD --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.
As mentioned above: what if the remote branch does not exist? Maybe it's better to use a commit hash instead of the parsed branch name.
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.
Does this code also skips deleted files?
use GrumPHP\Collection\FilesCollection; | ||
|
||
/** | ||
* Class GitPrePushontext |
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.
You can remove this type of class docblocks.
I've taken a look at the pre-push examples from git. It seems like you can access the committed files from STDIN: #!/bin/sh
# An example hook script to verify what is about to be pushed. Called by "git
# push" after it has checked the remote status, but before anything has been
# pushed. If this script exits with a non-zero status nothing will be pushed.
#
# This hook is called with the following parameters:
#
# $1 -- Name of the remote to which the push is being done
# $2 -- URL to which the push is being done
#
# If pushing without using a named remote those arguments will be equal.
#
# Information about the commits which are being pushed is supplied as lines to
# the standard input in the form:
#
# <local ref> <local sha1> <remote ref> <remote sha1>
#
# This sample shows how to prevent push of commits where the log message starts
# with "WIP" (work in progress).
remote="$1"
url="$2"
z40=0000000000000000000000000000000000000000
while read local_ref local_sha remote_ref remote_sha
do
if [ "$local_sha" = $z40 ]
then
# Handle delete
:
else
if [ "$remote_sha" = $z40 ]
then
# New branch, examine all commits
range="$local_sha"
else
# Update to existing branch, examine new commits
range="$remote_sha..$local_sha"
fi
# Check for WIP commit
commit=`git rev-list -n 1 --grep '^WIP' "$range"`
if [ -n "$commit" ]
then
echo >&2 "Found WIP commit in $local_ref, not pushing"
exit 1
fi
fi
done
exit 0 So there are 2 things we can do:
|
In any case that script use a remote for a comparison so we need to accept that is not possible to do a check without a remote. I didn't find a way to detect if the remote is empty or exist. |
As you can see in the script, it can handle new remotes and deleted branches. |
If remote is ahead it's not possible to do the push because there are difference between repos so the developer need to fix them before to push so for me it's not an our problem. |
I know it's not possible to push if the remote is ahead, but I was wondering if the pre-push hook will be executed before that check. So at this point, these are the questions that should be solved at the end of the PR:
Feel free to add additional questions to the list. |
All interesting question and I think that the easiest approach probably will be:
So I will start work on how to get in the hook, the remote of the push with the branch in that way we can do our checks and see for the next points. |
Ok I find it a way for git in the hook to get the remote and branch and do a check with that. Now the problem is the grumphp pre-push command, without specify what is the remote and the branch I think we can use the origin/master. Actually the php code miss the part to check if the user added that parameter ( |
resources/hooks/local/pre-push
Outdated
done | ||
|
||
# Clean exit: | ||
exit 0; |
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.
You will want to return the Grumphp exit code. 0 = always push
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.
right
resources/hooks/local/pre-push
Outdated
DIFF=$(git diff --name-only "$remote"/"$local_ref"..HEAD --oneline) | ||
|
||
# Run GrumPHP | ||
(cd "./" && printf "%s\n" "${DIFF}" | exec 'vendor/phpro/grumphp/bin/grumphp' 'git:pre-push') |
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 will run grumphp multiple times on multiple commits in a push.
You want to create ONE diff from ALL the changed commits and run grumphp only once with this diff.
You could also pass the parameters that you receive in this hook as parameters to the cli.
This way it is possible to add them to the context and use them in the tests. For example: You can create a task that does not allow you to push upstream.
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.
the diff contain all the files for the push so is not executed multiple times
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 line is inside a while which is executed for every line in the STDIN.
More info:
The pre-push hook runs during git push, after the remote refs have been updated but before any objects have been transferred. It receives the name and location of the remote as parameters, and a list of to-be-updated refs through stdin. You can use it to validate a set of ref updates before a push occurs (a non-zero exit code will abort the push).
while read local_ref local_sha remote_ref remote_sha
Every input line is a commit which is pushed to the remote. So when there are many commits in one push, the grumphp executable is executed multiple times.
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.
you are right -.-', I will fix it :-)
src/GrumPHP/Locator/ChangedFiles.php
Outdated
*/ | ||
public function locateFromGitPushedRepository() | ||
{ | ||
$diff = explode("\n", \Gitonomy\Git\Repository::run('diff origin/master..HEAD --name-only --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.
Mostly you aren't pushing to master. Most of my repositories dont even have a remote named origin :)
As you mentioned: this is something we can ask to the user. But maybe it's better to detect the defaults in the command?
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.
yep this code require improvements :-)
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.
there is a way to get the parameters in the cli?
because I was thinking that when someone do grumphp pre-push
we need the other two parameter, the remote and the branch.
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.
any suggestions for that?
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've found following page with some handy commands:
http://stackoverflow.com/questions/171550/find-out-which-remote-branch-a-local-branch-is-tracking
If your branch is tracking a remote, you can use following commands:
LOCAL_BRANCH=`git name-rev --name-only HEAD`
TRACKING_BRANCH=`git config branch.$LOCAL_BRANCH.merge`
TRACKING_REMOTE=`git config branch.$LOCAL_BRANCH.remote`
REMOTE_URL=`git config remote.$TRACKING_REMOTE.url`
If your branch is not tracking a remote, it is not possible to detect remote info.
In the git hook, you get these as parameters from git.
So a possible solution would be: if the branch is not tracking and you don't get the info from the git hook, the user gets an exception telling them to manually add the parameters to the grumphp command.
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.
for us is not enough because we need to know what is the remote branch, a repo can have different remote so I think that we needto ask to them only if use grumphp pre-push
because on hook we already know that info.
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.
If there is one registered remote, we might just use that one? On multiple, we won't know the remote. This PR is getting a bit complex :)
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.
If there is one registered remote, we might just use that one? On multiple, we won't know the remote. This PR is getting a bit complex :)
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.
ok, when someone execute the command I will autodetect :-)
We've moved from PSR-0 to PSR-4. Can you rebase your changes against current master? |
I can move to psr-4 but I have no idea how to check if the code where needs checks. |
The pr is complete, remain only tests (that I have already done for the hook and for the |
any suggestions for psr-4? there is a tool for that? |
Hi @Mte90, Sorry for the late response... |
rebase done without errors on git :-) |
done | ||
|
||
# Fetch the GIT diff and format it as command input: | ||
DIFF=$(git diff --name-only "$remote"/"$local_ref"..HEAD --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.
I have not looked at the full PR and comments, but I have some experience with pre-push commands and I have a few suggestions.
I think it will be better to pass the 4 variables local_ref
, local_sha
, remote_ref
and remote_sha
to PHP instead of doing a diff and passing this data. Let's handle the logic on the PHP side. Passing the diff makes it impossible to retrieve basic information about the current state of the branch on the PHP end which is possibly needed for certain tasks to run on pre-push. Some examples:
- If
local_ref
contains the string(delete)
we are currently deleting the current branch. This will mean there is still a diff but we don't need to do any coding standards checks because the branch is being deleted. - If
local_sha
contains the SHA0000000000000000000000000000000000000000
then we do not have a local branch, meaning we are in a detached HEAD state. - If
remote_sha
is0000000000000000000000000000000000000000
then this is a brand new branch. There will not be a diff. We can use this information to perform a full coding standards check.
Also, this git diff
command is not bullet proof, it assumes that the developer is pushing their currently checked out branch, but this is not necessarily the case. For example, if you have switched to a new branch and realized you have not yet pushed your previous branch, you can do this command:
$ git push my-previous-branch:my-previous-branch
In this case the diff will be made between the HEAD of the current branch and the remote of the previous branch, this has not the intended result. A better version would be:
$ git diff-tree --no-commit-id --name-only -r '$local_sha' '$remote_sha'
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.
Grumphp internally base everything on the git diff
that parse to find all the files involved in the commit/push so I dont' think that actually is the best choice.
My hope is to have that pr integrated and start to work on this edge-cases that during my development doesn't happened.
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 believe up to now Grumphp works on pre-commit only? In that case the diff is fine, since you won't be deleting branches, and the status of the local and remote branches are not important.
But when you do a pre-push then this becomes important and acting solely on a diff is not going to be enough.
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.
ok so how suggest to change the script for the hook? The output is the same like actually?
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.
We need the information about the remote and local branches and this needs to be passed to the tasks as well, so I would suggest to:
- Instead of piping the list of files into the command, pass in the 4 parameters to the
GitPrePushCommand
as options. - Store the 4 parameters in the
GitPrePushContext
and provide getters for them so that this information is available to tasks:getLocalSHA1()
,getLocalRef()
,getRemoteSHA1()
,getRemoteSHA1()
. - Inside
GitPrePushContext::execute()
there are 3 different possibilities for generating the file list:- If the
remote_sha1
is empty (it equals0000000000000000000000000000000000000000
) then this means we are pushing a brand new branch. We have no comparison point since we don't know which of the ancestor commits have already been checked. In this case we need to check all files. - If the
local_sha1
is empty or iflocal_ref
matches the string(delete)
then this means that the push is intended to delete the remote branch. In this case we should not populate the file list at all, since it is pointless to do any checks on a branch that is about to be deleted. - When both
local_sha1
andremote_sha1
are not empty, andlocal_ref
does not match(delete)
we are doing a push that updates an already existing remote branch with new code. In this case we can generate a list of files that have changed between thelocal_sha1
andremote_sha1
and can pass it to the context.
- If the
To generate the diffs on the PHP side, you can inject the Repository
dependency into the PrePushCommand
and then get the file list by calling $this->repository->run('diff-tree', ['--no-commit-id, ...])
.
It would also be nice to have some helper methods on the context such as isNewBranch()
, hasRemoteBranch()
, isDeletingRemote()
.
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.
Uhm means a rewrite of the local pr and I have no time for it, but I am open for a pr on mine to speed the integration of this feature.
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 it's a better idea to create a new issue in which we can discuss what the pre-push should look like before we implement it. That way we can try to create a good architecture that fits every use-case before implementing 15 different versions?
This PR contains a lot of great findings and was good to determine the scope of the pre-push, but has become a bit messy due to the new issues we've detected during the development of this feature.
What do you think about this idea?
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 create a new branch of grumphp where work on this feature can be an idea with a roadmap, starting from a refactoring of this pr.
Any chance for this feature? |
Hi @Mte90, As mentioned above: there are some edge cases which need to be covered first. Some parts of this PR need a rewrite as well. So in the current state, I am afraid I can't merge this one in. |
Very sad for it but I understand the problem but it is possible to give it more priority on this? |
If you need this urgently and have the time for it you can have a look at implementing the missing functionality that I outlined in my analysis that I posted above in my comment from April 3. |
Unlucky I have no time anymore to work on this feature but I need it a lot. |
It is an experiment to get the #252 working, actually there is the problem that if we have a grumphp.yml like that:
It's working but codeception is executed also on commit, and I have no idea how to avoid that.
New Task Checklist: