robwierzbowski / grunt-build-control

Version control your built code.
MIT License
380 stars 36 forks source link

Don't push before adding. #17

Closed beilharz closed 10 years ago

beilharz commented 10 years ago

I use buildcontrol for heroku, which sometimes rejects a push (i.e. missing package.json etc) In this situation I have to manually add and commit the fix in git, because the task will already fail in initBranch().

https://github.com/robwierzbowski/grunt-build-control/blob/master/tasks/build_control.js#L126

beilharz commented 10 years ago

Thinking about this again, i guess it happens for every initial run of the task (i.e. branch doesn't exist anywhere => empty branch is created and committed => heroku rejects empty push => branch exists locally and push will always be rejected and gitCommit() is never called. This seems strange, i must be overlooking something.

robwierzbowski commented 10 years ago

This is a good bug, thanks for reporting.

If I understand, Heroku doesn't like the commit the plugin creates here: https://github.com/robwierzbowski/grunt-build-control/blob/master/tasks/build_control.js#L147-L148. Is that correct?

We need at least one commit to do the fancy local branch checkouts that don't affect the working directory. What's really happening is we're moving the HEAD of the branch to reference the current working directory without changing the working directory's files. Without at least one commit we don't have a branch HEAD to move. The pushing and fetching is necessary to avoid the remote from getting out of sync. I'd have to look closer to remember why we set up each part of the workflow but it works for all of the edge cases we ran into when initially testing.

We can't change this behavior, but maybe we can fake a non-empty commit if the branch doesn't exist, then revert it with the first real commit.

This SO answer leads me to believe Heroku can accept empty commits though: http://stackoverflow.com/a/12269801/530653. Can you explain what's causing the error a little more?

beilharz commented 10 years ago

Hi,

My real problem was that i accidentally deleted the package.json and then couldn't deploy even after restoring it. Thats where L126 got me.

I thought about the empty commit in #L147-L148 only later and I guess there was confusion in my head about being empty = 'no files' vs. 'no changes'. No changes is no problem, no files of course is:gist The initial commit in L147 is always empty (as in 'no files') as far as i can see. Thats why the whole task won't work for me even for a correct directory: gist

The cleanest solution would be probably to defer the pushing when the branch dosn't exist remotely till after the first git add . ?

robwierzbowski commented 10 years ago

The commit created on 147-148 will always be a no-files, no changes commit on an empty branch. The push happens so we can set up tracking for that remote branch, which I believe is because later status calls need a remote tracking branch to work.

I think we should be able to fix the issue, either by deferring pushing and tracking in that one case, or by ensuring that the first commit contains at least one file.

patelsan commented 10 years ago

Bump... just spent couple hours struggling with the same issue.

robwierzbowski commented 10 years ago

A quick workaround would be to push anything to your heroku branch manually first, before using build control. I'll see if I can fix this this week.

On Monday, January 27, 2014, beilharz notifications@github.com wrote:

Reopened #17https://github.com/robwierzbowski/grunt-build-control/issues/17 .

— Reply to this email directly or view it on GitHubhttps://github.com/robwierzbowski/grunt-build-control/issues/17 .

Rob Wierzbowski @robwierzbowski http://twitter.com/#!/robwierzbowski http://github.com/robwierzbowski http://robwierzbowski.com

robwierzbowski commented 10 years ago

Please test the current master and let me know how it works for you. The task should only do git actions when absolutely necessary now. Did some manual tests, but could use some collaboration since the library doesn't have test coverage yet (#19).