Gerrit Caveats and Hints: Difference between revisions

From Qt Wiki
Jump to navigation Jump to search
m (Ossi moved page Gerrit-Caveats-and-Hints to Gerrit Caveats and Hints: convention)
(merge in troubleshooting stuff from "code reviews")
Line 1: Line 1:
{{Cleanup | reason=Auto-imported from ExpressionEngine.}}
[[Category:Developing_Qt::Guidelines]]
[[Category:Developing_Qt::Guidelines]]


= Gerrit Caveats and Hints =
== Dos and Don'ts ==
 
=== Dos and Don'ts ===


* Do not reply to review comments by mail. It hurts collaboration (as the mails are not publicaly archived). The correct way is hitting the "review" button and adding followup comments. If you want to reply to particular inline comments, open the diff view for the patchset the comments pertain to and use the reply buttons there (note that you still need to use the "review" button (of the right patchset!) to actually submit the comments).
* Do not reply to review comments by mail. It hurts collaboration (as the mails are not publicaly archived). The correct way is hitting the "review" button and adding followup comments. If you want to reply to particular inline comments, open the diff view for the patchset the comments pertain to and use the reply buttons there (note that you still need to use the "review" button (of the right patchset!) to actually submit the comments).
* As an author, do not rebase your commits unless necessary, as that makes inter-patchset-diffs completely useless. Use rebase -i HEAD~n rather than rebase -i <code>{u} when you reshuffle commits and you fetched origin meanwhile. Do not pull for no good reason. When you rebased, try to push as little as possible (HEAD~n:refs/for/foo) to avoid invalidating previous reviews.
* As an author, do not rebase your commits unless necessary, as that makes inter-patchset-diffs completely useless. Use rebase -i HEAD~n rather than rebase -i @{u} when you reshuffle commits and you fetched origin meanwhile. Do not pull for no good reason. When you rebased, try to push as little as possible (HEAD~n:refs/for/foo) to avoid invalidating previous reviews.
* When you make multiple unrelated commits, it is best to throw away (reset —hard HEAD~n) the already pushed ones or make a new branch for each "topic". That way it is easier to amend commits without invalidating unrelated reviews.
* When you make multiple unrelated commits, it is best to throw away (reset --hard HEAD~n) the already pushed ones or make a new branch for each "topic". That way it is easier to amend commits without invalidating unrelated reviews.
* As a reviewer, do not be too trigger-happy with the "stage" button. Allow other reviewers to do their job as well, especially when domain experts were invited.
* As a reviewer, do not be too trigger-happy with the "stage" button. Allow other reviewers to do their job as well, especially when domain experts were invited.
* Don't do "fake" reviews. If you cannot honestly give a +2 (i.e., "I fully understand that patch and its implications, and I take the blame if it breaks"), then don't. If needed, refer to the Maintainer Priviledge codified in the [[Commit Policy]].
* Don't do "fake" reviews. If you cannot honestly give a +2 (i.e., "I fully understand that patch and its implications, and I take the blame if it breaks"), then don't. If needed, refer to the Maintainer Privilege codified in the [[Commit Policy]].
 
== Troubleshooting ==
 
=== Checklist for contributing ===
 
In order to contribute person needs to have:
 
* Register a JIRA account at https://bugreports.qt.io
* Log in to Gerrit using the same account at https://codereview.qt-project.org
* Accept the Contributor License Agreement
** Individual contributor does that in Gerrit (Settings -> Agreements-> New Contributor Agreement)
** Corporate contributor needs to fill in CLA for corporate contributor
* If code is contributed user needs to clone the source code (see: [[Setting up Gerrit]])
 
=== Troubleshooting Table ===
 
