[MERGE] ``BZR_LOG=disable`` suppresses writing messages to .bzr.log.

[John Arbash Meinel]

Alexander Belchenko wrote:
> John Arbash Meinel пишет:
>> Alexander Belchenko wrote:
>>> This patch adds support for BZR_LOG env variables that allows to 
>>> suppress .bzr.log.
>>>
>>
>> It seems to me that it should be done by the 'bzr' code, not inside 
>> the bzrlib code.
>>
>> Specifically, inside 'bzr' we have:
>> if __name__ == '__main__':
>>     bzrlib.trace.enable_default_logging()
>>     exit_val = bzrlib.commands.main(sys.argv)
>>
>> If someone is asking 'bzrlib' to enable the log, then I think we 
>> should be doing so.
>>
>> So I would prefer something like:
>>
>> if __name__ == '__main__':
>>   if os.environ.get('BZR_LOG', '').lower() != 'disable':
>>     bzrlib.trace.enable_default_logging()
> 
> I disagree. enable_default_logging() is also setup note/warning functions
> that goes to stderr. I don't think we want to suppress this messages
> as well. As another solution we can provide optional argument
> for enable_default_logging() to specify if we want to not open .bzr.log
> tracefile. Something like this:
> 
> === modified file 'bzrlib/trace.py'
> --- bzrlib/trace.py 28.01.2008 10:40:24
> +++ bzrlib/trace.py 28.01.2008 16:22:16
> @@ -223,7 +223,7 @@
>      mutter(traceback.format_exc())
> 
> 
> -def enable_default_logging():
> +def enable_default_logging(enable_tracefile=True):
>      """Configure default logging to stderr and .bzr.log"""
>      # FIXME: if this is run twice, things get confused
>      global _stderr_handler, _file_handler, _trace_file, _bzr_log_file
> @@ -233,7 +233,7 @@
>      _stderr_handler = logging.StreamHandler(stderr)
>      logging.getLogger('').addHandler(_stderr_handler)
>      _stderr_handler.setLevel(logging.INFO)
> -    if not _file_handler:
> +    if not _file_handler and enable_tracefile:
>          open_tracefile()
>      _trace_file = _bzr_log_file
>      if _file_handler:
> 
> 
> Is it OK for you?
> 

I like that.


>> As far as having it in a config file rather than in an environment 
>> variable... I understand the utility of it, but by the time we get to 
>> parsing config files, we've already started logging.
>>
>> So we *could* do that, but it would be difficult to make it not log 
>> anything.
>>
>> I'm also not a big fan of calling it "BZR_LOG=disable". I might prefer 
>> BZR_DISABLE_LOG=1
>> or
>> BZR_ENABLE_LOG=0
> 
> I'm not big fan of long names and numerical binary values, but I tend to 
> agree
> that BZR_LOG is better suited for controlling exact absolute path of 
> .bzr.log.
> 

I agree on all points. I wasn't sure what was best. Maybe "BZR_LOG=''" 
to indicate that it should not have a log file. I'm not a big fan of 
having an empty env variable mean something different than one with a value.

I would also be okay with the word "disabled" as an obvious flag that 
the log file should not be used. So if you did:

BZR_LOG=mytest.log bzr foo

versus

BZR_LOG=disabled bzr foo

It is a bit "magic", but I don't think anyone ever needs to log to a 
file named "disabled" and if they did, they could log to "./disabled".

So for purity sake, I prefer a separate enable/disable env var, but we 
could make double duty for BZR_LOG as long as we make it clear what is 
going on.

If you have the time, I know people have asked in the past for the 
ability to move the BZR_LOG (I think this was mostly people connecting 
to 'root@' but not wanting to give access to /root/.bzr.log, or 
something like that.)

John
=:->