Skip to content

Add doc for Git pre-push hook to block accidental pushes to main/master#1012

Merged
rparolin merged 5 commits intomainfrom
rparolin/pre-push-hook-check
Sep 24, 2025
Merged

Add doc for Git pre-push hook to block accidental pushes to main/master#1012
rparolin merged 5 commits intomainfrom
rparolin/pre-push-hook-check

Conversation

@rparolin
Copy link
Copy Markdown
Collaborator

Adds a Markdown doc showing how to set up a global Git pre‑push hook that blocks pushes to main (and master) unless the explicit override flag --break_glass_to_push_main is used. The override flag is intended to be loud and nosy to avoid accidental use.

@copy-pr-bot
Copy link
Copy Markdown
Contributor

copy-pr-bot bot commented Sep 24, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@rparolin rparolin requested a review from kkraus14 September 24, 2025 15:26
Comment thread docs/pre-push-branch-check.md Outdated

## 2. Create the Pre‑Push Hook
```bash
vim ~/.git-hooks/pre-push
Copy link
Copy Markdown
Contributor

@cpcloud cpcloud Sep 24, 2025

Choose a reason for hiding this comment

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

Won't this affect every git repo a user has cloned? That seems like a pretty ostentatious choice to make for people.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That's intended because the purpose of the hook is to prevent accidental pushes to main. This is more of a problem for people with admin access because we locked down direct commits to main outside of PR merges. But also, we haven't setup those rules for all repos yet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It sounds like you're assuming that all repos anyone will clone or has cloned will be either nvidia repos or repos that they will never want to or need to force push to.

Is that not a rather strong assumption?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Point taken. I've added per-repo instructions to the doc as an alternative to setting this pre-push hook globally.

Comment thread docs/pre-push-branch-check.md Outdated
@rparolin
Copy link
Copy Markdown
Collaborator Author

/ok to test 8764233

@github-actions

This comment has been minimized.

Comment thread docs/pre-push-branch-check.md Outdated
@rparolin rparolin requested review from cpcloud and rwgk September 24, 2025 20:51
Copy link
Copy Markdown
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Looks very useful to me! This is a great way to spread the trick.

FWIW, I metabolized this already into my personal config:

https://github.com/rwgk/rwgk_config/blob/09bac54bacc35b763bf76dfa5b2917a81827e5c6/git_stuff/pre-push

That code is entirely LLM-generated. I just told the LLM that I wanted to integrate the branch protection into what I had already.

I was already set up with an alias to copy that into a git repo (cwd). After learning about the global ~/.git-hooks trick here, I think that'll actually be better ... but I'm saving that for another day.

@rparolin
Copy link
Copy Markdown
Collaborator Author

/ok to test bbd8b0e

@rparolin
Copy link
Copy Markdown
Collaborator Author

Looks very useful to me! This is a great way to spread the trick.

FWIW, I metabolized this already into my personal config:

https://github.com/rwgk/rwgk_config/blob/09bac54bacc35b763bf76dfa5b2917a81827e5c6/git_stuff/pre-push

That code is entirely LLM-generated. I just told the LLM that I wanted to integrate the branch protection into what I had already.

I was already set up with an alias to copy that into a git repo (cwd). After learning about the global ~/.git-hooks trick here, I think that'll actually be better ... but I'm saving that for another day.

Thank you!

I was reading as part of the research for this that global pre-push hooks will override pre-repo unless you have your global hook call into the pre-repo hook. I never tested it so never added it to the doc but its a good piece of knowledge to file away if you need it.

@rparolin rparolin enabled auto-merge (squash) September 24, 2025 21:52
@rparolin rparolin disabled auto-merge September 24, 2025 21:52
@rparolin rparolin enabled auto-merge (squash) September 24, 2025 21:52
@rparolin rparolin merged commit 09bac29 into main Sep 24, 2025
70 checks passed
@rparolin rparolin deleted the rparolin/pre-push-hook-check branch September 24, 2025 22:17
@github-actions
Copy link
Copy Markdown

Doc Preview CI
Preview removed because the pull request was closed or merged.

@leofang
Copy link
Copy Markdown
Member

leofang commented Sep 24, 2025

@rparolin looks nice! We should hyperlink to it in the ToC here:

## Table of Contents
- [Pre-commit](#pre-commit)
- [Code signing](#code-signing)
- [Developer Certificate of Origin (DCO)](#developer-certificate-of-origin-dco)
- [CI infrastructure overview](#ci-infrastructure-overview)

Alternatively, we can move the entire doc to the contributing guide as a separate section

@leofang leofang added documentation Improvements or additions to documentation P1 Medium priority - Should do labels Sep 24, 2025
@leofang leofang added this to the cuda.core beta 7 milestone Sep 24, 2025
@kkraus14
Copy link
Copy Markdown
Collaborator

I didn't get a chance to review this before we merged, but I don't think documenting git / dev configs unrelated to the cuda-python project is something we should be doing.

We should have a GitHub ruleset and proper CI config that allows us to prevent everyone, including admins, from committing directly to protected branches while still allowing us to skip CI when needed.

@rparolin
Copy link
Copy Markdown
Collaborator Author

I didn't get a chance to review this before we merged, but I don't think documenting git / dev configs unrelated to the cuda-python project is something we should be doing.

We should have a GitHub ruleset and proper CI config that allows us to prevent everyone, including admins, from committing directly to protected branches while still allowing us to skip CI when needed.

👍 I'll remove it from the repo. #1026

@leofang leofang removed this from the cuda.core beta 7 milestone Oct 9, 2025
@leofang leofang removed documentation Improvements or additions to documentation P1 Medium priority - Should do labels Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants