Re: [Xen-devel] [PATCH OSSTEST v3] mg-all-branch-statuses: Show how up to date each branch is

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("[PATCH OSSTEST v3] mg-all-branch-statuses: Show how up to 
date each branch is"):
> Using report_find_push_age_info allows us to provide counts of
> attempts since the last baseline on current tip as well as the first
> attempt of each of those.
> 
> Since everything serialises on the repo lock I didn't bother trying to
> parallelise anything.
...
> Example output:
> Branch                       Basis    Tip      #Tip #Tot 1stTip     1stNew
> libvirt                      d10a5f58 845184b2    0   10 n/a        8 days
> linux-3.0                    e1c63f9f 5dba9ddd    2    9 2 days     868 days

I'm right into nits here, but:

Justification of the "nn days" values is wrong.

> linux-3.10                   b3d78448 UpToDate
> linux-3.14                   762167f9 UpToDate
> linux-3.16                   162d6432 26749e75    1    1 1 day      1 day
> linux-3.18                   d048c068 ea5dd38e    3    3 2 days     2 days
> linux-3.4                    bb4a05a0 cf1b3dad   15  161 11 days    212 days
> linux-4.1                    b953c0d2 6a010c0a    0    - n/a        n/a

This would be easier to read if n/a was RH aligned too.

> linux-arm-xen                64972ceb UpToDate
> linux-linus                  6aaf0da8 4da3064d    0    0 n/a        n/a
> linux-mingo-tip-master       d935d0f7 778a1ac5    0   16 n/a        1173 days
> linux-next                            c8a9ad22    0  219 n/a        447 days
> osstest                      15d2dd50 Error!      0    - n/a        n/a
> ovmf                         269e0aeb 288ed590    0    1 n/a        1 day
> qemu-mainline                d2966f80 UpToDate

That `UpToDate' is the same length and about as visually dense as a
commit id is unfortunate.


I should actually read the code:

> +     my $onevar = sub {
> +             my ($var,$dflt) = @_;
> +             $dflt //= "";
> +             print "export ".uc($var)."=\"";

Why are you exporting these values ?

> +             print $info->{$var}//$dflt;

Why do you abstract away $dflt ?

> +             print "\";\n";
> +     };
> +     my $oneflightvar = sub {

These subs are a lot more sophisticated than they need to be, aren't
they (eg, handling of $dflt) ?  I confess I don't understand why these
are anonymous subrefs rather than simply loop bodies, but fine.

> +             my ($flight,$var,$dflt) = @_;
> +             $dflt //= "";
> +             print "export ".uc($flight)."_".uc($var)."=\"";
> +             print $info->{$flight}{$var}//$dflt;
> +             print "\";\n";
> +     };

A simpler $onevar would let you write

> +     $onevar->($_, "-") foreach qw(CountTip CountAfterBasis);

        $onevar->($info->{$_}, $_)

> +     foreach my $flight (qw(Basis FirstAfterBasis FirstTip)) {
> +             $oneflightvar->($flight, $_) foreach qw(flight started);

                $onevar->($info->{$flight}{$_}, "${flight}_${_}")


> +days_since()
> +{
> +    then=$1 ; shift
> +
> +    if [ -z "$then" ] ; then
> +     echo "n/a"
> +     return
> +    fi
> +
> +    local now=$(date +%s)
> +    local days=$(( ($now - $then) / 86400 ))
> +
> +    if [ $days -eq 1 ] ; then
> +     echo "$days day"
> +    else
> +     echo "$days days"
> +    fi
> +}

Wow, this is quite verbose in shell, isn't it.


> +printf "%-28s %-8s %-8s %-9s %-10s %-10s\n" \
> +     "Branch" "Basis" "Tip" "#Tip #Tot" "1stTip" "1stNew"
> +
> +for branch in $@; do
> +    basis=`./ap-fetch-version-old $branch 2>/dev/null || true`
> +    tip=`./ap-fetch-version $branch 2>/dev/null || true`

This is quite fault-oblivious, isn't it.  Oh well.

> +    if [ x$basis != x ] ; then

I would write "x$basis" in case basis contains spaces.  It _shouldn't_
but the errors if it does will be mysterious.  It's also good
practice.

> +     basis=${basis:0:8}
> +    else
> +     basis=""
> +    fi

This is rather odd.  You truncate it to the first 8 characters but
only if it's not empty.  Why bother checking ?

> +    if [ x$tip != x ] ; then
> +     tip=${tip:0:8}
> +    else
> +     tip="Error!"
> +    fi

Did you know you could write
    ${tip-Error!}
?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel