kilabit.info
Build | sr.ht | GitHub | Twitter

Github PR is bad

If you active on Internet, you may have read Torvalds rant about how bad Github pull request (PR) is. If not, read the discussion here and here.

Actually, it is not only the pull request that is bad. There are many. I am collecting those things in these journal.

Note
I use the term PR and patches in the same way.

Github pull request flow is really bad

Let me show you.

I have four commits to be submitted to the upstream, each are independent.

One branch per pull request

To send each of this commit I need to,

  1. Create new branch based on the origin/master branch

  2. Cherry pick the commits

  3. Push to my remote

  4. Open the web, create pull request, select source and target branch, click Create; or using gh pr create, which is have several steps as for the web.

I need to repeat this steps for every PR.

Things get out of hand if the first branch is indirect dependency of second branch, because you cannot just based a branch on another branch in the PR, otherwise all commits in the first branch PR are included in the second PR branch.

Let me give you an example.

D -- branch-2
|
C
|
B -- branch-1
|
A
|
o -- upstream/master

If we submit branch-1, commits A and B are displayed on the PR. If we submit branch-2, that depends on A and B, commits A, B, C, and D are displayed on the PR (because A and B does not exist yet on upstream/master).

Why we are not basing the branch-2 on the upstream/master? Because it is not possible, the program is not buildable without branch-1.

Why not submit all commits into single branch? It is possible but in my books its not a good practices. Its hard to review and in case one of the commit need to be revised, I need to re-base the whole commits (adding another commit to fix PR also is not a good practices).

Can it be more simple? Yes, in fact, the de facto way to send "pull-request" is really simple.

This is how it should be. Lets view all of our commit hashes to be submitted.

$ git --no-pager log --oneline -n 4
8fd061dc (HEAD -> master, shulhan/master) docs: set environment CI=true when building from source
0985cbfe kms/uri: fix test on Parse for the next Go release
84a0a348 cas/cloudcas: update createPublicKey test for Go 1.19
fe04f93d all: reformat all go files with the next gofmt (Go 1.19)

To send the PRs for commit fe04f93d,

$ git send-email --to="recipient@domain.tld" --dry-run -1 fe04f93d

(The dry-run options is for testing.)

To send the PRs for the rest of commits, independently,

$ git send-email --to="recipient@domain.tld" --dry-run -1 84a0a348
$ git send-email --to="recipient@domain.tld" --dry-run -1 0985cbfe
$ git send-email --to="recipient@domain.tld" --dry-run -1 8fd061dc

Rebasing or ammending the patches break the web history

The more annoying than this is how Github handle reviewing the PR. If someone review your PR by commenting on the code and you push the fixes (by git rebase/git ammend) for the next round, the links between comments and previous patches is lose.

The "View changes" on the comment section open the new commits, not on previous patches.

Here is an example:

Github comment history lose history

The comment point to the line that has been fixed by the author. Now, can you figure it out what the line is from the linked Source?

Compare this with gerrit,

Gerrit review

At the left side you can see the offending code that needs to be fixed (this is Patchset 1), and on the right side you can see the fixes (Patchset 2). None of them mixed.

Reviewing only allowed on affected code

Given the following changes,

Github comment review bad

User cannot comment on expanded lines 151 that affected by the above changes.

Where is the open review?

Another developer create pull request and assign you as the viewer,

Github reviewer

But you cannot see it in the Pull requests page,

Github pull review request is empty.

Also, open the following links in your browser: https://github.com/pulls?q=is%3Aopen+is%3Apr

You will see all of the open PR from all repositories is listed.

Github PR list

The URL is "/pulls" but the query still need is:pr. If you remove the is:pr field, you will get list of PR and open issues. Talks about inconsistency.


Thats it for now, will update later when I have more screenshots.