llvm dev - Jan 2019 - [cfe-dev] [Github] RFC: linear history vs merge commits

On Wed, Jan 30, 2019 at 1:19 PM Eric Christopher via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
> >
> > Bruce Hoult via llvm-dev <llvm-dev at lists.llvm.org> writes:
> >
> > > How about:
> > >
> > > Require a rebase, followed by git merge --no-ff
> > >
> > > This creates a linear history, but with extra null merge commits
> > > delimiting each related series of patches.
> > >
> > > I believe it is compatible with bisect.
> > >
> > > https://linuxhint.com/git_merge_noff_option/
> >
> > We've done both and I personally prefer the strict linear history
by a
> > lot.  It's just much easier to understand a linear history.
> >
>
> Agreed. Let's go with option #1.
>
>
What is the practical plan to enforce the lack of merges? When we looked
into this GitHub would not support this unless also forcing every change to
go through a pull request (i.e. no pre-receive hooks on direct push to
master were possible). Did this change? Are we hoping to get support from
GitHub on this?

We may write this rule in the developer guide, but I fear it'll be hard to
enforce in practice.

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190130/9d5ed83f/attachment.html>

Mehdi AMINI <joker.eph at gmail.com> writes:
> What is the practical plan to enforce the lack of merges? When we
> looked into this GitHub would not support this unless also forcing
> every change to go through a pull request (i.e. no pre-receive hooks
> on direct push to master were possible). Did this change? Are we
> hoping to get support from GitHub on this?
>
> We may write this rule in the developer guide, but I fear it'll be
> hard to enforce in practice.
Again, changes aren't going through git yet, right?  Not until SVN is
decommissioned late this year (or early next).  SVN requires a strict
linear history so it handles the enforcement for now.

My personal opinion is that when SVN is decomissioned we should use pull
requests, simply because that's what's familiar to the very large
development community outside LLVM.  It will lower the bar to entry for
new contributors.  This has all sorts of implications we need to discuss
of course, but we have some time to do that.

If we don't use PRs, then we're pretty much on the honor system to
disallow merges AFAIK.

                             -David

On Thu, Jan 31, 2019 at 8:29 PM David Greene via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
>
> Mehdi AMINI <joker.eph at gmail.com> writes:
>
> > What is the practical plan to enforce the lack of merges? When we
> > looked into this GitHub would not support this unless also forcing
> > every change to go through a pull request (i.e. no pre-receive hooks
> > on direct push to master were possible). Did this change? Are we
> > hoping to get support from GitHub on this?
> >
> > We may write this rule in the developer guide, but I fear it'll be
> > hard to enforce in practice.
>
> Again, changes aren't going through git yet, right?  Not until SVN is
> decommissioned late this year (or early next).  SVN requires a strict
> linear history so it handles the enforcement for now.
> My personal opinion is that when SVN is decomissioned we should use pull
> requests, simply because that's what's familiar to the very large
> development community outside LLVM.  It will lower the bar to entry for
> new contributors.  This has all sorts of implications we need to discuss
> of course, but we have some time to do that.
My personal opinion is an opposite of that one.

*Does* LLVM want to switch from phabricator to github pr's?
I personally don't recall previous discussions.
Personally, i hope not, i hope phabricator should/will stay.

Low bar for entry is good, but not at the cost of raising the bar
for the existing development community.
In particular, i'm saying that github's ui/workflow/feature set
is inferior to that one of phabricator.

Is the low entry bar the only benefit?
While it is good, it should not be the only factor.
The contributors will already need to know how to build LLVM,
write tests, make meaningful changes to the code.
Additionally having to know how to work with phabricator
isn't that much to ask for, considering the other prerequisites..
> If we don't use PRs, then we're pretty much on the honor system to
> disallow merges AFAIK.
>
>                              -David
Roman.
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

