Code review comment for lp:~gnuoy/charm-helpers/add-vcsdirs-module

> > In _get_vcs_identifier, I think it would be best to catch exceptions a bit
> > more specifically. If the user doesn't have the git module, for example, they
> > might get back a generic "Exception" with no description of what went wrong.
> > It would be helpful to provide a description when you throw the exception
> > (e.g. Exception("Something bad happened")). Also, if the srcdir is neither a
> > git nor bzr repo, the function simply returns None. That looks problematic in
> > the places it's used.
>
> This was intentional as _get_vcs_identifier tries to autodetect which vcs
> system is in use. If it gets an exception because a particular vcs module
> isn't available or because the directory is not under the control of that vcs
> system then it moves on and tries the next one, hence not raising an exception
> back to the user. Do you think this approach is flawed ? or would it be ok to
> just make sure that if no vcs system is detected an exception, with useful
> message, is raised ?

I guess my main concern is that there will be no way to distinguish between those errors. They'll simply come out as a traceback ending in "raise Exception" necessitating a look at the code to figure out what went wrong. What I would expect is a message along the lines of "<dir> is not a VCS repository. Tried <repo types>."

> > In VcsDirs.deploy_dir() (line 174), I see:
> >
> > if check_newer and current_branchid >= int(candidate_branchid):
> >
> > I know git isn't your focus, but I'm pretty sure that the branchids won't be
> > comparable as ints. If fixing that isn't trivial, I certainly wouldn't
> > begrudge you to remove git support for this merge. Also (trivial), I believe
> > the "check_newer" boolean in this line is redundant.
>
> I left it up to the user to decide whether to compare the branchids based on
> the vcs system they're using. If the caller sets check_newer to be true when
> using git it will fail ( and I agree current_branchid >=
> int(candidate_branchid) does not look massively useful). Perhaps I need to
> take the decision about whether to compare ids away from the user and have it
> be dictated by the vcs system (bzr => yes, git => no).

I think it's useful to provide the option, as it seems quite helpful to have the library refuse to activate an older version unless it's explicitly asked to. It's still possible to compare branch IDs in git, but you have to use 'git rev-list' and work it out.