dim: Check for maintainer's ack when merging on behalf of another tree
We've recently had multiple occurences of people taking patches through the drm-misc tree for parts of the kernel that aren't under drm-misc maintenance, and without the maintainers Ack.
While merging patches on behalf of someone else is totally something we can do, we need to tell the maintainer and make sure he's fine with that.
This patch adds some logic to detect if the the range of commits belong indeed to the branch we are currently working on, and if not check for the maintainer's Acked-by or Signed-off-by.
Signed-off-by: Maxime Ripard mripard@kernel.org
Merge request reports
Activity
added 7 commits
-
711a932b...10b531c2 - 6 commits from branch
drm:master
- 01832414 - dim: Check for maintainer's ack when merging on behalf of another tree
-
711a932b...10b531c2 - 6 commits from branch
1028 1046 rv=1 1029 1047 fi 1030 1048 1049 options="--no-email --no-l --scm" 1050 maint_url=$(git show --no-use-mailmap --pretty=email $sha1 | 1051 ./scripts/get_maintainer.pl $options | head -n 1 | cut -d " " -f2) 1052 patch_repo=$(url_to_repo $maint_url) 1053 if [[ -z "$patch_repo" || "$repo" != "$patch_repo" ]]; then 1054 if ! checkpatch_maintainer $sha1 ; then 1055 echoerr "$cite: Mandatory Maintainer Acked-by missing." 1056 rv=1 It may have been by mistake, it may have been because of a bug in dim. Example: 5bc9de065b8bb9b8dd8799ecb4592d0403b54281 got pushed to drm-xe-next when drm-intel-gt-next should have been used.
Note that we have
- Developer push: dim_push_branch -> checkpatch_commit_push_range -> checkpatch_commit_push
- dim_pull_request -> checkpatch_commit_push_range -> checkpatch_commit_push
- dim_apply_pull -> checkpatch_commit_push_range -> checkpatch_commit_push
If something went wrong in (1) and we decide that remedying the situation is worse than you require maintainers to patch dim. See also !52 (merged)
No, it wasn't by mistake and was kind of deliberate (in good faith though).
We also had people explicitly bypassing checks in the past, so that's why I'm a bit two-minded about this. I get what you're saying, but also I don't think it's something that should be abused.
Using drm-misc as a vector to merge a patch affecting any other part of the kernel is a strict no-go.
I think we'll need a "I know what I'm doing" knob, though.
For one thing, unfortunately i915 and xe sharing display code is so convoluted atm that we'll probably want to have a slightly lower bar for requiring acks here. "Big changes" absolutely need an ack, but requiring acks for absolutely everything would drive the maintainers nuts.
Another example, I really don't think I'm going to wait for acks from everyone in this case: https://lore.kernel.org/r/20240422121011.4133236-1-jani.nikula@intel.com
In theory I should, but in practice changes like that just aren't going to happen at all if we strictly require acks from everyone. I've sent it three times, with separate pings for acks, and the response has been mostly silence. It's a judgement call, and within the subsystem I'm prepared to say "I know what I'm doing", and take the blame afterwards.
That said, the top level
dim -f
has become too powerful catch-all force.No, it wasn't by mistake and was kind of deliberate (in good faith though).
I think you misunderstood my point. My point is that something may have been applied already (due to a bug in dim, a mistake somewhere else, etc). The point I'm making in that comment is that this function is not only called when pushing a patch, but also when creating a pull request or applying a pull request. I don't think it's great to require maintainers to patch dim in that case.
@jani agreed. In !52 (merged) @rodrigovivi went with another flag instead of using -f. Should we do the same here?
I think we'll need a "I know what I'm doing" knob, though.
For one thing, unfortunately i915 and xe sharing display code is so convoluted atm that we'll probably want to have a slightly lower bar for requiring acks here. "Big changes" absolutely need an ack, but requiring acks for absolutely everything would drive the maintainers nuts.
Genuine question: Is it so convoluted that you can't make something like the display code is maintained by both, and the rendering parts are maintained by their own team in MAINTAINERS?
Another example, I really don't think I'm going to wait for acks from everyone in this case: https://lore.kernel.org/r/20240422121011.4133236-1-jani.nikula@intel.com
This one would be fine, most of these drivers are in drm-misc and you have my ack for it already.
But yeah, ok, point taken, you are both right
. I guess I'm just overly cautious after what happened recently.In !52 (merged) @rodrigovivi went with another flag instead of using -f. Should we do the same here?
I guess I'll go for -f for now, and we can always switch to another flag later on depending on how that conversation goes.
972 972 return $rv 973 973 } 974 974 975 function checkpatch_maintainer #sha1 976 { 977 local sha1 rv 978 979 sha1=$1 980 981 options="--nogit -m --nol --norolestats" 982 git show | scripts/get_maintainer.pl $options | ( local rv; rv=1; while read -r maint; do 983 if git show -s $sha1 | grep -qi "\(Acked\|Signed-off\)-by: $maint"; then I left this running for a few minutes to find commits that have both r-b and a-b by the same person. I don't know if I screwed something in bash, but it didn't really find any:
git log -z --format="%(trailers:key=Reviewed-by,key=Acked-by,valueonly)" | while IFS= read -r -d $'\0' commit; do echo $commit | sed -r '/^\s*$/d' | sort | uniq -c; done | grep -v " 1"
Not together with R-b for the same person. An A-b from a maintainer perspective means it's reviewed for the changes affecting the maintainer's tree. It doesn't mean the patch was reviewed in its entirety. So in drm-intel-* and drm-xe, a patch is never (or should never) be merged with only an A-b. What often happens in i915/xe is, in reply to a patch:
Reviewed-by: .... and ack on merging this through ...
The R-b will show up while applying with B4, but the A-b won't. Giving only the A-b will block the patch on someone else giving an additional R-b. Giving both A-b and R-b is something not present in the git history and would seem odd.
That's why I think we should also check by a R-b and at most prompt the user with "are you sure?"
Let me add some other people to chime in here. @rodrigovivi @jani @sima
Not together with R-b for the same person.
Ok, I get what you mean now, sorry. There's no such rule though. It's kind of weird, but nothing prevents it. Just like nothing prevents a Reviewed-by and a Signed-off-by, even though it would be weird too.
An A-b from a maintainer perspective means it's reviewed for the changes affecting the maintainer's tree. It doesn't mean the patch was reviewed in its entirety.
Right.
So in drm-intel-* and drm-xe, a patch is never (or should never) be merged with only an A-b.
I mean, Acked-by means someone has reviewed it and given their approval to merge it. It's literally the only way we have to record that a maintainer is ok with us merging a patch through another tree.
Assuming it goes through drm-xe, it means we have the review and approval of the "external" maintainer, plus the one of the committer through their Signed-off-by. It will never happen that we have only an Acked-by.
What often happens in i915/xe is, in reply to a patch:
Reviewed-by: .... and ack on merging this through ...
The R-b will show up while applying with B4, but the A-b won't. Giving only the A-b will block the patch on someone else giving an additional R-b.
No it won't? An Acked-by alone is fine to get a patch merged.
checkpatch_commit_push
checks for both:# check for a-b/r-b tag if ! git show -s $sha1 | grep -qi '\(reviewed\|acked\)\S*-by:' && \ ! [[ "$committer" != "$author" ]]; then echoerr "$cite: mandatory review missing." rv=1 fi
Giving both A-b and R-b is something not present in the git history and would seem odd.
Again, it's unusual indeed but not unheard of, see 00bf63122459e87193ee7f1bc6161c83a525569f for example, which has been merged this release.
In general I would tend to agree with Lucas that the rv-b would be enough and not require ack and rv-b by the same person. But given the recent state of things, I might be changing my mind here and thinking that maybe we should start to be more explicit about the tags. For instance, this i915 hwmon patch that ended up in the xe tree. It wasn't reviewed by me, but I certainly can imagine a scenario where I would have reviewed it and the patch would end up in the xe tree, so if the check was considering rv-b it would be "bypassed". On the other hand, even the ack can be dangerous anyway. Ack itself can have other meanings, that you are ack with the approach on some area that you are mostly active, but not necessarily acking as the maintainer to get that patch through that other path. So, we will never find the perfect solution here. So, we want to decide that we are going to be more verbose and give explicit acks in the mailing list (specially with b4 it is important to spell it out entirely), or if we are okay with the risk of the rv-b be enough. Well, maybe the middle ground is what Lucas suggested: "check by a R-b and"..."prompt the user with "are you sure?""
I'm sorry, but Reviewed-by has never been documented or used to indicate acceptance:
Reviewed-by:, instead, indicates that the patch has been reviewed and found acceptable according to the Reviewer’s Statement:
By offering my Reviewed-by: tag, I state that:
- I have carried out a technical review of this patch to evaluate its appropriateness and readiness for inclusion into the mainline kernel.
- Any problems, concerns, or questions relating to the patch have been communicated back to the submitter. I am satisfied with the submitter’s response to my comments.
- While there may be things that could be improved with this submission, I believe that it is, at this time, (1) a worthwhile modification to the kernel, and (2) free of known issues which would argue against its inclusion.
- While I have reviewed the patch and believe it to be sound, I do not (unless explicitly stated elsewhere) make any warranties or guarantees that it will achieve its stated purpose or function properly in any given situation.
Acked-by is documented as such however:
If a person was not directly involved in the preparation or handling of a patch but wishes to signify and record their approval of it then they can ask to have an Acked-by: line added to the patch’s changelog.
Acked-by: is often used by the maintainer of the affected code when that maintainer neither contributed to nor forwarded the patch.
Acked-by: is not as formal as Signed-off-by:. It is a record that the acker has at least reviewed the patch and has indicated acceptance. Hence patch mergers will sometimes manually convert an acker’s “yep, looks good to me” into an Acked-by: (but note that it is usually better to ask for an explicit ack).
Acked-by: does not necessarily indicate acknowledgement of the entire patch. For example, if a patch affects multiple subsystems and has an Acked-by: from one subsystem maintainer then this usually indicates acknowledgement of just the part which affects that maintainer’s code. Judgement should be used here. When in doubt people should refer to the original discussion in the mailing list archives.
So, Acked-by is used to record acceptance, Reviewed-by isn't. If that doesn't fit the way i915/xe operate, then that's on you to change it.
Think about it the other way around: would you be comfortable merging a memory-management or power management change through drm-xe or drm-i915 without the maintainer's Acked-by, but only their Reviewed-by?
I don't think we can fully automate this, and we'll just need a good enough approximation.
For example, the current check for pushing anything anywhere is that two people have been involved. Two Signed-off-by's, or Signed-off-by with Reviewed-by/Acked-by from someone else.
That's not always enough per the merge criteria, because the more complex or controversial the change, the more people, and the more senior people, you need reviews and acks from. It's about trust.
For checking acks for merging changes outside of the designated tree per MAINTAINERS, I think it should be sufficient to check for Acked-by from a maintainer for each of the trees affected.
If that allows merging stuff with just a single Acked-by, so be it. It can't be perfect.
Sorry, there's something I don't quite follow. You seem to be implying that this MR will reduce the amount of sanity checks we were doing. It doesn't, it adds new one and makes dim more restrictive that it was.
So, when you say
If that allows merging stuff with just a single Acked-by, so be it. It can't be perfect.
It's already the case today, what's the big deal about it?
I'm not intentionally trying to make a big deal out of anything, quite the opposite. We'll want this feature, and just trying to iron out the details.
And that statement was just about the fact that we can't distinguish between "ack, good to go via the tree under my maintenance" vs. "ack, good to go via another tree not under my maintenance". So be it. Let's check what we can, and for the rest, trust the committers.
It's getting difficult to handle in bash.
Incidentally, I have a draft plan and code to start migrating dim to python, piecemeal. Keep using dim the bash script, but offload parts of it (like independent checkpatchy stuff) to a python script. Move more and more, until we can flip the switch. Eventually make it a project that can be installed via pip/pipx.
FWIW here's what I got for python dim: https://gitlab.freedesktop.org/jani/maintainer-tools/-/commits/dimpy/?ref_type=heads
There's Python packaging for dim, so it could be published to and installed from pypi. Pip would install a dim wrapper to run the shell script, as well as a python dim helper that can be used to offload parts of the shell script.
It's very early stuff, and development is going to be slow as molasses. But I'm kind of hoping if I can get it kickstarted we could get more folks involved.
As you know, for our internal tooling we have a bunch of helpers in python that maybe could be leveraged. If we agree in moving to python, we could bring some of those. @gustavo is helping a lot with those tooling, maybe could also help here to speedup the switch.
Edited by Lucas De Marchi