[Github] RFC: linear history vs merge commits

classic Classic list List threaded Threaded
38 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
On Thu, Jan 31, 2019 at 4:56 PM David Greene via llvm-dev <[hidden email]> wrote:
Roman Lebedev <[hidden email]> writes:

> *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.

I find Phab pretty unintuitive.  I just started using it in earnest
about four months ago, so that's one datapoint from people new to it.
Having used both, I find GitHub tends to have many similar views that are slightly different. The way it decides that there are "outdated comments" and that those comments should be hard to does not win it any points in my book.
 

                            -David
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
In reply to this post by Nathan Ridge via cfe-dev

Script a bot to scrape the patches and post to phabricator?

On Fri, 1 Feb 2019 at 04:22, Roman Lebedev via llvm-dev <[hidden email]> wrote:
On Thu, Jan 31, 2019 at 8:29 PM David Greene via cfe-dev
<[hidden email]> wrote:
>
> Mehdi AMINI <[hidden email]> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
In reply to this post by Nathan Ridge via cfe-dev
I realise that llvm trunk can move fairly quickly. 
So my original, but brief, suggestion was to merge the current set of approved patches together rather than attempting them one at a time.
Build on a set of fast smoke test bots. If something breaks, it should be possible to bisect it to reject a PR and make forward progress.
Occasionally bisecting a large set of PR's should still be less bot time than attempting to build each of them individually.
Blocking the PR's due to target specific and or slow bot failures would be a different question.
You could probably do this with a linear history, so long as the final tree is the same as the merge commit, it should still build.
I'm just fond of the idea of trying to prevent bad commits from ever being merged. Since they sometimes waste everyone's time.

On Fri, 1 Feb 2019 at 04:02, David Greene <[hidden email]> wrote:
<[hidden email]> writes:

> Systems that I've seen will funnel all submitted PRs into a single queue
> which *does* guarantee that the trial builds are against HEAD and there
> are no "later commits" that can screw it up. If the trial build passes,
> the PR goes in and becomes the new HEAD.

The downside of a system like this is that when changes are going in
rapidly, the queue grows very large and it takes forever to get your
change merged.  PR builds are serialized and if a "build" means "make
sure it builds on all the Buildbots" then it takes a very long time
indeed.

There are ways to parallelize builds by speculating that PRs will build
cleanly but it gets pretty complicated quickly.

> But this would be a radical redesign of the LLVM bot system, and would
> have to wait until we're done with the GitHub migration and can spend
> a couple of years debating the use of PRs. :-)

Heh.  Fully guaranteeing buildability of a branch is not a trivial task
and will take a LOT of thought and discussion.

                              -David

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
In reply to this post by Nathan Ridge via cfe-dev


> -----Original Message-----
> From: cfe-dev [mailto:[hidden email]] On Behalf Of David
> Greene via cfe-dev
> Sent: Thursday, January 31, 2019 4:56 PM
> To: Roman Lebedev
> Cc: llvm-dev; cfe-dev; openmp-dev ([hidden email]); LLDB Dev
> Subject: Re: [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge
> commits
>
> Roman Lebedev <[hidden email]> writes:
>
> > *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.
>
> I find Phab pretty unintuitive.  I just started using it in earnest
> about four months ago, so that's one datapoint from people new to it.

Heh. Years ago I described it as "actively user-hostile."  Merely being
unintuitive is a huge step forward.  I don't know how much of that is
the local LLVM instance being massaged toward usefulness as opposed to
improvements in the underlying product.
--paulr

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
In reply to this post by Nathan Ridge via cfe-dev
Oh, I'm completely in favor of making bad commits much less likely.  I
simply think there is a decent solution between "let everything in" and
"don't let everything in unless its proven to work everywhere" that gets
90% of the improvement.  The complexity of guaranteeing a buildable
branch is high.  If someone wants to take that on, great!  I just don't
think we should reasonably expect a group of volunteers to do it.  :)

                        -David

