Gerrit » History » Version 15
neels, 06/17/2016 04:06 PM
1 | 1 | zecke | h1. Contributing using Gerrit |
---|---|---|---|
2 | |||
3 | 11 | laforge | {{>toc}} |
4 | |||
5 | 10 | laforge | At [[OpenBSC:OsmoDevCon2016]] we discussed problems with our past contribution / patch submission process using mails on the mailing list as well as patchwork. The result is that we want to give Gerrit a try for some time and see if it helps us to have a better process |
6 | 1 | zecke | |
7 | 10 | laforge | Gerrit is a review tool that integrates nicely with git and ssh. You can find general information about Gerrit at https://www.gerritcodereview.com/ |
8 | 1 | zecke | |
9 | 10 | laforge | The advantages of Gerrit are: |
10 | * patch submission status is automatically tracked, also with several revisions for a patch set. |
||
11 | * patches are build-tested (and possibly even further tested) by jenkins before they are applied |
||
12 | * developers + maintainers can formally vote on a patch (developer: -1/0/+1, maintainer: -2/0/+2) |
||
13 | * once a patch has +2 score, it can be (automatically) merged into master |
||
14 | * patch sumissions not via git send-email but direcly from git |
||
15 | |||
16 | h2. Osmocom Subprojects using Gerrit |
||
17 | |||
18 | 1 | zecke | The following projects use Gerrit to contribute changes: |
19 | |||
20 | * libosmocore.git |
||
21 | * libosmo-abis.git |
||
22 | * libosmo-netif.git |
||
23 | * libosmo-sccp.git |
||
24 | * libsmpp34.git |
||
25 | * openbsc.git |
||
26 | * osmo-bts.git |
||
27 | * osmo-iuh.git |
||
28 | * osmo-pcu.git |
||
29 | 5 | zecke | * cellmgr-ng.git |
30 | 1 | zecke | * osmo-sip-connector.git |
31 | |||
32 | h2. Configuring Gerrit/Account |
||
33 | |||
34 | 10 | laforge | You will need to sign-up at https://gerrit.osmocom.org/login/. If you have an Osmocom Redmine account you can use https://osmocom.org/openid as OpenID provider. If you have no Osmocom redmine account, you can simply create one online at the "Register" link in the upper right corner. |
35 | |||
36 | Even without an existing or new redmine account, you should also be able to use any other OpenID provider to authenticate against gerrit (untested). |
||
37 | |||
38 | After the initial sign-up you will need to: |
||
39 | 1 | zecke | |
40 | * Pick a username (can not be changed) |
||
41 | * Add your public ssh key(s) |
||
42 | * Add email addresses you intend to use as author/comitter |
||
43 | |||
44 | h2. Setting up Gerrit for commits and pushing |
||
45 | |||
46 | 2 | zecke | * Add the remote to be able to fetch and push to gerrit |
47 | * Fetch the commit hook that adds Change-Id to each commit to uniquely identify a commit |
||
48 | |||
49 | <pre> |
||
50 | 7 | neels | USERNAME=gerrit_user_name |
51 | PROJECT=$(basename $PWD) |
||
52 | git remote add gerrit ssh://$USERNAME@gerrit.osmocom.org:29418/$PROJECT.git |
||
53 | scp -P 29418 $USERNAME@gerrit.osmocom.org:hooks/commit-msg .git/hooks/ |
||
54 | </pre> |
||
55 | |||
56 | * In case your local username matches the gerrit username, the setup shortens to |
||
57 | |||
58 | <pre> |
||
59 | PROJECT=$(basename $PWD) |
||
60 | git remote add gerrit ssh://gerrit.osmocom.org:29418/$PROJECT.git |
||
61 | scp -P 29418 gerrit.osmocom.org:hooks/commit-msg .git/hooks/ |
||
62 | </pre> |
||
63 | |||
64 | Then |
||
65 | |||
66 | * Push for review |
||
67 | <pre> |
||
68 | git push gerrit HEAD:refs/for/master |
||
69 | </pre> |
||
70 | |||
71 | * Push a user branch |
||
72 | <pre> |
||
73 | 9 | neels | git push gerrit HEAD:refs/heads/users/$USERNAME/topic |
74 | 7 | neels | </pre> |
75 | |||
76 | * Directly push to master if you are allowed to |
||
77 | <pre> |
||
78 | git push gerrit HEAD:refs/heads/master |
||
79 | </pre> |
||
80 | |||
81 | * List changesets in gerrit |
||
82 | <pre> |
||
83 | git ls-remote gerrit changes/* |
||
84 | 2 | zecke | </pre> |
85 | 12 | msuraev | |
86 | h2. Tips and Tricks |
||
87 | |||
88 | If you need to adjust and re-submit patches the easiest way is to create throw-away branch ("R D" in magit-gerrit in emacs for example), make you changes/amendments and than send patch(es) back to gerrit while removing temporary branch automatically with "git review -f". |
||
89 | 13 | neels | |
90 | h2. Submitting Branches |
||
91 | |||
92 | On a feature branch, one typically has numerous commits that depend on their preceding commits. |
||
93 | Unfortunately, Gerrit has some issues with those, which boil down to these rules: |
||
94 | |||
95 | 1. Do not push your branch "privately" to the gerrit repository. You can, but Gerrit will force |
||
96 | you to remove all Change-Ids from commit logs once you want to submit it to master. |
||
97 | |||
98 | 2. To re-submit a branch, make sure to cosmetically tweak the branch's first commit log message |
||
99 | before each re-submission (keep the Change-Id, really just a cosmetic change). |
||
100 | |||
101 | How to edit commit logs: |
||
102 | |||
103 | <pre> |
||
104 | cd openbsc |
||
105 | git co my-branch |
||
106 | git rebase -i master |
||
107 | # replace all 'pick' with 'r' (or 'reword'), exit your editor |
||
108 | # git presents each commit log message for editing |
||
109 | </pre> |
||
110 | |||
111 | h3. Why is this is necessary? |
||
112 | |||
113 | There are different merge strategies that Gerrit performs to accept your patches. |
||
114 | Each project can be configured to a specific merge strategy, but unfortunately you can't |
||
115 | decide on a strategy per patch submission. |
||
116 | |||
117 | It seems that the "Merge if Necessary" strategy is best supported, but it creates non-linear |
||
118 | history with numerous merge commits that are usually not at all necessary. |
||
119 | |||
120 | Instead, the "Cherry Pick" strategy puts each patch onto current master's HEAD to create |
||
121 | linear history. However, this will cause merge failures as soon as one patch depends on |
||
122 | another submitted patch, as typical for a feature branch submission. |
||
123 | |||
124 | So we prefer the "Rebase if Necessary" strategy, which always tries to apply your patches to |
||
125 | the current master HEAD, in sequence with the previous patches on the same branch. |
||
126 | However, some problems still remain, including some bugs in "Rebase if Necessary". |
||
127 | |||
128 | There are in general three problems with "Rebase if Necessary", with different solutions: |
||
129 | |||
130 | |||
131 | h4. Fast-Forward |
||
132 | |||
133 | If your branch sits at master's HEAD, Gerrit may refuse to accept the submission, because |
||
134 | it thinks that no new changes are submitted. This is a bug in Gerrit, which holger has |
||
135 | fixed manually in our Gerrit installation. |
||
136 | |||
137 | https://bugs.chromium.org/p/gerrit/issues/detail?id=4158 |
||
138 | |||
139 | |||
140 | h4. Amending a Submitted Branch, First Commit Unchanged |
||
141 | |||
142 | Say you have submitted a branch with 5 commits, and the third commit has gotten a -1 vote. |
||
143 | You have fixed the problem in your local git and now would like to re-submit it. Of course |
||
144 | you want to keep the Change-Ids identical so that Gerrit shows them as amends. |
||
145 | |||
146 | Gerrit will refuse to accept your patch submission if the first commit on the branch is |
||
147 | still identical. |
||
148 | |||
149 | The quickest work-around: slightly reword the first branch commit's commit log message. |
||
150 | Yes, that's enough! All commits that are identical on the branch are now ignored, |
||
151 | it's only the first branch commit that Gerrit will complain about. |
||
152 | |||
153 | The cause: Gerrit refuses to accept a commit with a Change-Id that it already knows and |
||
154 | where the commit hash is identical. |
||
155 | |||
156 | Some details: you could modify all the Change-Ids, but now your branch submission would |
||
157 | open entirely new review entries and you would have to abandon your previous submission. |
||
158 | Comments on the first submission are lost and you cannot diff between patch sets. |
||
159 | Now, if you just cosmetically tweak the first commit's log message, the commit hash |
||
160 | is changed. Since the following commits contain their predecessor's commit hash, now |
||
161 | all of the branch's commit hashes are modified, and gerrit happily accepts them as a |
||
162 | new patch set. It will still pick up the Change-Ids (which you shouldn't edit) and |
||
163 | notice if commits have remained identical (keeping the votes). But with the minor |
||
164 | commit log tweak, it will no longer thwart your re-submission with an error message. |
||
165 | |||
166 | |||
167 | h4. Private Branches |
||
168 | |||
169 | Say you have an extensive feature in development, and you want to keep it on the |
||
170 | upstream git repository to a) keep it safe and b) collaborate with other devs on it. |
||
171 | So, of course, you have regularly pushed to refs/heads/users/me/feature. |
||
172 | |||
173 | Since you have the gerrit commit hook installed, your feature branch already has |
||
174 | Change-Id tags in all commit log messages. |
||
175 | |||
176 | Now your feature is complete and you would like to submit it to master. |
||
177 | Alas, Gerrit refuses to accept your patch submission for master, because it |
||
178 | knows the Change-Ids are also on a different branch. |
||
179 | |||
180 | IMHO this is a very clumsy non-feature of Gerrit. It enforces that a Change-Id |
||
181 | must be unique across all branches, so that each submission for review is |
||
182 | separate for each branch. Instead, it should handle Change-Ids per-branch, so |
||
183 | that you can have the same change submitted to different branches, as separate |
||
184 | patch submissions, without having to cosmetically adjust the Change-Id. |
||
185 | |||
186 | Solutions: |
||
187 | |||
188 | Currently the best solution is to *not* push private branches to the gerrit |
||
189 | repository. This makes collaborating publicly on branches currently impossible. |
||
190 | Since our git.osmocom.org now are non-pushable, the only openly public place |
||
191 | would be the gerrit repos; intead, you need a separate, "private" git repository |
||
192 | to collaborate. IMHO we must still find a good solution to this problem. |
||
193 | |||
194 | If you have already pushed your Change-Ids to refs/heads/* somewhere, the only |
||
195 | way to submit to master is to, yes, edit every single commit log message on |
||
196 | your branch to remove the Change-Id, to let the commit hook re-add a new one. |
||
197 | Of course you can't push your modified Change-Ids back to the private branch on |
||
198 | the gerrit repos, because then Gerrit will have the same problem all over. |
||
199 | That means you can't easily edit branch commits and keep them public until they |
||
200 | are through review, because between pushes to your private branch and pushes for |
||
201 | review, you have to switch the Change-Id for the proper branch. Note: If you remove |
||
202 | and re-create the Change-Ids over and over, you will create new review items instead |
||
203 | of amending previous patch submissions with fixes. |
||
204 | |||
205 | A third solution might be to not have Change-Ids on your private branch, but that |
||
206 | means that once you want to submit for review, you also need to edit every single |
||
207 | commit log message on your branch, and then you're again stuck with the same problem. |
||
208 | |||
209 | My wish would be to teach Gerrit to ignore all of our private branches |
||
210 | and simply not look for Change-Id tags in those, so we can push them for review |
||
211 | while also keeping them on a private branch at the same time. |
||
212 | |||
213 | An easier way might be to have a public collaboration repository, e.g. to allow |
||
214 | pushing to git.osmocom.org repositories again and only handle for/master reviews |
||
215 | via the Gerrit repos. This won't work if the gerrit repos fetches upstream branches, |
||
216 | so we might need yet another place for repositories -- we already have git.osmocom.org |
||
217 | and gerrit, so it would be confusing to have even a third set of public repositories. |
||
218 | |||
219 | Let's hope to find nicer solutions... |
||
220 | 14 | neels | |
221 | |||
222 | h2. Project Config Details |
||
223 | |||
224 | There are more non-obvious configuration items with Gerrit. |
||
225 | |||
226 | h3. Allow content merges |
||
227 | |||
228 | By default, gerrit compares patches only by the files' paths. If two paths are the same, |
||
229 | it immediately shows them as conflicts (path conflicts). |
||
230 | |||
231 | In software development, a conflict usually means an actual content conflict, so if the |
||
232 | edits are in two entirely separate places in the file, we don't consider this a conflict. |
||
233 | |||
234 | By setting 'Allow content merges' to TRUE in the git project config, we tell Gerrit to |
||
235 | perform text merges of the submitted patches and only complain about actual content |
||
236 | conflicts, in the usual software engineering sense. |