[merge][0.91] set default format to dirstate-tags

[Robert Collins]

On Tue, 2007-08-14 at 23:37 +1000, Martin Pool wrote:
> On 8/14/07, Robert Collins <robertc at robertcollins.net> wrote:

> >
> > I'm not entirely clear what you mean here. I guess you are saying that
> > 'tests which are using branches with a revision id *not* present in the
> > repository seem to be doing so accidentally so they no longer do'. (My
> > confusion is in the binding of 'this').
> 
> Yes: there is a situation S where a branch's last revision is set to a
> revision not in its repository.  There are no branch impl tests for S,
> so I guess it's not specifically supported.  There are some tests
> which incidentally depend on S being supported and not supported in
> particular cases, but the tests don't really seem to care about S, so
> I changed them.
> 
> But if someone strongly feels that this should either be supported or
> forbidden, we need to add tests for it and update the branch code.  I
> think that would not need to block this merge.

I think that its possible to create a branch in this situation by hand
(shared repo, then cp the branch), so it would be good to know what
happens when its like this, but operations on such a branch should fail
rather than giving wrong answers; and some branches can do more
operations before failing, so theres a little care needed in doing tests
about this. I strongly think our test suite should not test for this
situation as a 'normal' thing, rather its testing a specific form of
failure mode that we've seen in the field. I agree that future work is
fine for this rather than blocking the merge.


> > I think I'd prefer to see append_revision deleted, or the prior contract
> > upheld, because this is less likely to result in 'it did the wrong
> > thing' surprises.
> 
> I'm ok to delete it, aside from the general principle that we
> deprecate things rather than deleting them.  This does not seem like
> something many plugins would be using, aside from (as you said)
> importers.  Having seen a few inconsistent behaviours I would say that
> I haven't changed the contract, just moved within the existing
> untested wiggle room.  But maybe it will break callers that depend on
> the old code.

I think saying that you've changed the contract for the default branch
format is fair, even with the wiggle room that our test suite allowed.
And as thats what people depend on... please delete it as you say you
are ok with that.

> > When is branch.supports_tags() called? Would it be better for
> > RemoteBranch to proxy the method to the smart server, than to
> > unconditionally report True? (Put another way, what happens if it
> > reports true and a client tries to use tags, but the underlying branch
> > doesn't ? - and if this doesn't cause failures (other than tag commands
> > not tagging ;)), why do we have 'supports_tags' as a method?
> 
> It's branch_format.supports_tags(), so we can't pass it through.  It's
> only used from the test suite, which wants to check that either all
> the tag methods work or all raise notsupported.  Maybe it should check
> that directly, or maybe it should be renamed to _wants_tag_tests or
> similar.

Sounds to me like this should be on branch not on the format, because -
using the smart server as an example - its not an attribute of the
format, its an attribute of the instance. Doesn't need to block this
merge as it's existing code, but it would be nice to file a bug for both
this change and the aforementioned branch-tip-not-in-repository testing
that should be done.

-Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070815/f4d9570a/attachment.pgp