| Message ID | 20221122131714.3443238-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [MAINTAINER,TOOLS] docs: updated rules for topic/core-for-CI commit management | expand |
On Tue, Nov 22, 2022 at 03:17:14PM +0200, Jani Nikula wrote: > Introduce stricter rules for topic/core-for-CI management. Way too many > commits have been added over the years, with insufficient rationale > recorded in the commit message, and insufficient follow-up with removing > the commits from the topic branch. > > New rules: > > 1. Require maintainer ack for rebase. Have better gating on when rebases > happen and on which baselines. > > 2. Require maintainer/committer ack for adding/removing commits. No > single individual should decide. > > 3. Require gitlab issues for new commits added. Improve tracking for > removing the commits. > > Also use the stronger "must" for commit message requiring the > justification for the commit being in topic/core-for-CI. > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: David Airlie <airlied@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: intel-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Cc: dim-tools@lists.freedesktop.org > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drm-tip.rst | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/drm-tip.rst b/drm-tip.rst > index deae95cdd2fe..24036e2ef576 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. > > -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 > +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 must 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.) > + > 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. > -- > 2.34.1 >
On Tue, Nov 22, 2022 at 03:59:10PM +0100, Daniel Vetter wrote: > On Tue, Nov 22, 2022 at 03:17:14PM +0200, Jani Nikula wrote: > > Introduce stricter rules for topic/core-for-CI management. Way too many > > commits have been added over the years, with insufficient rationale > > recorded in the commit message, and insufficient follow-up with removing > > the commits from the topic branch. > > > > New rules: > > > > 1. Require maintainer ack for rebase. Have better gating on when rebases > > happen and on which baselines. > > > > 2. Require maintainer/committer ack for adding/removing commits. No > > single individual should decide. > > > > 3. Require gitlab issues for new commits added. Improve tracking for > > removing the commits. > > > > Also use the stronger "must" for commit message requiring the > > justification for the commit being in topic/core-for-CI. > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Cc: David Airlie <airlied@gmail.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: intel-gfx@lists.freedesktop.org > > Cc: dri-devel@lists.freedesktop.org > > Cc: dim-tools@lists.freedesktop.org > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > drm-tip.rst | 27 ++++++++++++++++++++------- > > 1 file changed, 20 insertions(+), 7 deletions(-) > > > > diff --git a/drm-tip.rst b/drm-tip.rst > > index deae95cdd2fe..24036e2ef576 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. > > > > -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 > > +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 must 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.) > > + > > 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. > > -- > > 2.34.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Tue, Nov 22, 2022 at 03:17:14PM +0200, Jani Nikula wrote: >Introduce stricter rules for topic/core-for-CI management. Way too many >commits have been added over the years, with insufficient rationale >recorded in the commit message, and insufficient follow-up with removing >the commits from the topic branch. > >New rules: Why not make a list like this the actual text? It's easier to follow a bullet/numbered list than the free form text. > >1. Require maintainer ack for rebase. Have better gating on when rebases > happen and on which baselines. What maintainer? drm-intel-gt-next/drm-intel-next/drm-misc/drm? Any? I don't want fingers pointed, but just to know the context: was there any event recently that triggered this? Because the last updates I've seen on topic/core-for-CI were not from maintainers and looking at the branch I don't see any issue with the recent commits. The issue actually seems to be the very old ones. I'm not sure such a measure will actually fix the problem. I myself pushed recently to topic/core-for-CI so I want to know if **I** caused any issue. > >2. Require maintainer/committer ack for adding/removing commits. No > single individual should decide. s@maintainers/committer @@? Or just let it have the same requirement as the drm-intel-* branches. It seems odd to raise the bar for topic/core-for-CI above the requirement for drm-intel-* branches (even though that latter is a r-b). From committer-drm-intel.rst: * Reviewed-by/Acked-by/Tested-by must include the name and email of a real person for transparency. Anyone can give these, and therefore you have to value them according to the merits of the person. Quality matters, not quantity. Be suspicious of rubber stamps. * Reviewed-by/Acked-by/Tested-by can be asked for and given informally (on the list, IRC, in person, in a meeting) but must be added to the commit. * Reviewed-by. All patches must be reviewed, no exceptions. Please see "Reviewer's statement of oversight" in `Documentation/process/submitting-patches <https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_ and `review training <http://blog.ffwll.ch/2014/08/review-training-slides.html>`_. > >3. Require gitlab issues for new commits added. Improve tracking for > removing the commits. > >Also use the stronger "must" for commit message requiring the >justification for the commit being in topic/core-for-CI. > >Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >Cc: David Airlie <airlied@gmail.com> >Cc: Daniel Vetter <daniel@ffwll.ch> >Cc: intel-gfx@lists.freedesktop.org >Cc: dri-devel@lists.freedesktop.org >Cc: dim-tools@lists.freedesktop.org >Signed-off-by: Jani Nikula <jani.nikula@intel.com> >--- > drm-tip.rst | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > >diff --git a/drm-tip.rst b/drm-tip.rst >index deae95cdd2fe..24036e2ef576 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:: there is a main issue here that is not being fixed: testing the merged branch. I think it would be much better to have the instruction here to rebuild drm-tip without pushing... This will use the local topic branch: dim -d rebuild-tip topic/core-for-CI It's the only way I ever update it because I don't want to push a branch and have a small window to potentially solve the merge conflicts (while leaving others wondering why the tip is broken). >@@ -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. > >-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 >+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 must 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.) >+ > 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. wouldn't it be better to apply the revert and only drop the commit on next rebase? This way it doesn't require the force push and it's easier to see what was done in the branch since we don't have to procure the right CI tag in which things got changed. ... I actually came here to ask: wasn't gitlab supposed to be used for patches in maintainer-tools? Lucas De Marchi
On Tue, 22 Nov 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Tue, Nov 22, 2022 at 03:17:14PM +0200, Jani Nikula wrote: >>Introduce stricter rules for topic/core-for-CI management. Way too many >>commits have been added over the years, with insufficient rationale >>recorded in the commit message, and insufficient follow-up with removing >>the commits from the topic branch. >> >>New rules: > > Why not make a list like this the actual text? It's easier to follow a > bullet/numbered list than the free form text. Gah, you want me to rewrite the text and get all acks again?! > >> >>1. Require maintainer ack for rebase. Have better gating on when rebases >> happen and on which baselines. > > What maintainer? drm-intel-gt-next/drm-intel-next/drm-misc/drm? Any? Basically any drm-intel maintainer. The branch is in drm-intel repo, and it exists at all to hotfix CI stuff like the name says. > > I don't want fingers pointed, but just to know the context: was there > any event recently that triggered this? Because the last updates I've > seen on topic/core-for-CI were not from maintainers and > looking at the branch I don't see any issue with the recent commits. > The issue actually seems to be the very old ones. I'm not sure such > a measure will actually fix the problem. > > I myself pushed recently to topic/core-for-CI so I want to know if **I** > caused any issue. This is not related to any individual commit or developer, at all. I've been meaning to do this for a very long time now. > >> >>2. Require maintainer/committer ack for adding/removing commits. No >> single individual should decide. > > s@maintainers/committer @@? Or just let it have the same requirement as > the drm-intel-* branches. It seems odd to raise the bar for > topic/core-for-CI above the requirement for drm-intel-* branches (even > though that latter is a r-b). From committer-drm-intel.rst: The bar *should* be raised for topic/core-for-CI. Yes, it's for hotfixes that can be added as well as removed, but it should never be the first option. Or the "can we just put it in core-for-CI" option. I *want* pushback to adding stuff there. > > * Reviewed-by/Acked-by/Tested-by must include the name and email of a real > person for transparency. Anyone can give these, and therefore you have to > value them according to the merits of the person. Quality matters, not > quantity. Be suspicious of rubber stamps. > > * Reviewed-by/Acked-by/Tested-by can be asked for and given informally (on the > list, IRC, in person, in a meeting) but must be added to the commit. > > * Reviewed-by. All patches must be reviewed, no exceptions. Please see > "Reviewer's statement of oversight" in `Documentation/process/submitting-patches > <https://01.org/linuxgraphics/gfx-docs/drm/process/submitting-patches.html>`_ > and `review training > <http://blog.ffwll.ch/2014/08/review-training-slides.html>`_. > >> >>3. Require gitlab issues for new commits added. Improve tracking for >> removing the commits. >> >>Also use the stronger "must" for commit message requiring the >>justification for the commit being in topic/core-for-CI. >> >>Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >>Cc: David Airlie <airlied@gmail.com> >>Cc: Daniel Vetter <daniel@ffwll.ch> >>Cc: intel-gfx@lists.freedesktop.org >>Cc: dri-devel@lists.freedesktop.org >>Cc: dim-tools@lists.freedesktop.org >>Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>--- >> drm-tip.rst | 27 ++++++++++++++++++++------- >> 1 file changed, 20 insertions(+), 7 deletions(-) >> >>diff --git a/drm-tip.rst b/drm-tip.rst >>index deae95cdd2fe..24036e2ef576 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:: > > there is a main issue here that is not being fixed: testing the merged > branch. I think it would be much better to have the instruction here > to rebuild drm-tip without pushing... This will use the local topic branch: > > dim -d rebuild-tip topic/core-for-CI > > It's the only way I ever update it because I don't want to push a branch > and have a small window to potentially solve the merge conflicts (while > leaving others wondering why the tip is broken). This isn't strictly related to core-for-CI, is it? Can happen with any branch. > >>@@ -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. >> >>-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 >>+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 must 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.) >>+ >> 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. > > wouldn't it be better to apply the revert and only drop the commit on > next rebase? This way it doesn't require the force push and it's easier > to see what was done in the branch since we don't have to procure the > right CI tag in which things got changed. Mmh, I guess revert could be left in as an option. > ... I actually came here to ask: wasn't gitlab supposed to be used for > patches in maintainer-tools? Was it? That's not what CONTRIBUTING.rst says. There's been talk about it, but no decisions to do so. And in any case I wanted more attention to this than gitlab pull requests would ever get. BR, Jani.
diff --git a/drm-tip.rst b/drm-tip.rst index deae95cdd2fe..24036e2ef576 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. -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 +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 must 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.) + 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.
Introduce stricter rules for topic/core-for-CI management. Way too many commits have been added over the years, with insufficient rationale recorded in the commit message, and insufficient follow-up with removing the commits from the topic branch. New rules: 1. Require maintainer ack for rebase. Have better gating on when rebases happen and on which baselines. 2. Require maintainer/committer ack for adding/removing commits. No single individual should decide. 3. Require gitlab issues for new commits added. Improve tracking for removing the commits. Also use the stronger "must" for commit message requiring the justification for the commit being in topic/core-for-CI. Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: David Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Cc: dim-tools@lists.freedesktop.org Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drm-tip.rst | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)