Disable squash merges

[amezin]

I want to keep the driver in liquidtux repo up to date with changes in the kernel tree.

This means I should import changes contributed by other people into liquidtux.

And, in turn, this means pull requests should not be squashed - to keep track of already imported patches, and to keep original authors.

Could we agree on disabling squash merge in the repository?

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests

[jonasmalacofilho]

I agree that keeping up with changes in the mainline tree is something we want.

to keep track of already imported patches,

Most of our PRs are created and merged before the driver is submitted to mainline, and thus not generally formatted as patches for it. I don't think this prevents keeping tracking of already imported patches, but thought it was worth mentioning since you brought up the relationship between our merge strategies and patches from/to upstream.

(Aside, I personally don't think we should internally work in patches before the first submission of a new driver to mainline. Keeping track of patch sets and revisions is just not fun, especially during early/experimental work on a driver).

and to keep original authors.

Squash merges (at least in the context of GitHub PR merges) already preserve authorship (example).

---

Overall, I think we can agree that squash merges should not be used when a PR contains commits prepared and formatted for mainline.

But I'm not sure we should forbid squash merges either, because of the more common case of PRs during the initial development of a driver (and all that work will ultimately get "squashed" anyway into one or just a few patches for actual upstreaming, per upstream procedures).

[amezin]

Most of our PRs are created and merged before the driver is submitted to mainline, and thus not generally formatted as patches for it. I don't think this prevents keeping tracking of already imported patches, but thought it was worth mentioning since you brought up the relationship between our merge strategies and patches from/to upstream.

All I wanted to say is "when there is a commit that happened in the kernel repository first, I want to have it in our repository with the same content, commit message, etc"

(Aside, I personally don't think we should internally work in patches before the first submission of a new driver to mainline. Keeping track of patch sets and revisions is just not fun, especially during early/experimental work on a driver).

Yes, I agree. However, when it's time to submit the patch, especially when multiple people worked on the driver, it might be useful to have something like stable-queue repo (repeating my comment https://github.com/liquidctl/liquidtux/issues/21#issuecomment-1374712682). Maybe its unnecessary when only one person had been writing the driver, but the discussion with multiple "please add this tag/change this" would be easier with the patch file in a repository (and pull requests into that repository) IMO, rather than re-uploading the patch file as an attachment.

Squash merges (at least in the context of GitHub PR merges) already preserve authorship

But not the original commit message and tags

But I'm not sure we should forbid squash merges either, because of the more common case of PRs during the initial development of a driver

I'd like to avoid accidental squash merges.

People writing kernel drivers usually know how to do interactive rebase to squash changes themselves, and when you submit patches upstream, it's your responsibility - as the author.

In https://github.com/liquidctl/liquidtux/pull/29 @aleksamagicka had logically well separated changes. Squashing them into one commit was unnecessary IMO.

[jonasmalacofilho]

In #29 @aleksamagicka had logically well separated changes. Squashing them into one commit was unnecessary IMO.

Fair enough.

But not the original commit message and tags

People writing kernel drivers usually know how to do interactive rebase to squash changes themselves, and when you submit patches upstream, it's your responsibility - as the author.

But then don't we get into asking authors to mind their commit history and commit messages? Not that I dislike clean history and descriptive commit messages, quite the opposite, but I also dislike being a nag about those things.

You do have a point about contributors to this repository generally being proficient in git and seeing more value in commit history and messages (as opposed to repositories for most userspace applications/libraries). So maybe it would be fine?

We would still need to remember to, before merging, ask the author to rebase and sign off the commits.

[jonasmalacofilho]

Yes, I agree. However, when it's time to submit the patch, especially when multiple people worked on the driver, it might be useful to have something like stable-queue repo (repeating my comment https://github.com/liquidctl/liquidtux/issues/21#issuecomment-1374712682). [...]

Oh, nearly forgot about this part. I think it's a good idea, maybe we could start using a patches/ directory?

[amezin]

We would still need to remember to, before merging, ask the author to rebase and sign off the commits.

Do you mean adding "signed-off-by" or GPG signing? In either case, do we really need it?

maybe we could start using a patches/ directory?

Yep. Or let's ask @aleksamagicka who's working on a patch right now

[jonasmalacofilho]

Do you mean adding "signed-off-by" or GPG signing? In either case, do we really need it?

"Signed-off-by" tags.

I originally asked because you implied you wanted to preserve

[...] the original commit message and tags

, and right now we have no policy regarding commit message styles or required tags.

But it also occurs to me that we asking for the signed-off-by tag (and acceptance of the DCO) would also allow us to manage the patch, including upstreaming it, without needing any further interaction with the author, which may eventually be hard to reach.

[amezin]

I meant I want to keep upstream commits in their original state as much as possible. For commits that happened upstream first. Just to keep track of what I imported already, without the need to compare diffs.

Regarding Signed-off-by: https://github.com/apps/dco

[jonasmalacofilho]

I think I'm missing important context: how would squash merges on our PRs affect the commit messages and tags of patches taken from upstream?[^1]

Do you want to route those patches through PRs, and are therefore worried I might accidentally squash merge them, in case there's more than one imported patch in a PR?

[^1]: Assuming the patch cleanly applies, which may not always be the case if both we and upstream concurrently touch the same code.

[amezin]

Do you want to route those patches through PRs, and are therefore worried I might accidentally squash merge them, in case there's more than one imported patch in a PR?

Yep. Actually, even when there's only one patch - squash merge will still throw away the original message, if I'm not mistaken.

Although maybe I should simply commit upstream changes directly, and all this discussion was unnecessary, yes :)

[jonasmalacofilho]

Yep. Actually, even when there's only one patch - squash merge will still throw away the original message, if I'm not mistaken.

I've given a try on some liquidtux and liquidctl PRs, and it seems that the UI prefills the message for the squashed commit similar to how the CLI handles it:

• a single commit is preserved;
• multiple commit messages are concatenated and itemized with *.

---

Although maybe I should simply commit upstream changes directly, and all this discussion was unnecessary, yes :)

The PR approach is probably better.

Only having a single imported patch per PR would prevent issues from squash merges, but that might not always be appropriate. Or we might forget about it.

So let's disable all but normal merge commits, like you suggested.

And, in the process (and to gently and indirectly nudge contributors towards better commits) let's also enforce a DCO sign-off.

[jonasmalacofilho]

So let's disable all but normal merge commits, like you suggested.

And, in the process (and to gently and indirectly nudge contributors towards better commits) let's also enforce a DCO sign-off.

Done and done. Closing.