Ancestors

Toot

Written by Jens Axboe on 2024-09-12 at 18:18

Having worked on the kernel for decades, and imposing a lot of the same code/git hygiene for liburing, there can be a disconnect for contributors on what is expected of a commit and commit message, and what series of commits should look like. I attempted to provide a basic guideline here:

https://github.com/axboe/liburing/blob/master/CONTRIBUTING.md

and would appreciate feedback from folks on what I missed, what isn't clear, etc.

=> More informations about this toot | More toots from axboe@fosstodon.org

Descendants

Written by Anisse on 2024-09-12 at 18:23

@axboe do you have any strategy for ensuring bisectability while also being on GitHub? A quick look at your workflow shows it does not build&test every commit, only PRs. Do you do that yourself locally?

=> More informations about this toot | More toots from Aissen@treehouse.systems

Written by Jens Axboe on 2024-09-12 at 18:24

@Aissen yeah I test this myself, though I do wish GH actions would just do the right thing. Bisectability is generally not a huge problem, but really annoying if an error does creep in.

=> More informations about this toot | More toots from axboe@fosstodon.org

Written by Mickaël Salaün on 2024-09-13 at 07:03

@axboe @Aissen You can do it. Every commits of the #Landlock Rust library are tested with the GitHub CI (and with all meaningful kernel versions) which ensures bisectability: https://github.com/landlock-lsm/rust-landlock/blob/main/.github/workflows/rust.yml

=> More informations about this toot | More toots from l0kod@mastodon.social

Written by Anisse on 2024-09-13 at 08:03

@l0kod @axboe thanks! I knew it was possible, but I like your implementation. I wonder if it's possible to put this in a common and reusable across projects "action library" to prevent doing this manually for every project because of a Github shortcoming.

=> More informations about this toot | More toots from Aissen@treehouse.systems

Written by Jens Axboe on 2024-09-13 at 13:34

@l0kod @Aissen ah nice, I'll check that out!

=> More informations about this toot | More toots from axboe@fosstodon.org

Written by Markus Werle on 2024-09-13 at 06:44

@Aissen @axboe the paragraph about coding style leaves me with a vague appeal. What does this really mean? Do you have a .clang-format file I can use to bring my code into shape? What else do you mean with coding style? Something beyond formatting and variable naming conventions?

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Markus Werle on 2024-09-13 at 06:46

@Aissen @axboe sed -e’s/should/must/g’ and then check if the advice given is sufficient to adhere to the rules.

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Markus Werle on 2024-09-13 at 06:49

@Aissen @axboe replace “it's not uncommon to have a pull request contain multiple commits” by “it's perfectly OK to have a pull request contain multiple commits” to avoid the double negation that let me misread it first and also sounds more positive.

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Markus Werle on 2024-09-13 at 06:52

@Aissen @axboe for “squash fixup” please give an example of what you really want to happen. Do you want a git amend for the wrong commit or a local history rewrite pushed? 3 lines of git commands remove all room for interpretation.

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Markus Werle on 2024-09-13 at 06:57

@Aissen @axboe I forgive you the “Don't explain what the code in commit does, that should be readily apparent from just reading the code.“ You are a very intelligent kernel hacker so I fully understand that “just read the code” is an explanation for you. In my world I want a high level explanation of what was intended. But I am a stupid slow learner and cannot grok Linux source code. I only see trees, not the woods.

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Markus Werle on 2024-09-13 at 07:00

@Aissen @axboe do we need “heinous” in

“liburing doesn't squash-on-rebase, or other heinous practices sometimes seen elsewhere.”?

For merge/rebase strategies I would go with YMMV.

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Markus Werle on 2024-09-13 at 07:02

@Aissen @axboe That’s my 2 cents. In general a good thing to have that document.

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Jens Axboe on 2024-09-13 at 13:35

@markuswerle @Aissen No we definitely don't, it's an attempt at humor. But squash or rebase on merge is heinous, objectively, so I'll keep it ;-)

=> More informations about this toot | More toots from axboe@fosstodon.org

Written by Markus Werle on 2024-09-13 at 17:34

@axboe @Aissen ROFL

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Jens Axboe on 2024-09-13 at 13:37

@markuswerle imho that belongs in code comments. The commit message can surely explain what the code does, but I've seen way too many commit messages that just painstakingly detail it. And I don't care about that, I can read it in the code. I care about knowing WHY you felt you need to make that change.

=> More informations about this toot | More toots from axboe@fosstodon.org

Written by Markus Werle on 2024-09-13 at 17:01

@axboe yes, and I fully understand that you do not need that information. For me the lack of that information is the major blocker for understanding the inner parts especially of complicated low level kernel code, BSPs or embedded stuff. The architecture is not available in the meta information, but has to be derived from the code itself which I am not good at.

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Jens Axboe on 2024-09-13 at 17:05

@markuswerle Maybe I wasn't fully clear - what I meant is that if the code does not stand on its own in terms of being understandable or needs explanations, then that should go IN THE CODE, not in the commit message. The commit message should just explain WHY the change is made, not details of how the code works. Generally speaking, obviously there are exceptions, arch type overviews I think would be fine (but still better in the code where people will see them!).

=> More informations about this toot | More toots from axboe@fosstodon.org

Written by Markus Werle on 2024-09-13 at 17:29

@axboe so you mean in the code as comment? That would be ok for me

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Jens Axboe on 2024-09-13 at 17:30

@markuswerle yep exactly

=> More informations about this toot | More toots from axboe@fosstodon.org

Written by Markus Werle on 2024-09-13 at 17:34

@axboe the we are on the same page and I fully agree

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Jens Axboe on 2024-09-13 at 13:38

@markuswerle good point! What I mean is fixup the original commit, which is amending it with the fixup. I do see people messing up that part, so adding some details on how to do that would be useful. I'll do that.

=> More informations about this toot | More toots from axboe@fosstodon.org

Written by Mathieu on 2024-09-12 at 20:30

@axboe Looks good. I'm from a kernel background so all looks pretty standard / expected so maybe I'm not really the target audience, but looks good never the less.

=> More informations about this toot | More toots from mathieu@fosstodon.org

Written by Jens Axboe on 2024-09-12 at 20:31

@mathieu as someone else said, "it's common sense!". But I think not if you haven't been through the ringer already. Thanks for reading!

=> More informations about this toot | More toots from axboe@fosstodon.org

Written by Mathieu on 2024-09-12 at 20:32

@axboe Yeah I had the same feeling initially reading it, then I remembered all the PRs etc I went through in python land (for $DAYJOB) and realised well... maybe it's really not that common for other people out there on GH and elsewhere.

=> More informations about this toot | More toots from mathieu@fosstodon.org

Written by Markus Werle on 2024-09-13 at 06:40

@axboe IMHO "Maintain bisectability” needs an explanation or a link to what it means. Most people don’t know about this method and supporting git features..

=> More informations about this toot | More toots from markuswerle@nrw.social

Written by Nik on 2024-09-13 at 16:31

@axboe Missing info on communicating what you're working on, how to find things to work on, expectations for reviewers, etc.

https://github.com/pachli/pachli-android/blob/main/docs%2Fcontributing%2Fcode.md is an example from one of my projects.

=> More informations about this toot | More toots from nikclayton@mastodon.social

Written by Jens Axboe on 2024-09-13 at 16:37

@nikclayton a lot of that is beyond the scope of my doc. It's really just about how to do the basics when contributing, not project management. In other words, the things I correct people on all the time. Now I can just point them at the doc rather than explain the same stuff over and over.

=> More informations about this toot | More toots from axboe@fosstodon.org