Merge requests

Steps to create new merge requests

Create a new local branch for the fix or feature

Once a local checkout of the project is available, the user can now create a new local branch for the new feature being developed or for the fix that is going to be suggested. The new branch should be based always on top of upstream/main.

  $ cd ModemManager
  $ git fetch upstream
  $ git checkout -b my-branch upstream/main

At this point, the user can start modifying files and using git to keep track of the changes, as usual.

Prepare and cleanup the branch

Once the user has developed a series of commits for a given fix or new feature, it is time to suggest the changes for review.

The first thing to do is to make sure the new branch is rebased on top of the latest upstream/main. E.g. if the branch where the fix was developed was created 2 months ago, it is very likely that new changes have gone to the main branch in the upstream repository; the user should make sure all those changes don’t conflict with the new ones being suggested.

  $ cd ModemManager
  $ git fetch upstream
  $ git checkout my-branch
  $ git rebase upstream/main

If the rebase cannot finish automatically, the user will need to perform an interactive rebase in order to fix any conflict that may have arised.

  $ git rebase -i upstream/main

Checklist before pushing the branch for review

Key requirements

Make sure the gitlab pipelines pass for your branch.
Make sure git commit style is according to the expected format.
Make sure coding style is according to the expected format.

The branch must allow git bisecting

In order to allow git bisecting the branch, no single commit should break the build at any point. This can be checked by running a build after each commit in the following way:

  $ git rebase -i upstream/main --exec "make -j8"

No new gtk-doc warnings

The developer should make sure the local builds are done with --enable-gtk-doc, and if there are new warnings introduced by the branch, these must be fixed before suggesting it for review.

E.g. adding a new MBIM service in libmbim could end up triggering the following warnings in the gtk-doc step:

  DOC   Scanning header files
  DOC   Introspecting gobjects
  DOC   Building XML
./libmbim-glib-unused.txt:1: warning: 21 unused declarations. They should be added to libmbim-glib-sections.txt in the appropriate place.
  DOC   Building HTML
  DOC   Fixing cross-references
html/libmbim-glib-QDU.html:287: warning: no link for: "MbimQduFileType" -> (<span class="type">MbimQduFileType</span>).

Learning how to fix these type of issues is easy, just investigate how other similar methods or types are documented, and do the same.

Push the branch and create a merge request

In order to create a new merge request against the upstream repository, the user should push the branch to the user-specific repository (origin remote).

  $ cd ModemManager
  $ git checkout my-branch
  $ git push origin my-branch
  Enumerating objects: 57, done.
  Counting objects: 100% (57/57), done.
  Delta compression using up to 4 threads
  Compressing objects: 100% (26/26), done.
  Writing objects: 100% (47/47), 8.76 KiB | 2.92 MiB/s, done.
  Total 47 (delta 35), reused 32 (delta 21), pack-reused 0
  remote:
  remote: To create a merge request for my-branch, visit:
  remote:   https://gitlab.freedesktop.org/username/ModemManager/-/merge_requests/new?merge_request%5Bsource_branch%5D=my-branch
  remote:
  To gitlab.freedesktop.org:username/ModemManager.git
   * [new branch]        my-branch -> my-branch

Once the branch is pushed, the git server will provide a link to create a merge request for the newly pushed branch, as seen above.

The user should open the given link, and gitlab will provide a short form to fill before the merge request is finished. Very important things to consider when filling in the form:

If the merge request contains a series of commits, the user should write a description that explains the purpose of the whole series.
If the merge request contains one single commit, the description of the merge request can be the commit message.
The Allow commits from members who can merge to the target branch option in the Contribution section must be enabled. With this option, the project maintainers are allowed to push or rework the changes of the merge request in its original location (the user branch).

Once the Submit merge request button is clicked, the process to create the merge request would be finished, and it would be now time for maintainers to review the changes suggested.

The review process

The review of the merge request is done commit by commit, and therefore each commit should be self-contained, in the sense that it should explain clearly the change being done by itself, even if it is part of a series of commits. If this is not done, e.g. if a huge 1000 line commit changing 100 different files at the same time is suggested, it is very likely that the merge request will be rejected. Not because the change is not good, it may be perfectly good, but because it’s not review-able.

Reworking the branch after review

