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

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

[Imre Deak]

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
+                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: .\+ <.\+@.\+>"
+        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
 
 	git push $DRY_RUN $DIM_DRM_INTEL_REMOTE $branch "$@"
 
-- 
2.5.0

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

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

[Jani Nikula]

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.

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

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

BR,
Jani.

>  
>  	git push $DRY_RUN $DIM_DRM_INTEL_REMOTE $branch "$@"

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

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

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

[Jani Nikula]

On Thu, 03 Mar 2016, Imre Deak <imre.deak@intel.com> wrote:
> 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.

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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[maintainer-tools PATCH v2] dim: Check for required tags as part of dim_checkpatch

[Imre Deak]

Check in dim_checkpatch if the committer's Signed-off-by line and a
Reviewed-by line exists in the commit message. If no Reviewed-by line
exists also accept two distinct Signed-off-by lines instead.

v2: (Jani)
- move the check from dim_push_branch to dim_checkpatch
- remove the check for the author's Signed-off-by line
- in case there is no Reviewed-by line also accept two distinct
  Signed-off-by lines

CC: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 dim | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/dim b/dim
index 1e7622a..2672fe0 100755
--- a/dim
+++ b/dim
@@ -706,6 +706,33 @@ function checkpatch_commit
 	shell_checkpatch "git show $commit --pretty=email"
 }
 
+function check_commit_tags
+{
+	local commit=$1
+
+	local commit_message=$(git show -s --format=%B $commit)
+	local committer_email=$(git show -s --format="%cn <%ce>" $commit)
+
+	local ret=0
+
+	local committer_sob="Signed-off-by: $committer_email"
+	if [ -z "$(echo "$commit_message" | grep "^$committer_sob")" ]; then
+		echo "Committer's tag missing: '$committer_sob'"
+
+		ret=1
+	fi
+
+	if [ -z "$(echo "$commit_message" | grep "^Reviewed-by:")" ] &&
+	   [ -z "$(echo "$commit_message" | grep "^Signed-off-by:" | \
+		  grep -v "^$committer_sob")" ]; then
+		echo "Reviewer's tag missing: one 'Reviewed-by' or two distinct 'Signed-off-by's"
+
+		ret=1
+	fi
+
+	return $ret
+}
+
 dim_alias_check_patch=checkpatch
 dim_alias_cp=checkpatch
 function dim_checkpatch
@@ -722,6 +749,7 @@ function dim_checkpatch
 
 	for commit in $(git rev-list --reverse $range); do
 		checkpatch_commit $commit || true
+		check_commit_tags $commit || true
 	done
 }
 
-- 
2.5.0

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