[llvm] [docs] Make rebase advice more explicit for cases where pre-commit CI is failing (PR #76704)

James Y Knight via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 2 07:25:58 PST 2024


================
@@ -105,7 +105,9 @@ branch that's the root of the Pull Request during the review. This action will
 make the context of the old changes and comments harder to find and read.
 
 Sometimes, a rebase might be needed to update your branch with a fix for a test
-or in some dependent code.
+or in some dependent code. This is especially encouraged if it turns out that
+the upstream base commit used for your branch had test failures, meaning the
+pre-commit CI results are not useful.
----------------
jyknight wrote:

Github provides very poor support for tracing the history of a PR, and looking at changes since you last reviewed, across a series of rebases. This is very much unlike gerrit/phab, where that's the primary way they provide to see changes.

But it provides good support for looking at a series of new patches pushed on top.

As a reviewer, I want two things:
1. look at new changes that were made in response to comments
2. look at the complete diff from mainline.

The first is easier when rebasing is avoided, and the latter is not harmed by the presence of merge-commits in the PR history. You can see an example of a PR with merge-commits in my pending PR here: https://github.com/llvm/llvm-project/pull/74275. (There's plenty more examples, though.)

Here's an example PR with lots of history and multiple rebases: https://github.com/llvm/llvm-project/pull/65534. I find it very hard to look at that and see what's actually changed. Note that all the commits in the timeline are listed as "3 weeks ago". But they're actually older. There are "force-pushed" notices interspersed elsewhere, but it's still quite difficult to reconstruct when they were actually introduced.

Also, once you have a big patch with lots of changes like that, doing rebases starts getting really noisy in notifications -- it resends the list of every commit in the PR, since they've all been pushed fresh.

Regarding "squash-and-merge", that takes the complete diff from mainline, and makes a new commit with that -- so it works just fine if you have merges in your PR branch.


https://github.com/llvm/llvm-project/pull/76704


More information about the llvm-commits mailing list