On Tue, Jan 14, 2025 at 11:02 AM Sasha Levin <sashal@xxxxxxxxxx> 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.
>
> e89afb51f97a ("drm/vmwgfx: Fix a 64bit regression on svga3") which
> landed as c2aaa37dc18f ("drm/vmwgfx: Fix a 64bit regression on svga3")
> rather than 873601687598.
>
> 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.
>
>
> 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.
In fairness, these pre-dated the amdgpu tree using cherry-pick -x. I
had stopped doing that for a while because I kept getting yelled at
for referencing commits that were only in -next. I've since started
using -x when I need to cherry-pick a fix to -fixes.
Alex
