Merges vs. rebases and squashes

[michaelgregorius]

Proposal to mostly use merges in development branches

I'd like to propose to use merges rather than rebases and squashes when working on development branches. The reason is that the history is maintained and that using merges is much less "dangerous" as it will not accidentally erase history or create potential problems in the working copies of other collaborators.

Just as an example what might go wrong with rebases/squashes: there was a specific case where a rebase was attempted which resulted in many commits being absorbed into one commit. I assume that it was rather a rebase followed by a squash which lead to this result. As a consequence all the changes done in individual commits by several people got squashed into the single commit which then was attributed to the person who did the rebase. IMO the history should be retained so that people can get credited for the work they did.

A better strategy would have been to merge master into the development branch instead of doing a rebase. In that case the commit history and the maintainers would have been kept intact. Even if the final merge would be done as a squash merge GitHub would still be able to credit all contributors in that merge because it can resolve the full history up to the final merge.

Rebases can also be dangerous because individual commits that might have built and worked before a rebase might not work build or work anymore after a rebase when all the commits are put on top of a new commit. This can create problems when doing git bisects.

Proposal to merge without squashing

I'd also like to discuss the current practice to do squash merges on all PRs when finally merging into master. As can be seen above one has to unnecessarily rely on GitHub to keep the credits and it can also hinder comprehending of how things came to be in the first place. This is especially true if the change set is rather large like for example with large collaborative PRs.

Keeping the history has other advantages as well. For example it's much easier to pinpoint a critical commit when doing a git bisect because the bisect can also search through the individual smaller commits. This can potentially make it much easier to find out the specific cause of a problem. With a squash merge the bisect might simply tell you the following at the end of the search: "The problem was introduced by the big wall of changes in this one large commit."

With regular merges it would also still be possible to just see the merges that are done in the direction of master. The following command for example shows the "clean" history of master without showing any commits from development branches:

git log --first-parent --pretty=format:"%Cblue %h %Cgreen %an %Creset %s" --graph

It's also possible to show only the merge commits that have gone into master:

git log --first-parent --pretty=format:"%Cblue %h %Cgreen %an %Creset %s" --graph --merges

So I guess my proposal here is: "Squash merge small PRs if wanted/needed but keep the history of PRs with large change sets intact."

Don't do force pushes

It's generally advised to not use force pushes in shared branches/repositories because they rewrite history and can lead to all types of problems.

It should almost always be possible to for example integrate other branches with a merge commit so that the target branch can simply develop in a forward fashion without the need to rewrite history. All collaborators can then simply pull the new changes without running into conflicts because their repositories now deviate from the pushed state.

So I propose to not use force pushes, especially on branches that are worked on collaboratively and which include the work of many collaborators. Having to do a force push on such branches should be an indicator that something might be wrong and people should be aware of the risks. Force pushes should only be done if one absolutely knows what one is doing and in very isolated branches.

Edit: added the "force pushes" topic.
Edit 2: removed some personal aspects and elaborated a bit more on some general aspects.

[FyiurAmron]

My stance here:

1. Yes, individual commits track history better, iff the commits are already pruned to be concise and atomic and have clearly-written messages. If not, this would require manually squashes/amends to align with those rules. This requires force-pushes done by people who know what would be those clearly-written messages and which commits would need to be squashed to reduce the noise. Usually, that's only the person doing the PR. Iff there's a consensus that we change the PR rules so that we require the PRs to align with those rules, I'd be happy to oblige. I work like that in my day job, and I can do that. However, AFAIK, the rules are lax because enforcing them is an uphill battle, because doing so creates endless bikeshedding opportunities (What are the rules for commit messages? Are we using e.g. commitlint? If so, what settings should we use? How long can commit messages be? How many commits are allowed for PR? What constitues an "atomic" commit? etc. etc.). Squashing was accepted as a unfortunate but acceptable solution. That's usually the case, I think. I see squashing as a pragmatic response to the actual problem with the style consistency of the code LMMS receives. People here have different backgrounds, they write code differently, and people prefer writing working code or fixing bugs instead of endless style/guideline discussions, because, well, they don't really help you create music that much :D Slightly related: Why do we keep losing good developers? #3447 (i.e. would enforcing anything really help us here?)
2. That said, I think that we can safely say "prefer merges if possible over XYZ", but then again, if it would be a soft rule, you would get exactly the situation we had now, and if it were a hard rule, it would create other problems (and it would no longer be "prefer" then).
3. That said, preserving contributor lists can be done via commit messages, and I think that's the industry standard nowadays. Only the physical core author of the commit (in this case, person doing the squashing) needs to be listed, for practical reasons (i.e. so that you know who did the squash). I think that any X who actually contributed in any way can propose such amends and I think nobody will protest to include X there then.
4. As to force pushes - they are physically required if you do squashes or rebases. It follows that you can't have a strict "no force pushes" rule if you squash/rebase/amend, and all exceptions will have the above problems with exceptions (i.e. would need to have agreed-upon rules and would need people who know the rules to enforce the rules etc.).

