search

Glavin001 / GitLab-Pages

:eyes: GitHub Pages, for GitLab.
MIT License
203 stars 30 forks source link

Does not checkout the `afterCommit` in the cloned repo #9

Closed octomike closed 9 years ago

octomike commented 9 years ago

Without this addition we are only seeing master checked out. Are we doing it wrong or was that bug?

Glavin001 commented 9 years ago

It uses the after commit from the webhook payload from GitLab, and then checks out that commit at https://github.com/Glavin001/GitLab-Pages/blob/master/routes/webhooks.js#L57 It should only apply when the test passes for if the deployBranch is the branch that has been pushed, see https://github.com/Glavin001/GitLab-Pages/blob/master/routes/webhooks.js#L22-L27

tl;dr

So really it does not even checkout the branch, it checks out the commit that was most recently pushed, and only if that commit lies within that deployBranch.

I'll take a look later this week when I get a chance and see if it's a true bug. Could you detail some steps I could use to reproduce? For instance, enable repo for GitLab Pages, set deployBranch to gl-pages, push branch master, notice that it does still change. This would indicate there's a bug in the check at https://github.com/Glavin001/GitLab-Pages/blob/master/routes/webhooks.js#L22-L27

octomike commented 9 years ago

Thanks for the explanation! But unfortunately it still does not work here. Is repo.getCommit() implicitly checking out that commit in the tmp folder? Because that's not happening in our installation.

Glavin001 commented 9 years ago

Yes, it should be checking that commit out in the clone, which is in the tmp directory.

Could you:

octomike commented 9 years ago

Manually checking it out in .tmp/ works fine. How do I debug that repo.getCommit(afterCommit)[https://github.com/Glavin001/GitLab-Pages/blob/master/routes/webhooks.js#L57] call further?

Glavin001 commented 9 years ago

It looks like a NodeGit bug. I've found others, too. See https://github.com/nodegit/nodegit/issues/341#issuecomment-71384969

They recommend that you discuss with them on their Gitter channel. Let me know if you're able to resolve this and we can fix this for everyone. Thanks!

octomike commented 9 years ago

As suspected.

Hi @octomike! getCommit(sha) simply returns information about a commit in the repo, it doesn’t affect he working directory. ( https://gitter.im/nodegit/nodegit )

Glavin001 commented 9 years ago

Well, that explains a lot.. haha. Thanks for looking into that!

Glavin001 commented 9 years ago

Use Checkout.tree, see http://www.nodegit.org/#Checkout/function/tree

Thanks to @orderedlist

octomike commented 9 years ago

Meh, the updated pull request doesn't work. Ignore that please :)

Glavin001 commented 9 years ago

Does it not checkout the commit still? Bummer. If it's not that then what NodeGit API call?

octomike commented 9 years ago

Tested, works fine. Once they actually implemented what's in the API, you can use the line in the comment. :smile_cat:

Glavin001 commented 9 years ago

So Checkout.tree is not implemented in NodeGit API yet? I thought that's what was recommended to use.. :confused:

octomike commented 9 years ago

Yes, I just asked in their gitter channel.

Glavin001 commented 9 years ago

Wow, it's unfortunate that NodeGit is so unusable in it's current state. I may even investigate using exec or spawn as a more viable solution for the time being.

acao commented 9 years ago

Probably because js-git has become so popular! This is a node wrapper for it.

https://www.npmjs.com/package/git-node

Glavin001 commented 9 years ago

Great! Then it looks like switching to use jsgit is a good idea.

Glavin001 commented 9 years ago

Closing this and moving to #11. I am open to reviewing Pull Requests for #11. Thanks!

Glavin001 commented 9 years ago

I think git checkout tree is implemented now! See https://github.com/nodegit/nodegit/pull/402 and http://www.nodegit.org/api/checkout/#tree

Glavin001 commented 9 years ago

I recommend updating GitLab Pages and this should now work. Let me know if there are any other problems. Thanks!