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().
I *think* that should work; I tried a couple of symlink tests (though not with the /net automounter)... it requires it to be run out of the root of the repository though, so as long as you're in it - it should just work. > 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. I went through and hopefully added sufficient comments/docs. Let me know if anything seems insufficiently documented. > Nits: > > * checkpaths.sh: > > - line 105: continuation line should only be indented by 4 spaces Done > * exception_list.open: > > - lines 8, 14: "ususr" -> "usr" Oops, errant regex substitution. :) Thanks. And not really a nit since that's clearly going to fail the CDDLchk. :-P > * findunref.py: > > - Should '.hgignore' be in the built-in exception list? Yes, thanks. > - 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. Okay, done. > - line 59: why not just line.strip()? I preferred the symmetry of lstrip() especially since we also call rstrip(). It makes it more obvious this way which one does what. >>>>>> "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. Okay, I'll keep what I have then which maintains that same behaviour. cheers, steve -- stephen lau // stevel at sun.com | 650.786.0845 | http://whacked.net opensolaris // solaris kernel development
