Code review comment for lp:~vila/bzr/new-config

Revision history for this message
Martin Pool (mbp) wrote :

I've read all the individual branches, and though there are a few questions in detail, I think the overall shape looks pretty nice.

I am finishing off Sergei's rules patch, and in the context of that I was wondering whether we should really have a major structural separation between per-file rules and the other configuration schemes. They are all just name-value pairs. The query to get the value for a particular file in a particular well could equally well go through a config-type api, and also read the general configuration stuff as well as per-file rules. (Indeed it can do either of them but I think quite reasonably it could do both.)

So for instance if you wanted to set eol=native just globally or in locations.conf you could do that.

Obviously not all settings make sense to read per-file. Also, as we will eventually read some settings from the tree contents, not all of them are safe to read from the untrusted content in there. But that should be pretty easy to do, just by constructing the right stack. We can perhaps also constrain this when we later get a registry of available options: mark some of them as per-file and some as safe. (eol conversion is safe; sending email or running external commands is not safe.)

We would have to think carefully about the performance considerations because we couldn't afford to construct a new section or stack for every file we want to find out about. But if there is a get_tree_file_config_stack(...) then perhaps that can be smart about reusing existing objects.

« Back to merge proposal