Hi Shawn, Thanks for the review!
On Tue, 2010-02-23 at 15:06 -0600, Shawn Walker wrote: > > 14022 traceback when pkgsend create-repository is given a path with too > > many /'s > > > > http://cr.opensolaris.org/~timf/14022-pkgsend-fileurl > > modules/publish/transaction.py: > > line 24: s/2009/2010/ Oops. > line 569: I'd suggest: > "'%s' contains host information, which is not supported for > filesystem operations." Ok - I'm a bit uneasy that the above might imply that we think it's an invalid URI, rather than one we just can't deal with, but your suggestion is less wordy than what I had, so I'll use it - thanks. > lines 571-576: rather than trying to deal with the four '////' case > specifically, I'd rather see this just try to replace any '/' at the > beginning of the string with a single '/'. os.path.normpath() was doing that already. It will leave a '//' at the beginning of the path if it's there, hence the check to replace '//'. I've used a different implementation in the updated webrev, but both would achieve the same thing. > src/publish.py: > line 52: don't import this; see below > > line 134: the publication tools are currently not expected to catch > client ApiExceptions, and the publication API is intentionally setup to > completely wrap all errors raised so that callers only catch > TransactionError exceptions. What was the case for this change? I think I had been trying to see if there was a better exception I could raise for this case, where we have a valid URL that we just can't support, as opposed to an invalid URL and when I didn't find one, I forgot to remove the additional exception we were catching. I didn't know that the publication tools were sectioned off in this way though - thanks. I've updated the webrev, adding a test I should also have included: checking that pkgsend create-repository returns 0 isn't enough, we need to also make sure that the repository we think we created actually exists each time we try a different URL. The updated webrev is at http://cr.opensolaris.org/~timf/14022-pkgsend-fileurl cheers, tim _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
