Versions Compared

Old Version 1

changes.mady.by.user Former user

Saved on

compared with

New Version Current

changes.mady.by.user Former user

Saved on

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.

When using Linaro's Git server, the way we do patch review is using Gerrit.

...

  • Git will create one review per commit, so if you are in a branch with more than one commit ahead of master, you'll end up with multiple reviews.
  • Gerrit works by annotating the commit message, adding "Change-Id: HASH". If you update the patch, make sure you keep that line when sending another review.
  • Reviews are based on master by default. If you want to push a review to a specific branch, you need to specify "-R branch".
  • Gerrit can only see what's committed. Make sure you have added/removed and committed all files in your directory before review.

So, a normal sequence would be to make the modifications you need, add all new files and commit, with a nice commit message, then:
$ git review

This will make sure everything is correct and send Gerrit a review of your patch, and show you the link, so that you can see the result, add reviewers, etc. (see below).

...

  • 0 or neutral: when you have only comments and you're not sure how you feel about a patch.
  • +1 or approved: when you're confident that the patch is good, but it would be good to have someone else to look at it.
  • +2 or merge: when you're absolutely confident that no one else needs to see the patch to merge.
  • -1 or cautious: when you don't think the patch is good, but it can either be made better, or you could just be wrong about it.
  • -2 or refusal: when you're pretty sure the patch is bad or completely broken and no one else should approve before convincing you.

howeverHowever, these statuses have different meanings, depending on which repository / branch the patch is proposed to.

...

Given how easy it is to manipulate Git repositories, accidents can be fixed mostly painlessly, so allowing for a bit extra power for our engineers pays off in the long term.

How many +1s equal one +2?

Some teams have rules like: "2 +1s are equal 1 +2". Meaning that, if two people +1, another can +2 and merge.

Some teams consider only reviewers' +1s, others allow for the author to +1 too (meaning any +1 means merge).

Mostly, these rules depend on what kind of repository we have and what we use it for. Ideally, we should have different rules for different use cases, but this complicates things.

So, to simplify, we should follow the simple rule that, if someone else added a +1, you can +2 and merge.

If that rule proves problematic, we can review on a case by case basis.

Self Approval

As mentioned above, it's ok to approve your own patches on a repository that you are developing by yourself, or on a local branch. Given that Linaro's Git doesn't allow certain types of pushes (mostly to master branches on official repositories - ex. leg/hpc/*), Gerrit is our only choice.

...