[RFC] dim: Add warncheck subcommand

[Arkadiusz Hiler]

On Thu, May 24, 2018 at 02:50:52PM +0200, Daniel Vetter wrote:
> 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

The issue with simillar approach is that `make W=1` stops on the first
error. If the reported issue was not fixed by the following patch each
one will stop at the same point. This would lead to lengthy and
repetitive log which also is not very helpful to the user.

I admit that I have designed this with CI usage in mind, but for
maintainers your suggestion makes some sense. If you really think that
this early exit is bad and you are okay with extra possible noise then
let it be. Would you be okay with having a switch for early bail-out
just for the CI use?

-- 
Cheers,
Arek