On Thu, May 24, 2018 at 02:50:52PM +0200, Daniel Vetter wrote: > On Thu, May 24, 2018 at 11:18 AM, Jani Nikula > <[email protected]> wrote: > > On Thu, 24 May 2018, Arkadiusz Hiler <[email protected]> wrote: > >> This subcommand runs 'make W=1' on the provided range. > >> > >> Signed-off-by: Arkadiusz Hiler <[email protected]> > >> --- > >> 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 <[email protected]> > 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 _______________________________________________ dim-tools mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dim-tools
