GitHub Pull Request Pitfalls
GitHub's pull request features and workflow have a few pitfalls that catch new and experienced users alike. Other Git services besides GitHub may have some or all of these issues too, so look it over regardless of what you use.
These aren't in any particular order, they're all important, so here's the list up front:
- Use a new branch, not main
- Fork and open using the same user
- Check the target branch
- Draft must be enabled from the start
That first one is somewhat well known, I think, but I haven't seen anyone talk about the others before. People are consistently surprised when I explain the unexpected effects of these when reviewing their PR. And it's not their fault, GitHub's UI for the PR workflow makes it so easy to make these mistakes that I still do even though I'm aware of them all.
Use a new branch, not main
As a maintainer, I absolutely love that GitHub allows me to push to PR branches opened by contributors. Rather than needing to nitpick about every minor thing in review, or explain complicated git operations, I can go in and write a changelog, clean up commits, etc, saving time.
I do this at the end, after the bug fix or feature itself has been finished. If I push more commits, the user would need to remember to pull those before working further, not a common thing to remember in the PR workflow most people learn. If I rebase to clean up the PR and force push, it would be even more confusing to resync. So I save it until right before I'm ready to merge.
But that consideration falls apart if they open the PR from their fork's main branch (or other "main" sort of branch, like 2.3.x). main continues on after a PR is finished. So if I force push to their main branch (or use the "rebase" or "squash" merge options), they either need to wipe and re-clone, or know the following incantation to reset.
$ git fetch upstream
$ git reset --hard upstream/main
$ git push -f origin/main
This not only assumes they're comfortable with less common git commands, but that they added a second remote called upstream pointing to the original repository. That probably won't be the case unless they used GitHub's gh CLI or the desktop client.
To save everyone some potential confusion, make sure to start a new branch to work on a new PR.
$ git switch -c descriptive-branch-name
Fork and open using the same user
Some users have multiple accounts or organizations, usually to separate personal and professional work. It's possible to fork a repository to one account or organization, but then work on it and open the PR as another account.
If the user that owns the branch and the user that opens the PR don't match, GitHub's "allow maintainers to edit" feature does not work. There is no warning about this mismatch or consequence when opening the PR. If you notice this, you can fork to your own user, push the branch there, then open the PR with everything matching.
As a maintainer, you can get around this similarly by checking out the PR locally, closing the existing PR, and pushing the branch as a new PR. I put "continue #...` as the description. The user still gets credit for the commits they made, and you can do any additional cleanup. Wait until after they've responded to review though, since they won't be able to work on the new PR branch.
Check the target branch
Flask and most of my other projects use a main branch and maintenance branches for each feature release. Changes intended for the next feature release merge into main, while bug and docs fixes against the current release merge into the corresponding maintenance branch, for example 2.3.x. Most projects have a similar scheme of separating future features from current fixes.
Be aware of the appropriate target branch when starting a branch and when opening a PR. If you're submitting a bug or docs fix, create your branch from the appropriate maintenance branch instead of main. Additionally, make sure that branch is selected when creating the PR. There's a [target] <- [source] set of dropdowns, allowing you to change the target. If you don't set the target correctly, then you'll see a confusing list of hundreds of commits, because you told GitHub to try to merge your changes into a main that has significantly divered, or backwards from main into a maintenance branch.
This is especially annoying for documentation fixes. Often, it's single typo. Our docs are set up to build and publish after every merge, and users expect to see their fix in the current version's docs. If the change is submitted against main instead, I need to change the target on GitHub, clone locally, rebase, force push, and wait for CI to pass again. It's a huge imbalance in time taken to make the PR vs merge it.
You can fix this after opening your PR. If you notice it, first change the target in the GitHub UI by clicking "Edit" next to the title, and selecting the correct branch from the dropdown that appears. Then rebase onto the correct base branch and force push. For example, if you meant to branch off 2.3.x instead of main:
$ git rebase --onto upstream/2.3.x upstream/main
$ git push -f
Draft must be enabled from the start
The draft feature allows opening a PR in "draft" mode, providing a "Ready for review" button for you to click after the fact. PRs opened in draft mode will not send notifications on every push, which means that you won't annoy maintainers and other subscribed users as you experiment and get CI to pass. Once it's ready, clicking "Ready for review" sets the PR to normal mode and sends notifications for further changes.
The key here is "opened in draft mode" though. It's possible to open a PR in normal mode, then convert it to draft mode, but at that point it's too late. Every watcher will already have been subscribed to notifications for that PR, which isn't undone by changing it after the fact.
So if you want to show some work that's not ready to merge yet, be sure to click the arrow on the "Create Pull Request" button and select "Create Draft Pull Request" first.