CONTRIBUTING.md: correction in description "Squash commits after review"?

[jkarinkl]

In the contributing guidelines there is a description on how to squash commits after review.

Is it correct that in line 323, "$ git rebase -i --autosquash", the branch name to where you want to rebase is missing? (something like "upstream/master"?)

Since I am not totally sure if this should be in the description, and if so, what branch name exactly, I made an issue instead of a PR. I'm happy to PR this after feedback.

[maribu]

This is a tad more complex, sadly. The full command is git rebase -i --autosquash <BASE>.

<BASE> can either be a branch name (in which case it will be rebased on to the HEAD commit of that branch) or a commit by ID or tag.

Borrowing the terminology used in this lwn.net article, there are different things a rebase can do:

• reparenting: The commits of your PR will be based upon a new state. E.g. when <BASE> is upstream/master and new commits have been merged to upstream, this will change your PR to be based on top of the new state of master. In the diff output (e.g. via git diff <OLD_HEAD>..<NEW_HEAD> or via the "force pushed" link in the GitHub web interface) from the state before and after the rebase, only unrelated changes will appear: Those that have been added to upstream/master since you last rebased.
• changing the history: <BASE> is given e.g. by the commit ID of the first commit that is not part of your PR. In this case, no unrelated commits get added and the diff output will not contain changes unrelated to the PR. If you just squashed, the diff will be empty (as the state before and after is still the same, just how the changes up to that change are structured into commits and their text changes).

When squashing, we usually do not want to re-parent a PR, as this makes it more difficult to track changes before and after the force-push via the diff shown by the GitHub UI or the git diff command.

However: If changes have been merged into master that your PR depends upon, reparenting is needed. (E.g. consider PR A introduced a fancy new API and PR B adds an implementation of that API, PR B will depend on PR A. So PR B will have the "state: waiting for other PR" tag and include a state of PR A. Typically, PR A gets reviewed and merged first, possibly after changes due to the review process. When PR A finally is merged, PR B will need to be reparented on master to no longer include the now outdated commits or an earlier state of PR A.) Specifically: Reparenting is needed when merge conflicts arise or another PR got merged that a given PR depended upon.

[jkarinkl]

Thanks for the comprehensive response, @maribu! It is indeed a tad (or lot) more complex than I thought.

I suppose it is more convenient to keep it as it is then? When contributors are confused about this, they can ask for advice in the chat and on the forum.

[maribu]

Maybe we should give two or three examples and a link to detailed documentation?

[jkarinkl]

Sounds good. Would you be ok with making a PR for that? I am happy to review, but do not think I am the best person to write this down correctly atm :).