On Fri, May 4, 2018 at 4:06 PM, Jani Nikula <jani.nik...@linux.intel.com> wrote: > On Fri, 04 May 2018, Daniel Vetter <daniel.vet...@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 <airl...@redhat.com> >> Cc: Jani Nikula <jani.nik...@linux.intel.com> >> Cc: Dave Airlie <airl...@gmail.com> >> Signed-off-by: Daniel Vetter <daniel.vet...@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.nik...@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 _______________________________________________ dim-tools mailing list dim-tools@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dim-tools