Richard Lowe wrote: > Nathan Bush <nathan.bush at sun.com> writes: > >> Hi, >> >> Please review: >> http://cr.opensolaris.org/~nbush/scm-migration/483/webrev/ >> >> This change should address all of the following bugs: >> 465 webrev permission display fails for files in workspace root. >> 470 noise in webrev log for deleted files >> 471 "ERROR: set mode of" errors from webrev >> 483 webrev permission display change #372 has poor performance >> > > 2271/2509: If the filename were of the form /tmp/$$.* this would happen > anyway (and would be done in a trap that dealt with most > signals, too). > > Same comment for SEDFILE, though less worthwhile there, I guess.
Good point, especially regarding the trap. Here's a revised version: http://cr.opensolaris.org/~nbush/scm-migration/483/webrev.v2/ % diff webrev/raw_files/new/usr/src/tools/scripts/webrev.sh webrev.v2/raw_files/new/usr/src/tools/scripts/webrev.sh 2260c2260 < SEDFILE=`mktemp /tmp/webrev.sed.XXXXXX` --- > SEDFILE=/tmp/$$.manifest.sed 2271c2271 < HG_PARENT_MANIFEST=`mktemp /tmp/webrev.manifest.XXXXXX` --- > HG_PARENT_MANIFEST=/tmp/$$.manifest 2274,2275d2273 < < rm -f $SEDFILE 2506,2512d2503 < # < # For Mercurial, remove the cache of manifest entries. < # < if [[ -f "$HG_PARENT_MANIFEST" ]]; then < rm -f $HG_PARENT_MANIFEST < fi < I think I'd like it best if webrev created a private temporary directory with mktemp, then put all of its temporary files inside there, but that could be addressed later in a separate bug. > The logic to actually build HG_PARENT_MANIFEST leaves me wondering if > there's a more obvious way to go about it, but I admit that right now > I can't think of one, and the comment describes what's going on well > enough. Yes, it's still a bit convoluted, but hopefully it's a little easier to follow than the old method. This new approach should be much faster in almost every case; for example, the time to generate something similar to the nightly project webrev falls from 742 seconds to 291 seconds (on my machine). --Nathan