{| class="wikitable"
!Symptom
!Cause
!Solution
|-
|When I click the email verification link in Outlook, Gerrit told "Invalid token"
|Outlook doesn't activate the link correctly. Specifically, the last two "==" are omitted when clicking on the link
|Copy and paste the link into a web browser, instead of clicking on it
|-
|"git push" fails with: "error: failed to push some refs to …"
|Many possible causes. Generic error message.
|Please read the rest of the output of "git push" to determine the root cause (especially lines beginning with "remote:").
|-
|"git push" fails with: "ERROR: missing Change-Id in commit message".
|You tried to push a commit whose commit message didn't contain a "Change-Id" line. This may be:
|
|-
|
|commit is new
|To prevent the problem occurring for new commits, install the commit message hook according to the instructions under [[Setting up Gerrit
|-
|
|a commit you have cherry-picked from outside of Gerrit
|Use one of these tools to add a Change-Id (roughly listed in order of expertise required): git commit --amend, git-edit-commit (from qtrepotools), git rebase --interactive
|-
|"git push" fails with "ERROR: you are not allowed to upload merges"
|You tried to push a merge commit for review, which is not allowed by default.
|Contact one of the administrators listed in the Gerrit Registration section and ask for permission to do merges. The primary reason for this is to reduce the risk of accidental pushing of merges (e.g. by users who use Git in a CVS-like manner, without learning about effective usage of merges and rebases).
|-
|I can't give +2 to anything, or submit or stage anything
|You're not in the "Approvers" group.
|Apply for approver status according to the [[The Qt Governance Model]]
|-
|"Qt Sanity Bot" gave a –1 to my change!
|An automated sanity check bot found out that change contains some problem.
|First, really try to understand the problem. Don't proceed until you understand the output of the bot. Then make an informed decision to either fix it (e.g. by amending your change with the fix and pushing it again), or ignore it. The sanity bot's output is advisory in cases where it is determined heuristically.
|-
|"Qt Sanity Bot" gave a –2 to my change!
|An automated sanity check bot found out that change contains some serious problem.
|First, really try to understand the problem. If you are convinced that the bot made a mistake, contact a member of the Administrators group on Gerrit.
|-
|"Qt Continuous Integration System" said my change failed, but I'm sure my change is OK!
|The test procedure for your change failed. It could be:
|
|-
|
|a regression caused by someone else's change which was tested at the same time
|Simply stage your change again.
|-
|
|a regression caused by a change in some dependency of the module you're working on (e.g. a qtbase bug breaking qtdeclarative)
|Fix the bug in the dependency, or ensure someone else fixes it, before re-staging your change.
|-
|
|an unstable failure due to bugs in the code (e.g. an autotest which fails ~50% of the time, seemingly at random)
|Fix the instability in a separate commit before re-staging your change, or consult QA for advice.
|-
|
|bugs or infrastructure problems in the CI system.
|Consult QA for advice.
|-
|
|a regression genuinely caused by your change
|Reconsider if you should really be "sure" that your change is OK. For a project the size of Qt, it is common for changes in one part of the code to affect other parts of the code in unexpected ways.
|}
 
=== A Real World Troubleshooting Example ===
 
This is a lightly edited log of troubleshooting an attempt to push a commit to gerrit for Qt 4.8.
 
<pre style="white-space: pre-wrap;">
[20:16] <curius> git gives me this "error: failed to push some refs to 'ssh://giotis.nikos@codereview.qt-project.org:29418/qt/qt'"
[20:16] <richmoore1> curius: login to gerrit
[20:16] <richmoore1> curius: click settings at the top right
[20:17] <richmoore1> curius: click Agreements on the left at the bottom of the list
[20:17] <curius> verified
[20:17] <richmoore1> ok, good
[20:17] <richmoore1> now click ssh public keys
[20:18] <richmoore1> do you see your key?
[20:19] <curius> yes
[20:19] <richmoore1> okay
[20:20] <richmoore1> so we know that in theory you can push but there's likely some problem with /what/ your trying to push, or where it's going to
[20:20] <peppe> curius: Change-Id installed?
[20:21] <curius> Change-Id? don't know what that is :)
[20:21] <curius> the git/hook thing?
[20:21] <richmoore1> yes, it should be in your commit message
[20:21] <richmoore1> do you see a line like Change-Id: Iffbdf8f5ec366d9f41debcf8301340b5ff2207bb
[20:21] <qt_gerrit> https://codereview.qt-project.org/#q,Iffbdf8f5ec366d9f41debcf8301340b5ff2207bb,n,z
[20:22] <curius> oh.. ok, there is a change-id, yes
[20:23] <curius> Change-Id: Ibc7546c522506b2c4e7a729a8de319aea788b662
[20:23] <qt_gerrit> https://codereview.qt-project.org/#q,Ibc7546c522506b2c4e7a729a8de319aea788b662,n,z
[20:24] <richmoore1> okay, so we know you have the git hooks working
[20:25] <richmoore1> curius: the problem is probably that you've fallen behind the current tree
[20:25] <richmoore1> curius: do a git pull to get yourself up to date
[20:26] <richmoore1> curius: you may need to tweak your change to make it work after this
[20:28] <curius> git pull ssh://giotis.nikos@codereview.qt-project.org:29418/qt/qt
[20:28] <curius> this says 'Already up-to-date'
[20:29] <richmoore1> okay, what else did it say when you tried to push?
[20:30] <curius> http://pastebin.com/NqbJZBaS
</pre>
 
The contents of the pastebin were as follows:
 
<pre>
Counting objects: 7, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (4/4), done.
Writing objects: 100% (4/4), 555 bytes, done.
Total 4 (delta 3), reused 0 (delta 0)
remote: Resolving deltas: 0% (0/3)
To ssh://giotis.nikos@codereview.qt-project.org:29418/qt/qt
! [remote rejected] HEAD -> refs/for/master (branch master not found)
error: failed to push some refs to 'ssh://giotis.nikos@codereview.qt-project.org
:29418/qt/qt'
</pre>


=== Useful Gerrit Queries ===
<pre style="white-space: pre-wrap;">
[20:31] <richmoore1> aha, see line 8?
[20:31] <curius> yes? branch master not found ?
[20:32] <richmoore1> yes, that's the problem. iirc you're trying to push to qt 4.8 yes?
[20:32] <curius> yes, 4.8 master
[20:32] <richmoore1> okay, so we need to find out what the branch is
[20:33] <richmoore1> answer 4.8
[20:33] <curius> on branch 4.8
[20:33] <curius> yes
[20:33] <curius> ohh.. refs/for/4.8 ??
[20:33] <richmoore1> yep
[20:34] <curius> thank you :D
[20:34] <curius> http://codereview.qt-project.org/#change,15262
[20:34] <richmoore1> bingo :-)
[20:35] <curius> who's going to review it? it's very very trivial fix?
[20:35] <richmoore1> it's qmake. so add ossi
[20:36] <richmoore1> or i can add him for you if you want
[20:36] <curius> yes, please :)
[20:36] <curius> add him
[20:36] <richmoore1> done
[20:37] <curius> ahhh, nice :) … and now i wait for ossi to see it, right?
[20:37] <richmoore1> yep
[20:37] <richmoore1> you'll get mailed when there's comments etc.
[20:38] <richmoore1> and you can push again using the same commit id (eg. via updating your commit with git commit --amend to address any issues found during the review)
</pre>

