> Thank you. > > I have addressed these comments and the one other that I got privately. > > The incremental webrev of the affected files is here: > > http://anthrax.central/net/mrliberal/export/ws/mjnelson/onnv-transition-dev/webrev.incremental.codereview/
Damn, running with scissors. I fed the file list to webrev manually, and left out Rti.py. Suffice it to say I removed the ident string therefrom. > (also available at > http://cr.opensolaris.org/~mjnelson/webrev.incremental.codereview) > > ...and my responses are inline below. > > --Mark > > > >>> http://anthrax.central/net/mrliberal/export/ws/mjnelson/onnv-transition-dev/webrev.transition/ >> [...] >>> I need review of the "interesting stuff" and "6729074" links, and I would >>> be thrilled if somebody besides me took the time to wade through the rest. >> >> Despite the clear warnings otherwise, I went through all of it. ;-} > > And, as I promised, I'm thrilled. :) > > Even though I said it above, I'll repeat it here: thank you, Jim. This is > great. > >> In general, the CRs and other bug entries need evaluations and the >> other usual attributes. They're mostly stuck in "Accepted" state or >> something like that. > > Yes, sorry about that, I'm updating 'em all this morning. > > I'm also filing a single bugster bug to use as a bucket for the stuff we > tracked via bugs.grommit.com. > > I'll send out links to each of the five bugs when I get the bookkeeping > work done. > >> In the Python code, I did what I could, but I'm no Python expert. The >> one thing that jumped out at me was the funky spelling of 'honour'. >> (Just a nit, but the tradition in at least ON [and probably other >> consolidations as well] is to use American English spellings rather >> than "International" English. If this were user-visible, I'd call it >> a bug rather than just a nit.) > > Because they were comments, and not generally user visible, I elected to > honour the author's preferred spelling. :) > >> For 444 (.hgignore), the fix (apparently just ignoring everything) >> doesn't seem to match the description in the bug report itself. > > I added a comment to the bug. > >> For the .java files, do the "@version" things matter in any way for >> javadoc? If they weren't important, then just stripping them seems >> good. Otherwise, we might need some other fix. (I'm asking because >> [a] I'm no java expert and [b] CR 4758439 is still in 'Accepted' state >> and doesn't have an Evaluation.) > > They "matter" to javadoc, in that they cause false positives on a wsdiff. > I do not believe that they matter for anything useful. > >> For pkgdefs/SUNW{mibii,sacom,sasnm}/pkginfo.tmpl, the PSTAMP entry was >> entirely removed. This is usually commented out like so: >> >> #PSTAMP="<developer defined>" >> >> I'm guessing that it doesn't actually matter, but I'm not sure what's >> done with this upstream during WOS construction. (I looked at >> SUNWmibii in the delivered product, and pkginfo there has the swilly >> string, so this will make an observable change. Probably for the >> better, but may need to be at least disclosed in the CR.) > > I will need (as I previously promised Rich) to upgrade test these > packages. > > In reviewing this change initially, I checked for the absence, comment, > or definition of PSTAMP in ON packages, and I found a sufficient number of > each case that I didn't feel strongly, especially since "absent" and > "commented" are functionally equivalent. > >> For tools/codereview/codereview.1, and as long as you're changing this >> anyway, I suggest fixing line 22 to look more like the other headers. >> In other words: >> >> .TH codereview 1 "10 June 2005" >> >> ... lower-case name, section 1 (not fictitious "1-LOCAL"), and spelled >> out date rather than funky SCCS reversed date. > > Done. > >> A similar comment applies to the change in tools/scripts/acr.1:22. > > Done. > >> tools/cw/cw.c:1864: if this "-_versions" thing is needed at all, then >> I recommend making that version string a #define, and defining it near >> the top of the file along with a block comment. If it's useful, then >> it'll need to be updated each time someone modifies the file, and >> burying it in the code makes it unlikely that'll happen. > > I changed this to a #define, and also filed: > > 6733832 Need to revisit version output of compiler wrapper script > >> tools/env/opensolaris.sh:82: the "//hg" here doesn't look right to me. >> All of my working paths to hg.opensolaris.org use a relative reference >> to "hg/onnv/onnv-gate" and not an absolute path. Is this right? > > Because hg.os.o does a chroot, absolute and relative paths are > functionally equivalent. Rich and I were surprised to find each other > using different forms of the same path, but I tested both, and each works. > > I have a slight preference for the absolute, because I wonder if the > chroot will someday go away, and if so, I think that means the paths won't > need to change, as long as we keep the repos in the obvious location. > >> tools/scripts/wx.sh:28-29: these can probably go now as well. > > Done. > >> I don't see how the change in tools/onbld/Scm/Backup.py is related to >> bug ID 303. > > It's this single line (from the new file): > > 363 self.files = ('hgrc', 'localtags', 'patches', 'cdm') > > that adds the new .NOT files in the .hg/cdm/ directory to the list of > files to be included in a backup. > >> I like that some of the things you nuked were old, broken, expanded >> SCCS keywords in the SLP code. ;-} > > Thank you. I like a lot of what we're doing, too. :) > > --Mark > > _______________________________________________ > scm-migration-dev mailing list > scm-migration-dev at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/scm-migration-dev >
