On Tue, Jan 14, 2025 at 10:51:54AM -0500, Sasha Levin wrote:
> On Tue, Jan 14, 2025 at 04:03:09PM +0100, Simona Vetter wrote:
> > On Tue, Jan 14, 2025 at 11:01:34AM +1000, Dave Airlie wrote:
> > > > > > We create a "web" when we backport commits, and mark things for "Fixes:"
> > > > > > When we get those ids wrong because you all have duplicate commits for
> > > > > > the same thing, everything breaks.
> > > > > >
> > > > > > > I just don't get what the ABI the tools expect is, and why everyone is
> > > > > > > writing bespoke tools and getting it wrong, then blaming us for not
> > > > > > > conforming. Fix the tools or write new ones when you realise the
> > > > > > > situation is more complex than your initial ideas.
> > > > > >
> > > > > > All I want to see and care about is:
> > > > > >
> > > > > > - for a stable commit, the id that the commit is in Linus's tree.
> > > > > > - for a Fixes: tag, the id that matches the commit in Linus's tree AND
> > > > > > the commit that got backported to stable trees.
> > > > > >
> > > > > > That's it, that's the whole "ABI". The issue is that you all, for any
> > > > > > number of commits, have 2 unique ids for any single commit and how are
> > > > > > we supposed to figure that mess out...
> > > > >
> > > > > Pretty sure we've explained how a few times now, not sure we can do much more.
> > > >
> > > > And the same for me.
> > > >
> > > > > If you see a commit with a cherry-pick link in it and don't have any
> > > > > sight on that commit in Linus's tree, ignore the cherry-pick link in
> > > > > it, assume it's a future placeholder for that commit id. You could if
> > > > > you wanted to store that info somewhere, but there shouldn't be a
> > > > > need.
> > > >
> > > > Ok, this is "fine", I can live with it. BUT that's not the real issue
> > > > (and your own developers get confused by this, again, look at the
> > > > original email that started this all, they used an invalid git id to
> > > > send to us thinking that was the correct id to use.)
> > >
> > > I'm going to go back and look at the one you pointed out as I'm
> > > missing the issue with it, I thought it was due to a future ID being
> > > used.
> >
> > I think the issue is that with the cherry-picking we do, we don't update
> > the Fixes: or Reverts: lines, so those still point at the og commit in
> > -next, while the cherry-picked commit is in -fixes.
> >
> > The fix for that (which our own cherry-pick scripts implement iirc) is to
> > keep track of the cherry-picks (which is why we add that line) and treat
> > them as aliases.
> >
> > So if you have a Fixes: $sha1 pointing at -next, then if you do a
> > full-text commit message search for (cherry picked from $sha1), you should
> > be able to find it.
> >
> > We could try to do that lookup with the cherry-pick scripts, but a lot of
> > folks hand-roll these, so it's lossy at best. Plus you already have to
> > keep track of aliases anyway since you're cherry-picking to stable, so I
> > was assuming that this shouldn't cause additional issues.
> >
> > The other part is that if you already have a cherry picked from $sha1 in
> > your history, even if it wasn't done with stable cherry-pick, then you
> > don't have to cherry-pick again. These should be easy to filter out.
> >
> > But maybe I'm also not understanding what the issue is, I guess would need
> > to look at a specific example.
> >
> > > > > When future tools are analysing things, they will see the patch from
> > > > > the merge window, the cherry-picked patches in the fixes tree, and
> > > > > stable will reference the fixes, and the fixes patch will reference
> > > > > the merge window one?
> > > >
> > > >
> > > > > but I think when we cherry-pick patches from -next that fix
> > > > > other patches from -next maybe the fixes lines should be reworked to
> > > > > reference the previous Linus tree timeline not the future one. not
> > > > > 100% sure this happens? Sima might know more.
> > > >
> > > > Please fix this up, if you all can. That is the issue here. And again,
> > > > same for reverts.
> > > >
> > > > I think between the two, this is causing many fixes and reverts to go
> > > > unresolved in the stable trees.
> > > >
> > > > > Now previously I think we'd be requested to remove the cherry-picks
> > > > > from the -fixes commits as they were referencing things not in Linus'
> > > > > tree, we said it was a bad idea, I think we did it anyways, we got
> > > > > shouted at, we put it back, we get shouted that we are referencing
> > > > > commits that aren't in Linus tree. Either the link is useful
> > > > > information and we just assume cherry-picks of something we can't see
> > > > > are a future placeholder and ignore it until it shows up in our
> > > > > timeline.
> > > >
> > > > I still think it's lunacy to have a "cherry pick" commit refer to a
> > > > commit that is NOT IN THE TREE YET and shows up in history as "IN THE
> > > > FUTURE". But hey, that's just me.
> > > >
> > > > Why do you have these markings at all? Who are they helping? Me?
> > > > Someone else?
> > >
> > > They are for helping you. Again if the commit that goes into -next is immutable,
> > > there is no way for it to reference the commit that goes into -fixes
> > > ahead of it.
> > >
> > > The commit in -fixes needs to add the link to the future commit in
> > > -next, that link is the cherry-pick statement.
> > >
> > > When you get the future commit into the stable queue, you look for the
> > > commit id in stable history as a cherry-pick and drop it if it's
> > > already there.
> > >
> > > I can't see any other way to do this, the future commit id is a
> > > placeholder in Linus/stable tree, the commit is immutable and 99.99%
> > > of the time it will arrive at some future point in time.
> > >
> > > I'm open to how you would make this work that isn't lunacy, but I
> > > can't really see a way since git commits are immutable.
> >
> > Yeah the (cherry picked from $sha1) with a sha1 that's in -next and almost
> > always shows up in Linus' tree in the future shouldn't be an issue. That
> > part really is required for driver teams to manage their flows.
> >
> > > > > I think we could ask to not merge things into -next with stable cc'ed
> > > > > but I think that will result in a loss of valuable fixes esp for
> > > > > backporters.
> > > >
> > > > Again, it's the Fixes and Reverts id referencing that is all messed up
> > > > here. That needs to be resolved. If it takes you all the effort to
> > > > make up a special "stable tree only" branch/series/whatever, I'm all for
> > > > it, but as it is now, what you all are doing is NOT working for me at
> > > > all.
> > >
> > > I'll have to see if anyone is willing to consider pulling this sort of
> > > feat off, it's not a small task, and it would have to be 99% automated
> > > I think to be not too burdensome.
> >
> > It's not that hard to script, dim cherry-pick already does it. It's the
> > part where we need to guarantee that we never ever let one slip through
> > didn't get this treatment of replacing the sha1.
> >
> > The even more insideous one is when people rebase their -next or -fixes
> > trees, since then the sha1 will really never ever show up. Which is why
> > we're telling people to not mess with git history at all and instead
> > cherry-pick. It's the lesser pain.
>
> But this does happen with cherry picks... A few examples from what I saw
> with drivers/gpu/drm/ and -stable:
>
> 5a507b7d2be1 ("drm/mst: Fix NULL pointer dereference at
> drm_dp_add_payload_part2") which landed as 8a0a7b98d4b6 ("drm/mst: Fix
> NULL pointer dereference at drm_dp_add_payload_part2") rather than
> 4545614c1d8da.
This one also landed through Alex' tree, and before he switched over to
cherry-pick -x and not trying to fix things up with rebasing. Because in
theory rebasing bugfixes out of -next into -fixes avoids all that trouble,
in practice it just causes a reliably even bigger mess.
> e89afb51f97a ("drm/vmwgfx: Fix a 64bit regression on svga3") which
> landed as c2aaa37dc18f ("drm/vmwgfx: Fix a 64bit regression on svga3")
> rather than 873601687598.
This one is from 2021. Iirc it's the case that motivated us to improve the
commiter documentation and make it clear that only maintainers should do
cherry-picks. Occasionally people don't know and screw up.
> a829f033e966 ("drm/i915: Wedge the GPU if command parser setup fails")
> which indicates it's a cherry-pick, but I couldn't find the equivalent
> commit landing at any point later on.
This one was a maintainer action by Dave and me, where we went in and
rebased an entire -next tree. Also from 2021, even more exceptional than
the "committer cherry-picked themselves and screwed up".
I'm not saying that the cherry-pick model with committers is error free.
Not at all. It's just in my experience substantially less error prone than
anything else, it's simply the less shit option.
Roughly the options are:
- rebase trees to not have duplicated commits. Breaks the committer model,
pretty much guarantees that you have commit references to absolutely
nowhere at all in practice because people butcher rebases all the time.
Also pisses off Linus with unecessary rebases that don't reflect actual
development history.
Plus we'd insta run out of maintainers in drm if we do this.
I think this is also what Alex tried to do until very recently.
- cherry-pick, but pretend it didn't happen. This means either people
perfectly fix up all tags (see above, doesn't happen in practice) or you
need to do title based guessing games. Plus you need to do title-based
guessing games with the duplicates anyway.
- cherry-pick -x. You can actually handle this one with scripts and no
human shouting. Unless people forgot to use -x or screwed up something
else (which is why we have a script and docs). Which does happene, but
the two examples you've found for that flow are from 2021. There should
also be some that are more recent.
- we give up on stable for drm.
Cheers, Sima
> Or the following 3 commits:
>
> 0811b9e4530d ("drm/amd/display: Add HUBP surface flip interrupt
> handler") which has a stable tag, and no cherry-pick line.
>
> 4ded1ec8d1b3 ("drm/amd/display: Add HUBP surface flip interrupt
> handler") which is a different code change than the previous commit, and
> a completely different commit message, no stable tag, and no cherry-pick
> line.
>
> 7af87fc1ba13 ("drm/amd/display: Add HUBP surface flip interrupt
> handler") which has the same code change as above, and it has the same
> commit message as 4ded1ec8d1b3 but with an added stable tag, and again -
> no cherry-pick line.
>
> --
> Thanks,
> Sasha
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
