Merge lp:~nmb/brz/warn-bazaar-directory into lp:brz

[Neil Martinsen-Burrell]

This prints a warning ONCE when the user is using the `.bazaar` configuration directory. Further uses of the directory are still logged to `.brz.log` as before. We could easily make that logging also happen only once if we would prefer less spam in that file.

[Martin Packman (gz)]

There are two complicating factors here that this change does address:

* We don't want to do all the work to load config each time this function is called
* We still need a generic don't multiply warn for various things

As a concrete suggestion, I think a better choice is storing the config on global state rather than having a warning_printed attr, which will also happen to make any warning only appear once.

Is there any reason to refresh the config dir mid process execution?

[Jelmer Vernooij (jelmer)]

> There are two complicating factors here that this change does address:
>
> * We don't want to do all the work to load config each time this function is
> called
> * We still need a generic don't multiply warn for various things
Such a system is tricky for this case though, since we'd probably use 'suppress_warnings=config_dir' or somesuch in the config to suppress it.

> As a concrete suggestion, I think a better choice is storing the config on
> global state rather than having a warning_printed attr, which will also happen
> to make any warning only appear once.
>
> Is there any reason to refresh the config dir mid process execution?
Environment variables can be changed, e.g. by tests. It doesn't seem like this MP is making the overhead of determining the config dir much worse though. The code path is also specific to the fallback case where ~/.config/breezy doesn't exist, but ~/.bazaar does.

[Neil Martinsen-Burrell (nmb)]

> * We don't want to do all the work to load config each time this function is
> called

This proposal doesn't materially change the work that this function was already doing. If it's desired to optimize the performance, that could be a separate change.

> * We still need a generic don't multiply warn for various things

Yes, that need will still exist after this change.

> As a concrete suggestion, I think a better choice is storing the config on
> global state rather than having a warning_printed attr, which will also happen
> to make any warning only appear once.

This attribute is only checked inside this function and shouldn't affect any other warning.

> Is there any reason to refresh the config dir mid process execution?

I have no idea, but this function certainly gets called many times during the execution of a single command such as "brz status", at least in my testing. In fact, this function gets called at plugin import time, before logging is even configured.

[Martin Packman (gz)]

Okay, looking over again there are two bits to address.

First is the warnings/stderr thing - which I think we're fine punting on to a later branch, so the output code is fine (but just use stderr directly, see below).

Second is we end up calling config.config_dir() and while it doesn't do *much* work, we do keep adding to it. So, we should dangle the result off the library state. We can come up with a nice idiom for that later, but for something like:

    existing_dir = getattr(breezy.global_state, 'config_dir', None)
    if existing_dir is not None:
        return existing_dir
    ...
    breezy.global_state = resolved_dir
    return resolved_dir

Note, this means you don't need/want the warn once logic.

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

Hi Neil,

If there is anything Martin or I can do to help with this, please let us know. :-)

Jelmer

[Neil Martinsen-Burrell (nmb)]

I fixed the error message according to Jelmer's comment. I am not able to use `breezy.global_state` to store this value because that object is not initialized when `config_dir()` is called for the first four times. The advantage of storing an attribute on the `config_dir` function within `config.py` is that it is guaranteed to be available when the function is called.

Here's the experiment I tried:

--- old/breezy/config.py 2017-06-15 01:07:35 +0000
+++ new/breezy/config.py 2017-07-03 18:35:48 +0000
@@ -1405,10 +1405,16 @@

     By default this is %APPDATA%/breezy on Windows, $XDG_CONFIG_HOME/breezy on
     Mac OS X and Linux. If the breezy config directory doesn't exist but
