search

eclipse-wakaama / wakaama

Eclipse Wakaama is a C implementation of the Open Mobile Alliance's LightWeight M2M protocol (LWM2M).
BSD 3-Clause "New" or "Revised" License
501 stars 374 forks source link

Merge strategy #601

Open rettichschnidi opened 3 years ago

rettichschnidi commented 3 years ago

When merging a PR, we currently can do either "Rebase and merge" or "Squash and merge". The later we should probably not use as we try to produce nicely crafted commits in the first place and GitHub adjust the subject line, adding the PR number, therefore potentially violating the 72 character rule commanded by Eclipse.

The "Create a merge commit" option however is greyed out, essentially forcing all PRs to be rebased when merging. Rebasing in turn causes the ID of all merged commits to change, rendering the .git-blame-ignore-revs useless/broken.

As far as I can see, this boils down to either enabling the "Create a merge commit" (and using it for, at least, all PRs which change the .git-blame-ignore-revs file, or to get rid of .git-blame-ignore-revs. The obvious workaround of adjusting .git-blame-ignore-revs in yet another PR seems too tedious to me.

My questions therefore:

qleisan commented 3 years ago

I prefer a clean history where the master branch has commits with only one parent. Also if the developer branch has several commits that make sense to squash they should be squashed so the master branch doesn't get cluttered.

rettichschnidi commented 3 years ago

I prefer a clean history where the master branch has commits with only one parent.

Same here, but not necessarily at all costs. Keeping git blame useful while allowing for cosmetic changes in existing code is something I value more.

Another workaround would be to agree on generally doing "Rebase and merge", but "Create a merge commit" for PRs which contain cosmetic changes. Would be a bit hit-and-miss, because deciding which one to do will rely on us humans, and is not enforceable (?) by tooling.

Also if the developer branch has several commits that make sense to squash they should be squashed so the master branch doesn't get cluttered.

If a PR has commits that should be squashed, then the developer should squash them locally and (force push) them before merging. Only this way we can actually review how the commit looks like before merging (and enforcing rules over it).

qleisan commented 3 years ago

because deciding which one to do will rely on us humans, and is not enforceable (?) by tooling.

You have a challenge ;-)

sbernard31 commented 3 years ago

Why are the current settings the way they are?

In a previous private discussion at the very beginning of the revival, I proposed to keep a clean and readable history and so don't use merge anymore . @sbertin-telular validate it so we go for it in a first time.

I deactivated the "Create merge commit" as it sounds obvious that it was not compatible with decision took before and let "Squash and merge" because I was not sure how it behave. ( because I don't use the github button :grimacing:)

Can we disable the option "Squash and merge"?

I can disable "Squash and merge" if needed. (one of the rare setting I have access to)

Could we switch to a "Create a merge commit" workflow? Maybe even using only this one?

My opinion is clearly less important than the dev ones, but I strongly discourage you to follow that way. Merge commit bring to unreadable spaghetti code repository.

should we remove the .git-blame-ignore-revs and the tooling around it?

Still just my opinion but If I should chose between "readable history" and .git-blame-ignore-revs, I largely prefer the readable history.

Anything else we should consider?

If you want to keep both and also keep "pass validation(github action) before to integrate". You can "rebase --force" the PR on master then just do a fast-forward (manually or with github if it behave like this, again I'm not sure how it behave) to integrate it inmaster.

rettichschnidi commented 1 year ago

I can disable "Squash and merge" if needed. (one of the rare setting I have access to)

That would be great.

Could we switch to a "Create a merge commit" workflow? Maybe even using only this one?

My opinion is clearly less important than the dev ones, but I strongly discourage you to follow that way. Merge commit bring to unreadable spaghetti code repository.

Let's not use merge commits then.

I will update the .git-blame-ignore-revs documentation in the README.md file accordingly.

sbernard31 commented 1 year ago

I can disable "Squash and merge" if needed. (one of the rare setting I have access to)

That would be great.

Done :)