Infrastructure/GitLab: Difference between revisions
(Update "fork the project" instructions to reflect the current GitLab UI) |
(Add a link to Google's guide for code review, which is at least better than the nothingness we have there right now) |
||
| Line 319: | Line 319: | ||
=== Perform code review === | === Perform code review === | ||
A good overview for how to review changes can be found at https://google.github.io/eng-practices/review/reviewer/. | |||
=== Engage with the author and other reviewers === | === Engage with the author and other reviewers === | ||
Revision as of 23:29, 16 September 2021
KDE uses a GitLab instance at https://invent.kde.org for code review (as well as hosting and other important collaboration tasks). This page is intended to serve as a general-purpose introduction to the most important aspects: submitting and reviewing Merge Requests.
Information
Workflow
The sanest and easiest way to submit code to KDE is by following a typical feature branch workflow: keep your master branch synchronized with the origin repository, and make all changes on separate branches. Each Merge Request needs its own private, temporary branch. Once your Merge Request has been merged, delete the feature branch, and make another new branch for the next Merge Request. In this way, you can be working on multiple changes at once without them colliding with one another because each one lives on its own branch.
Logging in
Navigate to https://invent.kde.org/users/sign_in and log in using the username and password for your KDE Identity account. If you don't have one, you can sign up for one here.
Setting up git
You will need to set up git to use you account details to help identify your work:
git config user.name <Your Real Name>
git config user.email <Your identity.kde.org email>
(You can set it up globally with --global)
In order to authenticate yourself when pushing code changes, you need to add a ssh key to your GitLab profile as described here.
Submitting a merge request
Contributing to KDE code using GitLab involves submitting a Merge Request. A Merge Request is a request to merge some of your code into the project's permanent source code repo. Here's how:
Build the project from source and make your change
First you need to check out the project, compile it from source, and make some changes that you would like to submit to KDE! Instructions for doing this can be found at Get Involved/development. You will wind up with a checkout of the project at ~/kde/src/[the project name] with some local changes applied to it.
Information
Fork the project
Note
Once you have made some local changes that you would like to submit to KDE, you need to create a personal fork of the project and push your changes to the forked copy.
Navigate to https://invent.kde.org/kde and locate the project. If it is not visible in the list, you can use the search field. Once you find the project, click on it:
On the project's page, click on the "Fork" button in the top-right corner of the screen:
This will take you to a page asking you some details about how you want to fork the project. Select your own name in the "Project URL" section and "Public" in the "Visibility" section:
After a moment, the system will finish creating the fork and take you to the page for your fork. On that page, click on the blue "Clone" button in the top-right corner:
In the pop-up that appears, click on the "copy" button to the right of the upper text field. This will copy the URL for the fork onto your clipboard.
Add the fork to your source checkout
Next, open your terminal app and navigate to the location where the project's repo lives (i.e. ~/kde/src/[the project name]).
You need to add your fork as a remote to the existing repo:
git remote add fork [the URL you copied to your clipboard]
Run git remote -v. You should see something like this:
$ git remote -v fork [email protected]:ngraham/kid3.git (fetch) fork [email protected]:ngraham/kid3.git (push) origin https://invent.kde.org/kde/kid3.git (fetch) origin https://invent.kde.org/kde/kid3.git (push)
This means you have two remotes set up for your repo: "origin" points to the original repo, and "fork" points to your fork of it.
Make a branch and commit
Now that you have your fork set up, it's time to create a branch to track your work and make a commit.
git checkout -b my_awesome_feature git add [the files you changed] git commit
Note
Write a good commit message
With Gitlab, once a merge request is merged, the first commit in the merge request will wind up in the Git history, so it is very important that the first commit in the merge request be formatted properly.
Please follow commit message best practices: write a descriptive title in the form of an imperative sentence (e.g. "Fix button disappearing when view is changed") and on the next line, write at least one sentence describing your change and why it is necessary, adding more details if necessary.
If your patch is intended to fix a Bugzilla ticket, include the following on its own line:
BUG: 385942
(The number should be just the Bugzilla ticket number, not the full URL)
For example a commit message might read:
Close memory leak in GC The GC was doing a bad job and leaking memory. Explicitly call delete where necessary. BUG: 385942 FIXED-IN: 5.0.0
Here is more information about other special messages that interact with Bugzilla tickets.
Note
Note
Push to your fork
At this point you have a branch in your local repo called "my_awesome_feature" (Hopefully in reality it is named something a bit more appropriate!) that has a commit on it with your work. Now push it to your fork:
git push fork my_awesome_feature
This will produce a message somewhat like this:
$ git push fork my_awesome_feature Enumerating objects: 5, done. Counting objects: 100% (5/5), done. Delta compression using up to 4 threads Compressing objects: 100% (3/3), done. Writing objects: 100% (3/3), 303 bytes | 303.00 KiB/s, done. Total 3 (delta 2), reused 0 (delta 0) remote: This commit is available for viewing at: remote: https://invent.kde.org/ngraham/kid3/commit/23a702439f494806cf3cfe14f212df58a0075bba remote: remote: To create a merge request for my_awesome_feature, visit: remote: https://invent.kde.org/ngraham/kid3/merge_requests/new?merge_request%5Bsource_branch%5D=my_awesome_feature remote: To invent.kde.org:ngraham/kid3.git * [new branch] my_awesome_feature -> my_awesome_feature
Create the Merge Request
Notice the "To create a merge request for my_awesome_feature..." message in the output of the push command (explained in the previous section). You can copy-and-paste the URL shown below it into a web browser. On some terminal apps, such as Konsole and Yakuake, you can ctrl+click on the link to go right there!
You will be taken to a web page that looks like this:
In the Description section, write at least one sentence describing your change and why it is necessary, adding more details if needed. For Merge Requests that change something about the appearance or user interface, it's customary to include a screenshot of how the interface looks after your change has been applied. Bonus points for including a "Before" image too, so reviewers can easily compare them.
If your patch is intended to fix a Bugzilla ticket, include the ticket number at the bottom of the description, just like how you did in the commit message (as explained in Infrastructure/GitLab#Write_a_good_commit_message):
BUG: 385942 FIXED-IN: 5.21
Note
BUG: keyword must be added both in the Merge Request description--which notifies the bug ticket of the Merge Request creation--and in the message of the first commit--which closes the bug ticket once the Merge Request is merged.In case you don't want to close the bug, but only add the information to it you can use
CCBUG:. This can be the case if there are commits required before the bugfix can be done.
Once you're done with that, click the "Submit Merge Request" button!
What happens next?
After you've submitted your Merge Request, KDE developers who work with the software in question will review it and provide feedback. This can often take a few days. However, if nobody has responded after a week, it's likely that the review was overlooked (sorry about that!) and it's appropriate to make a comment saying, "Ping!" or something to that effect.
Once the Merge Request is accepted, KDE Developers will merge it for you!
Making changes to a Merge Request
Oftentimes, reviewers will request changes before the Merge Request can be merged. To accomplish this, you make the requested changes locally, then create a new commit including your changes. First, stage all changed files:
git add -u
Now make a new commit with the staged files:
git commit
Then push the local branch with the new commit on it up to the remote branch:
git push fork
Rebasing a Merge Request
When other changes have been made to the project's source code repo since you submitted your merge request, you will need to rebase the Merge Request to incorporate those changes.
Confirm that you have the origin setup correctly by issuing git remote -v:
fork [email protected]:developer/projectfork.git (fetch) fork [email protected]:developer/projectfork.git (push) origin https://invent.kde.org/applicationgroup/originalproject.git (fetch) origin https://invent.kde.org/applicationgroup/originalproject.git (push)
It doesn't matter whether the original project is of type https:// or git@, as you'll just fetch it.
If you instead see:
origin [email protected]:developer/projectfork.git (fetch) origin [email protected]:developer/projectfork.git (push)
You'll need to rename your origin to fork or any similar name that is easily recognizable and add the original project as your new origin:
# Rename your origin to "fork" git remote rename origin fork # Add new origin project git remote add origin https://invent.kde.org/applicationgroup/originalproject.git
Now you may proceed:
# First, make sure you are on the branch for your merge request git checkout mybranch # Now fetch the new changes in the repository git fetch # And finally rebase it git pull --rebase origin master
At this point, there may be merge conflicts. If there are, git will tell you which files have conflicts. You can then open each file and resolve the conflict by exiting the contents to keep only the appropriate change; for that you can either make each change manually or use git mergetool, which opens a diff tool of your choice (if there's any installed).
If you use a diff tool like meld, for instance, the origin will be shown on the left, your fork will be shown on the right, and you may pick which changes to apply to the middle file, the new one.
After resolving the existing conflicts, run git add [file path] on each conflicted file once all the conflicts have been resolved. Lastly, run git rebase --continue to finish the rebase.
Now, you need to overwrite the version of the Merge Request on your remote branch with the version on your local branch. To do this, you have to force-push:
git push --force fork
Note
git rerere immediately before and after resolving those conflicts.
Testing someone else's Merge Request
First you'll need a development environment set up. If you haven't done that yet, it's time to do so. Follow the instructions on Get_Involved/development#Set_up_your_development_environment. It is also advisable to set up the git mr tool, which makes testing Merge Requests a breeze. Here's how to install it, depending on your operating system:
Arch / Manjaro
yay -S git-mr
Debian/Ubuntu/KDE Neon
sudo apt install git-extras
Fedora
sudo dnf install git-extras
OpenSUSE
sudo zypper install git-mr
Check out the Merge Request and compile the software
First check out or enter the source repository for the software that's being patched. For example, let's say you want to test a Merge Request for Okular. If you've never built it before, check it out and build it once first:
kdesrc-build okular
Now go to its source directory:
cd ~/kde/src/okular
Find the Merge Request's ID. For example, for https://invent.kde.org/kde/okular/merge_requests/80, the ID is 80.
...and apply the Merge Request:
git mr 80
Now it's time to compile and run the software to make sure that the Merge Request does what it says it does and doesn't cause any regressions! Compile the patched source code:
kdesrc-build okular --no-src --resume-from okular
Those arguments will tell kdesrc-build to not update the source code and to not build any dependencies.
If it didn't compile, that's reason alone to reject the Merge Request! Go to the web page for the Merge Request and report your findings.
Perform QA
If it did compile, then it's time to perform QA, because it's important to thoroughly test Merge Requests to ensure that bad code and regressions don't slip in. This is the entire purpose of having a review infrastructure; it is very important.
First make sure the unit tests all pass:
cd ~kde/build/kde/applications/okular ctest
If any tests fail, report this through a comment on the Merge Request's web page.
Does everything all still work for you? If not, return to the web page and request changes, writing a detailed comment explaining what didn't work. It is permissible to do this even if you have not been specified as a reviewer! Anyone can reject a Merge Request on the grounds that it does not work, does not do what it says it does, or causes regressions.
Next, try to break the Merge Request. Here are some ideas:
- Remove the program's configuration file (~/.config/<program name>rc ) and re-open it
- Try the program with a HiDPI scale factor (or without one) or with a different default font size
- If it's a new feature, feed it unexpected input
- Test related functionality
A good Merge Request will handle corner cases and variations in configuration. The price of configurability is vigilant testing! We owe it to our users to test using many configurations, not just the defaults or our personal settings.
Perform code review
A good overview for how to review changes can be found at https://google.github.io/eng-practices/review/reviewer/.
Engage with the author and other reviewers
After you have run the program and evaluated the Merge Request, it's time to leave some review comments on the webpage. If you have been specified as a reviewer, or are a member of a group that has been specified as a reviewer, it is permissible to Accept the Merge Request. But keep in mind that reviewing involves responsibility: you are giving a thumbs-up to code that will be run potentially by millions of people. If you accept and land a Merge Request that causes regressions, you will be expected to help fix it if the original author cannot or has disappeared. It's important to take the reviewer role seriously.
Advanced topics
Curating your merge request commit history
For large or complex merge requests, it is strongly recommended to separate the pieces of your proposed change into individual commits--one for each component of the proposed change. For example, perhaps you are working on a feature that consists of multiple logically separable elements that nonetheless all live in the same source repo, or perhaps you are first doing some code refactoring, then you add a backend feature, then finally you add a front-end user interface for it.
For this workflow, specifically mention in the merge request description that you would like reviewers to review individual commits separately, and not squash when merging.
If you need to later make changes to your Merge Request, do not add new commits; instead, combine the new changes with one of the existing commits using git's interactive rebasing feature. To learn how to do this, see https://git-rebase.io/
Note
Using work branches instead of forks
If you have a developer account, you don't need to use forks to submit merge requests. Instead, make sure your branch name begins with work/ and push it straight to the main repo. work/ branches are treated specially, as you can use git push --force with them. By convention, it is recommended to add your KDE username to the branch, so the final name would end up looking like work/your-username/my-awesome-feature.
To make it easier creating new work branches (instead of typing work/your-username/branch-name every time), you can use a Git script to do that; for example, if you create a shell script somewhere in your PATH, and name it e.g. git-work (the script name must start with git-), and make the script executable:
#!/usr/bin/bash git checkout -t origin/master -b work/your-user-name/$1
you can then use:
git work branch-name
and this will create a branch work/your-username/branch-name, that tracks origin/master, and switch to the newly created branch.
Switching the target branch of a Merge Request
Sometimes you will submit a merge request that is targeting the master branch, but will later be asked to target the stable branch instead because it is a bugfix. Or perhaps you have targeted the stable branch but the commit is considered too invasive and you are asked to target the master branch instead. In either of these circumstances, you will need to re-target your Merge Request. Here's how:
git checkout [the local branch for your merge request] git fetch git rebase -i origin/[the name of the remote branch you want to target; for example release/19.12]
This will open a text editor showing a list of commits, each on a separate line. Delete all of the lines of text except for the ones corresponding to new commits you have added as part of your merge request. Next, fix any merge conflicts if there are any.
Then force-push the branch up to gitlab again:
git push --force fork
Finally, edit the merge request by clicking the "Edit" button in the top-right corner of the page and choose the desired different target branch:
Note that after merging a Merge Request to the stable branch, you are expected to merge that branch back to master afterwards:
git checkout release/19.12 git pull git checkout master git pull git merge -s recursive -Xours release/19.12 git diff origin/master # Are the changes what you expected? git push
Warning
Pushing commits to somebody else's fork
Sometimes someone will say "hey let's work on my branch together." So you will be pushing commits not to origin, and not to your fork, but to someone else's fork. Let's say you want to work on joliveira's "gsoc2019_numberFormat" branch.
First you would need to add the URL for his fork as a remote:
$ cd ~/kde/src/okular $ git remote add joliveira_fork [email protected]:joliveira/okular.git $ git remote -v aacid_fork [email protected]:aacid/okular.git (fetch) aacid_fork [email protected]:aacid/okular.git (push) joliveira_fork [email protected]:joliveira/okular.git (fetch) joliveira_fork [email protected]:joliveira/okular.git (push) origin https://invent.kde.org/kde/okular.git (fetch) origin https://invent.kde.org/kde/okular.git (push)
Notice how there are now multiple forks set up as remotes.
Next, you need to fetch all the repo metadata from the new remote:
git fetch joliveira_fork
This will download the list of branches. The next step is to switch to the one you want to collaborate on:
git checkout --track joliveira_fork/gsoc2019_numberFormat
This will create a local branch named "gsoc2019_numberFormat" from the contents of the remote branch joliveira_fork/gsoc2019_numberFormat and that also "tracks" it. This means that if someone else pushes changes to a remote version of that branch, you can run git pull --rebase while on your local "gsoc2019_numberFormat" branch to bring it up to date.
Next, make your changes, add and commit. Then push the changes to the remote joliveira_fork remote:
git push joliveira_fork gsoc2019_numberFormat
Generating "eternal" URLs to commits or objects in a repository
History has taught that no system used by KDE around the code repositories stays forever. Quickgit, CGit, Phabricator & Co. came and at one point were replaced again. Sadly also taking with them the service-specific URLs (and host names).
To give documentation, blog posts, commit messages and other long-living documents a way to reference commits or objects in the repository like directories or files, at given branches or at given tags, the service commits.kde.org exists. It maps and forwards URLs to the respective current service URLs.
The pattern for URLs to commits is this:
https://commits.kde.org/<repo-id>/<commit-id>
Example:
https://commits.kde.org/kcoreaddons/d2f4d353327b322ee6bfcc303169190ae44393f0
The pattern for URLs to objects is like this:
https://commits.kde.org/<repo-id>[?[path=<pathToFileOrDirectory]&[branch=<branch>|tag=<tag>]]
<path> should be without a leading /. It defaults to the top-level directory if not set. Either a branch or tag can be passed at which the objects should be shown. It defaults to the main branch (master usually).
Examples:
https://commits.kde.org/kcoreaddons?path=src # points to src/ directory in master branch https://commits.kde.org/kcoreaddons?path=README.md&tag=v5.0.0 # points to README.md file at tag v5.0.0 https://commits.kde.org/kdelibs?path=kdecore/tests&branch=KDE/3.5 # points to kdecore/tests directory in branch KDE/3.5
There currently is no service to generate commit.kde.org URLs from URLs for the actual system. This has to be done manually.
Creating a merge request using the command line
Gitlab supports git push options that can push your branch and create a merge request in one go. For example:
git push -o merge_request.create -o merge_request.target=master -o merge_request.remove_source_branch -o merge_request.assign=foo
replace foo with your actual user name. This command will push the branch, create a merge request, targeting the master branch, enable the "delete branch on merge" option, and assign the merge request to user foo.
You can change any of those parameters to suit your usage. For more details on Gitlab push options see this.
If you have a branch with several commits, most likely you'll still need to tweak the merge request description ...etc.
Of course that's a lot to type, so it's easier to use a Git alias, for example this command add an alias push-mr to your Git config:
git config --global alias.push-mr 'push -o merge_request.create -o merge_request.target=master -o merge_request.remove_source_branch -o merge_request.assign=foo'
then you can use git push-mr like any other Git command. For more information about Git aliases see this