Revision as of 19:28, 3 March 2015


Dos and Don'ts

  • Do not reply to review comments by mail. It hurts collaboration (as the mails are not publicaly archived). The correct way is hitting the "review" button and adding followup comments. If you want to reply to particular inline comments, open the diff view for the patchset the comments pertain to and use the reply buttons there (note that you still need to use the "review" button (of the right patchset!) to actually submit the comments).
  • As an author, do not rebase your commits unless necessary, as that makes inter-patchset-diffs completely useless. Use rebase -i HEAD~n rather than rebase -i @{u} when you reshuffle commits and you fetched origin meanwhile. Do not pull for no good reason. When you rebased, try to push as little as possible (HEAD~n:refs/for/foo) to avoid invalidating previous reviews.
  • When you make multiple unrelated commits, it is best to throw away (reset --hard HEAD~n) the already pushed ones or make a new branch for each "topic". That way it is easier to amend commits without invalidating unrelated reviews.
  • As a reviewer, do not be too trigger-happy with the "stage" button. Allow other reviewers to do their job as well, especially when domain experts were invited.
  • Don't do "fake" reviews. If you cannot honestly give a +2 (i.e., "I fully understand that patch and its implications, and I take the blame if it breaks"), then don't. If needed, refer to the Maintainer Privilege codified in the Commit Policy.

