Re: [Monotone-devel] comments on merging nvm.error-handling?

[Markus Wanner]

Hi,

I also had this on my list for reviewing, but didn't get around doing
much...

Timothy Brownawell wrote:
> net.venge.monotone.error-handling is meant to clean up our E(), I(), N()
> usage somewhat, so that for example receiving an invalid revision from
> the network will be treated differently than internally generating an
> invalid revision and won't crash the server.

Yeah, that sounds fine.

> N() is gone, and E() takes an extra argument, E(condition, origin,
> message). The origin is an origin::type enum (see origin_type.hh), and
> indicates where the data causing the error is from. E(...,
> origin::user, ...) is equivalent to what N() was.

Hm.. okay.

> All vocab types and most other classes now track where their data came
> from with a origin::type made_from member (which can be obtained by
> inheriting from origin_aware), and this is required when making a vocab
> type from a string (so "branch_name(arg())" won't work, but
> "branch_name(arg(), origin::user)" or "typecast_vocab<branch_name>(arg)"
> will work.
> 
> This also removes informative_failure, and replaces it with
> recoverable_failure and unrecoverable_failure. Which one is thrown by
> E() depends on the origin, currently unrecoverable for internal and
> database origins, and recoverable for others.

Sounds good. I'm thinking of E() as 'E'xception. I don't mind much, if
the outer code can recover it or not. Giving an additional origin
information is roughly equivalent to different exception types, IMO.


Please remove commented out code, as in transforms.hh, ~ line 98. And
sanity.hh, ~ line 21. Maybe other places as well.


I for one am always confused about the conditions in E(), because I
always have to look up which direction they work. And it gets especially
confusing with negated conditions. The additional origin argument
doesn't increase readability either.

I'd even prefer writing out the condition to de-obfuscate, for example not:

   E(!null_id(edge_old_revision(i)), made_from,
     F("Merge revision has a null parent"));

but better:

   if (null_id(edge_old_revision(i))
     E(made_from, F("Merge revision has a null parent"));

Or similar. Those very few additional characters would IMO help
readability and understandability a lot. But maybe that's just me.

> Does anyone have problems with merging this?

No, do a little cleanup (no commented out code, please) and then go
ahead and merge.

Regards

Markus Wanner

P.S.: I'd like to review nvm.tbrownaw.serve_automate as well, but don't
know if I get around doing it soon...