Skip to content

[TASK] Add rebasing guidelines to CONTRIBUTING.md #1220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 27, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,73 @@ what a commit is about.

When you create a pull request, please
[make your PR editable](https://github.com/blog/2247-improving-collaboration-with-forks).

## Rebasing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO doing a rebase is not suited to git novices, they would be much better off just merging the latest main into their feature branch. And experienced users probably don’t need instructions on how to rebases, which makes me wonder whom exactly these guidelines are written for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote these after @oliverklee asked me to do rebase rather then merge (I don't recall exactly why), and had many issues with rebase going wrong, resulting in having to start from scratch with a new branch and new PR - which was simpler and easier than trying to sort out the mess that had arisen.

I'm still a novice with git (though not GitHub). (I've only ever used git for work on these projects, or cloning a repository. Normally I use Perforce, and before that IIRC it was MS Visual Studio Solutions.)

I find it a valuable checklist/procedure. I added it here because I needed to rebase a WIP branch, and had to look up the instructions I wrote from the sister Emogrifier project.

Maybe these are partly a note-to-self. But I thought they would be useful to others.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an explanation why we should do rebases for PR branches:

https://stackoverflow.com/questions/804115/when-do-you-use-git-rebase-instead-of-git-merge

In short, we want to rebase PR branches instead of merging the main branch so that the changes of the PR always sit "on top" of the changes from main.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stackoverflow.com/questions/804115/when-do-you-use-git-rebase-instead-of-git-merge

In short, we want to rebase PR branches instead of merging the main branch so that the changes of the PR always sit "on top" of the changes from main.

TBH I did not read through all of the answers on the linked SO question so I might be missing something but the bulk of the answers I skimmed did not pass value judgment on one vs. the other, they only explained the difference. That tracks with my experience of 10+ years of using git: having non-linear history and cross-merging is rarely an issue (if ever). I think making lookups in the code history a little bit more cumbersome is well worth the price of making contributing easier. My vote would be not to force people to rewrite PRs to be semi-linear.

Copy link
Collaborator Author

@JakeQZ JakeQZ Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote would also be to make contributing as easy as possible, and not insist on too much. Quite often we get PRs submitted without any corresponding tests. When we ask for some unit tests, the contributor gives up, saying that it's beyond their capabilities, or that they do not have the time. I don't know what the solution is in those situations.

For PRs with merge conflicts, GitHub itself offers an option to resolve them using the UI. I've used this a few times without issue. It does get merge main on the branch, not a rebase. (And, as a novice git user, I don't really understand the difference, but maybe will after having the SO article, which I haven't looked at yet, as my bedtime reading... 😉.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, those rebasing guidelines are mostly for our (particularly, for your @JakeQZ's) benefit. I think the occasional contributor will seldomly run into cases where they really need a rebase, or where a rebase would result in a conflict. (Also, rebasing is different when a contributor is working on a fork.)

So I'd be fine with moving the rebasing guidelines out of CONTRIBUTING.md and make them maintainer-only.


If other PRs have been merged during the time between your initial PR creation
and final approval, it may be required that you rebase your changes against the
latest `main` branch.
Comment on lines +130 to +132
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe explain when exactly a rebase is required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, only if merge conflicts have arisen.


There are potential pitfalls here if you follow the suggestions from `git`,
which could leave your branch in an unrecoverable mess,
and you having to start over with a new branch and new PR.
Comment on lines +134 to +136
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found suggestions from git to always be pretty good at guessing the likely use-cases of my current operation. Where will they lead users astray?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you try git push after rebasing locally and resolving conflicts, it will suggest

hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

If you follow the git pull hint, it will splatter your local branch with a duplicate set of commits, undoing the rebase and all the conlflict resolutions. So the branch becomes a mess and you have to start over.

Maybe this is because I am doing it wrong. But documentation on best practices is thin on the ground.


The procedure below is tried and tested, and will help you avoid frustration.

To rebase a feature branch to the latest `main`:

1. Make sure that your local copy of the repository has the most up-to-date
revisions of `main` (this is important, otherwise you may end up rebasing to
an older base point):
```bash
git switch main
git pull
```
Comment on lines +142 to +148
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching branches is not necessary and incurs some overhead (updating the worktree twice). We could just instruct to use git fetch and then later merge with with the fetched ref (e.g. origin/main).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a git novice, I found a workflow that worked, and documented that.

Having now read this, I can see that fetch might be more useful if multiple people are making changes to a PR - but then you have to run merge as a separate step. And most PRs are an individual contributor.

It's not clear to me if either pull or fetch will update all branches or only the currently active one. Hence I went for a more robust approach that was guaranteed to work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Git pull is a Git command that performs both git fetch and git merge simultaneously.

So git fetch updates (local) remote tracking branches (i.e., "shadow copies" of the remote branches), while git pull updates a local branch by doing a fetch and then a merge.

1. Switch to the (feature) branch to be rebased and make sure your copy is up to
date:
```bash
git switch feature/something-cool
git pull
```
1. Consider taking a copy of the folder tree at this stage; this may help when
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I don’t like that every instruction list item starts with 1. The entire point of markdown is that it’s possible to understand even when reading plain text.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. The advantage of 1. is that it minimizes diffs if an item is added within the list, in turn helping with merging (if required). I think you are suggesting that Markdown should not have this feature at all :)

resolving conflicts in the next step.
1. Begin the rebasing process
```bash
git rebase main
```
1. Resolve the conflicts in the reported files. (This will typically require
reversing the order of the new entries in `CHANGELOG.md`.) You may use a
folder `diff` against the copy taken at step 3 to assist, but bear in mind
that at this stage `git` is partway through rebasing, so some files will have
been merged and include the latest changes from `main`, whilst others might
not. In any case, you should ignore changes to files not reported as having
conflicts.

If there were no conflicts, skip this and the next step.
1. Mark the conflicting files as resolved and continue the rebase
```bash
git add .
git rebase --continue
```
(You can alternatively use more specific wildcards or specify individual
files with a full relative path.)
Comment on lines +175 to +176
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find “relative” and “full” to be a contradiction in terms.

People familiar with command lines probably don’t need to be told how to reference files, so the following should suffice:

(You can alternatively specify individual files by path or wildcard.)


If there were no conflicts reported in the previous step, skip this step.

If there are more conflicts to resolve, repeat the previous step then this
step again.
1. Force-push the rebased (feature) branch to the remote repository
```bash
git push --force
```
The `--force` option is important. Without it, you'll get an error with a
Comment on lines +184 to +186
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to use --force-with-lease in case you’re not the only one working on the feature branch (which is quite likely if the PR’s “allow changes from maintainers” flag is checked).

hint suggesting a `git pull` is required:
```
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
```
***DO NOT*** follow the hint and execute `git pull`. This will result in the
set of all commits on the feature branch being duplicated, and the "patching
base" not being moved at all.