That said, I generally agree with the sentiment (regardless if it's doable at the time or not), but I just wanted to point out that,

So I propose to not use force pushes, especially on branches that are worked on collaboratively and which include the work of many collaborators. Having to do a force push on such branches should be an indicator that something might be wrong and people should be aware of the risks

^ this is AFAIK the unspoken norm right now. Maybe it's time to formalize it? Dunno, frankly. We have a lot of guideline in my corpo, and people just ignore them or not read them. Maybe it's worth it however to have them in one and easy to access place, or emphasize them more? I've been looking around and the only thing that comes when I search for "LMMS contribution guidelines" are https://lmms.io/get-involved & https://github.com/LMMS/lmms/wiki/Contributing-Step-by-Step , and maybe https://github.com/LMMS/lmms/wiki/Reviewing-Pull-Requests can be inferred... but some pages there should probably discuss this stuff, instead we have e.g. https://github.com/LMMS/lmms/wiki/Becoming-a-coder - while I really like what's written there, I don't think it answers many of the practical issues we have :D It doesn't even link to https://github.com/LMMS/lmms/wiki/Coding-conventions e.g. What do you think @tresf , is it worth it to try to get some additional codification of the rules or do a cleanup of the newcomer/guideline stuff? (and TBH I only found video "developer tutorials", no text ones, frankly?)

[tresf]

@tresf , is it worth it to try to get some additional codification of the rules? (and TBH I only found video "developer tutorials", no text ones, frankly?)

With regards to developer rules:

• PRs: I believe strongly that squashing is easier to support long-term over merge commits. This doesn't work well for all cases, but the vast majority of cases, I believe this to be the best strategy. It prevents the authors from having to do this at nearly no detriment to the project. For the use-cases that need merge commits, they can be requested by the author.
• Rebasing: I don't have enough experience resolving conflicts to have a strong opinion on one approach over another. Our tutorial has basic "fast-forward" steps and that's the basis of my merge strategy. It's just a git pull --rebase origin master, fix conflicted files, add to git, force push. I really dislike the rebasing process so I generally allow other, more experienced people to do this for the project as my efforts are often better spent elsewhere, but sometimes I have to step in and help which is what was done on the PR that inspired this conversation. I mentioned to the team that the conflicts had to be fixed, waited about 10 days, did it myself.

[Rossmaxx]

I haven't read the entire discussion but here's my 2¢:

I know how much effort MG put into getting qt6 builds working, and to see all those work getting lost over a force push and then getting rewritten by another dev (tres in this case) is a bit too much for him. I would agree to remove all the trial commits but there's no need since we do a squash merge. Also removing trial commits shouldn't remove the actual working code. I would see this as a case of "the baby has been thrown out with the bath water" situation.

I didn't try building qt6 branch after the force push as my activity got reduced due to college and I have minimized windows usage since. I could have caught this earlier but didn't because i was focusing on my college instead of lmms work.

I am not against squashing but in this situation, there's more harm than good.

[tresf]

Proposal to mostly use merges in development branches

In my (now hidden) comment, despite criticism on this technique I've yet to see specific instructions to the contrary... Here's what I found:

Here's a S.O. Q&A on the topic...

• Rebase (1,139 upvotes): https://stackoverflow.com/a/5340773/3196753

  git checkout qt6
  git rebase master
• Merge (449 upvotes) https://stackoverflow.com/a/5340774/3196753

  git checkout qt6
  git merge origin/master

... one of the commenters was kind enough to share this atlassian article on the topic:

• Merging vs. Rebasing: https://www.atlassian.com/git/tutorials/merging-vs-rebasing

.. and another commenter shared this blog post:

• Stop merging master: https://web.archive.org/web/20220518223433/https://kristopherwilson.com/2015/02/12/stop-merging-master/

As a frequent flyer of rebase, I'm personally open to ways to do things better. This is my attempt to paint this topic in black & white. I don't have a strong opinion on this yet because I only have experience with rebase.

[FyiurAmron]

TBH, I've used both, and both has their own sets of cons and pros. Indeed, if a lot of people collaborate, merges are safer. Here, however, it's always tricky to know if someone still collaborates or no... and yes, I'd say that >95% of the time I rebase instead of merging. However, those <5% still exist strictly because they are done in the "danger zone" as in this case. I feel this will ultimately be always a judgement call in tight spots; like I said, even with people working together in the office day-by-day f-ups like this happen. You can undo git history changes, but the actual work/time/effort spent on rewrites can't be undone if they're borked - hence merges in corner cases, to avoid wasting time on fixups.

[michaelgregorius]

... one of the commenters was kind enough to share this atlassian article on the topic:

* Merging vs. Rebasing: https://www.atlassian.com/git/tutorials/merging-vs-rebasing

Thanks for sharing the link @tresf! IMO it gives a good and concise overview of the pros and cons of the different approaches and of the situations where they are applicable and where not.

I propose to add the link to a fitting Wiki page and perhaps to extract some coarse rules from it and to provide them as well. It might be something like:

• Rebase and squash to your hearts content in your local working copy but only do so with commits that you have not yet pushed to a shared/public branch/repository which is shared by many collaborators. Otherwise you might create problems for the other collaborators working on the branch because their working copies will diverge. This means that every commit that is already visible in the shared repository should not be touched upon by a rebase or squash.
• If you are working on a shared branch please do not perform any actions that will rewrite the commit history. For example, if you need to merge another branch do so with a merge because it is non-destructive and other collaborators will simply be able to pull your changes and move forward.

I can understand that doing the final merge with a squash looks nicer and personally I'd still prefer the merge commits for the reasons mentioned above. However, this is a collective decision and they both have their pros and cons.

The advantage of going by the rules above is that while development branches might look "wild" in the development phase no problems will be created between the collaborators while working on them. Another advantage would be that when a final squash merge is done then GitHub should be able to pickup all collaborators automatically and reference them so there would not even be a need to manually check if everyone contributing is attributed.

What do you think?

[JohannesLorenz]

I cannot agree to this. As a DevOp who works intensively with both Git and a painfully outdated "centralized VCS" (ClearCase), I am an expert for how much you can suffer if you cannot rewrite your history. Developers send an unordered mess of fixups to their reviewers, like

A
B
Fixup A
Oh this should be before A
Forgot proper commit messages
Fixup B

Also, if I combine the OP headings "don't do force pushes" and "don't do squashes" I get that you would either rebase+ff or merge-commit merge 50 or 100 fixup comments into master? That sounds totally wrong.

In Git, a branch is not a "chain of commits", but solely a (movable) HEAD pointer. That means if you merge a branch into your branch, all the fixup comments are suddenly on master and appear in "git log", "gitk" and more (Imagine git log -G through 100 fixups, or scrolling through a 100 times larger history in gitk). And that explains why you should be very careful with merge commits towards master:

master
| \
|   \
|   100 fixups -> now also on master
|    |
|
old master    

When do I use what?

• Rebase + FF -> When multiple PR commits are perfectly in order and atomic, or can be interactively-rebased as such. I.e. if the author had multiple atomic changes in their PR and want to document them on master as such.
• Squash -> All the commits of the PR result in only one atomic commit.
• Merge -> Almost never. A good example were the stable-1.2 merges where Phys brought these commits into our master. Or in zyn, where we have GitHub and SourceFourge simultaneously - i.e. 2 masters - and cannot rewrite history on either.

How to solve the problem with multiple authors?

• git commit --amend --author=... --committer=...
• Or add Co-authored-by: ... to the commit message

Is an exact accreditation more important than a compact documentation, such that we merge commits un-squashed on master? IMO no.

[FyiurAmron]

💯 & +1, however you speak about the case of going ->main/master (or any other similar long-lived branch) mainly (sic!). There, my experience is exactly the same as yours.

There is a distinct case of "how to handle collaboration on a WIP branch before it's ready for review and going ->main", where I think that merges are sometimes a valid and simple way to handle collaboration in groups. After the branch is no longer WIP, then indeed, it's either interactive rebase or squash to clean up the history (depending on whether a single commit is better of multiple), then rebase the feat branch on the target dev branch, then just FF the main branch pointer to the feat tip.

There's also a distinct case of "how to handle multiple long-lived branches which are generally disjoint but sometimes are sync-ed to each other". Wouldn't call it "almost never" (when I have e.g. 1.2 and 1.3 versions of an app and am doing backporting, merging is sometimes really handy), but I agree it's very situational.

[michaelgregorius]

Thanks for your input @JohannesLorenz and @FyiurAmron! I hope you will find it reflected well in the following comment. It's a bit longer but I hope that it helps to work towards mutual understand and to a consensus.

I agree that the current description with the headers reads too strict. That's because it is intended to mostly relate to the scenario of a branch that is collaborated on by several/many developers.

There are definitively situations where rebases and squashes can help to clean up commits. And of course if these actions are performed then in most cases a force-push to upstream/origin must be done.

However, it's also important to consider how these actions can affect other developers, e.g. due to diverging branches. So there are situations where they are okay and where they are not so okay (at least if one does not want to drive collaborating developers insane).

Dimensions

I'd say the "applicability" of rebases and squashes (or history rewriting actions in general) varies among different dimensions:

• Level of collaboration: A developer is working alone in a branch and the chance of it being cloned is low vs. several developers work on an integration branch for a big feature that has already been cloned several times.
• Local or shared: The actions are performed in a local repository on unpushed commits vs. the actions are performed on commits that have already been shared with others.
• Development phase: We are in the midst of development where more incoming commits are expected vs. we in the phase very shortly before the final merge into master.

If a developer is working on a branch completely alone then they are obviously free to do whatever they want in it. It's not shared and so they can force-push lots of times because nobody else can get "hurt" by these actions. If the branch is worked on by many developers then these actions will definitively create work for the other developers as soon as a force-push is involved.

If a developer is working on local commits that have not been pushed/shared yet then they can also perform these actions to their hearts content because nobody else has seen the commits yet, i.e. no conflicts can arise. If a developer performs these actions on commits that have already been shared then all people with these shared commits in their working copies will get problems once they want to push their own commits.

In the midst of development it's not a good idea to perform these actions on highly shared branches because the chances are very high that it will create pain/work for all other developers. If a branch is squash merged during the final merge then it is no problem as it is considered done anyway. It's a final cleanup.

@JohannesLorenz, which scenario are you referring to in you first fixup example? Is it a lone developer scenario? Are these actions done on local unpushed commits? Are they done to tidy up before a final merge?

Small example

This example can as well be skipped if the above is clear/known. It's intended to show a scenario where rebases and squashes can hurt other developers.

Let's say developers A and B are working collaboratively on branch X and there are already some commits in it:

1. Developer A checks out branch X.
2. Five minutes later developer B wants to tidy up the branch and force pushes some commits which got created by squashing some of the existing commits. The exact actions are not important. Let's just say history on shared commits is rewritten.
3. Developer A develops for several days.
4. Developer B pushes more commits to the branch.
5. Developer A now wants to push their changes to X.

Developer A gets an error because the branches have diverged. Fixing this creates (potentially lots of) work for developer A. If this happens on a branch where there are several developers then the work and effort multiplies among them if they all have local changes they want to push.

Merges to master

Regarding doing merges to master: we follow this model at work and we are around 100 developers. Yes, the Git graph might look like spaghetti but the overload of information can be filtered out with tools, e.g. with:

git log --first-parent --pretty=format:"%Cblue %h %Cgreen %an %Creset %s" --graph

However, it can also have lots of advantages, some of which I have already mentioned: more effective git bisects to find the cause of bugs/problems, maintained and detailed authorship.

Another advantage is that nothing can really go missing if you merge into master. Let's say that during development a merge between two branches is done and there are merge conflicts. During the resolution of the merge conflicts the developer accidentally decides to remove code which was originally added during development and is intended to be there. The branch is then merged into master with the missing code. The developer who wrote the now missing code realizes this and can get it back from the history because the full history of how the feature came to be is still there. If the final merge is a squash merge then this information is gone from master. The best chance is that it can be found in the local working copy of the original developer. It might also be possible to retrieve it via the reflog but I am not sure about this one.

To prevent having hundreds of fixup commits in the branch that is merged into master developers should be encouraged to not push hundred of fixups in the first place. Instead they should clean them up before pushing them, e.g. by reorganizing and squashing them.

Squashes and authorship

Squashing commits of several developers during development, i.e. not using GitHub's squash magic, unfortunately makes it necessary to manually keep track of authorship. I have asked perplexity.ai if there is a command line switch in git so that it automatically adds Co-authored-by trailers to the squashed commit but this unfortunately does not seem to be the case. So this must be done manually and consciously.

Commit cleanup

Developers should always be encouraged to clean up their commits before they push them for review.

Nicely packaged commits can even make the review easier by just reviewing commit by commit. At work we usually write: "Can best be reviewed commit by commit" in the description if we believe that it's the case. However, during the fixup the different situations described above should be considered.

Conclusion

I'd say there are at least two types of branches in LMMS:

1. Single developer branches
2. Multi developer branches for large features

In the first case the "anything goes" rule can be applied as nobody else will be affected. This type of branch likely is the majority of branches for LMMS.

However, there are also larger multi developer branches and for these I'd like to propose the following model:

1. A "big feature" branch is opened.
2. The branch is cloned and being worked on by many developers.
3. During active development it should be attempted to always use actions which move the HEAD forward, e.g. using merges and nothing that rewrites history. This makes it easier for all developers to update and integrate their changes. Authorship is maintained and at least during development everything can be reconstructed and nothing goes missing.
4. The final merge is done with a squash merge to tidy up everything. GitHub can do its magic and collect all authors and there result is a single commit in the history.

Can we find a consensus on this?

[FyiurAmron]

@michaelgregorius a weak "yes" from me, especially for the summary bullet points, in the sense - I do agree with the rationale and I understand your point, however I wonder, regardless of actual gain vs effort ratio here, whether it's actually possible to enforce this at all ATM. Also, I'm not a stakeholder here, I'm not suffering this and I guess the blast radius is really limited. Anyway, the final list you proposed is IMO reasonable and reflects how I usually work with collaborative projects. Whether it's acceptable to the majority here and whether it's worth pursuing and spending time/effort on it, that's another story.

[JohannesLorenz]

As you have both pointed out, it depends on the number of involved developers (and partially, reviewers and testers). If there is only few people looking at it, I usually ask those before rebasing. But on a PR with multiple developers like #5990 or probably #7339, a rebase can indeed drive the other developers "insane".

My example above was from a "lone developer scenario", though some of the requests to make changes came from reviewers. My frustration here comes from a VCS where you cannot even rebase anything at all, even if you are a single lone developer 😆

Agreeing to what you say in "Conclusion".

Considering git log --first-parent: This will work if none of your master commits was done using fast forward. But if you first merged master into the feature and then merged it into master using an ff-merge, the first parent is the feature commit. So this means you have to prevent merge commits on master at all, which can in the worst case result in 50% of your master commits being merge commits. For me, this does not look like "the git way", but then, I look at the Linux kernel which is full of merge commits 😆

I would have almost thought that most large repos do not allow merge commits, but interestingly, there are counterexamples like linux or vscode. I really wonder what most repos prefer or if there are trends in larger growing repos. At least TypeScript and LMMS have switched to squash commits in the last years.

[michaelgregorius]

[...] however I wonder [...] whether it's actually possible to enforce this at all ATM.

This is not intended as something that it enforced on a technical level but more as something to be documented in the Wiki so that people can be made aware of these conventions if necessary. Perhaps the additional aspects could be added to the "Submitting a patch" page.

Also, I'm not a stakeholder here, I'm not suffering this and I guess the blast radius is really limited. Anyway, the final list you proposed is IMO reasonable and reflects how I usually work with collaborative projects. Whether it's acceptable to the majority here and whether it's worth pursuing and spending time/effort on it, that's another story.

It's mostly interesting for the members/people working on collaborative branches. IMO collaboration should also encompass a set of conventions on how to use Git so that the collaboration can work in a smooth way and so that it feels like a collaboration in the first place and not like a "cacophony" of different people all potentially doing things differently.

My example above was from a "lone developer scenario", though some of the requests to make changes came from reviewers. My frustration here comes from a VCS where you cannot even rebase anything at all, even if you are a single lone developer 😆

I can feel your pain. I remember working with CVS (which has the excuse of being very old) in university or working with Perforce at a client which IIRC was using some weird locking mechanism. 😅

Considering git log --first-parent: This will work if none of your master commits was done using fast forward. But if you first merged master into the feature and then merged it into master using an ff-merge, the first parent is the feature commit. So this means you have to prevent merge commits on master at all, which can in the worst case result in 50% of your master commits being merge commits. For me, this does not look like "the git way", but then, I look at the Linux kernel which is full of merge commits 😆

IMO part of the problem is that there is no "Git way". Git is a very low level tool that sometimes allows you to work in many different ways to achieve a result. When working in a team many things therefore boil down to finding conventions to follow.

With regards to the example you gave one convention might be: "Don't merge master into a feature and then FF-merge it into master because it will look confusing in the graph and make it hard to work with it." If one wanted to allow merge commits into master then the convention could be changed to: "If you merge master into a feature then only merge the feature back into master with a merge commit so that master will always be the first parent."

I would have almost thought that most large repos do not allow merge commits, but interestingly, there are counterexamples like linux or vscode. I really wonder what most repos prefer or if there are trends in larger growing repos. At least TypeScript and LMMS have switched to squash commits in the last years.

I think it will always depend. Both approaches have their pros and cons so it ultimately boils down to which pros and cons a team prefers.

By the way, if you're not a fan of merge commits then better not look at this commit. 😉 It's an octopus merge in the Linux kernel where 66 parents got merged. Or as Linus Torvalds has put it: "Christ, that's not an octopus, that's a Cthulhu merge". 😆

[tresf]

So I just tried to use git merge instead of git rebase... I'm following the steps from this tutorial. What am I doing wrong?

git checkout qt6
git pull
git merge master

(fix conflicts)

git add path/to/conflict

git merge --continue

Fixing the merge was editing a few lines, yet this is the commit that gets created:
bf2b36d per master...tresf:lmms:qt6-testing.

This is clearly not what this tutorial is describing (right?), so I assume one of these commands is wrong. Please help by providing the corrected command only.

[michaelgregorius]

To me everything looks correct.

Commit bf2b36d states that commit 7f2761d (which is the HEAD of master as of writing) was merged into 3bf6805 (which is the HEAD of the Qt6 branch as of writing). This is also what gitk shows for https://github.com/tresf/lmms/tree/qt6-testing.

Therefore the diff of bf2b36d should show all changes that have been made to master since the last time the Qt6 branch was rebased onto master or the last time that master was merged into the Qt6 branch.

The diff of master...tresf:lmms:qt6-testing should show all commits that have been made in the Qt6 branch and which will go into master when these changes are integrated. These changes start with the one fat flattened commit "Rebase against master" (124113e) which contains lots of other commits and everything that has happened afterwards.

Is there anything unexpected or missing in it?

[tresf]

Therefore the diff of bf2b36d should show all changes that have been made to master since the last time the Qt6 branch was rebased onto master or the last time that master was merged into the Qt6 branch.

Ok, thanks for the sanity check.