[TASK] Add rebasing guidelines to CONTRIBUTING.md

[JakeQZ]

This section is copied directly from the sister project, Emogrifier.

Resolves #1215.

[coveralls]

coverage: 52.82%. remained the same
when pulling a5a498b on docs/contributing/rebasing
into 11214f0 on main.

[oliverklee]

Looks good.

[sabberworm]

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).

[JakeQZ]

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.

[oliverklee]

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.

[sabberworm]

Maybe explain when exactly a rebase is required.

[JakeQZ]

Yes, only if merge conflicts have arisen.

[sabberworm]

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.

[JakeQZ]

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.

[oliverklee]

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.

[sabberworm]

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.

[JakeQZ]

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... 😉.)

[oliverklee]

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.

[sabberworm]

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?

[JakeQZ]

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.

[sabberworm]

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.

[JakeQZ]

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 :)

[sabberworm]

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.)

[sabberworm]

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).

[JakeQZ]

I've replied to some comments, and reopened #1220 for this to be reworked.

I'm a git novice, so can I leave it to you, @sabberworm and @oliverklee, to come up with an alternative advisory section for when a PR has merge conflicts?

[JakeQZ]

I'm a git novice

I never work directly on the local git repository. I always copy the directory elsewhere, and merge changes back with a folder diff tool. That's the only way I can be sure of not losing work through making 'mistakes' with git.