Rockbox.org home
release
dev builds
extras
themes manual
wiki
device status forums
mailing lists
IRC bugs
patches
dev guide



Rockbox mail archive

Subject: Proposal for git commit guidelines/policy

Proposal for git commit guidelines/policy

From: Torne Wuff <torne+rockboxdev_at_wolfpuppy.org.uk>
Date: Thu, 20 Oct 2011 22:26:09 +0100

Hi folks,

I'm looking at the next stages of the git migration and this is one of
the things we need to resolve. This is a first proposal (subject to
input from you all) of a git commit policy (or guidelines, if you
prefer), including some rationale. Please read the whole thing
carefully before commenting :)

This assumes some knowledge of how git works, and is not intended to
be used as a tutorial document. This is also not intended to cover use
of Gerrit's patch review functionality. Uploading patches for review
works somewhat differently from directly pushing, and we don't need to
decide how this should work right away.

Why do we need a commit policy?
================================

SVN only has one way of working: you check out, you make changes, you
try and commit, and it fails if anyone else committed before you. You
then have to update, resolve conflicts, and try and commit again. This
means there's not really any policy element there: you are always
committing directly on top of all existing changes.

Git allows virtually an infinite flexibility of workflow: you can
rebase, you can merge, you can merge-without-fastforward, you can
create patches and email them, you can do various other weird
combinations of things. Different git users, and different git-based
projects, use different workflows, and not always the same workflow
all the time. This is a feature, of course: being limited to SVN's
strict only-commit-on-top workflow is a pain sometimes. It's also a
potential problem for other people that work on the project: if you
manage to create some particularly weird shaped and weird looking
history, you can end up pushing something to the server that confuses
other developers greatly (as well as users trying to bisect
revisions). If you don't believe that this is true, then I can
probably find some examples for you to look at - I accidentally create
them while doing complex rebases on a reasonably frequent basis ;)

So, in the interests of everyone's sanity, it's good to have some best
practises. They are not meant to apply to 100% of cases, but they
should only be ignored when you have a really good reason, and ideally
when other people also concur that you have a really good reason :)

Also, users who are new to git may find the lack of workflow
constraints to be difficult: they may get different advice on how to
work depending who they ask, and so it helps to have one suggestion to
refer people to.

The policy
================================

Throughout this section, when we refer to "origin/master" we mean the
canonical copy of master on the Rockbox server, not any branch named
"master" in any other remote repository.

1) If you started with some version of origin/master, and then made a
local commit, and then when you try to push you find you are outdated:
please rebase your local branch onto the latest version of
origin/master. Avoid merging origin/master into your branch.

Rationale: If you have just made one change to the branch, then either
that change conflicts with the latest version, in which case both a
rebase and a merge will require you to resolve those commits by hand
identically, or that change does not conflict with the latest version,
in which case a rebase will succeed with no manual intervention. There
is no extra effort for you, and the result is a cleaner history which
does not contain a pointless merge commit and is easy to read due to
its linearity.

2) If you started with some version of origin/master, and then made
multiple local commits in a series: consider whether you should
flatten the commits before pushing.

A good indicator that a series of commits should probably be flattened
into a single commit is if the intermediate states do not compile or
work, or if you cannot think of any way to describe them individually
other than "part N of M". The goal is for each commit to represent a
*logical* change: if your overall change is divided into three logical
steps, feel free to make three commits with appropriate titles and
descriptions, but if your three commits are "the changes I made on
Monday, the changes I made on Tuesday, and the changes I made on
Wednesday" then it is likely that the intermediate states are
interesting to nobody, not even you in the future, and flattening them
is a clear improvement.

Rationale: Making multiple local commits is perfectly sensible and a
good way to track any work that you don't complete in a single
session, but often once the work is complete the split into multiple
commits is arbitrary and the intermediate states are useless.
Flattening into a single commit is trivial and makes history simpler
for everyone in future. Reworking a set of N commits into a different
set of M commits is more effort, but can be done to turn a series of
time-based commits into a logical series.

3) If you started with some version of origin/master, made some number
of local commits, and now want to update your branch to a newer
version of origin/master without submitting: rebase your local branch
onto the latest version of origin/master. Avoid merging origin/master
into your branch.

Rationale: This is the same case as 1). If your work lives in a
private branch on your local repository, there is rarely a reason to
prefer merging to rebasing.

4) If you publish a branch for collaboration with other people:

a) Don't rebase that branch while working on it. Rebasing a branch you
have published means everyone who has a copy of that branch needs to
manually fix things by rebasing their own work onto your rebased
changes, and nobody will be happy about this. In order to get new
changes from origin/master into a published branch, you will need to
merge origin/master into your branch.

b) When the work in the branch is done and you are ready to push back
to origin/master: please consider rewriting the branch (some
combination of flattening and rebasing), rather than just pushing it
as-is.

The same logic as in 2) applies: it's nicer if the changes that end up
on origin/master represent logical steps, and it's likely that many of
the intermediate states are not interesting to anyone now that the
change is complete. However, this can be substantially more difficult,
depending exactly what changes occurred. It's always possible to
simply flatten the entire branch to a single commit, but that may
remove too much information. More complex rewrites are often hard. If
you are sufficiently confident of your git abilities, please do
attempt it, but it is okay to admit defeat and leave the branch either
as-is, or in a partially rewritten state which still contains merges.
Before pushing a branch in this condition, talk to other committers
about it.

Rationale: History cleanliness is strongly preferred in cases where it
is no more effort than merging, but lack of any sensible way to reach
a clean history should not block a commit. If nobody sees a sane way
to make things clean, then everyone will have to tolerate a patch of
more complex history which contains merges.

Overall thoughts
================================

This policy may seem somewhat unusual to some git users, but in most
cases it should not actually be any more difficult than other
workflows. The process of creating a neat patch series should be
familiar to anyone who has worked on the Linux kernel, and Linux's
history is generally fairly tidy as a result. This kind of commit
structure is easier to review and easier to read through.

Enforcement
================================

There will be very soft technical enforcement of this policy. By
default, Gerrit (our git hosting platform) will not allow you to push
a merge commit to origin/master; you will be forced to rebase before
you can submit. This serves two ends: helping committers who are new
to git avoid complicated mistakes, and encouraging committers who are
experienced with git and normally use a different workflow to seek out
the policy document and review it.

There is a Gerrit user group containing users who are allowed to push
merge commits. Any committer may add themselves to the group (or ask
another committer to do it for them, if they don't know how to
navigate Gerrit). The only restriction on joining the group is that we
ask you to read and consider this policy document first, and use your
power of merging wisely.

Thanks in advance for reading this! Thoughts, comments?

-- 
Torne Wuff
torne_at_wolfpuppy.org.uk
Received on 2011-10-20

Page was last modified "Jan 10 2012" The Rockbox Crew
aaa