[merge] BzrDir controls file modes, not LockableFiles

[Martin Pool]

On Wed, May 21, 2008 at 12:05 PM, Ian Clatworthy
<ian.clatworthy at internode.on.net> wrote:
> The API CHANGES section needs to be put inside IN DEVELOPMENT,
> not above it.

Yes, I just put it there to make sure it wasn't incorrectly put into
1.5 as that was merging around the same time.

>
>
>> @@ -70,13 +73,17 @@
>>          os.mkdir('c')
>>          os.chmod('c', 02750)
>>          b = self.make_branch('c')
>> -        self.assertEqualMode(02750, b.control_files._dir_mode)
>> +        self.assertEqualMode(02750, b.bzrdir._get_dir_mode())
>> +        self.assertEqualMode(02750, b.bzrdir._get_dir_mode())
>> +        self.assertEqualMode(00640, b.control_files._file_mode)
>>          self.assertEqualMode(00640, b.control_files._file_mode)
>
> This change to the tests isn't right. You want to be testing
> _dir_mode, _get_dir_mode, _filemode and _get_file_mode. Currently
> you're testing only two of those and doing each one twice. :-)

Nice catch.

>> There is one open issue: at the moment LockableFiles has this:
>>
>>     # If set to False (by a plugin, etc) BzrBranch will not set the
>>     # mode on created files or directories
>>     _set_file_mode = True
>>     _set_dir_mode = True
>>
>> I haven't duplicated this functionality, and I'm not sure it is still
>> needed.  Does anyone actually use such a plugin?  If we want the option
>> for users to turn off mode setting or force a particular mode maybe that
>> would be better controlled by a config setting?
>
> I'd suggest using annotate to find out when these lines were added.
> If early on, they may well be yagni. If added explicitly later, you'd
> imagine their author put them in for a particular reason.

It was John, but the message is not particularly illuminating.  I'll
add something better using configs if it turns out to be needed; for
now I've removed this interface.

Thanks for the review.

-- 
Martin <http://launchpad.net/~mbp/>