Process: investigate GitHub code review replacements

[keith-zephyr]

The GitHub review UI is missing several features present in other review tools that make reviews more effective.

Some missing features of the GitHub review system include:

1. Ability to see deltas between different versions of the same PR
2. Ability to comment on lines of code not changed by the PR
3. Ability to highlight and comment on less than a full line of code

Any replacement review interface needs to work in conjunction with the GitHub review system, at least initially.

Possible alternatives:

• reviewable.io: Solves limitation 1. Adds some unwanted spam into the comment history of PRs. Individual users can use reviewable now on specific PRs. Comments and approvals on reviewable propagate to GitHub automatically. We could enable reviewable to automatically track all PRs.
• gerrithub.io: Gerrit solves all the limitations noted above. It should be possible to use Gerrit and GitHub in parallel on the Zephyr repo, but the comment history and approvals are not shared between Gerrit and GitHub (I think). This may require Zephyr community members to monitor both Gerrit and GitHub for changes. This is a model followed by the flashrom, most changes are made through Gerrit, and then mirrored over to GitHub.

[jeremybettis]

Other problems if you are not a collaborator on the project.

If someone leaves a comment or requests changes, the PR is blocked until those changes are resolved, but only the person who made the command can resolve them. And since you can't assign reviewers either, there isn't even a way to signal that you need that person to come back and resolve those comments.

If you force-push, which is required, then no one can resolve the requested changes.

[mbolivar-nordic]

Process WG minutes:

• @nashif: GitHub originally supported diffs between versions at the time of Zephyr's switchover from Gerrit to GH. This subsequently changed. The move from Gerrit was also partly due to infrastructure issues with self-hosting the instance in the Linux Foundation.
• @keith-zephyr: reviewable offers desired features, but spams the comment section a whole bunch. Meant more for small teams since it pings everyone on the PR when a version is reviewed. Worth discussing process improvements in general.
• @dleach02: another issue with GH is that an approval can come in before a major code change
• @keith-zephyr : do we have any contacts at GH to discuss features/improvements?
• @nashif: we tried it in the past; it's a known problem at GH. Question is if GH refuses to implement the above requirements, should we move to another Git host? Would be a major change. We can probably do something on our side, like to create the diff with a bot or something like that
• @keith-zephyr: if Zephyr becomes a contributor or paying member of reviewable, perhaps they'd be willing to change their feature set. Another issue is we have too many reviewers being assigned per PR.
• AI @keith-zephyr : search for open issues in GH to see diffs between patch sets, etc
• @nashif : moving away from GH would be a major change; we're very committed to it at present
• @mbolivar-nordic : https://docs.zephyrproject.org/latest/contribute/index.html#contribution-workflow is our existing workflow; has many issues in my opinion

[keith-zephyr]

GitHub has Pull request revisions listed under the Future work of the public roadmap.

It was added to roadmap June'21 and we don't have visibility as to when this will be added into the a named release.

[marc-hb]

Ability to see deltas between different versions of the same PR

This and other features are missing simply because Zephyr does not use Github the way it's meant to be used, at least not before https://github.com/github/roadmap/issues/211 gets implemented. Right now Github tolerates force-pushes but does not really support them. Github is supposedly to be used like this:

• Single commit per Pull Request
• Never rewrite git history and never force push to address review comments. Add "fixup" commits instead.
• Optionally, squash before merge

When using Github "natively", deltas between versions become obvious and many other problems go away too.

• doc: add note about force pushes to contrib guide #14444 (comment)

The "native" Github workflow is very different from how "plain git" and Gerrit users work. Most Github users and projects don't care. I recently had to "train" a new hire on rewriting git history to address review comments. He was proficient with Github but not with git, this was all completely new to him.

Gitlab fares a bit "better" but not that much: https://gitlab.com/gitlab-org/gitlab/-/issues/24096

cc:

• Review process: force push should not require re-approval #43701

[mbolivar-nordic]

Process WG:

• @keith-zephyr : GitHub has a feature proposal for seeing deltas between force pushes but it's not in any roadmap. The one I found was opened last year (Process: investigate GitHub code review replacements #39194 (comment)).
• @MaureenHelm : is this on the roadmap or not?
• @keith-zephyr : not assigned a release version yet; nothing we can do for now
• @MaureenHelm : my memory was the move to GH was because it was a lower barrier for new contributors
• @keith-zephyr : another issue is that you can approve a PR, then the code can be changed substantially but the approval is still there
• @carlescufi : you can configure GH to remove the approvals, but this would slow the merge process
• @keith-zephyr : are there thresholds? e.g. in Gerritt you have the option to keep reviews if the set of files changed is the same between two force-pushes
• @carlescufi : it's all or nothing in GH; a push either resets all reviews or it doesn't
• @nashif : I propose enabling this. I've seen such situations often, including today, and sometimes incorrect things get merged
• take it to the TSC

