* Re: [PATCH] dim: Enforce review requirements
2017-05-24 9:28 [PATCH] dim: Enforce review requirements Daniel Vetter
@ 2017-05-24 9:42 ` Lukas Wunner
2017-05-24 12:59 ` Deucher, Alexander
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Lukas Wunner @ 2017-05-24 9:42 UTC (permalink / raw)
To: Daniel Vetter
Cc: Intel Graphics Development, DRI Development, Alex Deucher,
Daniel Vetter
On Wed, May 24, 2017 at 11:28:12AM +0200, Daniel Vetter wrote:
> We can't check this when applying (since r-b/a-b tags often get added
> afterwards), but we can check this when pushing. This only looks at
> patches authored by the pusher.
>
> Also update the docs to highlight that review requirements hold
> especially also for bugfixes.
>
> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <deathsimple@vodafone.de>
> Cc: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
Thanks,
Lukas
> ---
> dim | 42 ++++++++++++++++++++++++++++++++++++++----
> drm-misc.rst | 4 +++-
> 2 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/dim b/dim
> index baa0b3832314..79738a1b37a0 100755
> --- a/dim
> +++ b/dim
> @@ -340,6 +340,15 @@ function git_branch_exists # branch
> fi
> }
>
> +function git_committer_email
> +{
> + if ! commiter_email=$(git config --get user.email) ; then
> + commiter_email=$EMAIL
> + fi
> +
> + echo $commiter_email
> +}
> +
> # get message id from file
> # $1 = file
> message_get_id ()
> @@ -632,11 +641,32 @@ function dim_rebuild_tip
> exit 1
> fi
> }
> +
> +# additional patch checks before pushing, e.g. for r-b tags
> +function checkpatch_commit_push
> +{
> + local sha1
> +
> + sha1=$1
> +
> + # check for a-b/r-b tag
> + if git show -s $sha1 | grep -qi '\(reviewed\|acked\)\S*-by:' ; then
> + return
> + fi
> +
> + # check for commiter != author
> + if [[ $(git show -s $sha1 --format="format:%ce") != $(git show -s $sha1 --format="format:%ae") ]]; then
> + return
> + fi
> +
> + warn_or_fail "$sha1 is lacking mandatory review"
> +}
> +
> # push branch $1, rebuild drm-tip. the rest of the arguments are passed to git
> # push.
> function dim_push_branch
> {
> - local branch remote
> + local branch remote commiter_email
>
> branch=${1:?$usage}
> shift
> @@ -645,6 +675,12 @@ function dim_push_branch
>
> remote=$(branch_to_remote $branch)
>
> + commiter_email=$(git_committer_email)
> +
> + for sha1 in $(git rev-list $branch@{u}..$branch --committer="$commiter_email" --no-merges) ; do
> + checkpatch_commit_push $sha1
> + done
> +
> git push $DRY_RUN $remote $branch "$@"
>
> update_linux_next $branch drm-intel-next-queued drm-intel-next-fixes drm-intel-fixes
> @@ -690,9 +726,7 @@ function dim_apply_branch
>
> message_id=$(message_get_id $file)
>
> - if ! commiter_email=$(git config --get user.email) ; then
> - commiter_email=$EMAIL
> - fi
> + commiter_email=$(git_committer_email)
>
> patch_from=$(grep "From:" "$file" | head -1)
> if [[ "$patch_from" != *"$commiter_email"* ]] ; then
> diff --git a/drm-misc.rst b/drm-misc.rst
> index caba8dc77696..d56c3c7f92a3 100644
> --- a/drm-misc.rst
> +++ b/drm-misc.rst
> @@ -90,7 +90,9 @@ Merge Criteria
> Right now the only hard merge criteria are:
>
> * Patch is properly reviewed or at least Ack, i.e. don't just push your own
> - stuff directly.
> + stuff directly. This rule holds even more for bugfix patches - it would be
> + embarrassing if the bugfix contains a small gotcha that review would have
> + caught.
>
> * drm-misc is for drm core (non-driver) patches, subsystem-wide refactorings,
> and small trivial patches all over (including drivers). For a detailed list of
> --
> 2.11.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH] dim: Enforce review requirements
2017-05-24 9:28 [PATCH] dim: Enforce review requirements Daniel Vetter
2017-05-24 9:42 ` Lukas Wunner
@ 2017-05-24 12:59 ` Deucher, Alexander
2017-05-25 5:37 ` Lukas Wunner
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Deucher, Alexander @ 2017-05-24 12:59 UTC (permalink / raw)
To: 'Daniel Vetter', DRI Development
Cc: Intel Graphics Development, Daniel Vetter
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch]
> Sent: Wednesday, May 24, 2017 5:28 AM
> To: DRI Development
> Cc: Intel Graphics Development; Daniel Vetter; Patrik Jakobsson; Lukas
> Wunner; Deucher, Alexander; Christian König; Sean Paul; Daniel Vetter
> Subject: [PATCH] dim: Enforce review requirements
>
> We can't check this when applying (since r-b/a-b tags often get added
> afterwards), but we can check this when pushing. This only looks at
> patches authored by the pusher.
>
> Also update the docs to highlight that review requirements hold
> especially also for bugfixes.
>
> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <deathsimple@vodafone.de>
> Cc: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Acked-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> dim | 42 ++++++++++++++++++++++++++++++++++++++----
> drm-misc.rst | 4 +++-
> 2 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/dim b/dim
> index baa0b3832314..79738a1b37a0 100755
> --- a/dim
> +++ b/dim
> @@ -340,6 +340,15 @@ function git_branch_exists # branch
> fi
> }
>
> +function git_committer_email
> +{
> + if ! commiter_email=$(git config --get user.email) ; then
> + commiter_email=$EMAIL
> + fi
> +
> + echo $commiter_email
> +}
> +
> # get message id from file
> # $1 = file
> message_get_id ()
> @@ -632,11 +641,32 @@ function dim_rebuild_tip
> exit 1
> fi
> }
> +
> +# additional patch checks before pushing, e.g. for r-b tags
> +function checkpatch_commit_push
> +{
> + local sha1
> +
> + sha1=$1
> +
> + # check for a-b/r-b tag
> + if git show -s $sha1 | grep -qi '\(reviewed\|acked\)\S*-by:' ; then
> + return
> + fi
> +
> + # check for commiter != author
> + if [[ $(git show -s $sha1 --format="format:%ce") != $(git show -s
> $sha1 --format="format:%ae") ]]; then
> + return
> + fi
> +
> + warn_or_fail "$sha1 is lacking mandatory review"
> +}
> +
> # push branch $1, rebuild drm-tip. the rest of the arguments are passed to
> git
> # push.
> function dim_push_branch
> {
> - local branch remote
> + local branch remote commiter_email
>
> branch=${1:?$usage}
> shift
> @@ -645,6 +675,12 @@ function dim_push_branch
>
> remote=$(branch_to_remote $branch)
>
> + commiter_email=$(git_committer_email)
> +
> + for sha1 in $(git rev-list $branch@{u}..$branch --
> committer="$commiter_email" --no-merges) ; do
> + checkpatch_commit_push $sha1
> + done
> +
> git push $DRY_RUN $remote $branch "$@"
>
> update_linux_next $branch drm-intel-next-queued drm-intel-next-
> fixes drm-intel-fixes
> @@ -690,9 +726,7 @@ function dim_apply_branch
>
> message_id=$(message_get_id $file)
>
> - if ! commiter_email=$(git config --get user.email) ; then
> - commiter_email=$EMAIL
> - fi
> + commiter_email=$(git_committer_email)
>
> patch_from=$(grep "From:" "$file" | head -1)
> if [[ "$patch_from" != *"$commiter_email"* ]] ; then
> diff --git a/drm-misc.rst b/drm-misc.rst
> index caba8dc77696..d56c3c7f92a3 100644
> --- a/drm-misc.rst
> +++ b/drm-misc.rst
> @@ -90,7 +90,9 @@ Merge Criteria
> Right now the only hard merge criteria are:
>
> * Patch is properly reviewed or at least Ack, i.e. don't just push your own
> - stuff directly.
> + stuff directly. This rule holds even more for bugfix patches - it would be
> + embarrassing if the bugfix contains a small gotcha that review would have
> + caught.
>
> * drm-misc is for drm core (non-driver) patches, subsystem-wide
> refactorings,
> and small trivial patches all over (including drivers). For a detailed list of
> --
> 2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] dim: Enforce review requirements
2017-05-24 9:28 [PATCH] dim: Enforce review requirements Daniel Vetter
2017-05-24 9:42 ` Lukas Wunner
2017-05-24 12:59 ` Deucher, Alexander
@ 2017-05-25 5:37 ` Lukas Wunner
2017-05-26 6:40 ` Daniel Vetter
2017-05-26 13:51 ` Sean Paul
2017-05-30 7:25 ` Neil Armstrong
4 siblings, 1 reply; 7+ messages in thread
From: Lukas Wunner @ 2017-05-25 5:37 UTC (permalink / raw)
To: Daniel Vetter
Cc: Intel Graphics Development, Christian König, DRI Development,
Alex Deucher, Daniel Vetter
On Wed, May 24, 2017 at 11:28:12AM +0200, Daniel Vetter wrote:
> We can't check this when applying (since r-b/a-b tags often get added
> afterwards), but we can check this when pushing. This only looks at
> patches authored by the pusher.
BTW, can we also have a rule that large drivers (i.e. with a large user
base) are required to provide at least one person at all times (or at
least outside the merge window) who is able to review fixes, push them
to the driver's repo and send a timely pull to Dave?
On multiple occasions I've seen Dave send a fixes pull with an apology
along the lines of "a bit more than usual at this time of the cycle
because it includes two weeks worth of driver XYZ fixes".
Thanks,
Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] dim: Enforce review requirements
2017-05-25 5:37 ` Lukas Wunner
@ 2017-05-26 6:40 ` Daniel Vetter
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2017-05-26 6:40 UTC (permalink / raw)
To: Lukas Wunner
Cc: Intel Graphics Development, Christian König, DRI Development,
Alex Deucher, Daniel Vetter
On Thu, May 25, 2017 at 7:37 AM, Lukas Wunner <lukas@wunner.de> wrote:
> On Wed, May 24, 2017 at 11:28:12AM +0200, Daniel Vetter wrote:
>> We can't check this when applying (since r-b/a-b tags often get added
>> afterwards), but we can check this when pushing. This only looks at
>> patches authored by the pusher.
>
> BTW, can we also have a rule that large drivers (i.e. with a large user
> base) are required to provide at least one person at all times (or at
> least outside the merge window) who is able to review fixes, push them
> to the driver's repo and send a timely pull to Dave?
>
> On multiple occasions I've seen Dave send a fixes pull with an apology
> along the lines of "a bit more than usual at this time of the cycle
> because it includes two weeks worth of driver XYZ fixes".
AMD is already planning to swtich to a group/committer model, I just
discussed that with Alex. That takes care of the huge drivers
(intel/amd) and the small ones (if they are in drm-misc). No idea what
we'll end up doing with the medium sized drivers, but I guess that's a
discussion for the next months/years ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] dim: Enforce review requirements
2017-05-24 9:28 [PATCH] dim: Enforce review requirements Daniel Vetter
` (2 preceding siblings ...)
2017-05-25 5:37 ` Lukas Wunner
@ 2017-05-26 13:51 ` Sean Paul
2017-05-30 7:25 ` Neil Armstrong
4 siblings, 0 replies; 7+ messages in thread
From: Sean Paul @ 2017-05-26 13:51 UTC (permalink / raw)
To: Daniel Vetter
Cc: Intel Graphics Development, DRI Development, Alex Deucher,
Daniel Vetter
On Wed, May 24, 2017 at 11:28:12AM +0200, Daniel Vetter wrote:
> We can't check this when applying (since r-b/a-b tags often get added
> afterwards), but we can check this when pushing. This only looks at
> patches authored by the pusher.
>
> Also update the docs to highlight that review requirements hold
> especially also for bugfixes.
>
> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <deathsimple@vodafone.de>
> Cc: Sean Paul <seanpaul@chromium.org>
I think I acked this on IRC, but just in case,
Acked-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> dim | 42 ++++++++++++++++++++++++++++++++++++++----
> drm-misc.rst | 4 +++-
> 2 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/dim b/dim
> index baa0b3832314..79738a1b37a0 100755
> --- a/dim
> +++ b/dim
> @@ -340,6 +340,15 @@ function git_branch_exists # branch
> fi
> }
>
> +function git_committer_email
> +{
> + if ! commiter_email=$(git config --get user.email) ; then
> + commiter_email=$EMAIL
> + fi
> +
> + echo $commiter_email
> +}
> +
> # get message id from file
> # $1 = file
> message_get_id ()
> @@ -632,11 +641,32 @@ function dim_rebuild_tip
> exit 1
> fi
> }
> +
> +# additional patch checks before pushing, e.g. for r-b tags
> +function checkpatch_commit_push
> +{
> + local sha1
> +
> + sha1=$1
> +
> + # check for a-b/r-b tag
> + if git show -s $sha1 | grep -qi '\(reviewed\|acked\)\S*-by:' ; then
> + return
> + fi
> +
> + # check for commiter != author
> + if [[ $(git show -s $sha1 --format="format:%ce") != $(git show -s $sha1 --format="format:%ae") ]]; then
> + return
> + fi
> +
> + warn_or_fail "$sha1 is lacking mandatory review"
> +}
> +
> # push branch $1, rebuild drm-tip. the rest of the arguments are passed to git
> # push.
> function dim_push_branch
> {
> - local branch remote
> + local branch remote commiter_email
>
> branch=${1:?$usage}
> shift
> @@ -645,6 +675,12 @@ function dim_push_branch
>
> remote=$(branch_to_remote $branch)
>
> + commiter_email=$(git_committer_email)
> +
> + for sha1 in $(git rev-list $branch@{u}..$branch --committer="$commiter_email" --no-merges) ; do
> + checkpatch_commit_push $sha1
> + done
> +
> git push $DRY_RUN $remote $branch "$@"
>
> update_linux_next $branch drm-intel-next-queued drm-intel-next-fixes drm-intel-fixes
> @@ -690,9 +726,7 @@ function dim_apply_branch
>
> message_id=$(message_get_id $file)
>
> - if ! commiter_email=$(git config --get user.email) ; then
> - commiter_email=$EMAIL
> - fi
> + commiter_email=$(git_committer_email)
>
> patch_from=$(grep "From:" "$file" | head -1)
> if [[ "$patch_from" != *"$commiter_email"* ]] ; then
> diff --git a/drm-misc.rst b/drm-misc.rst
> index caba8dc77696..d56c3c7f92a3 100644
> --- a/drm-misc.rst
> +++ b/drm-misc.rst
> @@ -90,7 +90,9 @@ Merge Criteria
> Right now the only hard merge criteria are:
>
> * Patch is properly reviewed or at least Ack, i.e. don't just push your own
> - stuff directly.
> + stuff directly. This rule holds even more for bugfix patches - it would be
> + embarrassing if the bugfix contains a small gotcha that review would have
> + caught.
>
> * drm-misc is for drm core (non-driver) patches, subsystem-wide refactorings,
> and small trivial patches all over (including drivers). For a detailed list of
> --
> 2.11.0
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] dim: Enforce review requirements
2017-05-24 9:28 [PATCH] dim: Enforce review requirements Daniel Vetter
` (3 preceding siblings ...)
2017-05-26 13:51 ` Sean Paul
@ 2017-05-30 7:25 ` Neil Armstrong
4 siblings, 0 replies; 7+ messages in thread
From: Neil Armstrong @ 2017-05-30 7:25 UTC (permalink / raw)
To: Daniel Vetter, DRI Development
Cc: Alex Deucher, Daniel Vetter, Intel Graphics Development
On 05/24/2017 11:28 AM, Daniel Vetter wrote:
> We can't check this when applying (since r-b/a-b tags often get added
> afterwards), but we can check this when pushing. This only looks at
> patches authored by the pusher.
>
> Also update the docs to highlight that review requirements hold
> especially also for bugfixes.
>
> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <deathsimple@vodafone.de>
> Cc: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> dim | 42 ++++++++++++++++++++++++++++++++++++++----
> drm-misc.rst | 4 +++-
> 2 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/dim b/dim
> index baa0b3832314..79738a1b37a0 100755
> --- a/dim
> +++ b/dim
> @@ -340,6 +340,15 @@ function git_branch_exists # branch
> fi
> }
>
> +function git_committer_email
> +{
> + if ! commiter_email=$(git config --get user.email) ; then
> + commiter_email=$EMAIL
> + fi
> +
> + echo $commiter_email
> +}
> +
> # get message id from file
> # $1 = file
> message_get_id ()
> @@ -632,11 +641,32 @@ function dim_rebuild_tip
> exit 1
> fi
> }
> +
> +# additional patch checks before pushing, e.g. for r-b tags
> +function checkpatch_commit_push
> +{
> + local sha1
> +
> + sha1=$1
> +
> + # check for a-b/r-b tag
> + if git show -s $sha1 | grep -qi '\(reviewed\|acked\)\S*-by:' ; then
> + return
> + fi
> +
> + # check for commiter != author
> + if [[ $(git show -s $sha1 --format="format:%ce") != $(git show -s $sha1 --format="format:%ae") ]]; then
> + return
> + fi
> +
> + warn_or_fail "$sha1 is lacking mandatory review"
> +}
> +
> # push branch $1, rebuild drm-tip. the rest of the arguments are passed to git
> # push.
> function dim_push_branch
> {
> - local branch remote
> + local branch remote commiter_email
>
> branch=${1:?$usage}
> shift
> @@ -645,6 +675,12 @@ function dim_push_branch
>
> remote=$(branch_to_remote $branch)
>
> + commiter_email=$(git_committer_email)
> +
> + for sha1 in $(git rev-list $branch@{u}..$branch --committer="$commiter_email" --no-merges) ; do
> + checkpatch_commit_push $sha1
> + done
> +
> git push $DRY_RUN $remote $branch "$@"
>
> update_linux_next $branch drm-intel-next-queued drm-intel-next-fixes drm-intel-fixes
> @@ -690,9 +726,7 @@ function dim_apply_branch
>
> message_id=$(message_get_id $file)
>
> - if ! commiter_email=$(git config --get user.email) ; then
> - commiter_email=$EMAIL
> - fi
> + commiter_email=$(git_committer_email)
>
> patch_from=$(grep "From:" "$file" | head -1)
> if [[ "$patch_from" != *"$commiter_email"* ]] ; then
> diff --git a/drm-misc.rst b/drm-misc.rst
> index caba8dc77696..d56c3c7f92a3 100644
> --- a/drm-misc.rst
> +++ b/drm-misc.rst
> @@ -90,7 +90,9 @@ Merge Criteria
> Right now the only hard merge criteria are:
>
> * Patch is properly reviewed or at least Ack, i.e. don't just push your own
> - stuff directly.
> + stuff directly. This rule holds even more for bugfix patches - it would be
> + embarrassing if the bugfix contains a small gotcha that review would have
> + caught.
>
> * drm-misc is for drm core (non-driver) patches, subsystem-wide refactorings,
> and small trivial patches all over (including drivers). For a detailed list of
>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread