launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/archivepublisher/publishing.py'
> --- lib/lp/archivepublisher/publishing.py	2016-04-14 10:26:19 +0000
> +++ lib/lp/archivepublisher/publishing.py	2016-04-27 16:22:00 +0000
> @@ -783,7 +775,27 @@
>                      if file_exists(release_path):
>                          self.release_files_needed.add(
>                              (distroseries.name, pocket))
> -                self._writeSuite(distroseries, pocket)
> +
> +                if (distroseries.name, pocket) in self.release_files_needed:
> +                    if not is_careful:
> +                        if not self.isDirty(distroseries, pocket):
> +                            self.log.debug("Skipping release files for %s/%s" %
> +                                           (distroseries.name, pocket.name))
> +                            continue
> +                        self.checkDirtySuiteBeforePublishing(
> +                            distroseries, pocket)
> +                    self._writeSuite(distroseries, pocket)
> +                elif ((distroseries, pocket) in archive_file_suites and
> +                      distroseries.publish_by_hash):
> +                    # We aren't publishing a new Release file for this
> +                    # suite, probably because it's immutable, but we still
> +                    # need to prune by-hash files from it.
> +                    suite = distroseries.getSuite(pocket)
> +                    release_path = os.path.join(
> +                        self._config.distsroot, suite, "Release")
> +                    with open(release_path) as release_file:
> +                        release_data = Release(release_file)
> +                    self._updateByHash(suite, release_data)

I'd extract the _updateByHash bits from _writeSuite. It will probably mean parsing the Release that we just wrote out, but it avoids weird slightly differentiated duplication that's no doubt going to go wrong at some point and is not clearly thoroughly tested.

release_path is also already calculated in a branch above.

>  
>      def _allIndexFiles(self, distroseries):
>          """Return all index files on disk for a distroseries.


-- 
https://code.launchpad.net/~cjwatson/launchpad/by-hash-prune-immutable/+merge/293145
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.