llvm dev - Mar 2019 - [cfe-dev] [GitHub] RFC: Enforcing no merge commit policy

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

On Tue, Mar 19, 2019 at 10:00 PM Tom Stellard via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
>
> 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.
> [1] http://lists.llvm.org/pipermail/llvm-dev/2019-January/129723.html
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

> On Mar 19, 2019, at 12:14 PM, Roman Lebedev via cfe-dev <cfe-dev at
lists.llvm.org> wrote:
> 
> On Tue, Mar 19, 2019 at 10:00 PM Tom Stellard via cfe-dev
> <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>>
wrote:
>> 
>> 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.
It sounds like GitHub folks are very eager to help the LLVM project:
https://twitter.com/natfriedman/status/1086470665832607746
<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.


>> -Tom
> Roman.
> 
>> [1] http://lists.llvm.org/pipermail/llvm-dev/2019-January/129723.html
<http://lists.llvm.org/pipermail/llvm-dev/2019-January/129723.html>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
<https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
<https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190319/46e74ae0/attachment.html>

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.

On Tue, Mar 19, 2019 at 3:15 PM Roman Lebedev via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> On Tue, Mar 19, 2019 at 10:00 PM Tom Stellard via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
> >
> > 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.
>
> > [1] http://lists.llvm.org/pipermail/llvm-dev/2019-January/129723.html
> > _______________________________________________
> > cfe-dev mailing list
> > cfe-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190319/9144f1b6/attachment.html>

Tom Stellard via llvm-dev <llvm-dev at 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

On Tue, Mar 19, 2019 at 12:00 PM Tom Stellard via lldb-dev <
lldb-dev at lists.llvm.org> wrote:
> 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.
>
>
Why isn't this enforceable with a server-side pre-receive hook?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190320/ee7354de/attachment.html>

On 03/20/2019 10:41 AM, Zachary Turner wrote:
> 
> 
> On Tue, Mar 19, 2019 at 12:00 PM Tom Stellard via lldb-dev <lldb-dev at
lists.llvm.org <mailto:lldb-dev at lists.llvm.org>> wrote:
> 
>     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.
> 
>  
> 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

On Tue, Mar 19, 2019 at 2:00 PM Tom Stellard via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> 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?
>
>
>
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.


-- 
-Brian
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190320/6f5f6fe2/attachment.html>

On 03/19/2019 11:59 AM, Tom Stellard via Openmp-dev
wrote:
> 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?
> 
Thanks everyone for responding to this thread.  Based on the responses,
the preferred solution is none of these options and instead having a
server-side check to prevent merge commits.  I have been in contact with someone
at GitHub who is looking into this for us to see if this would be possible.

As a backup plan, I am going to look into implementing option #1, but instead
of having git-llvm create a pull request, it would push first to a staging
branch and then push to master if a 'no-merge commit' status checks
pass.  This
is essentially the same flow as using pull requests, but without all the
pull request noise.

The reason to go #1 as a backup is that it preserves the status quo we have now
where developers commit using git-llvm and based on the limited feedback seems
like the preferred option of these 3.

- Tom 
> -Tom
> 
> [1] http://lists.llvm.org/pipermail/llvm-dev/2019-January/129723.html
> _______________________________________________
> Openmp-dev mailing list
> Openmp-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/openmp-dev
>