[RFC] dim: Add warncheck subcommand

[Daniel Vetter]

On Thu, May 24, 2018 at 11:18 AM, Jani Nikula
<jani.nikula at linux.intel.com> wrote:
> On Thu, 24 May 2018, Arkadiusz Hiler <arkadiusz.hiler at intel.com> wrote:
>> This subcommand runs 'make W=1' on the provided range.
>>
>> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
>> ---
>>  dim     | 38 ++++++++++++++++++++++++++++++++++++++
>>  dim.rst |  7 +++++++
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/dim b/dim
>> index ed26033f5aba..988e83e691c6 100755
>> --- a/dim
>> +++ b/dim
>> @@ -1506,6 +1506,44 @@ function dim_sparse
>>       return $rv
>>  }
>>
>> +function dim_warncheck
>> +{
>> +    _restore_head_on_exit
>
> Please use tabs for indentation throughout.
>
>> +
>> +     local range make_output make_rc commits
>> +
>> +     range=$(rangeish "${1:-}")
>> +    commits=( $(git rev-list --reverse $range) )
>> +
>> +    git checkout --detach  ${commits[0]}~ > /dev/null 2>&1
>> +
>> +    if ! make -j$(nproc) W=1 > /dev/null 2>&1; then
>> +        echo "The base for the provided range does not build cleanly."
>> +        echo "Commit: $(git log -n1 --format='%s' HEAD)"
>> +        echo "Bailing out to cut on the noise."
>> +        return 1
>> +    fi
>> +
>> +     for commit in "${commits[@]}"; do
>
> How about doing
>
>         baseline="${commits[0]}~"
>
>         for commit in $baseline "${commits[@]}"; do
>
> and handling the baseline in the same loop? In the error case, you can
> check if [[ "$commit" = "$baseline" ]] and output a different error
> message.
>
>> +        git checkout --detach $commit >/dev/null 2>&1
>> +
>> +        set +e
>> +        make_output="$(make W=1 2>&1)"
>> +        make_rc=$?
>> +        set -e
>> +
>> +        if [ "$make_rc" -ne 0 ]; then
>> +            echo "Commit: $(git log -n1 --format='%s' $commit)"
>
> We have dim_cite function to pretty format commits.
>
>> +            echo "$make_output"
>
> Please use echoerr for error messages throughout.
>
>> +
>> +            # error will probably propagate, let's cut the noise
>> +            echo
>
> I'm not fond of superfluous whitespace in output.
>
>> +            echo "Quitting early. Please fix and check the other commits as well."
>> +            return 1

This kind of early exit isn't really user friendly. I think you want
at least a warn_or_fail so that users can see everything with -d. But
even better would be if you do the same I've done for the commit
checking when pulling/pushing in

commit c0c4dc1c924bd1d94315601026a2f9facd8a7f73
Author: Daniel Vetter <daniel.vetter at ffwll.ch>
Date:   Wed May 2 15:08:24 2018 +0200

    dim: check all commits in dim apply-pull and push-branch

Cheers, Daniel

>> +        fi
>> +     done
>> +}
>> +
>>  function dim_checker
>>  {
>>       rm -f drivers/gpu/drm/i915/*.o drivers/gpu/drm/i915/*.ko
>> diff --git a/dim.rst b/dim.rst
>> index e2c5fd6e6d0a..c73af992243f 100644
>> --- a/dim.rst
>> +++ b/dim.rst
>> @@ -144,6 +144,13 @@ Run sparse on the files changed by the given commit range.
>>  If no commit-ish is passed, defaults to HEAD^..HEAD. If one commit-ish is passed
>>  instead of a range, the range commit-ish..HEAD is used.
>>
>> +warncheck [*commit-ish* [.. *commit-ish*]]
>> +---------------------------------------
>
> Doesn't make check complain about this? Underline should be the same
> length as the heading.
>
>> +Runs incremental make W=1 on the given commit range.
>> +
>> +If no commit-ish is passed, defaults to HEAD^..HEAD. If one commit-ish is passed
>> +instead of a range, the range commit-ish..HEAD is used.
>> +
>>  checker
>>  -------
>>  Run sparse on drm/i915.
>
> --
> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> dim-tools mailing list
> dim-tools at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dim-tools



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