[PATCH 8/8] docs: propose new rules for topic/core-for-CI management

[Rodrigo Vivi]

On Tue, Oct 25, 2022 at 04:37:59PM +0300, Jani Nikula wrote:
> On Tue, 25 Oct 2022, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> wrote:
> > On 25/10/2022 11:20, Jani Nikula wrote:
> >> Require maintainer ack for rebase.
> >> 
> >> Require maintainer/committer ack for adding/removing commits.
> >> 
> >> Require gitlab issue for each new commit.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> ---
> >>   drm-tip.rst | 23 ++++++++++++++++++-----
> >>   1 file changed, 18 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drm-tip.rst b/drm-tip.rst
> >> index deae95cdd2fe..fde62feca296 100644
> >> --- a/drm-tip.rst
> >> +++ b/drm-tip.rst
> >> @@ -203,11 +203,13 @@ justified exception. The primary goal is to fix issues originating from Linus'
> >>   tree. Issues that would need drm-next or other DRM subsystem tree as baseline
> >>   should be fixed in the offending DRM subsystem tree.
> >>   
> >> -Only rebase the branch if you really know what you're doing. When in doubt, ask
> >> -the maintainers. You'll need to be able to handle any conflicts in non-drm code
> >> -while rebasing.
> >> +Only rebase the branch if you really know what you're doing. You'll need to be
> >> +able to handle any conflicts in non-drm code while rebasing.
> >>   
> >> -Simply drop fixes that are already available in the new baseline.
> >> +Always ask for maintainer ack before rebasing. IRC ack is sufficient.
> >> +
> >> +Simply drop fixes that are already available in the new baseline. Close the
> >> +associated gitlab issue when removing commits.
> >>   
> >>   Force pushing a rebased topic/core-for-CI requires passing the ``--force``
> >>   parameter to git::
> >> @@ -225,11 +227,22 @@ judgement call.
> >>   Only add or remove commits if you really know what you're doing. When in doubt,
> >>   ask the maintainers.
> >>   
> >> +Always ask for maintainer/committer ack before adding/removing commits. IRC ack
> >> +is sufficient. Record the ``Acked-by:`` in commits being added.
> >> +
> >>   Apply new commits on top with regular push. The commit message needs to explain
> >>   why the patch has been applied to topic/core-for-CI. If it's a cherry-pick from
> >>   another subsystem, please reference the commit with ``git cherry-pick -x``
> >>   option. If it's a patch from another subsystem, please reference the patch on
> >>   the mailing list with ``Link:`` tag.
> >>   
> >> +New commits always need an associated gitlab issue for tracking purposes. The
> >> +goal is to have as few commits in topic/core-for-CI as possible, and we need to
> >> +be able to track the progress in making that happen. Reference the issue with
> >> +``References:`` tag. Add the ``core-for-CI`` label to the issue. (Note: Do not
> >> +use ``Closes:`` because the logic here is backwards; the issue is having the
> >> +commit in the branch in the first place.)
> >> +

good idea!

> >
> > In some cases we can have two situations depending on which team is 
> > quicker - 1) there may be a gitlab issue created already by the 
> > regression tracking team, 2) maintainer/committer may need to create a 
> > new issue. (Duplicates may arise as well, although I don't think that is 
> > a problem. The only important thing is that the core-for-CI commit gets 
> > tracked/justified.) So perhaps just clarify if the idea is for person 
> > adding a commit to search for bugs or just create a new issue?
> 
> I don't really care who creates it, as long as there's one. But anyone
> tracking regressions might file an issue, and then close it because the
> commit in core-for-CI "fixes" it. For this, the issue should be fixed by
> removing the commit from core-for-CI. The "bug" is having the commit in
> core-for-CI. So the two are kind of backwards. Maybe we should just file
> a new bug always?

yeap, in the worst case we can always mark the duplication and point to
the real issue. The important part is to have a path of information from
the patch itself to the history, even if this end up in some extra
issues.gitlab hops.

> 
> > But in general I think it is a good idea to record justification in 
> > gitlab. In practice we ended up with a number of "permanent" patches in 
> > there, which probably are not ever going away and those are probably the 
> > ones which are most important to document why they are there. Or at 
> > least no one is doing anything to upstream them or something. With 
> > gitlab tracking it is not guaranteed that would be better, but at least 
> > there would be some conversation history.
> 
> I look at most of the old ones, and I can't help thinking we've just
> completely dropped the ball after merging the changes to
> core-for-CI. And that's exactly why I want tracking, and I want the
> commits upstreamed or simply dropped. We can't keep maintaining this
> stuff ourselves.
> 
> BR,
> Jani.

Nice doc clean-up and updates!

Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

for the series

> 
> 
> >
> > Regards,
> >
> > Tvrtko
> >
> >>   Instead of applying reverts, just remove the commit. This implies ``git rebase
> >> --i`` on the current baseline; see directions above.
> >> +-i`` on the current baseline; see directions above. Close the associated gitlab
> >> +issue when removing commits.
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center