Jeremy Lakeman <[hidden email]> writes:

> I realise that llvm trunk can move fairly quickly.
> So my original, but brief, suggestion was to merge the current set of
> approved patches together rather than attempting them one at a time.
> Build on a set of fast smoke test bots. If something breaks, it should
> be possible to bisect it to reject a PR and make forward progress.
> Occasionally bisecting a large set of PR's should still be less bot
> time than attempting to build each of them individually.
> Blocking the PR's due to target specific and or slow bot failures
> would be a different question.
> You could probably do this with a linear history, so long as the final
> tree is the same as the merge commit, it should still build.
> I'm just fond of the idea of trying to prevent bad commits from ever
> being merged. Since they sometimes waste everyone's time.
>
> On Fri, 1 Feb 2019 at 04:02, David Greene <[hidden email]> wrote:
>
>     <[hidden email]> writes:
>    
>     > Systems that I've seen will funnel all submitted PRs into a
>     single queue
>     > which *does* guarantee that the trial builds are against HEAD
>     and there
>     > are no "later commits" that can screw it up. If the trial build
>     passes,
>     > the PR goes in and becomes the new HEAD.
>    
>     The downside of a system like this is that when changes are going
>     in
>     rapidly, the queue grows very large and it takes forever to get
>     your
>     change merged. PR builds are serialized and if a "build" means
>     "make
>     sure it builds on all the Buildbots" then it takes a very long
>     time
>     indeed.
>    
>     There are ways to parallelize builds by speculating that PRs will
>     build
>     cleanly but it gets pretty complicated quickly.
>    
>     > But this would be a radical redesign of the LLVM bot system, and
>     would
>     > have to wait until we're done with the GitHub migration and can
>     spend
>     > a couple of years debating the use of PRs. :-)
>    
>     Heh. Fully guaranteeing buildability of a branch is not a trivial
>     task
>     and will take a LOT of thought and discussion.
>    
>     -David
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
My two cents on git:
- A linear history is important for "git bisect", "git blame", and just general developer sanity. Rebase should be preferred over merge.
- Rewriting the history of "master" (by force-pushing to "master") should never be done. GitHub Enterprise can enforce this by server-side hooks. I know because I've done it for a previous employer's repositories (although I forget the details; it might have involved a customer support ticket).
- Murphy's Law: Force-pushes will happen, by accident, if they are not prevented by technological means. (Again, I know from first-hand experience.)
- Murphy's Law: Merge commits will happen, by accident, if they are not prevented by technological means. (Again, I know from first-hand experience.)  And, because rewriting history should be prevented, once the history contains a "merge bubble," it will always contain it; there's no way to recover a completely bisectable/blameable linear history after a merge has been committed.
- Full disclosure: "Revert" commits also wreak havoc on "git bisect" and "git blame", and LLVM does a lot of reverts.

Conclusion: LLVM should find a technological way to prevent force-pushes to master (and to release branches) and to prevent multi-parent merge commits.  If you maintain your own "git" server, then you can use git's server-side hooks to reject offending commits very easily.  Since your "git" server is hosted on GitHub, you'll have to ask GitHub to install the appropriate server-side hooks.  I know GitHub can be configured to reject force-pushes to {any, a specific} branch.  I strongly suspect that if the maintainers of LLVM asked nicely, GitHub would also be able to reject merges to {any, a specific} branch.

