How to resolve conflicts between 2 successive PRs?

[fyliu]

When 2 unmerged PRs exist, sometimes the 2nd one will be in conflict after the first one is merged. We need a developer guide on how to handle this.

[fyliu]

The high level is this:

The developer needs to rebase PR #2 on the new main branch and resolve any conflicts. That way, PR #2 can be merged cleanly by the repo maintainers without extra work.

[joey-ma]

I've always done the git pull / git merge method, but after looking it up, git rebase makes sense, too.

The Rebase Option

As an alternative to merging, you can rebase the feature branch onto main branch using the following commands:

git checkout feature
git rebase main

This moves the entire feature branch to begin on the tip of the main branch, effectively incorporating all of the new commits in main. But, instead of using a merge commit, rebasing re-writes the project history by creating brand new commits for each commit in the original branch.

Reference: https://www.atlassian.com/git/tutorials/merging-vs-rebasing

[fyliu]

This is what I’ve done for a long time as a code contributor and it’s what I see open source maintainers request contributors to do.

It used to be a scary thing to rebase and people discourage using it, especially for beginning devs who are trying to learn programming and git at the same time. Mainly, it’s the rewriting history part. Rebasing the main branch will cause trouble to a lot of people. Rebasing a feature branch is mostly safe and affects only the developers working on it.

The idea is that contributors should specify how they want their changes applied on top of the main branch because they know their changes best. Maybe the order of code statements matter. It’s assumed that the code in main is working and tested, and the incoming code is the portion in question. If there’s a conflict, only the contributor knows what order the code needs to be added in to work. So it makes sense that the contributor should be the one to resolve the conflict and make sure the code still runs as expected before submitting the PR again.

There’s scary things that can happen with the git merge method of merging as well. When there’s a merge conflict, the merge is stopped and the be user gets a prompt to resolve the conflict before continuing. This is where the problems can sneak in. This mechanism is meant to allow the developer to “say” whether their new changes are in addition to the ones in main, or replacements of those changes. That’s fine if its the only thing the developer does. But sometimes extra changes sneak into these merge resolution commits and they’re hidden from git history. Even if it’s a tiny typo fix, it should go into a regular commit and be tracked. This makes me see merge commits as dangerous. This scenario has no way of happening if the feature branch is rebased.

A related thing to rebase but unrelated to merge conflicts is if the contributor made 100 nonsense small step commits, the maintainer might ask them to squash them into fewer commits that make more sense. That can be done by git commit as well, using the command git commit -i HEAD~101. The idea is that code changes should be clean, meaning you apply each commit and it should still work. Commits like “let’s add a line before X” and then “nope, let’s move that line after X” should be squashed into one saying “”. It’s perfectly fine to commit often during development and try one thing or another. But it should be cleaned up into a few commits before merging. This is why some teams prefer to use GitHub’s squash merge method. It collapses all contributing commits into one and combines all the commit messages. The problem I have with it is that it takes control of the changes away from the contributor. It’s like saying “here’s all the code changes that the contributor made”. I would like it more if the contributor can clean it up themselves into a few commits. Maybe “refactor the code in preparation for the new change”, “make a change”, “make the next change”, “make the final change”. Each commit might have something in the message on the reasoning of the change if it’s not obvious. Like if the order of the lines matter, or if the order of the function arguments matter.

It’s all meant to elevate the skills of the contributors. On the one hand, I think we should nudge developers to pick up these skills and discipline, but I also don’t want to over do it. So maybe I try to pick and work on one thing for each PR. I also try to offer to do some of this for the contributor, since their code might be working fine before the merge. I’d make sure to keep them as author in the commit metadata of course. That’s another detail maintainers should know and do.