[merge][0.11] 'bzr annotate' fails on empty files

[Wouter van Heyst]

On Thu, Sep 21, 2006 at 06:26:40PM -0500, John Arbash Meinel wrote:
> Wouter van Heyst wrote:
> > On Thu, Sep 21, 2006 at 02:43:24PM -0500, John Arbash Meinel wrote:
> >> 'bzr annotate' calls max(list) but the list is empty if there are no
> >> lines in the file.
> >>
> >> The attached patch adds a test case, and a fix for bug:
> >> https://launchpad.net/products/bzr/+bug/56814
> >>
> >> John
> >> =:->
> > 
> > 
> > +1
> > 
> > ...
> > 
> >>      annotation = list(_annotate_file(branch, rev_id, file_id))
> >> -    max_origin_len = max(len(origin) for origin in set(x[1] for x in annotation))
> >> +    if len(annotation) == 0:
> >> +        max_origin_len = 0
> >> +    else:
> >> +        max_origin_len = max(len(origin) for origin in set(x[1] for x in annotation))
> >>      for (revno_str, author, date_str, line_rev_id, text ) in annotation:
> > 
> > Any reason you didn't go with
> > 
> > try:
> >     max_origin_len = max(len(origin) for origin in set(x[1] for x in annotation))
> > except ValueError:
> >     max_origin_len = 0
> > 
> > 
> > The former is perhaps clearer, and directly convertable to the 2.5
> > conditional expression, but these cases always makes me wonder what
> > approach to use.
> > 
> > Wouter van Heyst
> 
> Personally, I believe in checking your data before, rather than waiting
> for an exception. I use the latter for stuff like Transport, because
> there is a specific performance issue with doing a round trip to
> validate. Also, there could be many reasons why the loop might raise a
> ValueError, and I only know that I'm fixing 1 case.
> 
> Especially something like ValueError, lots of things raise ValueError. I
> try to only catch it when there is only one source, so I don't suppress
> other bugs.

True. There are two issues I keep in mind, being the possible race
condition and the cost (in this case, len(annotation) is probably ok).
It remains a tradeoff, I agree catching ValueError opens up
correctnes problems and the race shoudln't be a problem here.


Wouter van Heyst