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

Alex Bradbury via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 2 08:00:25 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.
----------------
asb wrote:

Thanks for explaining in more detail. I am all too familiar with GitHub's poor support for tracking PR evolution vs the Gerrit/Phab model, but hadn't seen that merge commits provided a good solution for this even if you end up with a squash+merge at the end.

I do think the argument for recommending merge commits is stronger for the cases already explicitly covered in the documentation rather than this new case I'm adding, on the basis that if you see failing CI you likely want to rebase rather quickly to fix it rather than much later in the review.

As I don't think my proposed change is really altering our advice to contributor, just highlighting a case where a rebase can be particularly useful, how would you feel about this change landing and separately initiating a discussion about whether we should change our current contributor advice to recommend merge commits when necessary while evolving a PR instead?

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


More information about the llvm-commits mailing list