Gerrit Caveats and Hints: Difference between revisions
Jump to navigation
Jump to search
No edit summary |
(add a new line for latest ssh connection error) |
||
| (6 intermediate revisions by 3 users not shown) | |||
| Line 2: | Line 2: | ||
[[Category:Tools::Gerrit]] | [[Category:Tools::Gerrit]] | ||
== Dos and Don'ts == | ==Dos and Don'ts== | ||
* Do not reply to review comments by mail. It hurts collaboration (as the mails are not publicly 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 publicly 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. | *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 Privilege codified in the [[Review 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 [[Review Policy]]. | ||
== Troubleshooting == | ==Troubleshooting== | ||
First, make sure you followed _all_ points of the [[Setting_up_Gerrit#How_to_get_started_-_Gerrit_registration|Gerrit setup procedure]]. | First, make sure you followed _all_ points of the [[Setting_up_Gerrit#How_to_get_started_-_Gerrit_registration|Gerrit setup procedure]]. | ||
=== Troubleshooting Table === | ===Troubleshooting Table=== | ||
{| class="wikitable" | {| class="wikitable" | ||
| Line 35: | Line 35: | ||
| | | | ||
|commit is new | |commit is new | ||
|To prevent the problem occurring for new commits, install the commit message hook according to the instructions under [[Setting up Gerrit | |To prevent the problem occurring for new commits, install the commit message hook according to the instructions under [[Setting up Gerrit]] | ||
|- | |- | ||
| | | | ||
| Line 42: | Line 42: | ||
|- | |- | ||
|"git push" fails with: "Unable to negotiate with xx.xx.xx.xx port 29418: no matching cipher found. Their offer: aes128-cbc,aes192-cbc,aes256-cbc,blowfish-cbc" | |"git push" fails with: "Unable to negotiate with xx.xx.xx.xx port 29418: no matching cipher found. Their offer: aes128-cbc,aes192-cbc,aes256-cbc,blowfish-cbc" | ||
|Our gerrit continues to depend on ciphers that have recently been deemed obsolete in certain recent Linux distros (such as Arch), as well as macOS 10.13.2 and later. | |Our gerrit continues to depend on ciphers that have recently been deemed obsolete in certain recent Linux distros (such as Arch), as well as macOS 10.13.2 and later. (Note: this probably no longer happens, since we upgraded Gerrit to a version with modern ciphersuites; check with a Gerrit admin if this does happen.) | ||
|Edit <tt>~/.ssh/config</tt> and add at least one of the supported ciphers so that your ssh client will support it too; for example | |Edit <tt>~/.ssh/config</tt> and add at least one of the supported ciphers so that your ssh client will support it too; for example | ||
<code>Host codereview.qt-project.org | <code>Host codereview.qt-project.org | ||
Port 29418 | Port 29418 | ||
User myusername | User myusername | ||
Ciphers aes256-cbc | Ciphers +aes256-cbc | ||
</code> | </code> | ||
|- | |- | ||
| Line 93: | Line 94: | ||
|Module dependencies come from the versions in qt5.git, not the latest branch. Your change to module a needs to go into qt5.git first. | |Module dependencies come from the versions in qt5.git, not the latest branch. Your change to module a needs to go into qt5.git first. | ||
|If you want to check if your commit to module a is in a branch of qt5.git, check out the latest version of the branch of qt5.git, run git submodule update --init module_a, then change into the module a sub-directory in your qt5 checkout and run git branch --contains=<sha1 of your change to module a>. If that prints out nothing, then your change is not in qt5.git yet. Watch Gerrit for module updates to qt5.git. | |If you want to check if your commit to module a is in a branch of qt5.git, check out the latest version of the branch of qt5.git, run git submodule update --init module_a, then change into the module a sub-directory in your qt5 checkout and run git branch --contains=<sha1 of your change to module a>. If that prints out nothing, then your change is not in qt5.git yet. Watch Gerrit for module updates to qt5.git. | ||
|- | |||
|Permission denied (publickey), and send_pubkey_test: no mutual signature algorithm when ssh -vT | |||
|Perhaps the algorithm of your key is out of date. | |||
|Try to use *ssh-keygen -t ed25519* to get a key with latest algorithm. See also [https://medium.com/risan/upgrade-your-ssh-key-to-ed25519-c6e8d60d3c54 Medium:Upgrade Your SSH Key to Ed25519]. | |||
|} | |} | ||
=== A Real World Troubleshooting Example === | ===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. | This is a lightly edited log of troubleshooting an attempt to push a commit to gerrit for Qt 4.8. | ||
| Line 170: | Line 175: | ||
[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) | [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> | </pre> | ||
== Working on top of other patches: checkout vs cherry-pick == | |||
Sometimes you'll need to rebase your work on top of someone else's. | |||
Let's take this example, where you have two changes: | |||
[[File:Gerrit-on-top-step-1.png]] | |||
Suddenly you realise that someone has patches that are relevant to your work: | |||
[[File:Gerrit-on-top-step-2.png]] | |||
You need to put (rebase) your patches on top of theirs: | |||
[[File:Gerrit-on-top-step-3.png]] | |||
To do so (after committing any unsaved changes and pushing them to Gerrit, if necessary): | |||
# Open the last patch in their series that you're interested in (Change C) in Gerrit. Copy the checkout link, paste it into your shell, and run it. | |||
# Open the first change in your series in Gerrit (Change 1). Copy the cherry-pick link, paste it into your shell, and run it. | |||
# Repeat step 2 for each subsequent change in your series (Change 2). | |||
# Push to Gerrit. | |||
Now Change 1 and 2 will depend on the other patch series. | |||
When the other patch series changes and you want to rebase your work on top of it, repeat the steps above. | |||
Cherry-picking other patches can be done locally to test changes out, but you shouldn't push after doing so unless you intend to rebase those other patches. | |||
<br /> | |||
Latest revision as of 11:12, 25 November 2021
Dos and Don'ts
- Do not reply to review comments by mail. It hurts collaboration (as the mails are not publicly 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 Review Policy.
Troubleshooting
First, make sure you followed _all_ points of the Gerrit setup procedure.
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: "Unable to negotiate with xx.xx.xx.xx port 29418: no matching cipher found. Their offer: aes128-cbc,aes192-cbc,aes256-cbc,blowfish-cbc" | Our gerrit continues to depend on ciphers that have recently been deemed obsolete in certain recent Linux distros (such as Arch), as well as macOS 10.13.2 and later. (Note: this probably no longer happens, since we upgraded Gerrit to a version with modern ciphersuites; check with a Gerrit admin if this does happen.) | Edit ~/.ssh/config and add at least one of the supported ciphers so that your ssh client will support it too; for example
Host codereview.qt-project.org
Port 29418
User myusername
Ciphers +aes256-cbc
|
| "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. | |
| My change to module b depends on change in module a. The latter is in, but it seems the CI keeps compiling against module a without my change! | Module dependencies come from the versions in qt5.git, not the latest branch. Your change to module a needs to go into qt5.git first. | If you want to check if your commit to module a is in a branch of qt5.git, check out the latest version of the branch of qt5.git, run git submodule update --init module_a, then change into the module a sub-directory in your qt5 checkout and run git branch --contains=<sha1 of your change to module a>. If that prints out nothing, then your change is not in qt5.git yet. Watch Gerrit for module updates to qt5.git. |
| Permission denied (publickey), and send_pubkey_test: no mutual signature algorithm when ssh -vT | Perhaps the algorithm of your key is out of date. | Try to use *ssh-keygen -t ed25519* to get a key with latest algorithm. See also Medium:Upgrade Your SSH Key to Ed25519. |
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)
Working on top of other patches: checkout vs cherry-pick
Sometimes you'll need to rebase your work on top of someone else's.
Let's take this example, where you have two changes:
Suddenly you realise that someone has patches that are relevant to your work:
You need to put (rebase) your patches on top of theirs:
To do so (after committing any unsaved changes and pushing them to Gerrit, if necessary):
- Open the last patch in their series that you're interested in (Change C) in Gerrit. Copy the checkout link, paste it into your shell, and run it.
- Open the first change in your series in Gerrit (Change 1). Copy the cherry-pick link, paste it into your shell, and run it.
- Repeat step 2 for each subsequent change in your series (Change 2).
- Push to Gerrit.
Now Change 1 and 2 will depend on the other patch series.
When the other patch series changes and you want to rebase your work on top of it, repeat the steps above.
Cherry-picking other patches can be done locally to test changes out, but you shouldn't push after doing so unless you intend to rebase those other patches.