[Fwd: Re: RFC: ERROR: Unsupported branch format (improve message)]

[Martin Pool]

On 3/30/07, John Arbash Meinel <john at arbash-meinel.com> wrote:
> >>  class UnsupportedFormatError(BzrError):
> >>
> >> -    _fmt = "Unsupported branch format: %(format)s"
> >> +    _fmt = "Unsupported branch format: %(format)s\nPlease run 'bzr upgrade'"
> >>
> >>
> >>  class UnknownFormatError(BzrError):
> >
> > +1, but i would also remove the word "branch" from the message since
> > in Jari's case it's a repository not a branch (i think).
> >
> > Also in general these would be clearer if they give the path - people
> > might wonder how init can complain  about an existing format;
> > presumably because of a containing bzrdir.
> >
>
> Are you thinking of the 'Warning Format out of date, you should upgrade'
> comments?

I am; if I say so myself I think it is pretty nice when you can just
give the user the command they ought to run, and at least when there
is a problem with an object or directory we should always try to say
*which*.

There are some caveats with this kind of thing: the user might
reasonably ask "well doesn't bzr do it automatically?", and conversely
if there are reasons like compatibility why they might want to not
upgrade we should make sure they understand that.  And (Ian?) as
previously noted with regard to tags we do need to make sure the
suggested action will actually fix the problem.

> This is more for 'bzr status' or 'bzr branch' from a 0.0.4 format branch
> (gzipped text files).
>
> I looked into adding a url parameter, but the way the code is
> structured, it requires a bit of work to pass everything around.
> (BzrDir._check_supported only gets a format parameter, and whether
> 'unsupported' formats are allowed, it doesn't know the url).
>
> If it is important we can fix things. But I felt UnsupportedFormat is
> rare enough that it isn't worth messing with.

It's not a big deal in this case; I would try to pass the url (or
object containing it) if restructuring this code though.

> Oh, and your 0.15rc3 changes also have a small problem with remote
> working trees.
>
> It seems they check that the format needs to be upgraded *before* it
> checks whether the WorkingTree is NotLocalUrl.
>
> Which means if someone else's branches have working trees, you can get:
>
> bzr: warning: you should run 'bzr upgrade http://someone/elses/branch'
>
> IMO the upgrade warning should be done inside the 'format.open()' rather
> than in _check_supported(). for that reason.

I think I did look at putting it there, but that seemed to cause more
duplication than going into _check_supported, partly because more
functions would have to pass along the option to disable warnings.

I think the right fix is probably that _cloning_metadata should say "I
don't care if it's an old format", as in this patch.

> 1) Are you planning on merging the changes to bzr.dev any time soon? I'm
> not sure exactly what hasn't been merged. But all of the tree upgrade
> recommendations are not in 'bzr.dev'.

Yes, I'll do that today.

> 2) Why did you chose this indenting:
>     def _check_supported(format, allow_unsupported,
>         recommend_upgrade=True,
>         basedir=None):
>
> I personally prefer:
>     def _check_supported(format, allow_unsupported,
>                          recommend_upgrade=True,
>                          basedir=None):
>
> It isn't a big deal, but we should try to be consistent. (I think PEP8
> gives an example of the latter form, but I didn't see it explicitly stated)

I slightly prefer the second too, partly because it looks different to
normal structural indentation.  However, since we tend to have pretty
long class/method names, that can mean the margin is a long way over
and so the parameters spill over many lines.  Like

 "here are some variables with a long string" % (a,
                                                          b,
                                                          c)

So I tend to use the second style unless it's "too wide" and I guess
this one tripped that threshold.

-- 
Martin