Hi,
I would like to follow up on the previous thread[1], where there was a consensus
to disallow merge commits in the llvm github repository, and start a discussion
about how we should enforce this policy.
Unfortunately, GitHub does not provide a convenient way to fully enforce this policy.
We can enforce it for pull requests, but not for direct pushes to the master branch,
so we will have to come up with our own solution if we want to completely prevent
merge commits. I've spent some time looking into this and here are a few
options that I've come up with (If you are a GitHub expert and have other
suggestions, please let us know):
1. Have either a status check or use the "Rebase and Merge" policy for pull requests
to disallow merge commits. Disable direct pushes to the master branch and update the
git-llvm script to create a pull request when a developer does `git llvm push` and
then have it automatically merged if there are no merge commits.
2. Enforce no merge commits for pull requests, but sill allow developers to push directly
to master without checking for merge requests. This is essentially a best effort
approach where we avoid having to implement our own custom work-flow for committing,
while accepting the possibility that someone might accidentally push a merge commit.
3. Disable direct pushes to master, don't update the git-llvm script and require all
developers to use pull requests, where this policy will be enforced.
Which option do you prefer?
-Tom
[1] http://lists.llvm.org/pipermail/llvm-dev/2019-January/129723.html
Hi,
I would like to follow up on the previous thread[1], where there was a consensus
to disallow merge commits in the llvm github repository, and start a discussion
about how we should enforce this policy.
Unfortunately, GitHub does not provide a convenient way to fully enforce this policy.
We can enforce it for pull requests, but not for direct pushes to the master branch,
so we will have to come up with our own solution if we want to completely prevent
merge commits. I've spent some time looking into this and here are a few
options that I've come up with (If you are a GitHub expert and have other
suggestions, please let us know):
1. Have either a status check or use the "Rebase and Merge" policy for pull requests
to disallow merge commits. Disable direct pushes to the master branch and update the
git-llvm script to create a pull request when a developer does `git llvm push` and
then have it automatically merged if there are no merge commits.
2. Enforce no merge commits for pull requests, but sill allow developers to push directly
to master without checking for merge requests. This is essentially a best effort
approach where we avoid having to implement our own custom work-flow for committing,
while accepting the possibility that someone might accidentally push a merge commit.
3. Disable direct pushes to master, don't update the git-llvm script and require all
developers to use pull requests, where this policy will be enforced.
Which option do you prefer?
All these options include at least partial usage of/switch to github pr's.
I don't think that was discussed before. (FWIW i'm opposed to that.)
Is there a fourth option - ask github whether they could make an exception
for LLVM to use server-side hooks? They are available in github enterprise.
-Tom
Roman.
Hi,
I would like to follow up on the previous thread[1], where there was a consensus
to disallow merge commits in the llvm github repository, and start a discussion
about how we should enforce this policy.
Unfortunately, GitHub does not provide a convenient way to fully enforce this policy.
We can enforce it for pull requests, but not for direct pushes to the master branch,
so we will have to come up with our own solution if we want to completely prevent
merge commits. I’ve spent some time looking into this and here are a few
options that I’ve come up with (If you are a GitHub expert and have other
suggestions, please let us know):
-
Have either a status check or use the “Rebase and Merge” policy for pull requests
to disallow merge commits. Disable direct pushes to the master branch and update the
git-llvm script to create a pull request when a developer does git llvm push and
then have it automatically merged if there are no merge commits.
-
Enforce no merge commits for pull requests, but sill allow developers to push directly
to master without checking for merge requests. This is essentially a best effort
approach where we avoid having to implement our own custom work-flow for committing,
while accepting the possibility that someone might accidentally push a merge commit.
-
Disable direct pushes to master, don’t update the git-llvm script and require all
developers to use pull requests, where this policy will be enforced.
Which option do you prefer?
All these options include at least partial usage of/switch to github pr’s.
I don’t think that was discussed before. (FWIW i’m opposed to that.)
Is there a fourth option - ask github whether they could make an exception
for LLVM to use server-side hooks? They are available in github enterprise.
It sounds like GitHub folks are very eager to help the LLVM project:
https://twitter.com/natfriedman/status/1086470665832607746
Server-side hooks seem like a good solution for what’s being discussed. We should ask GitHub about it.
I think we definitely will want to support github PRs, at the very least as an option, even if we continue running/preferring phabricator.
Github PRs are how everyone who is not already super-involved in the llvm project is going to want to contribute changes, and we ought to be as welcoming as possible to such users.
Honestly I’m looking forward to GitHub’s interface here.
Github PRs are how everyone who is not already super-involved in the llvm project is going to want to contribute changes, and we ought to be as welcoming as possible to such users.
Still we'd need some policy & checks here. Say, what if someone will
issue a PR to LLVM 4.0 branch? With clear code style violations, etc.
Then we should not accept it. What if someone did the same on a phabricator review?
I should not have mentioned PR's in my mail.
Can we please not go off-subject here?
Tom Stellard via llvm-dev <llvm-dev@lists.llvm.org> writes:
1. Have either a status check or use the "Rebase and Merge" policy for
pull requests to disallow merge commits. Disable direct pushes to the
master branch and update the git-llvm script to create a pull request
when a developer does `git llvm push` and then have it automatically
merged if there are no merge commits.
This seems to be the least disruptive to existing developers while
maintaining the invariant we want. It has the advantage of potentially
being extended in the future to add additional criteria for merges
(e.g. "it builds" or "it passes this set of tests").
2. Enforce no merge commits for pull requests, but sill allow
developers to push directly to master without checking for merge
requests. This is essentially a best effort approach where we avoid
having to implement our own custom work-flow for committing, while
accepting the possibility that someone might accidentally push a merge
commit.
To me this is the least desirable. I'd prefer we have one way of
getting changes into the repository.
3. Disable direct pushes to master, don't update the git-llvm script
and require all developers to use pull requests, where this policy
will be enforced.
I am completely fine with this. It's friendlier to new contributors.
That said, I assume with #1 we wouldn't prevent users from git push-ing
their local branch to GitHub (i.e. not using git llvm push) and manually
opening a PR, so either #1 or #3 works for new developers.
Which option do you prefer?
Maybe a slight preference for #1 as being less disruptive than #3 but I
really don't care either way.
-David
Why isn’t this enforceable with a server-side pre-receive hook?
GitHub[1] only supports pre-receive hooks in the 'Enterprise Server'
plan, which is for self-hosted github instances.
-Tom
[1] https://github.com/pricing
AIUI, the GitHub team is perfectly willing to help out the LLVM project in whatever way LLVM needs, including but not limited to turning on server-side hooks for us.
https://twitter.com/natfriedman/status/1086470665832607746
Server-side hooks are the answer to this problem. There is no problem. You just use a server-side hook.
(Whether or not to use GitHub PRs is an orthogonal question. You can use hooks with PRs, or hooks without PRs; PRs with hooks, or PRs without hooks.)
–Arthur
It sounds like we need to get someone from the Foundation (chandlerc@, lattner@, tanya@, someone else?) to reach out to them offline about this.
It sounds like we need to get someone from the Foundation (chandlerc@, lattner@, tanya@, someone else?) to reach out to them offline about this.
Yes, we will try to reach out to GitHub directly about this, but I still
think we need some kind of contingency plan in case pre-receive hooks
or even a new kind of branch protection won't be an option for us.
-Tom
Excuse my ignorance (I'm not great with Git) but how would it differ for workflows of people
who use a Git repository for local work but still use `svn up + patch + svn commit <list of
` to actually land post CR or for NFC patches, while resolving conflicts during a
pull into a local (non-trunk) branch manually, after the eventual full switch to GitHub?
I'm aware that SVN operates using the lock model as opposed to Git essentially making the
history linear; Are merge commits multiple commits that are landed as part of a single
Git "push" (ie. unsquashed), or attempts to do anything that would result in a creation
or merging of a branch on the remote?
Thank you.
Excuse my ignorance (I’m not great with Git) but how would it differ for workflows of people
who use a Git repository for local work but still use `svn up + patch + svn commit <list of
` to actually land post CR or for NFC patches, while resolving conflicts during a
pull into a local (non-trunk) branch manually, after the eventual full switch to GitHub?
I think SVN does not factor at all into where this thread is going: This is about what happens after the switch.
I’m aware that SVN operates using the lock model as opposed to Git essentially making the
history linear; Are merge commits multiple commits that are landed as part of a single
Git “push” (ie. unsquashed), or attempts to do anything that would result in a creation
or merging of a branch on the remote?
Git history is not inherently linear. Merge commits are where different branches of development are merged together. Instead of merging different branches together (and retaining the history of the branches), it is possible to chose one as the “base” branch and “rebase” the other onto that branch. The Git history then appears to be linear.
I don’t think I have a preference regarding merge commits but having a status check where some subset of build + test takes place is a really good idea, IMO. It would be great if it were applied for everyone but if that causes too many problems, I would settle for opt-in. Preferably not all-or-nothing on the check.
Regarding the scope of the check (boldly assuming one would be in place): the more, the better (so long as it’s stable and tolerable to the team). Some dev teams that use GH have a bot that optimistically batch builds-and-tests commits and when failures inevitably happen, contributors are encouraged to triage and re-enqueue their PR for being built if they can surmise the failure is not due to their change.
When contributing changes upstream, it takes away some of the stress/challenge if you have some independent automaton verify that your change doesn’t cause regressions. As it stands I feel like I should be on standby for 24 to 72 hours after the commit in order to investigate/revert if my change causes someone problems. I realize that it’s prudent to limit the scope such that the checks don’t create an enormous backlog.
It is quite unlikely that GitHub will allow LLVM (or any other project) to run arbitrary post-receive hooks. It is far more likely that they will give an option that disables the merge button in PRs. It is already possible to prevent direct pushes to master by anyone, so this would be a very small change (literally disabling one button in the UI).
David
It is quite unlikely that GitHub will allow LLVM (or any other project) to run arbitrary post-receive hooks. It is far more likely that they will give an option that disables the merge button in PRs. It is already possible to prevent direct pushes to master by anyone, so this would be a very small change (literally disabling one button in the UI).
When the goal is to have a linear history that’s not quite sufficient though. We do not only want to prevent someone from merging commits through the UI, but also from merging commits offline and then pushing the merge.
Cheers,
Philip
Disabling (or really, changing the semantics of) the merge button is actually a standard project setting for all GitHub projects, even personal ones. You find it under “Settings” here: https://i.imgur.com/w5zmNra.png
However, the solution to the LLVM project’s problem is a server-side pre-receive hook.
See this Twitter thread:
https://twitter.com/jessfraz/status/1086475273036578816
I’m looping in Chandler, since he seems to be the person who’s already talked to GitHub staff about LLVM’s needs. Chandler, have you discussed with them
- how to prevent accidental non-fast-forward merge commits
- installing server-side hooks
?
Thanks much,
–Arthur
