Add another reason to avoid rebasing #60
Conversation
It's important to note that rebasing can be as confusing to GitHub as it is to reviewers.
|
I may be in the minority on this, but I find it much easier both when I rebase and when others rebase. This could be because I use the Git GUI called SmartGit. I can understand that rebasing may be harder for command-line people (which is probably the majority). But I do believe that command-line people are not usually "seeing what's really going on." |
|
@HowardBraham The focus of this article is on how rebasing affects review flow, not commit history. |
When you rebase, what do you usually do? Are you trying to trim the number of commits so the history is easier to work with? Do you usually rebase as you go along or at the end before you merge? How do you find it easier when other people rebase? As @MajorLift alluded to, there are some undesirable effects it creates on pull requests which I've detailed here, but I'm curious if you've ever run into them? |
|
When I use rebase locally, typically I'm just cleaning up the commit history for my own benefit. I try to only do this when the PR is in draft as well, so that it doesn't impact reviewers. For some PRs it can be helpful to have commits that can be reviewed independently, which rebasing can help accomplish. I also prefer to use rebase when updating a PR on GitHub, because again it makes the commit history easier to read and navigate |
Sure, that makes sense. Do you rebase once people start leaving comments, though? On the other side, when you are a reviewer on a PR, do you notice any effects from rebasing? |
|
Typically no, I don't rebase after people start leaving comments. I try to avoid it as soon as the PR is out of draft. As reviewer, yes I have been confused sometimes in the past when a PR has been rebased. Though the examples I can think of are cases where the author squashed new commits into old ones. I can't think of any cases where the "Update with rebase" button has caused confusion, I tend to prefer that because it makes it easier to review the commits on that PR. |
Okay. I've removed that paragraph here: 73409f0 |
|
Also in response to both @HowardBraham and @Gudahtt, I've added a clause clarifying when to avoid rebasing: specifically, once people start leaving comments. This allows you as the author to rebase freely while in draft mode. Does that help or should I qualify this further? |
|
Sorry I didn't answer this question earlier. Yeah this change helps. I agree that rebasing can make the commit history easier to read and navigate when I'm a reviewer. The major disadvantage for me is occasionally you get a Git error and it's confusing. Another reason I would use rebase, force-push, and squash is if I'm pushing not to get feedback from reviewers, but just to make CI test my changes. |
|
Agreed and for me this is the major concern which I think of roughly as "once people depend on my work, rebasing and force-pushing must be done with consideration". Someone having forked from my branch (of which I may not be aware) would be even stronger consideration than a review in that sense. There's also a clear distinction in my mind between what I consider non-destructive rebases (what you'd get when clicking "rebase" to sync in the GH UI; cleaning up history, squashing, amending commit messages; when Consider a long-running open PR. At some point, upstream ( I would at the same time say "avoid merge commits whenever possible". There's certainly a tension there and while I don't think simple rules can capture all relevant situations for those judgment calls perfectly, it's certainly an important aspect of the development process worth spending attention on. |
It's important to note that rebasing can be as confusing to GitHub as it is to reviewers.