For my understanding, this suggestion would possibly skip comments that exist on commits, not comments posted as part of a "normal" review flow. Is that right? Or does this part of the query include all comments on the MR?

Yes, it would skip some comments if the commit limits are reached. That's still the case with 100 so it's an option to consider if that's a easier way out and if the comments on old commits are not as important as the new comments.

@dskim_gitlab the main thing I wonder about here is what type of comments are we limiting to 100. Is it only this type of comment: https://docs.gitlab.com/user/project/merge_requests/commits/#add-a-comment-to-a-commit ? Because if it is, we can likely lower this to 20 if that would help. I assume not many people actually use this feature of commenting on commits and then want to see those comments within MRs?

@phikai sorry, I'm only getting to this now. This looks reasonable to me, especially Phase 1. I do wonder if the devil will be in the details here, e.g. cherry-picking when it's a fast-forward merge of many commit, or do we require the initial source branch to not be deleted, etc.

I'm less sure about Phase 2 (Would it be convenient to maintain that list? What about edge cases?) and Phase 3 (E.g. internally we'd never want this because we time the merges with the security release process).

It would be good to have it reviewed by someone who's actually gone through those steps internally. @dskim_gitlab could you please have a look at this epic and share any feedback you have based on your experience with our internal security backport process? Are there meaningful gains to have from automating steps 2-3 in the problem statement, or is this something engineers already solve today with alternatives like local scripts or agent prompts?

@phikai @mle as a follow up from #590833 (comment 3118805405), could you please share what you think we should track here? cc @slashmanov

@slashmanov yes, good idea 👍 I've created one here: #592633. Let's continue the discussion there.

@phikai thanks, I'll earmark this for %18.11.

wdyt, is it maybe worth considering caching here as well?

@manuelgrabowski I will defer entirely to @marc_shaw and @vyaklushin's opinions on this one :) It looks like there are some performance improvements in the making already, which is promising.

Thanks @furkanayhan. I believe we are aware of this already as it surfaced via our error budget. Here's a relevant issue we have open to investigate slow ReactiveCachingWorker jobs: https://gitlab.com/gitlab-org/gitlab/-/work_items/588864. cc @egrieff in case this is actually something different

François Rosé (880305ca) at 06 Mar 19:29

François Rosé (a89a9f9f) at 06 Mar 19:29

... and 1 more commit

François Rosé (880305ca) at 06 Mar 19:27