On 01/30/2019 10:53 PM, Mehdi AMINI via llvm-dev wrote:
> 
> 
> On Wed, Jan 30, 2019 at 1:19 PM Eric Christopher via cfe-dev <cfe-dev at
lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
> 
>     On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev
>     <cfe-dev at lists.llvm.org <mailto:cfe-dev at
lists.llvm.org>> wrote:
>     >
>     > Bruce Hoult via llvm-dev <llvm-dev at lists.llvm.org
<mailto:llvm-dev at lists.llvm.org>> writes:
>     >
>     > > How about:
>     > >
>     > > Require a rebase, followed by git merge --no-ff
>     > >
>     > > This creates a linear history, but with extra null merge
commits
>     > > delimiting each related series of patches.
>     > >
>     > > I believe it is compatible with bisect.
>     > >
>     > > https://linuxhint.com/git_merge_noff_option/
>     >
>     > We've done both and I personally prefer the strict linear
history by a
>     > lot.  It's just much easier to understand a linear history.
>     >
> 
>     Agreed. Let's go with option #1.
> 
> 
> What is the practical plan to enforce the lack of merges? When we looked
into this GitHub would not support this unless also forcing every change to go
through a pull request (i.e. no pre-receive hooks on direct push to master were
possible). Did this change? Are we hoping to get support from GitHub on this?
> 
No enforcement plan at this point, but I was planning to contact github about
this to
see what options we had.  Last time you looked into it, did you talk to anyone
at github
support?

This is also why I think it's important to decide early so we have time to
look at
what our options are to enforce these policies. If pull requests are our only
option for enforcement, then I think it would be good to know that before we
have a large debate about Phabricator vs Pull Requests.

-Tom

> We may write this rule in the developer guide, but I fear it'll be hard
to enforce in practice.
>
 -- 
> Mehdi
> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

On Fri, Feb 1, 2019 at 4:44 PM Tom Stellard <tstellar at redhat.com>
wrote:
> On 01/30/2019 10:53 PM, Mehdi AMINI via llvm-dev wrote:
> >
> >
> > On Wed, Jan 30, 2019 at 1:19 PM Eric Christopher via cfe-dev <
> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>>
wrote:
> >
> >     On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev
> >     <cfe-dev at lists.llvm.org <mailto:cfe-dev at
lists.llvm.org>> wrote:
> >     >
> >     > Bruce Hoult via llvm-dev <llvm-dev at lists.llvm.org
<mailto:
> llvm-dev at lists.llvm.org>> writes:
> >     >
> >     > > How about:
> >     > >
> >     > > Require a rebase, followed by git merge --no-ff
> >     > >
> >     > > This creates a linear history, but with extra null merge
commits
> >     > > delimiting each related series of patches.
> >     > >
> >     > > I believe it is compatible with bisect.
> >     > >
> >     > > https://linuxhint.com/git_merge_noff_option/
> >     >
> >     > We've done both and I personally prefer the strict linear
history
> by a
> >     > lot.  It's just much easier to understand a linear
history.
> >     >
> >
> >     Agreed. Let's go with option #1.
> >
> >
> > What is the practical plan to enforce the lack of merges? When we
looked
> into this GitHub would not support this unless also forcing every change to
> go through a pull request (i.e. no pre-receive hooks on direct push to
> master were possible). Did this change? Are we hoping to get support from
> GitHub on this?
> >
>
> No enforcement plan at this point, but I was planning to contact github
> about this to
> see what options we had.  Last time you looked into it, did you talk to
> anyone at github
> support?
>
Yes, and they pointed to the web hooks they have (but these are only
post-commit I believe) and they pointed to the pull-request policy to
enforce this. They said they wasn't any other option (at the time).

If a custom pre-receive hook is out of the realm of possible, maybe they
could add as a new "branch protection" setting that would
"enforce linear
history" on a branch. This seems like a reasonable feature that other
people may want.

> This is also why I think it's important to decide early so we have time
to
> look at
> what our options are to enforce these policies. If pull requests are our
> only
> option for enforcement, then I think it would be good to know that before
> we
> have a large debate about Phabricator vs Pull Requests.
>
If we use pull-request as a tool for ensuring linear history, we don't
*have to* do the reviews there: it would be possible to continue doing the
reviews on the mailing list and on Phabricator, and just open a PR after
approval to use the "Rebase and merge" to get the change in
immediately.

I'm not sure the "not do any review on the pull request" while
having to
open a PR for any change anyway will be practical though, at this point
folks may just naturally start reviewing there directly.