[mbolivar-nordic]

@keith-zephyr have you looked at https://gitgitgadget.github.io/? There are some potentially interesting things in there.

[henrikbrixandersen]

I think it is worth considering what the low barrier of entry provided by using a well-known, user-friendly platform like GitHub for code reviews means for the projects popularity and growth, shortcomings or not.

[marc-hb]

Ability to see deltas between different versions of the same PR

This and other features are missing simply because Zephyr does not use Github the way it's meant to be used, at least not before https://github.com/github/roadmap/issues/211 gets implemented. Right now Github tolerates force-pushes and rewriting git history but does not really support them.

The extreme difficulty to compare two given versions of the same PR is one thing[*]. The fact that old versions get garbage-collected after a while is another. But the most irritating is probably the semi-random loss of review discussions. I'm guessing this happens when the corresponding code was changed and force-pushed but not always, sometimes there is an "Outdated" warning and the old discussion is still visible in the Conversation tab. Sometimes you can even answer "Outdated" discussions, others not. Mysterious.

For instance I received some time ago the following email notification (I use email as a "Github backup" - the irony)

Looks good to me (when other comments are addressed), but you'll need to rebase this after my dtrace buffer alignment PR.
In src/trace/dma-trace.c:

> @@ -228,7 +228,7 @@ static int dma_trace_buffer_init(struct dma_trace_data *d)
 	buf = rballoc(0, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA,
 		      DMA_TRACE_LOCAL_SIZE);
 	if (!buf) {
-		tr_err(&dt_tr, "dma_trace_buffer_init(): alloc failed");
+		mtrace_printf(LOG_LEVEL_ERROR, "dma_trace_buffer_init(): alloc failed");

Could start to use "%s(): ...", func now that you touch the line. <=== this discussion is GONE!

Reply to this email directly, view it on GitHub, or unsubscribe

I just spent 5+ minutes looking for some "resolved" conversation where this could have been hiding but no: Github lost that review comment. You have been warned.

More issues like this listed in #14444 (comment). No issue when using Github "correctly".

[*] click on "force-push" then enter the fully 40 digit SHA in the URL.

[mbolivar-nordic]

I just spent 5+ minutes looking for some "resolved" conversation where this could have been hiding but no: Github lost that review comment. You have been warned.

Are you sure the author didn't just delete the comment?

[jsarha]

I think I did indeed delete the comment, after I learned that that %s does not work with mtrace either. I am a bit surprised that the comment vanished without leaving any trace to history. Next time I'll edit the comment and just add a note that it can be ignored.

[marc-hb]

My bad, maybe Github never deletes comments. "Outdated" comments do disappear from the source code tab, which makes them nearly impossible to find in the "Conversation" tab in long reviews - especially when people use the "resolve" feature. Other issues listed in #14444 (comment) stand.

[mbolivar-nordic]

Process WG:

• @keith-zephyr : we've decided not to replace GitHub, but there is still an open question of how to improve the review process in general. Several issues, such as number of reviewers assigned leading to inaction, are still open
• @nashif : some issues should start to be addressed by CODEOWNERS being replaced with MAINTAINERS

[marc-hb]

There is now a Google Summer of Code project idea to add git range-diff and solve some of these problems in... Gitlab: https://gitlab.com/gitlab-org/gitlab/-/issues/24096
Give it a thumb up if you have a gitlab.com account. Competition never hurts.

[marc-hb]

"Outdated" comments do disappear from the source code tab, which makes them nearly impossible to find in the "Conversation" tab in long reviews

Fun fact: if some code is modified twice in the same PR in two different commits (e.g. 1. Cleanup commit 2. Functional change commit) then comments on the first commit are instantly marked as "Outdated" in the main Conversation tab and never appear in the default "Files Changed" tab. You can still see them if you select the first commit in the "Files Changed" tab, where they are not marked as "Outdated".

This is a pretty clear demonstration that latter commits in the PR are expected to amend earlier commits and that earlier commits represent an "Outdated" version of the code.

This and other features are missing simply because Zephyr does not use Github the way it's meant to be used, at least not before https://github.com/github/roadmap/issues/211 gets implemented.

This roadmap ticket 211 "Pull request revisions and improved workflows" has been... deleted! Thanks to the Wayback machine you can still find the UI mockup of a git range-diff feature:

Since 211 has been deleted the "permanent" workaround is to minize rebases:

https://github.com/zephyrproject-rtos/zephyr/pull/52731/files/e4433198f750a95e5a3602da398bc38c0c5e39ca#r1133166082

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#workflow-suggestions-that-help-reviewers

As GitHub does not implement git range-diff, try to minimize rebases in the middle of a review