Troubleshooting

Checklist for contributing

In order to contribute person needs to have:

  • Register a JIRA account at https://bugreports.qt.io
  • Log in to Gerrit using the same account at https://codereview.qt-project.org
  • Accept the Contributor License Agreement
    • Individual contributor does that in Gerrit (Settings -> Agreements-> New Contributor Agreement)
    • Corporate contributor needs to fill in CLA for corporate contributor
  • If code is contributed user needs to clone the source code (see: Setting up Gerrit)

Troubleshooting Table

Symptom Cause Solution
When I click the email verification link in Outlook, Gerrit told "Invalid token" Outlook doesn't activate the link correctly. Specifically, the last two "==" are omitted when clicking on the link Copy and paste the link into a web browser, instead of clicking on it
"git push" fails with: "error: failed to push some refs to …" Many possible causes. Generic error message. Please read the rest of the output of "git push" to determine the root cause (especially lines beginning with "remote:").
"git push" fails with: "ERROR: missing Change-Id in commit message". You tried to push a commit whose commit message didn't contain a "Change-Id" line. This may be:
commit is new To prevent the problem occurring for new commits, install the commit message hook according to the instructions under [[Setting up Gerrit
a commit you have cherry-picked from outside of Gerrit Use one of these tools to add a Change-Id (roughly listed in order of expertise required): git commit --amend, git-edit-commit (from qtrepotools), git rebase --interactive
"git push" fails with "ERROR: you are not allowed to upload merges" You tried to push a merge commit for review, which is not allowed by default. Contact one of the administrators listed in the Gerrit Registration section and ask for permission to do merges. The primary reason for this is to reduce the risk of accidental pushing of merges (e.g. by users who use Git in a CVS-like manner, without learning about effective usage of merges and rebases).
I can't give +2 to anything, or submit or stage anything You're not in the "Approvers" group. Apply for approver status according to the The Qt Governance Model
"Qt Sanity Bot" gave a –1 to my change! An automated sanity check bot found out that change contains some problem. First, really try to understand the problem. Don't proceed until you understand the output of the bot. Then make an informed decision to either fix it (e.g. by amending your change with the fix and pushing it again), or ignore it. The sanity bot's output is advisory in cases where it is determined heuristically.
"Qt Sanity Bot" gave a –2 to my change! An automated sanity check bot found out that change contains some serious problem. First, really try to understand the problem. If you are convinced that the bot made a mistake, contact a member of the Administrators group on Gerrit.
"Qt Continuous Integration System" said my change failed, but I'm sure my change is OK! The test procedure for your change failed. It could be:
a regression caused by someone else's change which was tested at the same time Simply stage your change again.
a regression caused by a change in some dependency of the module you're working on (e.g. a qtbase bug breaking qtdeclarative) Fix the bug in the dependency, or ensure someone else fixes it, before re-staging your change.
an unstable failure due to bugs in the code (e.g. an autotest which fails ~50% of the time, seemingly at random) Fix the instability in a separate commit before re-staging your change, or consult QA for advice.
bugs or infrastructure problems in the CI system. Consult QA for advice.
a regression genuinely caused by your change Reconsider if you should really be "sure" that your change is OK. For a project the size of Qt, it is common for changes in one part of the code to affect other parts of the code in unexpected ways.

A Real World Troubleshooting Example

This is a lightly edited log of troubleshooting an attempt to push a commit to gerrit for Qt 4.8. 

[20:16] <curius> git gives me this "error: failed to push some refs to 'ssh://giotis.nikos@codereview.qt-project.org:29418/qt/qt'"
[20:16] <richmoore1> curius: login to gerrit
[20:16] <richmoore1> curius: click settings at the top right
[20:17] <richmoore1> curius: click Agreements on the left at the bottom of the list
[20:17] <curius> verified
[20:17] <richmoore1> ok, good
[20:17] <richmoore1> now click ssh public keys
[20:18] <richmoore1> do you see your key?
[20:19] <curius> yes
[20:19] <richmoore1> okay
[20:20] <richmoore1> so we know that in theory you can push but there's likely some problem with /what/ your trying to push, or where it's going to
[20:20] <peppe> curius: Change-Id installed?
[20:21] <curius> Change-Id? don't know what that is :)
[20:21] <curius> the git/hook thing?
[20:21] <richmoore1> yes, it should be in your commit message
[20:21] <richmoore1> do you see a line like Change-Id: Iffbdf8f5ec366d9f41debcf8301340b5ff2207bb
[20:21] <qt_gerrit> https://codereview.qt-project.org/#q,Iffbdf8f5ec366d9f41debcf8301340b5ff2207bb,n,z
[20:22] <curius> oh.. ok, there is a change-id, yes
[20:23] <curius> Change-Id: Ibc7546c522506b2c4e7a729a8de319aea788b662
[20:23] <qt_gerrit> https://codereview.qt-project.org/#q,Ibc7546c522506b2c4e7a729a8de319aea788b662,n,z
[20:24] <richmoore1> okay, so we know you have the git hooks working
[20:25] <richmoore1> curius: the problem is probably that you've fallen behind the current tree
[20:25] <richmoore1> curius: do a git pull to get yourself up to date
[20:26] <richmoore1> curius: you may need to tweak your change to make it work after this
[20:28] <curius> git pull ssh://giotis.nikos@codereview.qt-project.org:29418/qt/qt
[20:28] <curius> this says 'Already up-to-date'
[20:29] <richmoore1> okay, what else did it say when you tried to push?
[20:30] <curius> http://pastebin.com/NqbJZBaS

The contents of the pastebin were as follows: 

Counting objects: 7, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (4/4), done.
Writing objects: 100% (4/4), 555 bytes, done.
Total 4 (delta 3), reused 0 (delta 0)
remote: Resolving deltas: 0% (0/3)
To ssh://giotis.nikos@codereview.qt-project.org:29418/qt/qt
 ! [remote rejected] HEAD -> refs/for/master (branch master not found)
error: failed to push some refs to 'ssh://giotis.nikos@codereview.qt-project.org
:29418/qt/qt'

[20:31] <richmoore1> aha, see line 8?
[20:31] <curius> yes? branch master not found ?
[20:32] <richmoore1> yes, that's the problem. iirc you're trying to push to qt 4.8 yes?
[20:32] <curius> yes, 4.8 master
[20:32] <richmoore1> okay, so we need to find out what the branch is
[20:33] <richmoore1> answer 4.8
[20:33] <curius> on branch 4.8
[20:33] <curius> yes
[20:33] <curius> ohh.. refs/for/4.8 ??
[20:33] <richmoore1> yep
[20:34] <curius> thank you :D
[20:34] <curius> http://codereview.qt-project.org/#change,15262
[20:34] <richmoore1> bingo :-)
[20:35] <curius> who's going to review it? it's very very trivial fix?
[20:35] <richmoore1> it's qmake. so add ossi
[20:36] <richmoore1> or i can add him for you if you want
[20:36] <curius> yes, please :)
[20:36] <curius> add him
[20:36] <richmoore1> done
[20:37] <curius> ahhh, nice :) … and now i wait for ossi to see it, right?
[20:37] <richmoore1> yep
[20:37] <richmoore1> you'll get mailed when there's comments etc.
[20:38] <richmoore1> and you can push again using the same commit id (eg. via updating your commit with git commit --amend to address any issues found during the review)
Retrieved from "https://wiki.qt.io/index.php?title=Gerrit_Caveats_and_Hints&oldid=10921"