Code review comment for lp:~stub/launchpad/librarian-gc

> Per Bug #437084, the librarian garbage collector is still failing on
> production. This is the fix.
>
> Now with test case.

Thanks stub - nicely tested.

r=me (Teeny style-guide questions below.)

> === modified file 'lib/canonical/librarian/ftests/test_gc.py'
> --- lib/canonical/librarian/ftests/test_gc.py 2009-09-26 09:06:13 +0000
> +++ lib/canonical/librarian/ftests/test_gc.py 2009-10-01 07:09:13 +0000
> @@ -15,6 +15,7 @@
> from pytz import utc
> from sqlobject import SQLObjectNotFound
> from storm.locals import SQL, AutoReload
> +import transaction
> from zope.component import getUtility
>
> from canonical.config import config
> @@ -560,6 +561,21 @@
> self.assert_(os.path.exists(noisefile2_path))
> self.assert_(os.path.exists(noisefile3_path))
>
> + def test_deleteUnwantedFilesBug437084(self):

Just looking at https://dev.launchpad.net/TestsStyleGuide#Python Test Cases
should this be:

       def test_deleteUnwantedFiles_bug437084(self):

or similar?

> + # There was a bug where delete_unwanted_files() would die
> + # if the last file found on disk was unwanted.
> + self.layer.switchDbUser(dbuser='testadmin')
> + content='foo'
> + self.client.addFile(
> + 'foo.txt', len(content), StringIO(content), 'text/plain',
> + )

I always get confused on this one, but I think you've used the tuple
style here, rather than the multiline function call style? That is,
I the closing ')' should be on the previous line and without the extra
comma? See

https://dev.launchpad.net/PythonStyleGuide#Multiline function calls

> + # Roll back the database changes, leaving the file on disk.
> + transaction.abort()

Nice!

> +
> + self.layer.switchDbUser(config.librarian_gc.dbuser)
> +
> + # This should cope.
> + librariangc.delete_unwanted_files(self.con)
>
> def test_cronscript(self):
> script_path = os.path.join(
>
> === modified file 'lib/canonical/librarian/librariangc.py'
> --- lib/canonical/librarian/librariangc.py 2009-09-26 08:43:07 +0000
> +++ lib/canonical/librarian/librariangc.py 2009-10-01 07:09:13 +0000
> @@ -578,6 +578,7 @@
> next_wanted_content_id = get_next_wanted_content_id()
>
> if (config.librarian_server.upstream_host is None
> + and next_wanted_content_id is not None
> and next_wanted_content_id < content_id):
> log.error(
> "LibraryFileContent %d exists in the database but "

And I asked:

<noodles775> stub: hi! Just looking at your MP... would it be worth in addition creating a separate condition to log a warning for this situation where the file exists on disk but not in the db?
<stub> That is actually normal - client.addFile(foo,...); transaction.abort() leaves a file on disk
<noodles775> OK - I assumed that the transaction is only aborted in abnormal circumstances.
<stub> Nah - anytime a web request needs to be retried after uploading a file for instance.
<noodles775> k. great.

--
Michael