On Thu, 2016-03-03 at 10:11 +0200, Jani Nikula wrote:
> On Wed, 02 Mar 2016, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
