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

[Jani Nikula]

On Thu, 03 Mar 2016, Imre Deak <imre.deak at intel.com> wrote:
> On Thu, 2016-03-03 at 10:11 +0200, Jani Nikula wrote:
>> On Wed, 02 Mar 2016, Imre Deak <imre.deak at 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 at 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.

I don't think that will fly either. You need to run this against a bunch
of commits in the tree now, and see what it says.

>
>> > +                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: .\+ <.\+ at .\+>"
>> 
>> 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.

Alternatively, you can check that the patch has either 1) two
signed-off-bys, or 2) one reviewed-by and one signed-off-by.

>
>> > +        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?

I think I'd like to have this in dim_checkpatch, and condition people to
run that before they push anything.

BR,
Jani.



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

-- 
Jani Nikula, Intel Open Source Technology Center