[MERGE][Bug #162469] log displayers for custom revision

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Guillermo Gonzalez wrote:
> I agree that it should be considered a bug, but I wasn't sure if
> propagating the error and stop the log for a broken handler or keep
> executing log.

I think it's better to be noisily broken than to silently continue as if
nothing was wrong.  Silently changing behavior is confusing.

> should the log stop if a handler is broken?

I think so.  The user can always remove the handler.

> Thanks for the tip.
> How should it handle this errors? (I don't know exactly what errors a
> handler can raise, actually it can be any python or bzr related error)

I would recommend just letting it propagate.

But if you did have to suppress errors, I guess I would

try:
    foo()
except (KeyboardInterrupt, MemoryError):
    raise
except OSError, e:
    if e.errno == errno.EPIPE:
        raise
except:
    pass

>>  > === modified file 'bzrlib/tests/blackbox/test_log.py'
>>  > --- bzrlib/tests/blackbox/test_log.py 2008-01-10 22:34:09 +0000
>>  > +++ bzrlib/tests/blackbox/test_log.py 2008-03-31 03:56:49 +0000
>>  > @@ -184,6 +184,56 @@
>>  >          self.assertNotContainsRe(log, r'revno: 1\n')
>>  >          self.assertContainsRe(log, r'revno: 2\n')
>>  >          self.assertContainsRe(log, r'revno: 3\n')
>>  > +
>>
>>  These don't look like UI tests to me.  They should probably be in
>>  bzrlib/tests/test_log, instead.
> 
> I didn't added those three asserts, but I don't have any problem to move them.

No, I meant your tests in general.  They're not tests of the commandline
UI, they're tests of log handing.  So they shouldn't be in blackbox.

> I'll work on this issues and resubmit.

One more thing:

> +    def test_log_with_custom_properties(self):
> +        tree = self._prepare()
> +        tree.commit(message='', revprops={'first_prop':'first_value'})
> +        
> +        # define a trivial custom property handler
> +        def trivial_custom_prop_handler(props_dict):
> +            return props_dict
> +        
> +        bzrlib.log.custom_properties_handler_registry.register(
> +            'trivial_custom_prop_handler', 
> +            trivial_custom_prop_handler)
> +        log = self.run_bzr("log --limit 1")[0]
> +        self.assertContainsRe(log, r'first_prop: first_value\n')
> +        log = self.run_bzr("log -r1")[0]
> +        self.assertNotContainsRe(log, r'first_prop: first_value\n')
> +        bzrlib.log.custom_properties_handler_registry.remove(
> +            'trivial_custom_prop_handler')

when you alter global state, you should always use a try/finally block
(or equivalent mechanism) to restore the state to what it was.

So there should be a try after
bzrlib.log.custom_properties_handler_registry.register and the
bzrlib.log.custom_properties_handler_registry.remove should be in the
finally block.

> Thanks for the comments and guidance.

You're welcome.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFH9DDT0F+nu1YWqI0RAiTVAJ0QqR86c6X7fBxLUExHHDT8XNqOtwCfWxnV
TiiC8bjIiRRAaefNBoSPIvo=
=vr5/
-----END PGP SIGNATURE-----