If the maintainer asks for changes to be done, work on them. Don’t add additional commits on top of the original ones for the changes suggested by the maintainer; instead, fixup the changes in the correct commit. It is very suggested that the user knows how to use git commit --fixup and git commit --squash, as these are very powerful tools to have the commits right. A simple usage example of --fixup can be seen in the following snippet, where a quick fix is added on a given already existing commit in 2 steps: first a new fixup commit is created, then an interactive rebase reorders the commits automatically to apply the fixup commit in the correct place:

  $ git log --oneline
  91b399cd shared-qmi: acquisition order preference TLV always same items
  ef55409b shared-qmi: process all feature checks in SSP response together
  1a3c3b9e shared-qmi: if getting TP/SSP fails, assume unsupported
  c1dc9339 shared-qmi: add missing feature check flag initialization to UNKNOWN

  $ git status
  On branch my-branch
  Changes not staged for commit:
    (use "git add <file>..." to update what will be committed)
    (use "git restore <file>..." to discard changes in working directory)
  	modified:   src/mm-base-modem.c

  no changes added to commit (use "git add" and/or "git commit -a")

  $ git commit -a --fixup 1a3c3b9e
  my-branch 9788d020] fixup! shared-qmi: if getting TP/SSP fails, assume unsupported
  1 file changed, 3 insertions(+), 1 deletion(-)

  $ git log --oneline
  9788d020 fixup! shared-qmi: if getting TP/SSP fails, assume unsupported
  91b399cd shared-qmi: acquisition order preference TLV always same items
  ef55409b shared-qmi: process all feature checks in SSP response together
  1a3c3b9e shared-qmi: if getting TP/SSP fails, assume unsupported
  c1dc9339 shared-qmi: add missing feature check flag initialization to UNKNOWN

  $ git rebase -i --autosquash upstream/main
     // inside editor, the fixup commit is reordered and setup to fixup
     pick c1dc9339 shared-qmi: add missing feature check flag initialization to UNKNOWN
     pick 1a3c3b9e shared-qmi: if getting TP/SSP fails, assume unsupported
     fixup 9788d020 fixup! shared-qmi: if getting TP/SSP fails, assume unsupported
     pick ef55409b shared-qmi: process all feature checks in SSP response together
     pick 91b399cd shared-qmi: acquisition order preference TLV always same items

  Successfully rebased and updated refs/heads/aleksander/my-branch.

  $ git log --oneline
  82ca83ad shared-qmi: acquisition order preference TLV always same items
  0a24c403 shared-qmi: process all feature checks in SSP response together
  0cdbba14 shared-qmi: if getting TP/SSP fails, assume unsupported
  c1dc9339 shared-qmi: add missing feature check flag initialization to UNKNOWN

Once the updated branch is considered ready, the user should re-push the branch to the same place, i.e. the origin remote and the same branch name as was originally used when creating the merge request. If the branch has been re-created (e.g. after applying fixup/squash commits), the user will need to force push it. There is no need to create a full new merge request after updating the branch.

  $ git push --force origin my-branch
  Enumerating objects: 9, done.
  Counting objects: 100% (9/9), done.
  Delta compression using up to 4 threads
  Compressing objects: 100% (5/5), done.
  Writing objects: 100% (5/5), 1.14 KiB | 1.14 MiB/s, done.
  Total 5 (delta 4), reused 0 (delta 0), pack-reused 0
  remote:
  remote: View merge request for my-branch:
  remote:   https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/477
  remote:
  To gitlab.freedesktop.org:username/ModemManager.git
   + 08c3de0b...9e75a1ff my-branch -> origin/my-branch (forced update)

As seen above, gitlab will provide the link to the already existing merge request that is being updated.

Updating the local branch after a forced push by others

Sometimes the maintainers themselves will do some reworks in the user branch, and ask the user for review of those changes. In that case, the forced push from the previous section may have been done by the maintainers (if the Allows commits from members who can merge to the target branch option has been enabled in the merge request), and the user will need to update the local branch with the latest commits from the remote branch before doing any additional change or in order to locally review the new updates.

The same will be required if the branch is re-created by gitlab itself (e.g. after running Rebase in the merge request page in gitlab).

Synchronizing the remote with the local branch can easily be done with a fetch operation first (see how the message reports a forced update), and then a hard reset to point the local branch to the new remote one:

  $ git fetch origin
  remote: Counting objects: 11, done.
  remote: Compressing objects: 100% (6/6), done.
  remote: Total 6 (delta 4), reused 0 (delta 0)
  Unpacking objects: 100% (6/6), 746 bytes | 746.00 KiB/s, done.
  From gitlab.freedesktop.org:username/ModemManager.git
   + b0b78e8...693b464 my-branch -> origin/my-branch  (forced update)

  $ git checkout my-branch

  $ git reset --hard origin/my-branch

After the last commit, the local branch will be in sync with the latest contents from the user remote.

Accepting the merge request and merging to main

Once the suggested changes are accepted by the maintainers, one of them will merge the merge request on top of git main. If the changes are bugfixes that apply to older stable releases, the maintainer may also decide to cherry-pick them to the older stable branches. The user should not need to do that at this point.