On Fri, Jan 09, 2026 at 07:08:40PM +0100, Quentin Schulz wrote: > Hi Tom, > > On 1/9/26 6:29 PM, Tom Rini wrote: > > As seen with a recent pull request, there have been implicit assumptions > > about how much custodians can change patches before applying and merging > > them. This has lead to an unfortunate situation with a contributor being > > understandably upset about the changes. > > > > To avoid this in the future, document a few things: > > - Non-trivial changes to make something apply need to have at least > > [name: what] in the commit message. > > - Only trivial and obviously correct changes can be made by the > > custodian, and ideally this is done in a merge commit and not the > > patch itself. > > - It is the responsibility of the custodian to gather relevant tags that > > have been provided by the community. > > > > Link: > > https://lore.kernel.org/u-boot/[email protected]/ > > Signed-off-by: Tom Rini <[email protected]> > > --- > > With this, Heinrich, per E Shattow's comment on IRC, can you please > > revert the commit in question, then apply his patch as-is, and then your > > changes as a new patch in order to show correct history? > > > > And I realize at this point the subsection of this document is a bit > > wordy. Maybe a follow-up of restructuring this part of the document > > would be helpful? I'm not sure what would flow better however. > > > > Cc: E Shattow <[email protected]> > > Cc: Heinrich Schuchardt <[email protected]> > > Cc: Quentin Schulz <[email protected]> > > --- > > doc/develop/process.rst | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/doc/develop/process.rst b/doc/develop/process.rst > > index 4bfbf0eb9c63..84ab09132a0b 100644 > > --- a/doc/develop/process.rst > > +++ b/doc/develop/process.rst > > @@ -244,7 +244,12 @@ like this: > > custodian can also accept patches against older code. It can be > > difficult to find the correct balance between putting too much work > > on > > the custodian or too much work on an individual submitting a patch > > when > > - something does not apply cleanly. > > + something does not apply cleanly. If non-trivial changes need to be > > made > > + for it to apply the custodian is expected to annotate the commit > > message > > + with ``[name: note about changes made]``. Furthermore, only trivial > > and > > We should provide an example, I think. > > commit 66be03b7ee19444b23aae3990a434a7470fc1641 > Author: Jérémie Dautheribes <[email protected]> > Date: Fri Nov 28 12:03:04 2025 +0100 > > binman: blob_dtb: improve error message when SPL is not found > > When using binman with the '-a spl-dtb=y' flag, if the SPL blob is not > found, binman throws a cryptic error message: > binman: 'NoneType' object has no attribute 'startswith' > > Let's improve the error message to explicitly state which SPL blob is > missing. > This is particularly useful when binman is used as a standalone tool > outside the U-Boot source tree. > > Signed-off-by: Jérémie Dautheribes <[email protected]> > [trini: Add '# pragma: no cover' because coverage doesn't seem to like > the documentation about this error] > Signed-off-by: Tom Rini <[email protected]> > > is a good one I think. > > You also added your SoB which isn't mentioned as a requirement here. I > believe it must, because whatever you added/changed, you must comply with > DCO to do it. > > I'm tempted to say we maybe should add a link to the original (unmerged) > commit so that people can actually do the diff themselves if they want to? > > For the above example, have > > Link: > https://lore.kernel.org/u-boot/[email protected]/ > > Maybe? Not sure how much friction we want to put on custodians. There's > probably already a lot to do :) > > I would really like also that custodians *tell* people that they have merged > the commits with changes. > https://docs.u-boot.org/en/latest/develop/process.html#work-flow-of-a-custodian > says it's only "ideally" they do for merged patches, but I really think we > should tell people when changes are made to their patches. > > > + obviously correct changes can be made, no other changes should be > > made to > > + the patch itself. Ideally all of these changes will be in a merge > > commit, > > + and not the commit itself, but that is not always possible. > > I have never been a maintainer in public spaces, and in private spaces I > enforce a strict rebase policy so I don't have merge commits ever. I > wouldn't know what this means (but maybe custodians do, and that's the most > important as this isn't something contributors need to know?). How are we > supposed to do that? Note also that not everybody know that git log -c (or > --cc? I use -c but it seems to be showing the diff of the whole merge and > not necessarily the conflicts?) needs to be used to see the content of a git > merge commit (it won't show with git log -p). > > Can we provide an example here of a good merge commit that highlights what > is expected from custodians? > > I cannot really comment on whether this will help custodians do the right > thing as I am no custodian. > > We need to set the expectations for contributors as well. > > """ > Trivial fixes may be applied by custodians when merging your patches, > sometimes without public notice. The operated changes are explicitly listed > at the end of the commit log or in a merge commit by the custodian. > """ > > or something like that? I'm wondering where we should put this though. > https://docs.u-boot.org/en/latest/develop/process.html#review-process-git-tags > or https://docs.u-boot.org/en/latest/develop/sending_patches.html? I think > this kind of change should be in the same commit (or one that follows after, > but is done now). > > We can have some words about proper etiquette too? Like avoiding big series, > waiting a bit between versions, not silently ignoring previous reviews for > newer versions (you can disagree but you need to speak your mind publicly > and "resolve the conflict" before sending a newer version (or at the very > least say why something wasn't done in the cover-letter or under the --- > section in a patch), waiting at least a week (more?) to send a ping on yet > unreviewed patches, ... That's what's coming to mind right now, there's > probably more to add? This is a separate topic and I don't want this to be a > blocker for this here.
That's a lot of feedback, thank you. I think this means for v2, I need to: - Move most of the existing wording to another section, and reference up to it. - Note that some things we have done before, really shouldn't be (in your good example, maybe I should have pushed back on the contributor to try and figure out how to convince Python it *had* coverage and not just pragma past it, to not have the contribution sitting out even logner). - Incorporate much of the above and try and remove other assumptions I did make / added in my wording above. -- Tom
signature.asc
Description: PGP signature
