PR Merge or Squash?

[Apollon77]

@mfucci Whats your opinion about merges of PRs? My experience in other projects is that PRs can include very "many commits" partially and using a normal "Merge commit" when merging the PR ends up in the single commits spread all over the timeline and afterwards hard to identify potentially problematic stuff introduced by a single PR.

In such projects we decided to only use "Squash" method on PR level. This combines anything for that PR and makes it one big commit - yes ou loose "history", but because it is just the history of that PR it mostly is irrelevant.

What do you think?

If we go for Squash we should disable the other options in the Repo settings

[mfucci]

Good suggestion!

Squash seems great, agreed with you that individual PR it is irrelevant.
I disabled "merge commits" option.

What about "rebase merging" ? Not sure for are the pros / cons for doing this.

[Apollon77]

In fact "rebase" should always be used before doing a PR to have a more clean code base ... But even with a Rebase you would get e.g. 20 commits when you "merge" it ... and also have that whole history in the main repo which might be "try and error stuff" or whatever ;-) So I would go with Squash only.

[mfucci]

Ok! I turned off "rebase" as well then

[mfucci]

Hmmm... "squash" seems to create problems when merging upcoming changes: I now have a bunch of nonsense conflicts that should have merged automatically.

I think the history of local commits for an individual PR is required to be able to merge correctly. For instance, if I solved a merge conflict in one PR, this resolution will be used for all other PRs built on top of that PR so I won't have to resolve over and over the same conflict...

I suggest to revert back to "Merge" mode.
Going back to your point of identifying bug introduced by a PR, how is the merge strategy making this more difficult? The only points to checks should be the one on the main branch, the history of individual PR should not add more points on the main branch.

Here is the specific issue I have right now:

• Previous branch chain: main -> add-logger -> time-abstraction
• add-logger squashed to main
• add-cla built on top of main and squashed in main
• add-cla changes on files added in add-logger cannot be merged automatically to time-abstraction, while they should be since they don't touch the same lines in the file, but the file appears as being creating in both branch which is not true.

Have constant merge issues will be a problem for collaboration...

[Apollon77]

Could it be that the reason is that you had chained branches? Because after you merged the one and merge another one I would say that you need to rebase before adding more changes ...

[Apollon77]

PS: I thought again and yes the reason for your issues was your "chained branches). In fact what you had was the following:

• add-logger squashed to main - means that all single commits where collected together and brought to master as ONE bug commit
• your chaned branch time-abstraction, which based on the "still having all the single commits from add-logger that never went into master" now have a different git history then the main branch ...

This means that also the normal merge strategies like fast forwards or whatever - that works if "time-abstraction" would have been newly branched from main - do not work and so it is a mess.

In fact: If you want to do chained branches then merge is the only way that might work ... or we could try if rebases could solve that

[mfucci]

I tend to do a lot of chained branches because this allow to not be blocked by PR in reviews...
My record is a chain of 15 PRs :-)

But I think chain branches is just part of the normal dev cycle.
For instance, right now I am still working on the fix-subscription branch. To finish it, I need a few refactoring and abstractions to be able to unit test it. I am writing those changes as independent branches and will merge into fix-subscription so I can use them there. So that will create a complex PR chain.

I don't think rebase solves the problem here. I only rebase when everything else fails and this usually creates tons of merge conflicts...

Or let me know how you use rebase.

Here is my usual dev cycle:

• I start working on add-feature branch, so the chain is main -> add-feature
• I figure out I need a refactoring which would be better reviewed by itself. So I create refactor branch from main and merge it into add-feature. So the chain will now be: main -> refactoring -> add-feature
• If main changes with conflicts for my branches, I merge main to refactoring then I merge again refactoring to add-feature so actual merge conflicts need to be solved only once
• If the review of the branches is not done, I am implement feature2 that require feature, I add a new branch at the head of the chain:
  main -> refactoring -> add-feature -> add-feature2

We might have soon long-lived branches, like node-matter-ble that depends on a platform-specific library so we don't want to merge to main but we want to make sure that main changes can be regularly merge into it.

[Apollon77]

Ok, I try to avoid chained branches because of all the issues in merging and transparency, but if it is your style to work then we need normal merges too ... Squash is for the easy cases maybe the best (still).
If all chained branches would be merged/sqashed into the initial feature branch then also squash would work here, but that would mean to "solve" all chanined branches back to the root :-)

I think it is more about favour and dev-process, so if you have the need for chained branches then allow it. It's easy (and also fine for me) :-))

[mfucci]

Sounds good!
Let's try to use merge with full history when merging branches and we can revisit this choice if any issues arise.

Note: I would recommend that you try once a branch chain. It is quite powerful for parallel and not be dependent of the speed of the PR reviews. But yes, a few times I completely screw up one branch with bad merging operations

[Apollon77]

;-) I did tried such constructs in the past if really needed, but it really needs tohat you know what you are doing ... 99% till now in open source area I was working alone on a project at a certain timepoint because it are "my projects" or I took something over or maintain a project where I have all relevant rights :-)

IN the other cases most of the time one branch for the PR was more than sufficient, so the "blocked from other topics" was not happening that often. And most of the time I have noone to review, so I would only block myself ;-)

When contributing to "projects from someone else" I personally see chaned branches risky because you never know what you get as review feedback (see my two PRs already for the cluster update ;-) ) ... and in fact review feedback can render a branch to be changed completely or even killed ... so basing another branch on that could screw up the whole work ...

[mfucci]

Intermediate branches in the chain chain changed completely: yeah, it happens but not that often in my experience.
In most case, if a reviewer was asking me to do a big refactoring in a PR at the beginning of the chain, I was doing the refactoring at the end of the chain and reviewers were usually okay with that since the end state is the same :-)