Hi! On Thu, 26 Sept 2024 at 12:40, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > On Thu, Sep 26, 2024 at 10:35:34AM +0000, Sakari Ailus wrote: > > Hi Laurent, > > > > On Thu, Sep 26, 2024 at 01:24:48PM +0300, Laurent Pinchart wrote: > > > On Thu, Sep 26, 2024 at 12:19:14PM +0200, Mauro Carvalho Chehab wrote: > > > > Em Thu, 26 Sep 2024 09:30:34 +0000 > > > > Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> escreveu: > > > > > > > > > > >>> Can you refresh my memory which processes need more work? > > > > > > >> > > > > > > >> I have the same doubt. As discussed during the summit, Hans and I had some > > > > > > >> discussions yesterday, to address a few details. For both of us the process > > > > > > >> sounds well defined. > > > > > > >> > > > > > > >> From my personal notes, this will be the new process: > > > > > > >> > > > > > > >> - committers will merge patches at media-committers.git tree at fdo, > > > > > > >> provided that they'll follow the rules defined on a committers agreement > > > > > > >> and (partially?) enforced by media-ci checks; > > > > > > >> - core committers follow the same rules, with a broader privilege of > > > > > > >> changing kernel API/ABI; > > > > > > >> - committers will ensure that patchwork will reflect the review process of > > > > > > >> the patches; > > > > > > >> - maintainers will double-check if everything is ok and, if ok, merge the > > > > > > >> changes at linuxtv.org. We intend to rename the tree there to "media.git", > > > > > > >> being the main tree to be used for development; > > > > > > >> - pull requests will keep using the same process as currently. The only > > > > > > >> change is that the media-stage.git tree will be renamed to "media.git"; > > > > > > >> - maintainers will periodically send patches upstream. > > > > > > >> > > > > > > >> The media-commiters.git tree at fdo might be rebased if needed; the > > > > > > >> media.git tree at linuxtv.org is stable. A large effort will be taken to > > > > > > >> avoid rebasing it. > > > > > > >> > > > > > > >> We may need some helper scripts and/or use pwclient to keep patchwork > > > > > > >> updated after committers reviews. > > > > > > > > > > > > > > What will happen if we update the status of patches in patchwork when > > > > > > > merging them to the fdo tree, and the tree is later rebased to drop > > > > > > > commits ? Will the person rebasing handle updating patchwork to move the > > > > > > > patches back from accepted to a different status ? > > > > > > > > > > > > That should be the responsibility of the person doing the rebase. I think > > > > > > that's what is done today as well in the rare cases we rebase. > > > > > > > > > > Sounds reasonable. I'd also like to avoid rebases as much as possible. > > > > > > > > > > Do we have a list of cases where a rebase would be needed? A license issue > > > > > or a missing Sob line, perhaps? > > > > > > > > No, and I don't think we can write a rule to cover such cases. The only rule > > > > is that it is up to maintainers to decide to do a rebase or not, and this > > > > will be done case by case. > > > > > > > > With regards to the cases you mentioned, it is almost surely that license > > > > issues will call for a rebase. The same may apply up to some point for > > > > missing/refused SoBs from authors, co-developers and from the committers. > > > > > > > > Yet, I would expect that a more common case is if someone touches the code > > > > and another committer/developer/author nacks with such changes. > > > > > > > > So, for instance, suppose you maintain driver A. Some other committer > > > > may end merging a patch for driver A without your ack. Depending on the > > > > patch that could be OK (trivial/doc changes, bugs with bug fixes that > > > > are available for some time, etc). > > > > > > > > Yet, even if the committer did an honest handling of the patch, you may > > > > still disagree or want some changes at the original patch. On such cases, > > > > the maintainers may decide to drop the changes and do a normal review > > > > process. They may otherwise request a patch on the top of the applied > > > > one to address the pointed issues. > > > > > > Let's do a revert in that case, and keep rebases for cases where having > > > content in the git history causes issues other than bisection problems. > > > > I'd very much prefer this as well: revert or fix, if at all reasonable, > > instead of rebasing should be a rule. > > > > > I'd argue that even a missing SoB may not be a cause for rebase if it's > > > an accident, but that's not worth debating because CI will make sure > > > this never happens. > > > > Does it? > > > > checkpatch.pl checks should just be warnings. And that should probably > > stay. Sob: and From: being different isn't necessarily that far-fetched as > > having an address in .mailmap may change From: field but not Sob:, > > resulting in a checkpatch.pl warning. > > > > I wonder if checkpatch.pl should know about .mailmap actually, currently it > > doesn't. I could send a patch. > > We have an explicit check in the CI to ensure that both the author and the > committer have a SoB line. It's not base on checkpatch.pl. Ricardo, > could you confirm this ? CI checks that there are at least 2 committers that agree with the code (SoB, Reviewed-by or Ack-by) https://gitlab.freedesktop.org/linux-media/media-ci/-/blob/main/lib/test-trust.py?ref_type=heads We also have a separate check for SoB: https://gitlab.freedesktop.org/linux-media/media-ci/-/blob/main/test-media-patchstyle.sh?ref_type=heads#L61 > > > > > There is also worse case scenarios, like a committer violating the > > > > committer's agreement. > > > > > > I'm fine with rebases if someone gets rogue and merges malicious code, > > > or commits with insults in the commit message. I don't foresee that > > > happening regularly, if ever. > > > > I'm more concerned of a malicious actor getting access to the committer's > > credentials rather than the committer him-/herself going crazy. And if this > > happens, changes are it won't be noticed immediately. > > That's not much different than someone impersonating me or you when > sending pull requests. Signing tags helps, but if someone steals > credentials, it's also game over. With the new workflow the malicious > changes will need to pass CI, so there may be more scrutiny :-) > > > Reminding of > > <URL:https://github.com/lfit/itpol/blob/master/linux-workstation-security.md> > > in the instructions might not be a bad idea. > > Agreed. > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda
