launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/archivepublisher/model/ftparchive.py'
> --- lib/lp/archivepublisher/model/ftparchive.py	2016-02-06 03:43:15 +0000
> +++ lib/lp/archivepublisher/model/ftparchive.py	2016-02-09 01:39:00 +0000
> @@ -50,9 +50,16 @@
>      return (os.path.basename(filename).split("_"))[0]
>  
>  
> -def safe_mkdir(path):
> -    """Ensures the path exists, creating it if it doesn't."""
> -    if not os.path.exists(path):
> +def make_empty_dir(path):
> +    """Ensure that the path exists and is an empty directory."""
> +    if os.path.isdir(path):
> +        for name in os.listdir(path):
> +            child_path = os.path.join(path, name)
> +            # Directories containing index files should never have
> +            # subdirectories.  Guard against expensive mistakes.
> +            assert os.path.isfile(child_path)
> +            os.unlink(child_path)

The assertion is sort of pointless unless you want to not delete device nodes or directory symlinks. os.unlink will already crash on directories.

> +    else:
>          os.makedirs(path, 0o755)
>  
>  
> @@ -754,17 +761,20 @@
>              distroseries.include_long_descriptions,
>              distroseries.index_compressors)
>  
> -        # XXX: 2006-08-24 kiko: Why do we do this directory creation here?
> +        # Make sure all the relevant directories exist and are empty.  Each
> +        # of these only contains files generated by apt-ftparchive, and may
> +        # contain files left over from previous configurations (e.g.
> +        # different compressor types).
>          for comp in comps:
>              component_path = os.path.join(
>                  self._config.distsroot, suite, comp)
> -            safe_mkdir(os.path.join(component_path, "source"))
> +            make_empty_dir(os.path.join(component_path, "source"))
>              if not distroseries.include_long_descriptions:
> -                safe_mkdir(os.path.join(component_path, "i18n"))
> +                make_empty_dir(os.path.join(component_path, "i18n"))
>              for arch in archs:
> -                safe_mkdir(os.path.join(component_path, "binary-" + arch))
> +                make_empty_dir(os.path.join(component_path, "binary-" + arch))
>                  for subcomp in self.publisher.subcomponents:
> -                    safe_mkdir(os.path.join(
> +                    make_empty_dir(os.path.join(
>                          component_path, subcomp, "binary-" + arch))

We don't rely on the old files for anything weird like contents generation, do we?

>  
>      def writeAptConfig(self, apt_config, suite, comps, archs,


-- 
https://code.launchpad.net/~cjwatson/launchpad/ftparchive-cleanup-old-indexes/+merge/285428
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.