Another idea I floated around is to force to use `git llvm-push` instead of
`git push` and have it check that no merge is about the be pushed. We could
prevent direct use of `git push` by using this "branch protection"
feature:
https://help.github.com/articles/about-required-status-checks/ ; we'd have
`git llvm-push` whitelisting the commit before actually pushing it.

-- 
Mehdi




>
> -Tom
>
>
> > We may write this rule in the developer guide, but I fear it'll be
hard
> to enforce in practice.
> >
>  --
> > Mehdi
> >
> >
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190201/a1c0cb6c/attachment.html>

I'm also in favor of linear history, option #1.

FWIW I don't think lacking tight controls to prevent merges is going to be
a huge deal.  We already restrict who can commit, and there are lots of
other rules you have to follow.

We might get an accidental merge or two every once in a while, but I expect
we'll all be able to live with that?

On Wed, Jan 30, 2019 at 10:53 PM Mehdi AMINI via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
>
>
> On Wed, Jan 30, 2019 at 1:19 PM Eric Christopher via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev
>> <cfe-dev at lists.llvm.org> wrote:
>> >
>> > Bruce Hoult via llvm-dev <llvm-dev at lists.llvm.org>
writes:
>> >
>> > > How about:
>> > >
>> > > Require a rebase, followed by git merge --no-ff
>> > >
>> > > This creates a linear history, but with extra null merge
commits
>> > > delimiting each related series of patches.
>> > >
>> > > I believe it is compatible with bisect.
>> > >
>> > > https://linuxhint.com/git_merge_noff_option/
>> >
>> > We've done both and I personally prefer the strict linear
history by a
>> > lot.  It's just much easier to understand a linear history.
>> >
>>
>> Agreed. Let's go with option #1.
>>
>>
> What is the practical plan to enforce the lack of merges? When we looked
> into this GitHub would not support this unless also forcing every change to
> go through a pull request (i.e. no pre-receive hooks on direct push to
> master were possible). Did this change? Are we hoping to get support from
> GitHub on this?
>
> We may write this rule in the developer guide, but I fear it'll be hard
to
> enforce in practice.
>
> --
> Mehdi
>
> _______________________________________________
> 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/20190204/58a8fff4/attachment.html>

Agreed. #1 is the most sensible and mistakes should be few and easy to fix.

On Mon, 4 Feb 2019, 22:11 Justin Lebar via llvm-dev <llvm-dev at
lists.llvm.org
wrote:
> I'm also in favor of linear history, option #1.
>
> FWIW I don't think lacking tight controls to prevent merges is going to
be
> a huge deal.  We already restrict who can commit, and there are lots of
> other rules you have to follow.
>
> We might get an accidental merge or two every once in a while, but I
> expect we'll all be able to live with that?
>
> On Wed, Jan 30, 2019 at 10:53 PM Mehdi AMINI via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>>
>>
>> On Wed, Jan 30, 2019 at 1:19 PM Eric Christopher via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>>
>>> On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev
>>> <cfe-dev at lists.llvm.org> wrote:
>>> >
>>> > Bruce Hoult via llvm-dev <llvm-dev at lists.llvm.org>
writes:
>>> >
>>> > > How about:
>>> > >
>>> > > Require a rebase, followed by git merge --no-ff
>>> > >
>>> > > This creates a linear history, but with extra null merge
commits
>>> > > delimiting each related series of patches.
>>> > >
>>> > > I believe it is compatible with bisect.
>>> > >
>>> > > https://linuxhint.com/git_merge_noff_option/
>>> >
>>> > We've done both and I personally prefer the strict linear
history by a
>>> > lot.  It's just much easier to understand a linear
history.
>>> >
>>>
>>> Agreed. Let's go with option #1.
>>>
>>>
>> What is the practical plan to enforce the lack of merges? When we
looked
>> into this GitHub would not support this unless also forcing every
change to
>> go through a pull request (i.e. no pre-receive hooks on direct push to
>> master were possible). Did this change? Are we hoping to get support
from
>> GitHub on this?
>>
>> We may write this rule in the developer guide, but I fear it'll be
hard
>> to enforce in practice.
>>
>> --
>> Mehdi
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190205/0365378b/attachment.html>