Vladimir Kotal wrote: > Mark J. Nelson wrote: >> Vladimir Kotal wrote: >>> Vladimir Kotal wrote: >>>> Hi all, >>>> >>>> Based on feedback from Dan Price, I've changed webrev.sh a bit to >>>> make it more pleasant to work with (in terms of webrev upload) and >>>> to fix 2 small bugs. >>>> >>>> webrev is here: >>>> http://cr.opensolaris.org/~vkotal/webrev-better_upload.onnv/ >>> >>> I know everyone is busy with more funny/interesting stuff but this >>> one still needs some review. >> >> Have you tested all of the output paths? It seems like you're using >> trailing spaces inconsistently, such that some messages will not have >> spaces in between them? > > I believe most of the output paths were tested and they seemed fine. > > However, after playing with the script a bit more, it became obvious > that error output should be better (because it collided with the single > line output) so I modified it a bit. Now if some of the upload commands > fails the error will be nicely printed (i.e. not in the middle of webrev > output). > > E.g. for SSH transport: > > $ webrev -n -U -t ssh://cr.opensolAaris.org:test-43 > SCM detected: mercurial > File list from: hg-active -p /ws-local/onnv-clone ... Done. > Upload to: ssh://cr.opensolAaris.org:test-43/ > Uploading: rmdir upload ... Failed > ERROR: scp failed > src dir: '/builds/vk199839/webrev-better_upload.onnv/webrev' > dst dir: 'cr.opensolAaris.org:test-43' > error messages: > > ssh: cr.opensolAaris.org: node name or service name not known > > lost connection > $ echo $? > 1 > > And for the failover case: > > $ RSYNC=/bin/false webrev -U -O -o $CODEMGR_WS/webrev-failover > SCM detected: mercurial > File list from: hg-active -p /ws-local/onnv-clone ... Done. > Workspace: /builds/vk199839/webrev-better_upload.onnv (at > d04ce5ecbe7d) > Compare against: /ws-local/onnv-clone (at 1eb5efc243ad) > Output to: /builds/vk199839/webrev-better_upload.onnv/webrev-failover > Output Files: > usr/src/tools/scripts/webrev.sh > patch cdiffs udiffs wdiffs sdiffs frames ps old new > Generating PDF: Done. > index.html: Done. > Upload to: rsync://cr.opensolaris.org:webrev-failover/ > Uploading: rsync ... Failed. (falling back to SSH) > Upload to: ssh://cr.opensolaris.org:webrev-failover/ > Uploading: rmdir upload ... Done. > $ echo $? > 0 > > > I have added SED variable in the process along with related substitutions. > >> What testing did you do for the remote target specification validation? > > It's only so much that can be tested given it now only checks invalid > prefix and missing colon. > > Apart from the unit tests [*] I have played a bit with the new script. > > I have actually spent some time thinking about making the validation > more strict, by checking the remote host and see if it maps to valid IP > address but it gets complicated because the host part could be also IP > address (not talking about the IPv6 address notation with square > brackets) so it ended up like this (better rudimentary checking for most > common errors than nothing). > >> Does scp not have a trailing-slash problem like rsync? (Ie it seems >> like webrev should be using "$WDIR/*" instead of "$WDIR" as the src arg?) > > scp deals with the trailing slash just fine. > > The following examples will do the right thing: > - without -o/-t > scp -r $CODEMGR_WS/webrev cr.opensolaris.org:bugfix-123 > - with -o: > scp -r $CODEMGR_WS/custom_dir cr.opensolaris.org:custom_dir > > In the case with "$WDIR/*" I'd have to create remote directory first > (via SFTP) for the simplistic case as well (as opposed to the current > way where SFTP is only used for the case with more than one remote > directory) which would unnecessarily slow down already slow SSH > uploading process. > >> Seems like lines 349 and 354 could be rolled together. > > Sure. Now done inside print_upload_header(). > >> Aren't you losing the return code from ssh_upload now, on line 375-378? > > D'oh, yes; it was done not very carefully before but now it is real > issue. Fixed. > > In the course of testing the changes I have discovered CR 6807228 so > there is now a fix for it too in the webrev. > > Incremental webrev is here: > http://cr.opensolaris.org/~vkotal/webrev-better_upload.onnv.2nd/ > > Up-to-date webrev against ON clone is here: > http://cr.opensolaris.org/~vkotal/webrev-better_upload.onnv.final/
The new line 276 isn't very informative about what the parenthesized number means. (Ie the number of args received when 1 was expected.) 342-345: can this really happen? No other comments. Sorry for the delay. You'll have a trivial merge with Salo's recent changes. --Mark
