Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss
Switching to the Big Green Button?
9 views
Skip to first unread message
Michael Cooper
Jan 20, 2015, 7:03:10 PM1/20/15
to dev-...@lists.mozilla.org
(Sorry this got a bit long. the short of it is I think we should stop
rebasing.)
I've been thinking about this for a while. The way we land code is a bit
goofy, compared to many other projects. It has advantages and
disadvantages. I'd like to consider switching. I'll summarize things:
# Rebase based landing (current)
When a PR is ready to to land, the author (in the case of in-team PRs), or
the reviewer (in the case of contributor PRs), rebases the commits into
(usually) one commit, and pushes that to master. There are no merge
commits, and history is (generally) linear
Pros:
* Linear history is easier to reason about.
* Status Quo.
Cons:
* More human action in the landing process, which makes it more error prone
and slower.
* Does not jive well with GitHub's PR UI.
# Green Button based landing
When a PR is ready to land, someone presses the big green "merge" button in
the GitHub UI. Who exactly does this is subject to more discussion. This
merges the PR to the target branch (generally master), and closes the PR.
This always creates a merge commit with summaries like "Merge pull request
#447 from rlr/upgrade-celery-1104863". The description of the commit is
user editable.
Pros:
* More automation in the landing process means there is a smaller chance of
human error in the landing process, and makes it faster.
* More familiar to contributors.
* Commits on master are linked to pull requests directly.
Cons:
* Non-linear history can be harder to reason about.
* Tools often make non-linear histories confusing to look at by default
(git log, GitHub commit log, etc.).
Additionally, the arguments I have seen have seen often involve hand-wavey
complaints about merge commits being hard/impossible to `git bisect`
through, various procedural problems, and some tools not working. I'm not
including these (yet) in the cons because I haven't actually ever run into
any of these. That being said, most of my git experience has been on
kitsune, where we have linear history.
# How should history be represented?
A big green button based approach creates a merge commit for every PR.
Personally, I think this better represents the truth of how development
works. Rebasing presents a linear version of history, which often
represents the truth of how things are deployed.
Additionally, some projects that use the big green button keep all the of
the messy details of the PR review process in the branch. This is sometimes
called "seeing how the sausage is made". Others rebase the branch into a
few clean commits, much how we rebase now. I believe Buddyup uses the
rebase-then-merge strategy, and that Socorro uses the "sausage making"
method.
I think rebase-then-merge is the worst of both worlds. It still requires
manual intervention, and also creates non-linear history. the sausage
making method requires only a click of a button in a web ui most of the
time (barring merge conflicts).
"Sausage making" has the further advantage that after something has gone
through review, the reviewer can click the green button to merge the code
immediately instead of waiting for the original author to come back and do
more work to rebase and land the code.
# What now?
Will has been using a green button based flow for a while on Fjord. I'm
very interested in how that has turned out. Is it easier? Is it less
error-prone? Is it hard to work with? Are tools annoying?
I think that non-linear history is not a problem for me. I'd prefer to
never have to change history or `git push -f`. For me that makes Socorro's
sausage making big green button the winner.
If Fjord's workflow is going well, then I'd like to propose we switch to a
sausage-making big green button workflow. I definitely want to hear from
Will about his experience here though.
-mythmon
rebasing.)
I've been thinking about this for a while. The way we land code is a bit
goofy, compared to many other projects. It has advantages and
disadvantages. I'd like to consider switching. I'll summarize things:
# Rebase based landing (current)
When a PR is ready to to land, the author (in the case of in-team PRs), or
the reviewer (in the case of contributor PRs), rebases the commits into
(usually) one commit, and pushes that to master. There are no merge
commits, and history is (generally) linear
Pros:
* Linear history is easier to reason about.
* Status Quo.
Cons:
* More human action in the landing process, which makes it more error prone
and slower.
* Does not jive well with GitHub's PR UI.
# Green Button based landing
When a PR is ready to land, someone presses the big green "merge" button in
the GitHub UI. Who exactly does this is subject to more discussion. This
merges the PR to the target branch (generally master), and closes the PR.
This always creates a merge commit with summaries like "Merge pull request
#447 from rlr/upgrade-celery-1104863". The description of the commit is
user editable.
Pros:
* More automation in the landing process means there is a smaller chance of
human error in the landing process, and makes it faster.
* More familiar to contributors.
* Commits on master are linked to pull requests directly.
Cons:
* Non-linear history can be harder to reason about.
* Tools often make non-linear histories confusing to look at by default
(git log, GitHub commit log, etc.).
Additionally, the arguments I have seen have seen often involve hand-wavey
complaints about merge commits being hard/impossible to `git bisect`
through, various procedural problems, and some tools not working. I'm not
including these (yet) in the cons because I haven't actually ever run into
any of these. That being said, most of my git experience has been on
kitsune, where we have linear history.
# How should history be represented?
A big green button based approach creates a merge commit for every PR.
Personally, I think this better represents the truth of how development
works. Rebasing presents a linear version of history, which often
represents the truth of how things are deployed.
Additionally, some projects that use the big green button keep all the of
the messy details of the PR review process in the branch. This is sometimes
called "seeing how the sausage is made". Others rebase the branch into a
few clean commits, much how we rebase now. I believe Buddyup uses the
rebase-then-merge strategy, and that Socorro uses the "sausage making"
method.
I think rebase-then-merge is the worst of both worlds. It still requires
manual intervention, and also creates non-linear history. the sausage
making method requires only a click of a button in a web ui most of the
time (barring merge conflicts).
"Sausage making" has the further advantage that after something has gone
through review, the reviewer can click the green button to merge the code
immediately instead of waiting for the original author to come back and do
more work to rebase and land the code.
# What now?
Will has been using a green button based flow for a while on Fjord. I'm
very interested in how that has turned out. Is it easier? Is it less
error-prone? Is it hard to work with? Are tools annoying?
I think that non-linear history is not a problem for me. I'd prefer to
never have to change history or `git push -f`. For me that makes Socorro's
sausage making big green button the winner.
If Fjord's workflow is going well, then I'd like to propose we switch to a
sausage-making big green button workflow. I definitely want to hear from
Will about his experience here though.
-mythmon
will kahn-greene
Jan 20, 2015, 7:47:45 PM1/20/15
to dev-...@lists.mozilla.org
I use the big green button on Fjord and that works fine. I think one
aspect this email doesn't cover is that in the years since we adopted
the rebase methodology, git has progressed and git-based tools are much
better. That's one of the things I was thinking about when I switched Fjord.
Most of the issues I had years ago with the big green button, I either
no longer have or can't reproduce, so they're probably non-issues now.
The big green button is generally easier for me and easier for
contributors. The last part is pretty key.
One thing I want to talk about is commits in a PR. If all commits in a
PR have the bug number and conform to git commit message conventions and
each commit passes the test suite, then I'm unlikely to say "squash
these commits" or "redo the commits". However, in most cases, what
happens is a PR is created, then comments happen, then the author
(including me) throws commits in with messages like "Fix typo" until the
PR is approved. This can be a lot of commits. Keeping the missteps which
sometimes fail tests and often have poor and unhelpful commit messages
isn't useful to me. In these cases I say, fix the commits in the PR
which is usually easiest done by squashing them.
Anyhow, that's my thoughts.
The one thing I want is a git-bgbmerge subcommand that would simulate a
big green button press and generate a similar-ish merge commit. This
would make it easier for me to manually merge some things that are
easier to manually merge.
/will
aspect this email doesn't cover is that in the years since we adopted
the rebase methodology, git has progressed and git-based tools are much
better. That's one of the things I was thinking about when I switched Fjord.
Most of the issues I had years ago with the big green button, I either
no longer have or can't reproduce, so they're probably non-issues now.
The big green button is generally easier for me and easier for
contributors. The last part is pretty key.
One thing I want to talk about is commits in a PR. If all commits in a
PR have the bug number and conform to git commit message conventions and
each commit passes the test suite, then I'm unlikely to say "squash
these commits" or "redo the commits". However, in most cases, what
happens is a PR is created, then comments happen, then the author
(including me) throws commits in with messages like "Fix typo" until the
PR is approved. This can be a lot of commits. Keeping the missteps which
sometimes fail tests and often have poor and unhelpful commit messages
isn't useful to me. In these cases I say, fix the commits in the PR
which is usually easiest done by squashing them.
Anyhow, that's my thoughts.
The one thing I want is a git-bgbmerge subcommand that would simulate a
big green button press and generate a similar-ish merge commit. This
would make it easier for me to manually merge some things that are
easier to manually merge.
/will
will kahn-greene
Jan 20, 2015, 7:48:49 PM1/20/15
to dev-...@lists.mozilla.org
Also, Kitsune should totally steal the git commit message linter we have
in Fjord that L. Guruprasad wrote. It's fab. Totally ended the
occasional bug number typo I sometimes did.
in Fjord that L. Guruprasad wrote. It's fab. Totally ended the
occasional bug number typo I sometimes did.
Michael Cooper
Jan 21, 2015, 8:36:34 PM1/21/15
to will kahn-greene, dev-...@lists.mozilla.org
+1 to having a git commit linter.
I glossed over the evolution of tools over the past few years mainly
because I don't know much about that. I'm glad to hear that tools have
gotten better, and I'm not just crazy when I don't see the tooling
difficulties.
I think this is pretty well received. I have some ideas for a policy now:
* PRs that pass review should be merged by the reviewer using the BGB.
* PRs not meant for merging should be marked as such, with things like
"f?", "WIP", or "DO NOT MERGE". No-one should merge these PRs.
* If a PR is up for review (r?), then clicking the merge button implies an
r+ by the merger.
* Anyone can merge a branch after it has been r+ed (though it should not be
sitting long after r+)
* Branches may be rebased to hide the review process (sausage making), but
do not need to be.
* All commits on a branch should contain bug numbers (where applicable) and
have good commit messages (not "doh" "wat" or "oops"). It doesn't matter if
the bad commits never exist or simply get rebased out before merge.
* Any multi step deploys (complicated migrations, es index changes, etc)
must not use the BGB or merge commits, and must create linear history on
master. bug 1124497 comment 1
<https://bugzilla.mozilla.org/show_bug.cgi?id=1124497#c1> has details about
why this is important, plus awesome ASCII pictures.
If these look goofy, I'd love someone to word smith them a bit more. Once
we are happy with these or a similar set of rules we should put them in the
docs.
I'm not totally sure of the utility of git-bgbmerge. I don't think I'd use
it much. But that doesn't mean it can't exist.
On Tue, Jan 20, 2015 at 4:48 PM, will kahn-greene <wkahn...@mozilla.com>
wrote:
>> _______________________________________________
> dev-sumo mailing list
> dev-...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-sumo
>
I glossed over the evolution of tools over the past few years mainly
because I don't know much about that. I'm glad to hear that tools have
gotten better, and I'm not just crazy when I don't see the tooling
difficulties.
I think this is pretty well received. I have some ideas for a policy now:
* PRs that pass review should be merged by the reviewer using the BGB.
* PRs not meant for merging should be marked as such, with things like
"f?", "WIP", or "DO NOT MERGE". No-one should merge these PRs.
* If a PR is up for review (r?), then clicking the merge button implies an
r+ by the merger.
* Anyone can merge a branch after it has been r+ed (though it should not be
sitting long after r+)
* Branches may be rebased to hide the review process (sausage making), but
do not need to be.
* All commits on a branch should contain bug numbers (where applicable) and
have good commit messages (not "doh" "wat" or "oops"). It doesn't matter if
the bad commits never exist or simply get rebased out before merge.
* Any multi step deploys (complicated migrations, es index changes, etc)
must not use the BGB or merge commits, and must create linear history on
master. bug 1124497 comment 1
<https://bugzilla.mozilla.org/show_bug.cgi?id=1124497#c1> has details about
why this is important, plus awesome ASCII pictures.
If these look goofy, I'd love someone to word smith them a bit more. Once
we are happy with these or a similar set of rules we should put them in the
docs.
I'm not totally sure of the utility of git-bgbmerge. I don't think I'd use
it much. But that doesn't mean it can't exist.
On Tue, Jan 20, 2015 at 4:48 PM, will kahn-greene <wkahn...@mozilla.com>
wrote:
> dev-sumo mailing list
> dev-...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-sumo
>
0 new messages