Richard Lowe wrote: > Nathan Bush <nathan.bush at sun.com> writes: > >> Hi, >> >> I need a code review for: >> 423 questionable format for rename-only changes in hg webrev >> >> The change to webrev is: >> http://cr.opensolaris.org/~nbush/scm-migration/423/webrev/ >> > > The only thing that jumps out is a nit I don't really mind either way: > > 2584: -n is perhaps better than ! -z > > The results also look ok, to me. > > -- Rich
Good point, there were a lot of double-negatives in there. I respun the webrevs, the new URLs are: http://cr.opensolaris.org/~nbush/scm-migration/423/webrev.v2/ http://cr.opensolaris.org/~nbush/scm-migration/423/sample.v2/ The change from v1 to v2 is: *** webrev.sh-423.v1 Sat Mar 1 22:01:20 2008 --- webrev.sh-423.v2 Tue Mar 4 20:06:04 2008 *************** *** 2510,2516 **** mv_but_nodiff= cmp $WDIR/raw_files/old/$PP $WDIR/raw_files/new/$P > /dev/null 2>&1 ! if [[ $? == 0 && ! -z "$oldname" ]]; then mv_but_nodiff=1 fi --- 2510,2516 ---- mv_but_nodiff= cmp $WDIR/raw_files/old/$PP $WDIR/raw_files/new/$P > /dev/null 2>&1 ! if [[ $? == 0 && -n "$oldname" ]]; then mv_but_nodiff=1 fi *************** *** 2581,2591 **** print "<b>$P</b>" # For renamed files, clearly state whether or not they are modified ! if [[ ! -z "$oldname" ]]; then ! if [[ -z "$mv_but_nodiff" ]]; then ! print "<i>(modified and renamed, was $oldname)</i>" ! else print "<i>(renamed only, was $oldname)</i>" fi fi --- 2581,2591 ---- print "<b>$P</b>" # For renamed files, clearly state whether or not they are modified ! if [[ -n "$oldname" ]]; then ! if [[ -n "$mv_but_nodiff" ]]; then print "<i>(renamed only, was $oldname)</i>" + else + print "<i>(modified and renamed, was $oldname)</i>" fi fi Thanks, --Nathan
