Skip to content

Optional hooks to prevent committing/pushing to master #270

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

Merged
merged 4 commits into from
Jul 19, 2018

Conversation

berquist
Copy link
Member

In case anyone wants to prevent themselves from committing to their local master branch or pushing to their own remote master.

@june128
Copy link
Member

june128 commented Jul 18, 2018

In case the community likes that PR: I would move the explanation into the Wiki of the this GitHub repo. I think the Wiki is a better place for developer oriented explanations/content.

@Butt4cak3
Copy link
Contributor

Butt4cak3 commented Jul 18, 2018

We were thinking about creating a directory under the project root that should hold scripts like update_site.sh. This is probably a good opportunity to act on that. I'd call it tools or scripts, although I personally lean towards tools because it's more flexible. What do you think? Let me also summon @leios real quick for this one.

@june128
Copy link
Member

june128 commented Jul 18, 2018

If we get more and more helper scripts/tools, a sub-directory is useful andtools sounds is more flexible. I support both :)


while read local_ref local_sha remote_ref remote_sha
do
if [ "$remote_ref" = "refs/heads/master" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will cause trouble when you have two remotes, origin and upstream where origin points to a personal fork and upstream points to this repository. When you pull changes from upstream and want to push them to origin to keep your fork up-to-date, this will block the push if I understand correctly.

It should be enough to block commits to master in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. It could be changed to allow the push if refs/remotes/upstream/master and refs/heads/master are equal, but if the remote isn't upstream it fails, and making this bulletproof is simply not worth it.

@Butt4cak3
Copy link
Contributor

Also, will these hooks prevent local fast-forward merges into master?

@berquist
Copy link
Member Author

Also, will these hooks prevent local fast-forward merges into master?

No, you'll need something like

git config --add branch.master.mergeoptions --no-ff

@Butt4cak3
Copy link
Contributor

Sounds good at first, but I can imagine this blocking git pull as well. I guess preventing commits directly to master should be enough. I don't think accidentally merging into master is as common as accidentally commiting directly to it.

@berquist
Copy link
Member Author

Sounds good at first, but I can imagine this blocking git pull as well.

Are you referring to the --no-ff modification to .git/config? There are no problems with doing:

git checkout master
git fetch upstream
git rebase upstream/master
git push

I don't remember the git pull workflow, since I only ever rebase against upstream.

...since it has the undesirable behavior of not allowing push after
rebasing against upstream, for example.
@Butt4cak3
Copy link
Contributor

Are you referring to the --no-ff modification to .git/config?
Yes.

Rebasing master onto upstream/master should technically not even be necessary, because there shouldn't be anything to rebase on master in the first place. In that case, even a git reset --hard upstream/master should work.

Anyway, I tested the script and hook and it works as expected. The only thing left is to decide on the directory name for the scripts.

@leios
Copy link
Member

leios commented Jul 18, 2018

If this does what we expect, then it prevents people from pushing to their local masters, which means they need to use feature branches. That seems like a good thing. I know I could have used the protection once or twice.

I think tools is a fine name for the script directory.

@Butt4cak3 Butt4cak3 merged commit 1401594 into algorithm-archivists:master Jul 19, 2018
@berquist berquist deleted the hooks branch July 21, 2018 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants