Re: [maintainer-tools PATCH] dim: Check for required tags before pushing a branch

[Imre Deak <imre.deak@intel.com>]

On Thu, 2016-03-03 at 10:11 +0200, Jani Nikula wrote:
> On Wed, 02 Mar 2016, Imre Deak <imre.deak@intel.com> wrote:
> > Check if the committer's and author's Signed-off-by line and at
> > least
> > one Reviewed-by line exists in each commit to be pushed.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  dim | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/dim b/dim
> > index 1addd6f..b951fb4 100755
> > --- a/dim
> > +++ b/dim
> > @@ -361,6 +361,37 @@ function dim_nightly_forget
> >  	git rerere forget
> >  }
> >  
> > +function assert_one_commit_tag
> > +{
> > +        local commit_message="$1"
> > +        local tag="$2"
> > +
> > +        if ! echo "$commit_message" | grep -q "^$tag"; then
> 
> I think you'll find this is too strict. There may be subtle
> differences
> in the author (which comes from the patch email From: header) and
> signed-off-by lines.

How about requiring that at least the name part matches? We have a few
existing cases where it doesn't due to the missing "name" part in
GIT_AUTHOR_EMAIL, but imo that's just incorrect setup on the author's
side.

> > +                echo "Tag '$tag' missing from $commit"
> > +                return 1
> > +        fi
> > +
> > +}
> > +
> > +function assert_all_commit_tags
> > +{
> > +        local branch=$1
> > +        local new_commits=$(git rev-list
> > $DIM_DRM_INTEL_REMOTE/$branch..$branch)
> > +
> > +        local commit
> > +        for commit in $new_commits; do
> > +                local commit_message=$(git show -s --format=%B
> > $commit)
> > +                local committer_email=$(git show -s --format="%cn
> > <%ce>" $commit)
> > +                local author_email=$(git show -s --format="%an
> > <%ae>" $commit)
> > +
> > +                assert_one_commit_tag "$commit_message" "Signed-
> > off-by: $author_email"
> > +                assert_one_commit_tag "$commit_message" "Signed-
> > off-by: $committer_email"
> > +                assert_one_commit_tag "$commit_message" "Reviewed-
> > by: .\+ <.\+@.\+>"
> 
> If you write a patch and I review and push it, I won't add an extra
> reviewed-by, just a signed-off-by.

To me it's clearer to state explicitly that you reviewed it, but if
people don't do that in general then I'll remove this check.

> > +        done
> > +        return 0
> > +}
> > +
> >  # push branch $1, rebuild nightly. the rest of the arguments are
> > passed to git
> >  # push.
> >  function dim_push_branch
> > @@ -374,6 +405,7 @@ function dim_push_branch
> >  	shift
> >  
> >  	assert_branch $branch
> > +	assert_all_commit_tags $branch
> 
> This should be added to the checkpatch part instead of push. First,
> we
> need to push a lot of non-i915 stuff when rebasing fixes or pushing
> backmerges. Second, it is useful to be able to non-intrusively check
> this before pushing.

Ok, I can move it to dim_checkpatch.

But I also wanted to actually prevent the push without proper commit
tags. So how about a customizable DIM_PRE_PUSH_CHECK that would be
called from dim_push_branch?

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx