LLVM GitHub User Guide — LLVM 18.0.0git documentation says “In general, you should avoid rebasing a Pull Request and force pushing to the branch that’s the root of the Pull Request during the review.”
I very much understand that
The fidelity of preserving inline comments after a force push has always been a weakness. The comments may be presented as “outdated”. In the past, there was a notorious “lost inline comment” problem.
Nowadays, the situation has improved, but some users still report that inline comments may occasionally become misplaced.
However, the strong words “should avoid” made me a bit concerned. I have some analysis here
In a large repository, avoiding rebases may not be realistic due to other commits frequently modifying nearby lines. Some people use a remote branch to save their work. Having to worry about whether a rebase could cause spam makes the branch more difficult to use. When working with both the latest main branch and the pull request branch, switching between branches results in numerous rebuilds. Rebasing follow-up commits could lead to merge conflicts and more pain. In addition, a popular convention among many LLVM contributors is to commit tests before landing the functional change, which also mandates force-push. Sometimes, only through rebasing can one notice that the patch needs adjustments to more code or tests due to its interaction with another landed patch:
- Another landed patch added some tests that would be changed by the current patch.
- Another landed patch added a new use of the function that is renamed by the current patch.
IIUC The CI does something like
git fetch -v --prune -- origin refs/pull/xx/head; git checkout -f commit. It doesn’t check the test result on the latest main branch.
I’d like to hear about:
- The fidelity of preserving inline comments and analysis of “Outdated”. How is it problematic after a rebase or a force push? IIUC, appending a new commit without a rebase/force-push makes nearly comments outdated as well.
- Whether there are other situations that a rebase is necessary.
- Whether a rebase followed by appending a fixup commit is recommended, or acceptable, i.e. the original commit set (A) becomes (A’ + B) where A’ is nearly identical to A but with a different base.
- Any possibility softening the word “should”.
- How to avoid force push in the presence of stacked patches.
- If the foundation has a channel to talk to GitHub, help bring awareness to Improve workflow when force-pushing during code reviews · community · Discussion #3478 · GitHub
I will be out of town for about 2 weeks and will not likely follow up with this thread soon.