On Thu, 03 Mar 2016, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> 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.
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: .\+ <.\+@.\+>"
>>
>> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
