Code review comment for lp:~stevenk/launchpad/fixes-bug-529950

Hi Steve and Jelmer,

Nice find Jelmer. It's a shame (but not a surprise :/), that we don't seem to have a test for the sourcepackagenames_only option. It's only referenced in the config. Steve, feel free to add one before fixing the issue :) Actually, I can't see it referenced anywhere in the lp-production-configs? Do we even use it? (if not, we could remove it).

I've just got a few style questions below.

Thanks for the fix!

> === modified file 'lib/lp/soyuz/scripts/gina/archive.py'
> --- lib/lp/soyuz/scripts/gina/archive.py 2009-06-25 04:06:00 +0000
> +++ lib/lp/soyuz/scripts/gina/archive.py 2010-03-17 11:15:37 +0000
> @@ -207,7 +207,7 @@
> log.exception("Invalid Sources stanza in %s" %
> info_set.sources_tagfile)
> continue
> - self.src_map[src_name] = src_tmp
> + self.src_map.setdefault(src_name, []).append(src_tmp)

Not sure what you prefer, but I think this is more readable like this:

http://pastebin.ubuntu.com/399865/

which also passes the test. Up to you.

> === modified file 'scripts/gina.py'
> --- scripts/gina.py 2009-10-13 14:38:07 +0000
> +++ scripts/gina.py 2010-03-17 11:15:37 +0000
> @@ -229,44 +229,45 @@
> npacks = len(packages_map.src_map)
> log.info('%i Source Packages to be imported' % npacks)
>
> - for source in sorted(packages_map.src_map.values(),
> - key=lambda x: x.get("Package")):
> - count += 1
> - package_name = source.get("Package", "unknown")
> - try:
> + for list_source in sorted(packages_map.src_map.values(),
> + key=lambda x: x[0].get("Package")):

The python style guide used to allow the previous indentation, but it now should be:

   for list_source in sorted(
       packages_map.src_map.values(), key=lambda x: x[0].get("Package")):

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

Or another option, if you rename your var 'list_source' to simply 'source_list', then you can create your source_lists prior to the iteration (for source_list in source_lists:). See what you think.

> + for source in list_source:
> + count += 1
> + package_name = source.get("Package", "unknown")
> try:
> - do_one_sourcepackage(source, kdb, package_root, keyrings,
> + try:
> + do_one_sourcepackage(source, kdb, package_root, keyrings,
> importer_handler)

Similar, if you need to wrap the args to a fn call, they should start on the next line.

> - except psycopg2.Error:
> - log.exception("Database error: unable to create "
> + except psycopg2.Error:
> + log.exception("Database error: unable to create "
> "SourcePackage for %s. Retrying once.."
> % package_name)

And again.

> - importer_handler.abort()
> - time.sleep(15)
> - do_one_sourcepackage(source, kdb, package_root, keyrings,
> + importer_handler.abort()
> + time.sleep(15)
> + do_one_sourcepackage(source, kdb, package_root, keyrings,
> importer_handler)

Here too.

> - except (InvalidVersionError, MissingRequiredArguments,
> + except (InvalidVersionError, MissingRequiredArguments,
> DisplayNameDecodingError):
> - log.exception("Unable to create SourcePackageData for %s" %
> - package_name)
> - continue
> - except (PoolFileNotFound, ExecutionError):
> - # Problems with katie db stuff of opening files
> - log.exception("Error processing package files for %s" %
> - package_name)
> - continue
> - except psycopg2.Error:
> - log.exception("Database errors made me give up: unable to create "
> + log.exception("Unable to create SourcePackageData for %s" %
> + package_name)

Ditto.

> + continue
> + except (PoolFileNotFound, ExecutionError):
> + # Problems with katie db stuff of opening files
> + log.exception("Error processing package files for %s" %
> + package_name)

Ditto.

> + continue
> + except psycopg2.Error:
> + log.exception("Database errors made me give up: unable to create "
> "SourcePackage for %s" % package_name)
> - importer_handler.abort()
> - continue
> - except MultiplePackageReleaseError:
> - log.exception("Database duplication processing %s" %
> + importer_handler.abort()
> + continue
> + except MultiplePackageReleaseError:
> + log.exception("Database duplication processing %s" %
> package_name)

Ditto.

> - continue
> + continue
>
> - if COUNTDOWN and count % COUNTDOWN == 0:
> - log.warn('%i/%i sourcepackages processed' % (count, npacks))
> + if COUNTDOWN and count % COUNTDOWN == 0:
> + log.warn('%i/%i sourcepackages processed' % (count, npacks))
>
>
> def do_one_sourcepackage(source, kdb, package_root, keyrings,
>
>