Comments (11)
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
from liquidtux.
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).
from liquidtux.
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 #21 (comment)). 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 #29 @aleksamagicka had logically well separated changes. Squashing them into one commit was unnecessary IMO.
from liquidtux.
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.
from liquidtux.
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 #21 (comment)). [...]
Oh, nearly forgot about this part. I think it's a good idea, maybe we could start using a patches/
directory?
from liquidtux.
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
from liquidtux.
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.
from liquidtux.
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?
Footnotes
-
Assuming the patch cleanly applies, which may not always be the case if both we and upstream concurrently touch the same code. ↩
from liquidtux.
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 :)
from liquidtux.
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.
from liquidtux.
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.
from liquidtux.
Related Issues (19)
- Kernel driver for corsair coolers HOT 6
- nzxt-kraken2: fan/pump control HOT 2
- Grid v3 upstreaming HOT 4
- Interest for EVGA and Corsair coolers using 5th gen. Asetek designs HOT 15
- nzxt-kraken3 upstreaming? HOT 30
- Usage with fancontrol HOT 2
- ci: enable codespell
- Match file locations with the kernel tree HOT 1
- nzxt-smart2: maybe error on stale data?
- ci: test CONFIG_DEBUG_FS=y/n
- nzxt-kraken3.c: ‘get_fw_version_cmd’ defined but not used with CONFIG_DEBUG_FS=n
- gitversion.sh (and then dkms) fails on clones without tags HOT 2
- Thanks HOT 1
- Migrate to Rust HOT 2
- NZXT Smartdevice v2 support HOT 5
- Building with W=1? HOT 1
- nzxt-kraken: Incorrect order of failing in probe HOT 1
- X63 support HOT 40
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from liquidtux.