My two cents on Phab:
- It's a little nicer than GitHub PRs for commenting-on, but both are cool.
- GitHub PRs have Travis integration so the reviewer can see if a particular patch is actually compileable at all, before spending time looking at it. I wish Phab had this feature (or maybe it exists and LLVM doesn't use it?).

My two cents on gating on buildbot:
- If buildbot runs only in master, then you wind up with a lot of revert commits (LLVM's status quo). This seems to be working fine for LLVM in my opinion; it is not pathological.
- If merging-to-master is gated on buildbot — that is, if clicking "OK to merge" on a patch means "rebase it on master, run buildbot, then merge to master only if green" — then you must solve at least two hard problems. Number one, buildbot becomes a single point of failure. If buildbot is heavily loaded, patches merge slowly (but this can be solved by clever engineering as already mentioned in this thread). If buildbot is down or unreachable, then no changes can be made to master (because no change can get through the gate). This requires a contingency plan. Number two, if buildbot goes permanently red because of a change outside the LLVM codebase — a dependency breaks, or something in buildbot's configuration is changed and can't easily be changed back, or a non-deterministic unit test was committed by accident — then again good patches will be rejected by the gate. This is almost the same as hard problem number one, and it would be reasonable to treat it as the same problem, but it is not 100% obvious that it should be treated as the same problem.  Because of these two hard problems, I believe that gating on buildbot has a high likelihood of becoming pathological, say, once a year or more.  I think it is a bad tradeoff.
- I strongly believe that the sweet spot is what GitHub does: run buildbot (Travis) on every pull request and display the results conspicuously, but do not gate on those results. Cultivate a human culture that we do not merge red PRs and we do not merge PRs whose buildbot run is still in progress, any more than we merge PRs without code review approval. (Which is to say, sometimes we do, and we certainly can in an emergency. But we know we shouldn't!)

HTH,
Arthur


On Fri, Feb 1, 2019 at 11:02 AM David Greene via cfe-dev <[hidden email]> wrote:
Oh, I'm completely in favor of making bad commits much less likely.  I
simply think there is a decent solution between "let everything in" and
"don't let everything in unless its proven to work everywhere" that gets
90% of the improvement.  The complexity of guaranteeing a buildable
branch is high.  If someone wants to take that on, great!  I just don't
think we should reasonably expect a group of volunteers to do it.  :)

                        -David

Jeremy Lakeman <[hidden email]> writes:

> I realise that llvm trunk can move fairly quickly.
> So my original, but brief, suggestion was to merge the current set of
> approved patches together rather than attempting them one at a time.
> Build on a set of fast smoke test bots. If something breaks, it should
> be possible to bisect it to reject a PR and make forward progress.
> Occasionally bisecting a large set of PR's should still be less bot
> time than attempting to build each of them individually.
> Blocking the PR's due to target specific and or slow bot failures
> would be a different question.
> You could probably do this with a linear history, so long as the final
> tree is the same as the merge commit, it should still build.
> I'm just fond of the idea of trying to prevent bad commits from ever
> being merged. Since they sometimes waste everyone's time.
>
> On Fri, 1 Feb 2019 at 04:02, David Greene <[hidden email]> wrote:
>
>     <[hidden email]> writes:
>     
>     > Systems that I've seen will funnel all submitted PRs into a
>     single queue
>     > which *does* guarantee that the trial builds are against HEAD
>     and there
>     > are no "later commits" that can screw it up. If the trial build
>     passes,
>     > the PR goes in and becomes the new HEAD.
>     
>     The downside of a system like this is that when changes are going
>     in
>     rapidly, the queue grows very large and it takes forever to get
>     your
>     change merged. PR builds are serialized and if a "build" means
>     "make
>     sure it builds on all the Buildbots" then it takes a very long
>     time
>     indeed.
>     
>     There are ways to parallelize builds by speculating that PRs will
>     build
>     cleanly but it gets pretty complicated quickly.
>     
>     > But this would be a radical redesign of the LLVM bot system, and
>     would
>     > have to wait until we're done with the GitHub migration and can
>     spend
>     > a couple of years debating the use of PRs. :-)
>     
>     Heh. Fully guaranteeing buildability of a branch is not a trivial
>     task
>     and will take a LOT of thought and discussion.
>     
>     -David
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
On Fri, Feb 01, 2019 at 12:41:05PM -0500, Arthur O'Dwyer via cfe-dev wrote:
> I know GitHub can be configured to reject force-pushes to {any, a
> specific} branch.  I strongly suspect that if *the maintainers of
> LLVM* asked nicely, GitHub would also be able to reject
> merges to {any, a specific} branch.

Anyone with admin permissions in a repo can add "branch protection
rules" that prevent force pushes, there is no need to contact Github
support for that.

> - GitHub PRs have Travis integration so the reviewer can see if a
> particular patch is actually compileable at all, before spending time
> looking at it. I wish Phab had this feature (or maybe it exists and LLVM
> doesn't use it?).

This kind of pre-merge testing is very valuable, it could potentially
prevent some unnecessary reverts by catching issues early.

On caveat is that the test coverage would be limited or else the build
times would be very long. There are quite some builders for various
projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms
and targets (Linux, macOS, Windows, Android, etc.).

With limited resources, Arthur's suggestion seems workable to me:
- Enable pre-commit testing of few configurations (in parallel).
- Encourage developers to wait for tests to pass before pushing changes.
- Do not block hard to avoid a situation where unrelated changes
  (commits or build environment) cause failures.

I would also be in favor of linear history for reasons mentioned before
(easier bisection, more readable logs). Though whatever choice happens
(cherry-pick vs merge), I think it is important to keep the link between
a change and the review. I have seen various strategies:

- Phabricator: manually include a "Differential revision" link in the
  commit message.
- Github (merge via web interface): automatically includes a reference
  to the "Pull Request".
- Github (cherry-pick / rebase and merge with no merge commit):
  unfortunately loses the review information.
- Gerrit: developers cannot merge directly, instead they use the web
  interface to submit a change. This will add a "Reviewed-on" link that
  references the review. (Used by Wireshark, Qt, boringssl, etc.)

Projects that are use mailing lists to review patches (like Linux and
QEMU) commonly include a Message-Id tag in the commit message that
references the original mailing list discussion.

The curl project also uses Github for reviews, but encourages developers
with push permissions to manually fetch the commit, edit the commit
message to reference the review and then push to the master branch (with
no merge commits). Again, this is a manual process and new developers
who are not familiar with this process accidentally create a merge
commit anyway (or forget to reference the review).

Ideally changes go through a single gatekeeper, a tool that recognizes
comments to a merge request and subsequently tries to add extra info to
a commit message (link to a review) and enforce a linear history (by
cherry-picking changes or rebase + merge commits). This tool could be
triggered by posting comments like "@name-of-the-bot merge this".
(If necessary this could be combined with requiring tests to pass.)

Such an extra indirection with a single gatekeeper could be a quite
drastic change from the current development model though. It could
reduce the number of broken builds and improve the quality of commit
messages though.

Just my two cents, hope it helps :)
--
Kind regards,
Peter Wu
https://lekensteyn.nl
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev


> -----Original Message-----
> From: cfe-dev [mailto:[hidden email]] On Behalf Of Peter
> Wu via cfe-dev
> Sent: Friday, February 01, 2019 5:49 PM
> To: Arthur O'Dwyer
> Cc: [hidden email]; [hidden email]; openmp-
> [hidden email]; [hidden email]
> Subject: Re: [cfe-dev] [llvm-dev] [Github] RFC: linear history vs merge
> commits
>
> On Fri, Feb 01, 2019 at 12:41:05PM -0500, Arthur O'Dwyer via cfe-dev
> wrote:
> > I know GitHub can be configured to reject force-pushes to {any, a
> > specific} branch.  I strongly suspect that if *the maintainers of
> > LLVM* asked nicely, GitHub would also be able to reject
> > merges to {any, a specific} branch.
>
> Anyone with admin permissions in a repo can add "branch protection
> rules" that prevent force pushes, there is no need to contact Github
> support for that.
>
> > - GitHub PRs have Travis integration so the reviewer can see if a
> > particular patch is actually compileable at all, before spending time
> > looking at it. I wish Phab had this feature (or maybe it exists and LLVM
> > doesn't use it?).
>
> This kind of pre-merge testing is very valuable, it could potentially
> prevent some unnecessary reverts by catching issues early.
>
> On caveat is that the test coverage would be limited or else the build
> times would be very long. There are quite some builders for various
> projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms
> and targets (Linux, macOS, Windows, Android, etc.).

Some platforms/projects are more prone to breakage than others.  This
has been a particular pain point for my team lately, our auto-merge
hasn't been very auto for quite a while now due to frequent build breaks.

>
> With limited resources, Arthur's suggestion seems workable to me:
> - Enable pre-commit testing of few configurations (in parallel).

So, I've seen that Phabricator has something in the build-it-for-you
vein, which somebody explained not too long ago but I've forgotten the
details and a quick look doesn't turn up anything in my archive.

If that can be made to work generally, and especially if it could run
one each of Linux, Windows, and Mac, that would go a *long* way toward
addressing build breaks (for patches that go through Phab, anyway).
--paulr

> - Encourage developers to wait for tests to pass before pushing changes.
> - Do not block hard to avoid a situation where unrelated changes
>   (commits or build environment) cause failures.
>
> I would also be in favor of linear history for reasons mentioned before
> (easier bisection, more readable logs). Though whatever choice happens
> (cherry-pick vs merge), I think it is important to keep the link between
> a change and the review. I have seen various strategies:
>
> - Phabricator: manually include a "Differential revision" link in the
>   commit message.
> - Github (merge via web interface): automatically includes a reference
>   to the "Pull Request".
> - Github (cherry-pick / rebase and merge with no merge commit):
>   unfortunately loses the review information.
> - Gerrit: developers cannot merge directly, instead they use the web
>   interface to submit a change. This will add a "Reviewed-on" link that
>   references the review. (Used by Wireshark, Qt, boringssl, etc.)
>
> Projects that are use mailing lists to review patches (like Linux and
> QEMU) commonly include a Message-Id tag in the commit message that
> references the original mailing list discussion.
>
> The curl project also uses Github for reviews, but encourages developers
> with push permissions to manually fetch the commit, edit the commit
> message to reference the review and then push to the master branch (with
> no merge commits). Again, this is a manual process and new developers
> who are not familiar with this process accidentally create a merge
> commit anyway (or forget to reference the review).
>
> Ideally changes go through a single gatekeeper, a tool that recognizes
> comments to a merge request and subsequently tries to add extra info to
> a commit message (link to a review) and enforce a linear history (by
> cherry-picking changes or rebase + merge commits). This tool could be
> triggered by posting comments like "@name-of-the-bot merge this".
> (If necessary this could be combined with requiring tests to pass.)
>
> Such an extra indirection with a single gatekeeper could be a quite
> drastic change from the current development model though. It could
> reduce the number of broken builds and improve the quality of commit
> messages though.
>
> Just my two cents, hope it helps :)
> --
> Kind regards,
> Peter Wu
> https://lekensteyn.nl
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [Openmp-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
In reply to this post by Nathan Ridge via cfe-dev
On 01/29/2019 02:48 PM, David Greene wrote:

> Tom Stellard via Openmp-dev <[hidden email]> writes:
>
>> As part of the migration of LLVM's source code to github, we need to update
>> our developer policy with instructions about how to interact with the new git
>> repository.  There are a lot of different topics we will need to discuss, but
>> I would like to start by initiating a discussion about our merge commit
>> policy.  Should we:
>>
>> 1. Disallow merge commits and enforce a linear history by requiring a
>>    rebase before push.
>>
>> 2. Allow merge commits.
>>
>> 3. Require merge commits and disallow rebase before push.
>>
>> I'm going to propose that if we cannot reach a consensus that we
>> adopt policy #1, because this is essentially what we have now
>> with SVN.
>
> I agree with proposing #1 in general.  It results in a nice clean
> history and will be familiar to everyone working on the project.
>
> However, there is a place for merge commits.  If there's a bug in a
> release and we want to make a point release, it might make sense to make
> a commit on the release branch and then merge the release branch to
> master in order to ensure the fix lands there as well.  Another option
> is to commit to master first and then cherry-pick to release.  A third
> option is to use the "hotfix branch" method of git flow [1], which would
> result in a merge commit to the release branch and another merge commit
> to master.
>

Our current bug-fix fix process is to commit to master first and then
cherry-pick to release branches.

> I've seen projects that commit to release first and then merge release
> to master and also projects that commit to master and cherry-pick to
> release.  I personally haven't seen projects employ the git flow hotfix
> branch method rigorously.
>
> But GitHub is read-only for almost a year, right?  So we really don't
> have to decide this for a while.  I have not tried using the monorepo
> and committing to SVN with it.  How does that work?  What would it do
> with merge commits?
>

It will be read-only until October 2019, but I think it's important to decide
on this issue early so we have time to research and implement ways to
automatically enforce our policy to make things as smooth as possible
when we do switch.  I really want us to be able to keep our current migration
schedule.

The GettingStarted patch has been updated with instructions for how to use
the new repo: https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git

-Tom
>                               -David
>
> [1] https://nvie.com/posts/a-successful-git-branching-model
>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
In reply to this post by Nathan Ridge via 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 <[hidden email] <mailto:[hidden email]>> wrote:
>
>     On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev
>     <[hidden email] <mailto:[hidden email]>> wrote:
>     >
>     > Bruce Hoult via llvm-dev <[hidden email] <mailto:[hidden email]>> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
In reply to this post by Nathan Ridge via cfe-dev
On 01/31/2019 09:51 AM, Roman Lebedev via llvm-dev wrote:

> On Thu, Jan 31, 2019 at 8:29 PM David Greene via cfe-dev
> <[hidden email]> wrote:
>>
>> Mehdi AMINI <[hidden email]> 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.
>

This hasn't been decided yet, but this is a whole other discussion
that deserves it own thread (not saying we need to decide this now though).

-Tom

> 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
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
In reply to this post by Nathan Ridge via cfe-dev


On Fri, Feb 1, 2019 at 4:44 PM Tom Stellard <[hidden email]> 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 <[hidden email] <mailto:[hidden email]>> wrote:
>
>     On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev
>     <[hidden email] <mailto:[hidden email]>> wrote:
>     >
>     > Bruce Hoult via llvm-dev <[hidden email] <mailto:[hidden email]>> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
In reply to this post by Nathan Ridge via cfe-dev
On 1 Feb 2019, at 22:48, Peter Wu via cfe-dev <[hidden email]> wrote:

>
> On caveat is that the test coverage would be limited or else the build
> times would be very long. There are quite some builders for various
> projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms
> and targets (Linux, macOS, Windows, Android, etc.).
>
> With limited resources, Arthur's suggestion seems workable to me:
> - Enable pre-commit testing of few configurations (in parallel).
> - Encourage developers to wait for tests to pass before pushing changes.
> - Do not block hard to avoid a situation where unrelated changes
>  (commits or build environment) cause failures.

GitHub does let you block merges (by anyone who isn’t an administrator) of PRs that don’t meet certain requirements.  For one project, we have it set up so that you need to have at least one reviewer saying approved, you have to have signed the CLA, and you have to pass all of the CI checks.  It’s then easy to set up automatic merging when all of the prerequisites are met (you can get a notification and process it automatically). We allow self approval for small changes (not automatically enforced, this is a judgement call, but you can remove people who abuse it from the team quite easily and then they can’t approve changes).

I’ve found it leads to a very nice workflow: developers aren’t in the loop for merges, they just prepare something that they think works, submit it, and get on with something else.  If you’re lucky, someone approves it and you pass CI first go, then everything is fine.  Reviewers can wait until CI is finished and not bother looking for things that are going to break.  If the change introduces new tests, reviewers know that those tests are passing.  If you broke a platform that you don’t test on locally, you get notified but no one else is spammed with breakage reports.  Once you’ve fixed it, you get a review, and make any changes.  The master branch is always buildable and passing CI.  If your changes break CI, you don’t have a race to fix things before someone reverts your change, you just fix things in the PR and wait for it to be automatically merged.

David

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
On Sat, Feb 2, 2019 at 7:32 AM David Chisnall via cfe-dev <[hidden email]> wrote:
On 1 Feb 2019, at 22:48, Peter Wu via cfe-dev <[hidden email]> wrote:
>
> On caveat is that the test coverage would be limited or else the build
> times would be very long. There are quite some builders for various
> projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms
> and targets (Linux, macOS, Windows, Android, etc.).
>
> With limited resources, Arthur's suggestion seems workable to me:
> - Enable pre-commit testing of few configurations (in parallel).
> - Encourage developers to wait for tests to pass before pushing changes.
> - Do not block hard to avoid a situation where unrelated changes
>  (commits or build environment) cause failures.

GitHub does let you block merges (by anyone who isn’t an administrator) of PRs that don’t meet certain requirements.  For one project, we have it set up so that you need to have at least one reviewer saying approved, you have to have signed the CLA, and you have to pass all of the CI checks.  It’s then easy to set up automatic merging when all of the prerequisites are met (you can get a notification and process it automatically). We allow self approval for small changes (not automatically enforced, this is a judgement call, but you can remove people who abuse it from the team quite easily and then they can’t approve changes).

I’ve found it leads to a very nice workflow: developers aren’t in the loop for merges, they just prepare something that they think works, submit it, and get on with something else.  If you’re lucky, someone approves it and you pass CI first go, then everything is fine.  Reviewers can wait until CI is finished and not bother looking for things that are going to break.  If the change introduces new tests, reviewers know that those tests are passing.  If you broke a platform that you don’t test on locally, you get notified but no one else is spammed with breakage reports.  Once you’ve fixed it, you get a review, and make any changes.  The master branch is always buildable and passing CI.  If your changes break CI, you don’t have a race to fix things before someone reverts your change, you just fix things in the PR and wait for it to be automatically merged.
Compared to the current model, the CI in your description operates on more branches. I imagine it uses more machine resources.
 

David

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
It's worth mentioning that Travis CI allows open source projects free access to their CI service, and they integrate with Github PRs. (https://travis-ci.com/plans)

On Sat, Feb 2, 2019 at 9:48 AM Hubert Tong via llvm-dev <[hidden email]> wrote:
On Sat, Feb 2, 2019 at 7:32 AM David Chisnall via cfe-dev <[hidden email]> wrote:
On 1 Feb 2019, at 22:48, Peter Wu via cfe-dev <[hidden email]> wrote:
>
> On caveat is that the test coverage would be limited or else the build
> times would be very long. There are quite some builders for various
> projects (llvm, cfe, compiler-rt, etc.) on a lot of different platforms
> and targets (Linux, macOS, Windows, Android, etc.).
>
> With limited resources, Arthur's suggestion seems workable to me:
> - Enable pre-commit testing of few configurations (in parallel).
> - Encourage developers to wait for tests to pass before pushing changes.
> - Do not block hard to avoid a situation where unrelated changes
>  (commits or build environment) cause failures.

GitHub does let you block merges (by anyone who isn’t an administrator) of PRs that don’t meet certain requirements.  For one project, we have it set up so that you need to have at least one reviewer saying approved, you have to have signed the CLA, and you have to pass all of the CI checks.  It’s then easy to set up automatic merging when all of the prerequisites are met (you can get a notification and process it automatically). We allow self approval for small changes (not automatically enforced, this is a judgement call, but you can remove people who abuse it from the team quite easily and then they can’t approve changes).

I’ve found it leads to a very nice workflow: developers aren’t in the loop for merges, they just prepare something that they think works, submit it, and get on with something else.  If you’re lucky, someone approves it and you pass CI first go, then everything is fine.  Reviewers can wait until CI is finished and not bother looking for things that are going to break.  If the change introduces new tests, reviewers know that those tests are passing.  If you broke a platform that you don’t test on locally, you get notified but no one else is spammed with breakage reports.  Once you’ve fixed it, you get a review, and make any changes.  The master branch is always buildable and passing CI.  If your changes break CI, you don’t have a race to fix things before someone reverts your change, you just fix things in the PR and wait for it to be automatically merged.
Compared to the current model, the CI in your description operates on more branches. I imagine it uses more machine resources.
 

David

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
The way it decides that there are "outdated comments" and that those comments should be hard to does not win it any points in my book (Hubert Tong)

IIUC, Github has recently changed this, and is now more like Phabricator. (in that it now seems to add a mark to say the diff is likely outdated, but has a separate "Resolve Conversation" button). That said, this might bring up a different incentive for staying with Phabricator, which is that Github can and does make minor changes to their PR workflow over time, while LLVM would retain more control over their Phabricator instance.


Full disclosure: "Revert" commits also wreak havoc on "git bisect" and "git blame"

As an outsider, I'm not sure I fully understand the comments about `git bisect` (and blame) not being able to deal with merge and revert commits. Having worked mostly on the JuliaLang project (which does not discourage merge commits), we've not typically had much difficulty with either. The bisect tool is intelligent about being able to isolate which branch of the graph introduced (or fixed) the behavior of interest, and can isolate within that branch which commit first show the change. The presence of a revert commit can be a minor speed-bump (especially if it means the build previously failed), but "wreak havoc" seems like a bit of hyperbole for needing to call git blame again—and doesn't seem much different from how svn would necessarily handle the same situation?

One point that may be worth mentioning though is that merge commits are also themselves unrestricted changes(*), which can be an interesting gotcha (it's hard to represent this diff). So if merges are allowed in the timeline, LLVM may additionally decide whether to permit human-assisted merges, or to ask that developers rebase the branch first in that case. (*In git, each commit is a record of the state of all contents of the repo, plus one or more parent links to the previous state. A merge commit is distinct only in that it has multiple parents that are indicated as contributing in some way to the final state, but they aren't otherwise restricted in what gets changed.)

There is however a philosophic difference of various git coding styles that LLVM can chose. One school of thought suggests that you should never rebase code, and instead frequently merge branches in all directions. This gives the advantage that a `git bisect` attempt will be considering the code as originally written, and so the result of `git bisect` or `blame` may be more likely to tell whether a later discovered issue (or other archeology reason) arose from an issue with the original commit, or arose as a result of a conflict with a simultaneous change.

A counter school of thought instead argues for preserving a simpler (or in the limit, a linear) ordering of all commits. I won't say as much about this, since I think the benefits of this are already obvious to a community that already requires it.

I also won't say that one school is better or worse than the other (I happen to use a hybrid in most of my projects). There are also other approaches to commit management I haven't mentioned, but I wanted to at least share some thoughts, in the hopes that someone might find the information useful.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
In reply to this post by Nathan Ridge via cfe-dev
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 <[hidden email]> wrote:


On Wed, Jan 30, 2019 at 1:19 PM Eric Christopher via cfe-dev <[hidden email]> wrote:
On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev
<[hidden email]> wrote:
>
> Bruce Hoult via llvm-dev <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [Github] RFC: linear history vs merge commits

Nathan Ridge via cfe-dev
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 <[hidden email] 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 <[hidden email]> wrote:


On Wed, Jan 30, 2019 at 1:19 PM Eric Christopher via cfe-dev <[hidden email]> wrote:
On Wed, Jan 30, 2019 at 12:42 PM David Greene via cfe-dev
<[hidden email]> wrote:
>
> Bruce Hoult via llvm-dev <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
12