One thing that just occurred to me, this findunref almost certainly breaks compatibility with the existing findunref - which we will need to continue supporting if we are to integrate tools back first.
So I see a few scenarios: 1) Tools that are ON-specific (like findunref) aren't integrated with our initial tools putback (as opposed to shared like nightly, webrev, etc.) and instead are integrated with the actual switch to Mercurial 2) We maintain two copies; the original (e.g.: findunref.c to work with SCCS) and the new findunref.py to work with Mercurial, and nightly calls whichever one depending on the SCM in use. 3) We support working with Teamware/SCCS in the new tool. #1 results in less duplication of work and is more preferable to me - but I'm not horribly averse to #2. #3 is the least desirable. cheers, steve Mike Kupfer wrote: > Overall it looks good, but I have a few concerns about findunref.py. > > First, I gave up trying to understand walk_scm_hg(). I don't know the > Mercurial API well enough, and I couldn't find any real documentation on > the website. I tried poking around in the Mercurial source, but the > comments are sufficiently sparse that I figured it would take me the > rest of the afternoon to figure it all out. For example, I have no idea > if repo.status() (no arguments) gives you what you need. Putting some > comments in walk_scm_hg() would help. > > Second, I wonder if symbolic links or lofs paths could give you > problems. Test case: the user specifies "/home/kupfer/ws/foo", which is > really a symbolic link to /net/somehost/export/kupfer/ws/foo. We want > this to work on both somehost and on other hosts. Note that > localrepo.root is generated using os.path.realpath(). > > Third, I found the code overall to be a bit sparsely commented. For > example, I think it would be helpful to document that > ExceptionList.match() takes a glob pattern. > > Nits: > > * checkpaths.sh: > > - line 105: continuation line should only be indented by 4 spaces > > * exception_list.open: > > - lines 8, 14: "ususr" -> "usr" > > * findunref.py: > > - Should '.hgignore' be in the built-in exception list? > > - the error exits all exit with status code 2, whereas I think the C > version exits with code 1. It's not documented anywhere what it > *should* exit with, so hopefully no scripts are depending on a > specific value. But I don't think we should use a different value > unless we have a good reason. > > - line 59: why not just line.strip()? > >>>>>> "stevel" == Stephen Lau <stevel at sun.com> writes: > > stevel> Other notes: > stevel> o nightly.sh:2795, you can see where I pass the findunref output > stevel> through sed to get the output to look like the form of the old > stevel> findunref output, that is to say, the usr/closed output will > stevel> look like: > stevel> ../closed/cmd/foo > stevel> and the usr/src output will look like: > stevel> ./uts/foo > stevel> I'm open to changing that. I just made it that way to > stevel> maintain uniformity for now. I personally think it makes more > stevel> sense to have it be usr/closed/cmd/foo and usr/src/cmd/foo, > stevel> but whatever. Your comments on this point would be > stevel> appreciated. > > The unref-*.out files are structured that way so that `cat > unref<mumble>.out` will give you a file list that you can use from the > same directory (e.g., ls -l `cat ...`). I don't know if anyone takes > advantage of that, but I'd rather not throw it away without good reason. > > cheers, > mike -- stephen lau // stevel at sun.com | 650.786.0845 | http://whacked.net opensolaris // solaris kernel development
