Code review comment for lp:~thomir-deactivatedaccount/bzr/add-global-config

> +class SystemGlobalStore(LockableIniFileStore):
> +
> + def __init__(self, possible_transports=None):
> + t = transport.get_transport_from_path(system_config_dir(),
> + possible_transports=possible_transports)
> + super(SystemGlobalStore, self).__init__(t, 'bazaar.conf')
>
> The point of passing possible_transports is basically so that we can reuse
> some connection objects, and that's mostly important for remote transports,
> which this won't be at present. I think the current approach of manually
> passing it down is not working very well and we should probably look at
> putting it into the library state instead.
>
> [tweak] However, for this patch, I don't think it's either used or
> particularly useful (since making new local transports is cheap), so you could
> just remove the parameter.

That sounds sensible. In fact, there's not a single place inside all of bzrlib where a transport is passed in. I've taken the liberty of removing that parameter from all the Store classes. If that was the wrong thing to do, please let me know.

>
> [suggestion] I'm not a big fan of creating new classes that only have
> different initial state, not different behaviour, so I would probably just
> make this a factory method.
>

That also sounds sensible. Since the Stores are an internal API (we want people to be using the Stack classes, right?), I guess I should follow naming convention and call these "_get_global_store" etc?

Also, I've renamed the GlobalStore to UserStore, and the "SystemGlobalStore" to just "GlobalStore", except that after the change above none of these will be visible, since they'll all come out of factory functions.

BranchStore is a bit more difficult, as it uses the branch lock for locking. I could keep the BranchStore class as-is, or I could wrap that up inside a factory function as well, which would make everything look a lot neater, but I'm not sure what your coding standards are regarding nested classes....

Finally, the win32utils unit tests fail, so I have some work to do there...