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

Fri Feb 1 09:41:05 PST 2019

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 <
cfe-dev at lists.llvm.org> 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 <Jeremy.Lakeman at gmail.com> 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 <dag at cray.com> wrote:
> >
> >     <paul.robinson at sony.com> 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
> 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/cfe-dev/attachments/20190201/8f1b8a54/attachment.html>