Re: [PATCH] dim: Enforce review requirements

[Lukas Wunner <lukas@wunner.de>]

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