[docs] Make rebase advice more explicit for cases where pre-commit CI is failing #76704
Conversation
… is failing You could argue that rebasing if pre-commit CI is failing due to issues unrelated to your patchset is already suggested, but I think it would be beneficial to make this advice more explicit. If pre-commit CI is failing because the base commit you used was bad, you're doing your reviewers a service by rebasing in order to fix that issue.
llvm/docs/GitHub.rst
Outdated
| 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. |
There was a problem hiding this comment.
It may be phrased a bit strongly: I often see test failures in pre-commit CI in other projects like while modifying MLIR: as long as MLIR and Flang tests are passing, the pre-commit CI results aren't "useless".
I would say only when there is a build failure and the tests can't run that the results aren't useful anymore.
That said the advice isn't bad for beginner.
There was a problem hiding this comment.
How about "not useful" => "misleading"? The big issue with failures is that you see that red cross and have to click through for more details to see if it's a "real" problem (an issue with the PR author's patch) or not.
There was a problem hiding this comment.
What about:
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 can't be shown as cleanly passing.
There was a problem hiding this comment.
Why are we encouraging rebase versus merging main into your branch to address this issue? IMO, we should explicitly discourage any use of rebase.
There was a problem hiding this comment.
@joker-eph: Thanks, I've adopted that change.
@jyknight: Given none of our current documentation discusses introducing merge commits (even if they disappear before hitting the main branch) I think this would be more complex to explain. For me, I'm only really used to this rebase-focused workflow so I'm not actually sure how GitHub would handle your suggestion in its UI - both when showing changes for review and in terms of allowing "squash and merge" to commit. I'd love to learn more though. Is there a particular advantage to doing it as you suggest?
There was a problem hiding this comment.
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:
- look at new changes that were made in response to comments
- 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: #74275. (There's plenty more examples, though.)
Here's an example PR with lots of history and multiple rebases: #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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Chiming in as the author of aforementioned #65534: I've decided to go with git merge instead of git rebase from now on exactly due to the UI reason -- i too find it much easier to navigate through changes this way, when imo rebasing just breaks history.
|
Thanks Mehdi for the review. I'll hold off on committing until James' query about recommending merging main instead is either resolved or we agree to defer to a separate discussion. |
|
I've started a discourse discussion here to try to move this forward, but as noted I don't think this clarification/refinement of existing recommended practice necessarily needs to block on the potential decision about changing that recommended practice. |
You could argue that rebasing if pre-commit CI is failing due to issues unrelated to your patchset is already suggested, but I think it would be beneficial to make this advice more explicit. If pre-commit CI is failing because the base commit you used was bad, you're doing your reviewers a service by rebasing in order to fix that issue.