[RFC] Conditionalize format warnings

[John Arbash Meinel]

Matthew D. Fuller wrote:
> On Mon, Apr 16, 2007 at 10:59:11PM -0400 I heard the voice of
> Aaron Bentley, and lo! it spake thus:
>> Well, you can still use locations.conf to configure it.  That's
>> where we tend to stuff config information for repositories.
> 
> Oh, yes.  It just seems a little asymmetrical that we have a local
> config in the branch for the branch, but none in the WT for the WT.
> 
> 
>> It is typical to invoke branch.get_config(), which allows the branch
>> to further customize the class if desired.
> 
> Whoops, didn't notice that.  Changed.
> 
> 
>>> +            else:
>>> +                fwarn = config.GlobalConfig().get_format_warning()
>> I don't think it's worth considering the case where there's a
>> working tree but no branch.  Such a construct is useless to Bazaar,
>> and is exceedingly rare.
> 
> Well, only one of the _check_supported() calls in the tree even passes
> in the basedir, so we need to do something where one isn't passed.
> 
> And while WT's are the only case that currently has a
> recommend_upgrade=True, presumably some day branches or repos will too
> (of course, they may start passing in the location then too, but...)
> 
> 
>> Also, we'll need some test cases before this request can be
>> accepted.
> 
> Well, that's why I'm RFC'ing, not MERGE'ing   :).  I want to make sure
> the implementation is (a) sound, and (b) what's desired.
> 
> I noticed another issue after I sent the last mail, which is that the
> error for invalid values wouldn't work right from the BranchConfig
> side.  Updated diff attached.  If this meets with general approval,
> I'll add test-writing to my todo for it, but that may take me a while
> to do as well as a while to get to (Real Work(tm) must get done,
> alas), so anybody who already understands the test suite as has some
> time to kill is quite welcome to beat me to it   ;)
> 
> 
> 
> 
> ------------------------------------------------------------------------
> 
> === modified file 'bzrlib/bzrdir.py'
> --- bzrlib/bzrdir.py	2007-04-01 06:19:16 +0000
> +++ bzrlib/bzrdir.py	2007-04-17 03:23:30 +0000
> @@ -40,6 +40,8 @@
>  
>  import bzrlib
>  from bzrlib import (
> +    branch,
> +    config,
>      errors,
>      lockable_files,
>      lockdir,
> @@ -58,6 +60,7 @@
>      sha_strings,
>      sha_string,
>      )
> +from bzrlib.branch import Branch
>  from bzrlib.store.revision.text import TextRevisionStore
>  from bzrlib.store.text import TextStore
>  from bzrlib.store.versioned import WeaveStore
> @@ -139,9 +142,16 @@
>              raise errors.UnsupportedFormatError(format=format)
>          if recommend_upgrade \
>              and getattr(format, 'upgrade_recommended', False):
> -            ui.ui_factory.recommend_upgrade(
> -                format.get_format_description(),
> -                basedir)
> +            # Warnings may be disabled by config
> +            if basedir is not None:
> +                branch = Branch.open(basedir)
> +                fwarn = branch.get_config().get_format_warning()
> +            else:
> +                fwarn = config.GlobalConfig().get_format_warning()
> +            if fwarn == "yes":
> +                ui.ui_factory.recommend_upgrade(
> +                    format.get_format_description(),
> +                    basedir)
>  
>      def clone(self, url, revision_id=None, force_new_repo=False):
>          """Clone this bzrdir and its contents to url verbatim.

It seems odd to me to have "BzrDir.open_workingtree()" have an internal
side effect of opening a branch (which is likely already open) and then
checking whether that Branch claims to have "recommend_upgrade" silenced.

I'm willing to chalk it up to bad layering of our Config code.

Also, this would be a case where to *me* it makes more sense to have
"format.open()" check the "recommend_upgrade" flag because then you
already have the wt.branch object.

It also fixed a problem where doing:

bzr get http://branch/with/tree

Was asking you to upgrade someone else's tree, even though it wasn't
getting far enough to actually open the tree. (Format.open() would be
raising NotLocalUrl, but we didn't get there yet).

> 
> === modified file 'bzrlib/config.py'
> --- bzrlib/config.py	2007-03-22 07:04:04 +0000
> +++ bzrlib/config.py	2007-04-17 03:13:12 +0000
> @@ -29,6 +29,7 @@
>  create_signatures=always|never|when-required(default)
>  gpg_signing_command=name-of-program
>  log_format=name-of-format
> +format_warning=no|yes(default)
>  
>  in locations.conf, you specify the url of a branch and options for it.
>  Wildcards may be used - * and ? as normal in shell completion. Options
> @@ -52,6 +53,8 @@
>                      branch is configured to require them.
>  log_format - this option sets the default log format.  Possible values are
>               long, short, line, or a plugin can register new formats.
> +format_warning - this option allows you to disable warnings for outdated
> +                 formats.
>  
>  In bazaar.conf you can also define aliases in the ALIASES sections, example
>  
> @@ -272,6 +275,20 @@
>      def _get_nickname(self):
>          return None
>  
> +    def get_format_warning(self):
> +        """Warn for outdated formats?"""
> +        dowarn = self._get_format_warning()
> +        if dowarn is not None and dowarn.lower() == "no":
> +            return "no"
> +        return "yes"
> +
> +    def _get_format_warning(self):
> +        dowarn = self.get_user_option('format_warning')
> +        if dowarn not in (None, "yes", "no"):
> +            raise AssertionError("Unknown format_warning value '%s' in %s"
> +                                 % (dowarn, self._get_filename()))
> +        return dowarn
> +
>  
>  class IniBasedConfig(Config):
>      """A configuration policy that draws from ini files."""
> @@ -682,6 +699,9 @@
>          """See Config.log_format."""
>          return self._get_best_value('_log_format')
>  
> +    def _get_format_warning(self):
> +        return self._get_best_value('_get_format_warning')
> +
>  


...

>  def ensure_config_dir_exists(path=None):
>      """Make sure a configuration directory exists.
> @@ -832,6 +852,8 @@
>      """Branch configuration data associated with its contents, not location"""
>      def __init__(self, branch):
>          self.branch = branch
> +        self._get_filename = \
> +            lambda: branch.control_files.get('branch.conf').name
>  
>      def _get_parser(self, file=None):
>          if file is not None:
> 

hmm.. opening a file to grab 'a_file.name' doesn't seem quite right.

*Especially* because branch.control_files.get() may be returning a
StringIO because Branch is remote.

If you just need the name you can use:

branch.control_files.controlfilename('branch.conf')

Which should give you a url for remote filenames. Actually, it will give
you a url for both local and remote names. But it is better than
fileobj.name().

Are you sure that you actually need _get_filename()? Since you can't
actually open that path.

John
=:->