ObjectNotLocked. What I should do?

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Aaron Bentley wrote:
> === modified file __init__.py
> +        if not set and not clear:
> +            raise errors.BzrCommandError(
> +                'You must supply either --set or --clear')
> +        if set and clear:
> +            raise errors.BzrCommandError(
> +                'You cannot supply both --set and --clear')
> +        # At this point 'set' will always be the desired value. Because if
> +        # 'set' is true, then the execute bit should be set, if it is
> False,
> +        # that means 'clear' must be True, which means the executable bit
> +        # should be False.
> 
> ^^ I think it would be clearer to use RegistryOption.from_kwargs:
> RegistryOption.from_kwargs('executable', value_switches=True,
>     enum_switch=False, set="set x-bit for file",
>     clear="clear x-bit for file")
> 

I'll agree with you, but I haven't used RegistryOption yet :)

There were a couple problems I ran into...

1) I needed to use:

run(self, files_list, executable=None):

  if executable is None:
    raise errors.BzrCommandError('you must supply...')

Because otherwise if they did not supply a value it wouldn't pass
'executable=', which means that it gets an AttributeError and a nasty
traceback.

2) For some reason I thought executable would be set to True/False
rather than 'set' or 'clear'. Again not a big problem, I just put
another if/else check to get a boolean.

3) However, TreeTransform.set_executability() will very happily allow
you to pass 'set' as the parameter.

tt.set_executability(trans_id, 'set')

And in fact, it passes all the way down until dirstate wants to write
the content out, when it tries do convert a True/False value into 'y' or
'n'. And the dictionary lookup fails.

I can't say who exactly to blame. Should TT.set_executability() check
that it's parameter is a boolean rather than a string?
Should DirState.set_state_from_inventory() handle when IE.executable is
a string rather than True/False?

Both cases could pretty simply do "executable = bool(executable)".

It would still be wrong for what I meant it to do for
"set_executability(trans_id, 'clear')". But at least it would just not
clear the bit, rather than causing the internals to puke.

I might argue that it is TT's responsibility, since it technically is
generating an invalid Inventory. The only way to make IE be forcibly
correct would be to change ie.executable to be a property rather than an
attribute.

Or we could have DirState.set_state_from_inventory() go through and
assert that the InventoryEntries are all sane.

I'm not 100% sure. But certainly we need to catch bogus data being input
earlier.


> 
> +        tree, relpaths = builtins.tree_files(files_list)
> +        tt = None
> +        tree.lock_tree_write()
> +        try:
> +            tt = transform.TreeTransform(tree)
> 
> TreeTransform takes out a write lock on construction, and releases it on
> TreeTransform.apply or TreeTransform.finalize.  So there is no need to
> explicitly lock the tree.

Ok. I misread the TreeTransform initializer. It has a 'try/except' which
I read as 'try/finally'.
So certainly that can be changed to:

tree, relpaths = builtins.tree_files(files_list)
tt = transform.TreeTransform(tree)
try:
...
finally:
  tt.apply()


> 
> +            for fname in relpaths:
> +                absfname = tree.abspath(fname)
> +
> +                fid = tree.path2id(fname)
> +                if fid is None:
> +                    self.outf.write("Path %s is not versioned"
> +                                    % (fname,))
> +                    continue
> 
> Is there a point determining absfname if it's not versioned?
> 
> Aaron

Not specifically, it was mostly the way the code was written before, so
I didn't pay a lot of attention to where it was directly needed.

Updated bundle attached.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGCsLrJdeBCYSNAAMRAmMGAJwJ/HCKVQbABp0Lu1PkYztZZqj9XgCfWPkg
Pa5dWFubizHt2H77nAqXp1k=
=IDv+
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: x_bit_no_inventory2.patch
Type: text/x-patch
Size: 12060 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070328/83e353d9/attachment-0001.bin