[PATCH 1/5] dim: check all commits in dim apply-pull and push-branch

[Daniel Vetter]

On Fri, May 4, 2018 at 4:06 PM, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> On Fri, 04 May 2018, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
>> When merging a pull requests there's potentially a long list of
>> problematic patches. But currently we only report the first issue and
>> then bail out.
>>
>> The upside here is that without this patch the workflow when
>> processing a pull is:
>>
>> $ dim apply-pull ...
>> -> dim refuses, only reports first problem
>> $ dim -d apply-pull
>> -> you see all the issues, make up your mind to merge or not
>> $ dim -f apply-pull
>>
>> With this patch we have one step less:
>>
>> $ dim apply-pull
>> -> refuses pull, but prints all the issues
>> $ dim -f apply-pull
>>
>> On the implementation:
>> - new helper function to check all the patches, that's the only place
>>   we now call warn_or_fail
>> - rework the check function to check for everything and return the
>>   final verdict
>>
>> v2: Complete rework on Jani's suggestion.
>>
>> Acked-by: Dave Airlie <airlied at redhat.com>
>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> Cc: Dave Airlie <airlied at gmail.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>> ---
>>  dim | 46 +++++++++++++++++++++++++++++-----------------
>>  1 file changed, 29 insertions(+), 17 deletions(-)
>>
>> diff --git a/dim b/dim
>> index d8288a342352..d2eb4a0cb6e2 100755
>> --- a/dim
>> +++ b/dim
>> @@ -734,10 +734,11 @@ function dim_rebuild_tip
>>  # additional patch checks before pushing, e.g. for r-b tags
>>  function checkpatch_commit_push
>>  {
>> -     local sha1 non_dim
>> +     local sha1 non_dim rv
>>
>>       sha1=$1
>>       managed_branch=${2:-1}
>> +     rv=0
>>
>>       # use real names for people with many different email addresses
>>       author=$(git show -s $sha1 --format="format:%an")
>> @@ -747,30 +748,45 @@ function checkpatch_commit_push
>>
>>       # check for author sign-off
>>       if ! git show -s $sha1 | grep -qi "S.*-by:.*\\($author\\|$author_outlook\\)" ; then
>> -             warn_or_fail "$sha1 is lacking author of sign-off"
>> +             echoerr "$sha1 is lacking author of sign-off"
>> +             rv=1
>>       fi
>>
>>       # check for committer sign-off
>>       if ! git show -s $sha1 | grep -qi "S.*-by:.*$committer"  ; then
>> -             warn_or_fail "$sha1 is lacking committer of sign-off"
>> +             echoerr "$sha1 is lacking committer of sign-off"
>> +             rv=1
>>       fi
>>
>>       # check for Link tag
>>       if [[ "$managed_branch" = "1" ]] && ! git show -s $sha1 | grep -qi 'Link:'  ; then
>> -             warn_or_fail "$sha1 is lacking of link tag"
>> +             echoerr "$sha1 is lacking of link tag"
>> +             rv=1
>>       fi
>>
>>       # check for a-b/r-b tag
>> -     if git show -s $sha1 | grep -qi '\(reviewed\|acked\)\S*-by:'  ; then
>> -             return
>> +     if ! git show -s $sha1 | grep -qi '\(reviewed\|acked\)\S*-by:' && \
>> +        ! [[ "$committer" != "$author" ]]; then
>> +             echoerr "$sha1 is lacking mandatory review"
>> +             rv=1
>>       fi
>>
>> -     # check for committer != author
>> -     if [[ "$committer" != "$author" ]]; then
>> -             return
>> -     fi
>> +     return $rv
>> +}
>>
>> -     warn_or_fail "$sha1 is lacking mandatory review"
>> +function checkpatch_commit_push_range
>> +{
>> +     local rv
>> +
>> +     rv=0
>> +
>> +     for sha1 in $(git rev-list $@ --no-merges) ; do
>
> I guess I'd pass --no-merges as params too, and maybe that should be
> "$@" to be pedantically correct.

Our current commit check function can't handle merge commits, that's
why I decided to not include it. Best to change that when we figure
out what we want to check with merge commits.

But shellcheck also noticed the $@, I'll fix that when pushing.
-Daniel

> But I guess this is good enough for starters,
>
> Reviewed-by: Jani Nikula <jani.nikula at intel.com>
>
>
>> +             checkpatch_commit_push $sha1 || rv=1
>> +     done
>> +
>> +     if [ $rv == "1" ] ; then
>> +             warn_or_fail "issues in commits detected"
>> +     fi
>>  }
>>
>>  # push branch $1, rebuild drm-tip. the rest of the arguments are passed to git
>> @@ -788,9 +804,7 @@ function dim_push_branch
>>
>>       committer_email=$(git_committer_email)
>>
>> -     for sha1 in $(git rev-list "$branch@{u}..$branch" --committer="$committer_email" --no-merges) ; do
>> -             checkpatch_commit_push $sha1
>> -     done
>> +     checkpatch_commit_push_range "$branch@{u}..$branch" --committer="$committer_email"
>>
>>       git push $DRY_RUN $remote $branch "$@"
>>
>> @@ -909,9 +923,7 @@ function dim_apply_pull
>>       echo Pulling $pull_branch ...
>>
>>       git fetch $pull_branch
>> -     for sha1 in $(git rev-list "HEAD..FETCH_HEAD" --no-merges) ; do
>> -             checkpatch_commit_push $sha1 0
>> -     done
>> +     checkpatch_commit_push_range "HEAD..FETCH_HEAD"
>>
>>       $DRY git pull $pull_branch
>
> --
> Jani Nikula, Intel Open Source Technology Center



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch