[MERGE] Remove most calls to safe_file_id and safe_revision_id.

Ian Clatworthy ian.clatworthy at internode.on.net
Wed Sep 26 06:49:53 BST 2007


Martin Pool wrote:
> As previously suggested by John, this removes the compatibility
> interface that accepted unicode objects for revision_id parameters.

The first question that came to mind in this review as "OK, what's
left?". Outside the definition and tests, I found 3 uses of
safe_revision_id left in the code:

* builtins.py
* workingtree.py
* bundle/bundle_data.py

All of those had the warn=False parameter (indicating user input) so
that looks good to me. Likewise, I found 3 uses of safe_file_id and all
of those had warn=False as well. Checking the code, it looks like
leaving them there is correct as well.

In summary, I don't believe you've missed anything.

bb:tweak


> === modified file 'bzrlib/workingtree.py'
> --- bzrlib/workingtree.py	2007-09-13 22:41:38 +0000
> +++ bzrlib/workingtree.py	2007-09-25 08:14:12 +0000

> -        # TODO: jam 20070209 This should be redundant, as the revision_id
> -        #       as all callers should have already converted the revision_id to
> -        #       utf8
> -        inventory.revision_id = osutils.safe_revision_id(revision_id)
>          return xml7.serializer_v7.write_inventory_to_string(inventory)

This is a semantic change in that inventory.revision_id is no longer
being set here. Was that deliberate? (It stands out as being more than a
mechanical change as 99% of the other changes in this patch are.) I
haven't analysed the various data flows but it doesn't look a safe
change.

> @@ -1975,15 +1951,6 @@
>      @needs_tree_write_lock
>      def set_root_id(self, file_id):
>          """Set the root id for this tree."""
> -        # for compatability 
> -        if file_id is None:
> -            symbol_versioning.warn(symbol_versioning.zero_twelve
> -                % 'WorkingTree.set_root_id with fileid=None',
> -                DeprecationWarning,
> -                stacklevel=3)
> -            file_id = ROOT_ID
> -        else:
> -            file_id = osutils.safe_file_id(file_id)
>          self._set_root_id(file_id)

This change breaks the test suite:

  File
"/home/ian/bzr/bzr.tmp/bzrlib/tests/workingtree_implementations/test_set_root_id.py",
line 33, in test_set_None
    tree.set_root_id, None)
AssertionError: not equal:
a = ['WorkingTree.set_root_id with fileid=None was deprecated in version
0.12.']
b = []

It wouldn't hurt to leave the if clause there. Otherwise, the test suite
will need a tweak if you'd prefer (0.12 was a while ago now).

Ian C.



More information about the bazaar mailing list