- the bazaar one (see bazaar_config_dir()) does, use that instead.
+ the bazaar one (see bazaar_config_dir()) does, use that instead, with a
+ warning.

     TODO: Global option --config-dir to override this.
     """
+ # return memo-ized result if we have already been here
+ existing_dir = getattr(breezy.global_state, 'config_dir', None)
+ if existing_dir is not None:
+ return existing_dir
+
     base = osutils.path_from_environ('BRZ_HOME')
     if sys.platform == 'win32':
         if base is None:
@@ -1423,12 +1429,21 @@
             base = osutils.pathjoin(osutils._get_home_dir(), ".config")
     breezy_dir = osutils.pathjoin(base, 'breezy')
     if osutils.isdir(breezy_dir):
+ print "setting global_state.config_dir", breezy_dir
+ breezy.global_state.config_dir = breezy_dir
         return breezy_dir
     # If the breezy directory doesn't exist, but the bazaar one does, use that:
     bazaar_dir = bazaar_config_dir()
     if osutils.isdir(bazaar_dir):
- trace.mutter(
- "Using Bazaar configuration directory (%s)", bazaar_dir)
+ sys.stderr.write("Using fallback Bazaar configuration directory (%s) "
+ "rather than (%s).\n" % (bazaar_dir, breezy_dir))
+ try:
+ print "setting global_state.config_dir", bazaar_dir
+ breezy.global_state.config_dir = bazaar_dir
+ except AttributeError:
+ print "failed to set global_state.config_dir"
+ import traceback; traceback.print_stack()
+ pass
         return bazaar_dir
     return breezy_dir

Gives the following output where it tries to set global_state.config_dir four times unsuccessfully:

$ ./brz --help
Using fallback Bazaar configuration directory (/Users/neil/.bazaar) rather than (/Users/neil/.config/breezy).
setting global_state.config_dir /Users/neil/.bazaar
failed to set global_state.config_dir
File "./brz", line 111, in <module>
    library_state = breezy.initialize()
  File "/Users/neil/proj/brz/warn-bazaar-directory/breezy/__init__.py", line 228, in initialize
    ui_factory = breezy.ui.make_ui_for_terminal(stdin, stdout, stderr)
  File "/Users/neil/proj/brz/warn-bazaar-directory/breezy/ui/__init__.py", line 528, in make_ui_for_terminal
    stdin = _wrap_in_stream(stdin)
  File "/Users/neil/proj/brz/warn-bazaar-directory/breezy/ui/text.py", line 651, in _wrap_in_stream
    encoding = _get_stream_encoding(stream)
  File "/Users/neil/proj/brz/warn-bazaar-dire...

[Martin Packman (gz)]

Yes, I see the tricky bit here. A fun combination of config having global state when it shouldn't and ui trying to set itself up with config which is also mostly a bad idea.

I'll have a go at unpicking the internal arrangements from the ui side and see how far I get.

Unmerged revisions

6709. By Neil Martinsen-Burrell on 2017-06-19

Warn when using .bazaar configuration directory

=== modified file 'breezy/config.py'
--- breezy/config.py	2017-06-15 01:07:35 +0000
+++ breezy/config.py	2017-06-19 15:41:24 +0000
@@ -1405,7 +1405,8 @@
 
     By default this is %APPDATA%/breezy on Windows, $XDG_CONFIG_HOME/breezy on
     Mac OS X and Linux. If the breezy config directory doesn't exist but
-    the bazaar one (see bazaar_config_dir()) does, use that instead.
+    the bazaar one (see bazaar_config_dir()) does, use that instead, with a
+    warning.
 
     TODO: Global option --config-dir to override this.
     """
@@ -1427,8 +1428,20 @@
     # If the breezy directory doesn't exist, but the bazaar one does, use that:
     bazaar_dir = bazaar_config_dir()
     if osutils.isdir(bazaar_dir):
-        trace.mutter(
-            "Using Bazaar configuration directory (%s)", bazaar_dir)
+        # only print this warning message to the user once by saving
+        # an attribute on this function object.
+        config_dir.warning_printed = getattr(config_dir, 'warning_printed', False)
+        if not config_dir.warning_printed:
+            config_dir.warning_printed = True
+            # logging isn't initialized when this is first called, so just
+            # write to stderr.
+            def _write_stderr(format_str, args):
+                sys.stderr.write(format_str % args + '\n')
+            output_fn = _write_stderr
+        else:
+            output_fn = trace.mutter
+        output_fn("Using fallback Bazaar configuration directory (%s)",
+                  bazaar_dir)
         return bazaar_dir
     return breezy_dir
 

=== modified file 'doc/en/release-notes/brz-3.0.txt'
--- doc/en/release-notes/brz-3.0.txt	2017-06-19 01:04:38 +0000
+++ doc/en/release-notes/brz-3.0.txt	2017-06-19 15:41:24 +0000
@@ -99,6 +99,9 @@
 * Support ``brz commit -x`` in combination with iter_changes.
   (Jelmer Vernooij, #796582, #403811, #694946, #268135, #299879)
 
+* Warn when user is running with the ``.bazaar`` configuration directory.
+  (Neil Martinsen-Burrell, #1698246)
+
 Documentation
 *************