search

pine64 / bl_iot_sdk

BL602 SDK (Pine64 fork)
https://pine64.github.io/bl602-docs/
Apache License 2.0
134 stars 59 forks source link

Can we turn off PR commit squashing for this group of repositories? #49

Closed tchebb closed 3 years ago

tchebb commented 3 years ago

I know this is a touchy topic, but I think it's a mistake to squash together the commits that make up a PR when merging that PR. Personally, I always split my changes up into logical commits, each with a relevant message. It's frustrating to see that work lost when a PR gets merged as one squashed commit and shows up to everyone else as just one big mess of changes in a single commit.

Furthermore, I'll often include small fixup commits (typo or formatting fixes), which aren't worth the overhead of their own PR, inside a larger, unrelated PR (see pine64/bl602-docs#5, for example). If the PR gets squashed, this is no longer a valid workflow since you end up with totally unrelated changes within the same commit.

Finally, squashing on merge breaks git branch -d locally, since Git doesn't realize that the squashed commit in the main branch corresponds to the commits in my local feature branch. Obviously, I can pass -D to force the deletion, but I quite like having the extra safety check that Git provides with -d.

Avamander commented 3 years ago

Personally, I always split my changes up into logical commits, each with a relevant message

Depends on the PR, not every PR is split up into logical commits, it's quite frequent that minor touchups that should've been a part of the initial commit are tacked on. I think squashing in those cases is a necessary evil and should thus stay enabled.

On PRs that have nice commit histories, yes, there's no need.

tchebb commented 3 years ago

Agreed. I filed this after noticing that one of mine had been squashed. Wasn't sure if it was something you could select per-PR, but it seems like it is. In that case, I'll just make sure to note in my PRs that I've already done the squashing I want in my feature branch.