[PATCH 2/2] dim: Make dim sparse check each commit in the range-ish

[Daniel Vetter]

On Tue, Nov 28, 2017 at 02:25:58PM +0200, Arkadiusz Hiler wrote:
> On Mon, Nov 27, 2017 at 04:39:57PM +0200, Arkadiusz Hiler wrote:
> > Previously 'dim sparse' just touched all the files affected by the range
> > and run sparse against that. Since this may show issues from included
> > headers and sparse-compliance does not seem to be strongly enforced the
> > output is rather unhelpful.
> > 
> > With this change the repo is checkout at each commit, sparse is run on
> > the whole drivers/gpu/drm/ directory for each step.
> > 
> > Each result is compared to it's parent's result using remap-log to
> > adjust for potential line numbering changes.
> > 
> > Only the difference is reported.
> > 
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> > ---
> >  dim | 40 ++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 36 insertions(+), 4 deletions(-)
> > 
> > diff --git a/dim b/dim
> > index ee958be231ab..c586883363e8 100755
> > --- a/dim
> > +++ b/dim
> > @@ -1398,6 +1398,13 @@ function checkpatch_commit
> >  	return $rv
> >  }
> >  
> > +# $1 is the git sha1 to check
> > +function sparse_commit
> > +{
> > +    git checkout --detach $1 >/dev/null 2>&1
> > +    make C=2 -j$(nproc) drivers/gpu/drm/ 2>&1 1>/dev/null | sort
> > +}
> 
> <ivyl> danvet: also do we want to have the sparse checks just for i915.ko,
>        whole i915/ or whole drm/ ?
> <danvet> ivyl, all that's changed?
> <danvet> for dim sparse $commit-ish
> <danvet> well $commit|range-ish
> 
> The sequence would look something like that:
> 
> ------------------------------------------------------------
> git checkout ${commits[0]}~
> make
> 
> for commit in commits:
>     # get reference from parent just for the changed files
>     touch --no-create $(git diff --name-only $commit)
>     prev_sr="$(make C=1)"
> 
>     # now get the state after the commit is applied
>     git checkout $commit
>     sr="$(make C=1)"
> 
>     # and compare
>     prev_remapped="$(remap_log ... prev_sr)"
>     diff <(echo $prev_remapped) <(echo $sr)
> ------------------------------------------------------------
> 
> For N patches that means 2*N spare runs, while the current
> implementation (seen below) is N+1 but checks the whole drm dir.
> 
> It does not look that bad, but I have to check how much longer does it
> take.
> 
> BTW, excerpt from the kernel's Docs:
> Do a kernel make with "make C=1" to run sparse on all the C files that
> get recompiled, or use "make C=2" to run sparse on the files whether
> they need to be recompiled or not.  The latter is a fast way to check
> the whole tree if you have already built it.

Can we do -j $cpu_threads with these too? Maybe even semi-automatically?

That should more than compensate for the 2x factor.

> >  # turn $1 in to a git commit range
> >  function rangeish()
> >  {
> > @@ -1470,13 +1477,38 @@ function dim_checkpatch
> >  
> >  function dim_sparse
> >  {
> > -	local range
> > +	local range rv sr prev_sr prev_remapped diff_result remap_log
> >  
> >  	range=$(rangeish "${1:-}")
> > +    remap_log=$DIM_PREFIX/maintainer-tools/remap-log
> >  
> > -	make $DIM_MAKE_OPTIONS
> > -	touch --no-create $(git diff --name-only $range) $(git diff --name-only)
> > -	make C=1
> > +    if [ ! -x  $remap_log ]; then
> > +        echo "$remap_log is not an executable, please run make in maintainer-tools dir!"

Just noticed: git tracks executable bits, this shouldn't be necessary.

> > +        exit 1
> > +    fi
> 
> https://github.com/zsh-users/zsh/blob/master/Functions/VCS_Info/Backends/VCS_INFO_get_data_git#L73
> We can use something like that to get the current ref here (whether it
> is a branch or detached head or anything else).
> 
> trap "git checkout $original_ref" 0
> 
> to restore the HEAD to it's original state after we are done here.

Yeah restoring should be good here, just in case. Or we do all the build
testing in a separate worktree (but that means clean build, ugh).
-Daniel

> 
> > +	for commit in $(git rev-list --reverse $range); do
> > +        if [ -z "$prev_sr" ]; then prev_sr="$(sparse_commit $commit~)"; fi
> > +        sr="$(sparse_commit $commit)"
> > +        prev_remapped="$(echo "$prev_sr" | $remap_log <(git diff HEAD~ | $remap_log))"
> > +        diff_result="$(diff -u <(echo "$prev_remapped") <(echo "$sr") || true)"
> > +
> > +        echo "Commit: $(git log -n1 --format='%s' $commit)"
> > +        if [ -n "$diff_result" ]; then
> > +            echo "$diff_result" | egrep '^[+-]' | egrep -v '^[+-]{3}'
> > +        else
> > +            echo "Okay!"
> > +        fi
> > +        echo
> > +
> > +        if (echo "$diff_result" | grep -q '^+'); then
> > +            rv=1
> > +        fi
> > +
> > +        prev_sr="$sr"
> > +	done
> > +
> > +	return $rv
> >  }
> >  
> >  function dim_checker
> > -- 
> > 2.13.6
> > 
> > _______________________________________________
> > dim-tools mailing list
> > dim-tools at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dim-tools
> _______________________________________________
> dim-tools mailing list
> dim-tools at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dim-tools

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch