Code review comment for lp:~benji/lpsetup/bug-1023519-store-version-information

> Benji,
>
> * It is a wee optimization in make_version_dir but since mkdirs creates
> parents you could eliminate the call mkdirs(version_path()) and just do
> mkdirs(originals). Doing so will have impact on your tests, though, as your
> mock mkdirs has different behavior.

Right, I think it's a slight win to keep the version and originals
directories explicitly created.

> * make_version_dir should utilize backup_path.

Good catch! Fixed.

> * Why explicitly use os.path.sep instead of os.path.join?

I guess that bit deserves a comment. Here is the one I added:

# We don't use os.path.join here because both of the input paths are
# absolute (i.e., they start with a slash) so we instead construct the
# path ourselves in order to "re-root" the otherwise absolute source path
# in the backup directory.

> * I would expand your comment in the docstring for initialize_lxc. I had to
> read it a few times before realizing what you meant. Perhaps adding "so that
> it is only called when in a container" would make it clearer.

Good idea. Done.

> * In make_backup I was initially confused until i saw that backup_path is not
> a path but a method. Perhaps a name that would make that more obvious could
> be used.

I vacillated a bit on that. I normally prefer verb function names too.
You mentioning it has pushed me back to the other side. I changed
backup_path and version_path to be get_backup_path and get_version_path.

> Does this strategy of using the version number for storing the original files
> put any restrictions on how we update the version? Or is the idea that when
> the version changes we'll first restore the files saved in the previous
> version, remove the saved copies, and then proceed, applying the new
> workarounds under the new version number?

We aren't exactly sure how the saved files will be used, if at all. At
a minimum they provide some protection against a bug in lpsetup
catastrophically modifying a person's system. They also provide some
hint as to what lpsetup has changed (after the fact).