Collaborator guide: force pushing

[jbergstroem]

There's been a few questions about force pushing. We should talk it out and add guidelines to our collaborator guide. To be honest, I think we should disallow it unless the tree is in a serious mess. It's error prone, often made in haste and since the git userbase of io.js is growing some people will get the sour apple. One approach to lessen the amount of mistakes is to have a pre-commit hook. Initially, we should focus on the commit message. Lengths, typos, verify email/user of reviewed-by. Perhaps even PR.

Until that happens; how about (this only applies to public branches): You're only allowed to force push within 10 minutes of your commit unless someone else has committed ahead of you.

[bnoordhuis]

how about (this only applies to public branches): You're only allowed to force push within 10 minutes of your commit unless someone else has committed ahead of you.

We used to have an unofficial '5 minutes' rule so that sounds fine to me.

[tellnes]

I agree with that the commit you want to amend must be at the top of the branch your are force pushing to, but I think the window could be bigger than 10 minutes. If you need to force push and first sees that after 10 minutes, I think it should be ok to do so as long as you are within the other rules.

Also, the guidelines should includes some notes about being explicit and git configuration. I have done some harm in the past by running git push -f with the intention of only force pushing the current branch, but force pushed all remote tracking branches.

[jbergstroem]

@tellnes If someone else pulls within the scope between your first and second (-f) push, you will be giving them a bad day -- and even worse: a inconsistent repo they are adding their changes to. Imagine someone force pushing while someone is doing a release, which is then tagged (we don't do git tag -a just yet afaik) and pushed.

Regarding just pushing to current branch -- that's a good point. I think the default changed in from pushing all to just one in git 2.0 which means you will have to enable this manually. Adding a lowest git version for collaborators would probably be a good idea as well.

Edit: This is why always working in branches is such a good practise. It gives you the freedom to treat it as your own, commits, squashes and force pushes at your service. We should just have tooling in place that lessens the amount of scenarios for forced pushes.

[tellnes]

Yeah, I now I will be giving them a bad day. But that holds true even if you force push after 1 minute. There is still a window. I'm just saying something like 30-60 minutes. If we allows force pushes at all, we should document it in the Readme and document how to fix the repo if someone is pulling in the window (git pull --rebase).

If someone is doing a release and creates bad release commits, they should get an error when they tries to push those commits. That should stop someone from creating a bad release, but it still gives them a bad day and extra work. Maybe we should have a rule against force pushing while someone is doing a release? The collaborators are probably monitoring the release issue anyway.

I understand people probably wont agree with me on extending the window, but I'm just making my points.

Regarding just pushing to current branch: Yes, I believe it was changed in git 2.0 also.

[Fishrock123]

Maybe we should have a rule against force pushing while someone is doing a release?

Might be good, but the release won't work unless the (manually triggered) CI build afterwards pulls up the correct tag, so even if the commits did briefly get overwritten, it wouldn't actually be catastrophic.

.. actually, on second thought, you might just build from an orphaned tag then, which would be bad. But you'd definitely have enough time to notice.

[silverwind]

+1 for a pre-commit hook that checks the commit message. Should write it in JS. 😆

[Fishrock123]

@silverwind actually, that is a really good idea.

[Fishrock123]

Fixed in 76f219c