Code review comment for lp:~jtv/launchpad/db-bug-752279

Thanks! You raise some good questions here.

> 45 +def get_backup_dists(archive_config):
> 46 + """Return the path of an archive's backup dists directory."""
> 47 + return os.path.join(archive_config.archiveroot + "-distscopy",
> "dists")
>
> Is archiveroot guaranteed to not have a trailing separator?

Internally it both constructs them in a way that can't produce one, and relies on there not being one in the same way that this code does.

> 95 + The publishable files are kept in the filesystem. Most of the
> work
> 96 + is done in a working "dists" directory in each archive's dists
> copy
> 97 + root, which then replaces the current "dists" in the archive
> root.
>
> Isn't that false now? Work is done in archiveroot, not distscopyroot.

You are correct. This text predated that change.

> 331 + For each archive, this switches the dists directory and the
> 332 + backup dists directory around.
>
> Lies! It rotates them through the three dirs.

That is implementation, and therefore suitable for a comment. This, however, is docstring and switching the two around is what the method does for the caller.

> 409 + def publish(self, security_only=False):
> 410 + """Do the main publishing work.
> [...]
>
> Could the except/recoverWorkingDists/raise be a finally instead? It also seems
> like rsyncBackupDists might belong in this method, as installDists is there.

It could be, but advantage of these working directories is that they are transient. The recoverBackupDists method is for failure paths only. The normal execution path does have a "finally" cleanup. I considered unifying the two, but it would mean either postponing the cleanup (turning the temporary directories back into state that needs managing) or hiding the "rename" cleanup matching the opening "rename" in a less symmetric, and much less precise, call to recoverBackupDists.

> 513 + """Write a marker file for checking direction movements.
>
> s/direction/directory/?

s.

Jeroen