On Sun, May 27, 2018 at 09:34:13PM +0200, Thomas Monjalon wrote:
> Hi Neil,
>
> Sorry, this patch has been forgotten during the whole release cycle.
>
Its ok, though that is frustrating, as we now need to re-familiarize ourselves
with how this patch works.
> I see an issue about the dependency on filterdiff.
> Is there a way to avoid it?
>
I suppose, but to do so would require some awk magic. Is there any advantage to
having an awk dependency rather than a filterdiff dependency? filterdiff is a
pretty universal tool.
> I have some minor comments below.
>
> 14/02/2018 20:19, Neil Horman:
> > @@ -61,6 +65,7 @@ print_usage () {
> > END_OF_HELP
> > }
> >
> > +
>
> This new blank line is probably a leftover.
Yeah, if I wind up respinning this, I'll remove it.
>
> > number=0
> > quiet=false
> > verbose=false
> > @@ -86,19 +91,50 @@ total=0
> > status=0
> >
> > check () { # <patch> <commit> <title>
> > + local ret=0
> > + TMPINPUT=`mktemp checkpatches.XXXXXX`
>
> It is preferred to use $() construct to be consistent in the file.
>
> > +
> > total=$(($total + 1))
> > ! $verbose || printf '\n### %s\n\n' "$3"
> > if [ -n "$1" ] ; then
> > report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
> > elif [ -n "$2" ] ; then
> > - report=$(git format-patch --find-renames --no-stat --stdout -1
> > $commit |
> > + git format-patch --find-renames --no-stat --stdout -1 $commit >
> > ./$TMPINPUT
> > + report=$(cat ./$TMPINPUT |
> > $DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
>
> No need to use cat here.
>
Actually, I vaguely recall with these changes I did encounter a problem with
passing
output to checkpatch, but its been so long I can't recall now.
> > else
> > - report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> > + cat > ./$TMPINPUT
> > + report=$(cat ./$TMPINPUT | $DPDK_CHECKPATCH_PATH $options -
> > 2>/dev/null)
> > + fi
>
> All calls to DPDK_CHECKPATCH_PATH can be moved to one call after the "fi".
> Just need to set TPMINPUT=$1 in first case (and not remove TPMINPUT
> in this case).
>
Sure.
> > + if [ $? -ne 0 ]
> > + then
>
> For consistency, the style in this file is:
> if [ $? -ne 0 ] ; then
>
> > + $verbose || printf '\n### %s\n\n' "$3"
> > + printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> > + ret=1
> > + fi
> > +
> > + ! $verbose || printf '\nChecking API additions/removals:\n'
> > +
> > + if [ -n "$1" ] ; then
> > + report=$($VALIDATE_NEW_API "$1")
> > + elif [ -n "$2" ] ; then
> > + report=$( cat ./$TMPINPUT |
> > + $VALIDATE_NEW_API -)
> > + else
> > + report=$(cat ./$TMPINPUT | $VALIDATE_NEW_API -)
> > + fi
>
> Same as above, it can be simplified by only one line for all cases.
Fine, at this point, I don't know when I'll get to this though, its pretty busy
here at the moment.
>
>
>
>
