Merge lp:~parthm/bzr/no-chown-if-bzrlog-exists into lp:bzr

[Parth Malwankar]

Only chown() the .bzr.log when creating it.

This mp fixes the issue discussed in this mail:
http://article.gmane.org/gmane.comp.version-control.bazaar-ng.general/67274

Basically, .bzr.log permissions were updated even when the file already existed rather than doing it just during file creation.

[Robert Collins (lifeless)]

This doesn't actually solve the issue, as there is a race condition :).

[Parth Malwankar (parthm)]

> This doesn't actually solve the issue, as there is a race condition :).

You are right of course. Thanks for pointing that out.

After experimenting for some time I can't think of a good solution for the race condition. One option might be to just open() the file with 'at' later chown only if the ownership is different from from parent dir. The disadvantage is that we will do two stats per invocation. 1 for the parent dir and 1 for the file itself. Also, this will lead to bzr _always_ maintaining .bzr.log ownership the same as that of the containing directory. The could be considered a bug or a feature :-)
I will explore some more and see if I can come up with something.

Suggestions welcome.

[John A Meinel (jameinel)]

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

Parth Malwankar wrote:
>> This doesn't actually solve the issue, as there is a race condition :).
>
> You are right of course. Thanks for pointing that out.
>
> After experimenting for some time I can't think of a good solution for the race condition. One option might be to just open() the file with 'at' later chown only if the ownership is different from from parent dir. The disadvantage is that we will do two stats per invocation. 1 for the parent dir and 1 for the file itself. Also, this will lead to bzr _always_ maintaining .bzr.log ownership the same as that of the containing directory. The could be considered a bug or a feature :-)
> I will explore some more and see if I can come up with something.
>
> Suggestions welcome.
>

There *is* a race condition, but the window is small, and it is better
to be racy and get it mostly right than to fail in obvious ways.

If someone has set perms on .bzr.log explicitly, I'd rather we didn't
keep changing it on them. Given that .bzr.log is in ~, it is *possible*
you'd have them set differently. (I don't want people to see what files
are in ~, but I want to share ~/.bzr.log, for example. Set the dir to
rwx--x--- and the file to rw-r-----)

If the file doesn't exist, we should try to be nice about what perms the
file gets.

The only way I see to avoid a race is to do something like
os.open(O_EXCL) and if that succeeds, then we chmod/chown. If it fails,
then we just fall back to opening regularly.

Also note that it is only 'racy' if they:

 1) For some reason create .bzr.log themselves
 2) Start 2 bzr processes simultaneously that are *different* versions.
    Otherwise, the 2 versions of bzr would use the same logic. And while
    it make chown twice, they would do the same thing.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkuuCMkACgkQJdeBCYSNAAM5dACgvJpiReYnhZzkJQP9xBLJOBE5
SLUAoL+Ew9G0uk4N88VQSdALDxIb1ySV
=zhfY
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

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

Parth Malwankar wrote:
>> This doesn't actually solve the issue, as there is a race condition :).
>
> You are right of course. Thanks for pointing that out.
>
> After experimenting for some time I can't think of a good solution for the race condition. One option might be to just open() the file with 'at' later chown only if the ownership is different from from parent dir. The disadvantage is that we will do two stats per invocation. 1 for the parent dir and 1 for the file itself. Also, this will lead to bzr _always_ maintaining .bzr.log ownership the same as that of the containing directory. The could be considered a bug or a feature :-)
> I will explore some more and see if I can come up with something.
>
> Suggestions welcome.
>

I should mention, *I* prefer having this patch to not having it. So
 review: approve

But it seems like Robert is either hesitant or objecting, though he
didn't actually vote.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkuuCZEACgkQJdeBCYSNAAPNVACeMPCZ70YgF2rQzEqvPLzb09aK
pOsAmwdDCRuFZMMqrW7Y/u+0XPejBFyv
=UQ/q
-----END PGP SIGNATURE-----

review: Approve

[Martin Packman (gz)]

This can be done completely non-racily with os.open - a really picky version could be a function something like:

    lomode = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT
    while True:
        try:
            return os.open(filename, lomode)
        except OSError, e:
            if e.errno != errno.ENOENT:
                raise
        try:
            fd = os.open(filename, lomode | os.O_CREAT | os.O_EXCL)
        except OSError, e:
            if e.errno != errno.EEXIST:
                raise
        else:
            copy_ownership(filename, ownership_src)
            return fd

And os.fdopen to make a regular file object.

Raises the question though, is it ever going to be right for a osutils.open_with_ownership function to chown the file if it's *not* just created it? Makes me wonder again whether bundling the chown into the open function makes any sense.

[Parth Malwankar (parthm)]

> This can be done completely non-racily with os.open - a really picky version
> could be a function something like:
>
> lomode = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT
> while True:
> try:
> return os.open(filename, lomode)
> except OSError, e:
> if e.errno != errno.ENOENT:
> raise
> try:
> fd = os.open(filename, lomode | os.O_CREAT | os.O_EXCL)
> except OSError, e:
> if e.errno != errno.EEXIST:
> raise
> else:
> copy_ownership(filename, ownership_src)
> return fd
>
> And os.fdopen to make a regular file object.
>

Thanks for this.
This certainly looks good to me. Would this approach work on Mac? In the python docs[1] the availability of os.open is shown as Unix and Windows. I don't know much about Mac so I ask.

> Raises the question though, is it ever going to be right for a
> osutils.open_with_ownership function to chown the file if it's *not* just
> created it? Makes me wonder again whether bundling the chown into the open
> function makes any sense.

I agree, I don't see too much use of open_with_ownership in a non-create case.
I can try to create and test a patch based on the above approach which also deals with open_with_ownership. Its used only in one another place IIRC.

[1] http://docs.python.org/library/os.html#os.open

[Martin Packman (gz)]

> This certainly looks good to me. Would this approach work on Mac? In the
> python docs[1] the availability of os.open is shown as Unix and Windows. I
> don't know much about Mac so I ask.

The Python docs are a little confusing on this, but by their definition there is no longer such a thing as a mac, anything running OS X just another unix. Upstream Python has pretty much dropped support for everything but posix and windows systems.

> > Raises the question though, is it ever going to be right for a
> > osutils.open_with_ownership function to chown the file if it's *not* just
> > created it? Makes me wonder again whether bundling the chown into the open
> > function makes any sense.

Got my thinking a little backwards here, if chown should only be on create, building that logic into open_with_ownership probably makes sense.

> I agree, I don't see too much use of open_with_ownership in a non-create case.
> I can try to create and test a patch based on the above approach which also
> deals with open_with_ownership. Its used only in one another place IIRC.

Worth pondering how you think the interfaces should work, there are currently three new public functions in osutils, and only as many callsites.

[Parth Malwankar (parthm)]

Updated based on file creation code by Martin [gz]. Thanks.
Removed open_with_ownership and copy_with_ownership. Now copy_ownership is used directly.

[Parth Malwankar (parthm)]

One minor annoyance with that is that .bzr.log is created with the executable bit set due to O_CREAT. As per the open(2) man page:

The effective permissions are modified by the process's umask in the usual way: The permissions of the created file are (mode & ~umask).

On my system the mask is 022. I suppose we can do a chown specifically to unset executable bit. Thoughts?

Note: This isn't tested on Windows yet as I don't have a windows system available at the moment.

[John A Meinel (jameinel)]

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

Parth Malwankar wrote:
> One minor annoyance with that is that .bzr.log is created with the executable bit set due to O_CREAT. As per the open(2) man page:
>
> The effective permissions are modified by the process's umask in the usual way: The permissions of the created file are (mode & ~umask).
>
> On my system the mask is 022. I suppose we can do a chown specifically to unset executable bit. Thoughts?
>
> Note: This isn't tested on Windows yet as I don't have a windows system available at the moment.
>

Why not just use "mode = source_mode & ~0111", which should always strip
the executable bit off. I don't see any need have the file executable.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkuw+WcACgkQJdeBCYSNAANCMQCgtEGTtaSAUqUVhiKnEFy2EMo4
pZEAoLDZtbpEfnWglc5kbwlMxcfUFs0E
=nMDG
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

+ """Create a log file in while avoiding race.

some kind of typo here.

I would make the docstring be:

  Create the .bzr.log file.

  It inherits the ownership and permissions (masked by umask) from the containing
  directory to cope better with being run under sudo with $HOME still set to the
  user's homedir.

Also the fact that you have a comment next to every copy_ownership() invocation suggests it should be renamed to copy_ownership_from_directory.

To set the permissions you need to pass a third parameter to os.open(), which here should be 0666. This is separate from the O_APPEND etc mode.

Thanks!

review: Needs Fixing

[Parth Malwankar (parthm)]

> + """Create a log file in while avoiding race.
>
> some kind of typo here.
>
> I would make the docstring be:
>
> Create the .bzr.log file.
>
> It inherits the ownership and permissions (masked by umask) from the
> containing
> directory to cope better with being run under sudo with $HOME still set to
> the
> user's homedir.
>

Done.

> Also the fact that you have a comment next to every copy_ownership()
> invocation suggests it should be renamed to copy_ownership_from_directory.
>

I have renamed it to copy_ownership_from_path as both file and
directory are accepted, though we use only parent directory in
the existing cases.

> To set the permissions you need to pass a third parameter to os.open(), which
> here should be 0666. This is separate from the O_APPEND etc mode.
>

I have added permission of 0644 and tested it out.
Thanks.

[Martin Pool (mbp)]

On 30 March 2010 16:37, Parth Malwankar <email address hidden> wrote:
> I have added permission of 0644 and tested it out.

No, it should actually be 0666. If the user sets their umask to allow
group-writable files we should respect that unless there is a good
reason.

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

[Parth Malwankar (parthm)]

> No, it should actually be 0666. If the user sets their umask to allow
> group-writable files we should respect that unless there is a good
> reason.
>

Makes sense. Changed permissions to 0666.

[Martin Packman (gz)]

Couple of style points, one of which causes a bug.

Generally you want the body of try/except loops to be as minimal, hence my use of else in the example before. I have mixed feelings about nested functions that aren't actually being used as closures. Factoring common logic branches into one route is generally a good thing, even if it means using break-as-goto.

Rather than things along the lines of:
     permissions = 0666
     fd = os.open(filename, flags, permissions)
it's better to use:
     fd = os.open(filename, flags, mode=0666)
which is still self-documenting, and avoids the variable.

As you do that with flags too, you're defeating the race protection of the loop by making all attempts after the first exclusive, potentially causing an infinite loop instead:
    flags = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT
    while True:
        ...
            fd = os.open(filename, flags)
        ...
            flags = flags | os.O_CREAT | os.O_EXCL
            permissions = 0666
            fd = os.open(filename, flags, permissions)

Also bt.test_trace.test__open_bzr_log_uses_stderr_for_failures was failing, which can be fixed by moving a later catch one step up the exception hierarchy.

Pull <lp:~gz/bzr/no-chown-if-bzrlog-exists> for my fixes for these. I've poked a couple of other minor bits while I was there, but didn't do anything major. (I'd really like windows to avoid this codepath entirely, but this doesn't need to be done right now.)

review: Needs Fixing

[Parth Malwankar (parthm)]

> Pull <lp:~gz/bzr/no-chown-if-bzrlog-exists> for my fixes for these. I've poked
> a couple of other minor bits while I was there, but didn't do anything major.
> (I'd really like windows to avoid this codepath entirely, but this doesn't
> need to be done right now.)

Thanks for the updates Martin [gz]. I have merged in the changes.

[Martin Packman (gz)]

Passes tests here.

review: Approve

[Parth Malwankar (parthm)]

merged in changes from trunk and moved NEWS entry to 2.2b2

[Robert Collins (lifeless)]

I think this is an improvement. However my experience with race conditions is that we always, eventually, run into them.

So I'd like a new bug filed saying that we have this race and should fix it. That can be a wishlist bug, but we know the defect exists so lets capture the thinking about it now to save a future user having to figure it out.

review: Approve

[Parth Malwankar (parthm)]

> I think this is an improvement. However my experience with race conditions is
> that we always, eventually, run into them.
>
> So I'd like a new bug filed saying that we have this race and should fix it.
> That can be a wishlist bug, but we know the defect exists so lets capture the
> thinking about it now to save a future user having to figure it out.

Filed Bug #567036 for this.

[Robert Collins (lifeless)]

So, this is failing on PQM. I don't have a detailed diagnostic for you yet, but there are 14 test failures.

[Robert Collins (lifeless)]

Some failures:

======================================================================
FAIL: bzrlib.tests.test_decorators.TestDecoratorDocs.test_read_lock_passthrough
----------------------------------------------------------------------
_StringException: Text attachment: log
------------

------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line 368, in _run_test_method
    testMethod()
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/test_decorators.py", line 170, in test_read_lock_passthrough
    self.assertDocstring('Frob the sample object', sam.frob)
AssertionError: 'Frob the sample object' is not None.
------------

FAIL: bzrlib.tests.test_decorators.TestDecoratorDocs.test_write_lock_passthrough
----------------------------------------------------------------------
_StringException: Text attachment: log
------------

------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line 368, in _run_test_method
    testMethod()
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/test_decorators.py", line 177, in test_write_lock_passthrough
    sam.bank)
AssertionError: 'Bank the sample, but using bar and biz.' is not None.
------------

FAIL: bzrlib.tests.test_decorators.TestPrettyDecorators.test__fast_needs_read_lock
----------------------------------------------------------------------
_StringException: Text attachment: log
------------

------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line 368, in _run_test_method
    testMethod()
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/test_decorators.py", line 225, in test__fast_needs_read_lock
    self.assertDocstring(
AssertionError: 'Just a function that supplies several arguments.' is not None.
------------

FAIL: bzrlib.tests.test_decorators.TestPrettyDecorators.test__fast_needs_write_lock
----------------------------------------------------------------------
_StringException: Text attachment: log
------------

------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line 368, in _run_test_method
    testMethod()
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/test_decorators.py", line 254, in test__fast_needs_write_lock
    self.assertDocstring(
AssertionError: 'Just a function that supplies several arguments.' is not None.
------------

FAIL: bzrlib.tests.test_decorators.TestPrettyDecorators.test__pretty_needs_read_lock
------------...

[Parth Malwankar (parthm)]

On Wed, Apr 21, 2010 at 1:11 PM, Robert Collins
<email address hidden> wrote:
> Some failures:
>
> ======================================================================
> FAIL: bzrlib.tests.test_decorators.TestDecoratorDocs.test_read_lock_passthrough
> ----------------------------------------------------------------------
...
> Which is pretty strange. Its possible that I've got the wrong mp here, but tomorrow we should have the reporting code all fixed up in pqm anyhow. Have you tried a full test run?
> Its very odd to see these failures on this proposal, as I'm sure you can guess ;).
>

These results do look surprising. I am able to run all the failing
tests without any problems. I ran bt.test_decorators on WinXP
+ Linux and others on linux.
Any ideas?

----------------------------------------------------------------------
Ran 3 tests in 0.142s

OK
[no-chown-if-bzrlog-exists]% ./bzr --no-plugins selftest -s
bt.test_decorators
bzr selftest: /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzr
   /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib
   bzr-2.2.0dev1 python-2.6.4
Linux-2.6.31-20-generic-i686-with-Ubuntu-9.10-karmic

----------------------------------------------------------------------
Ran 34 tests in 0.121s

OK
[no-chown-if-bzrlog-exists]% ./bzr --no-plugins selftest -s bb
bzr selftest: /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzr
   /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib
   bzr-2.2.0dev1 python-2.6.4
Linux-2.6.31-20-generic-i686-with-Ubuntu-9.10-karmic

----------------------------------------------------------------------
Ran 1214 tests in 326.842s

OK (known_failures=2)
Missing feature 'case-insensitive case-preserving filesystem' skipped 18 tests.
Missing feature 'case-insensitive filesystem' skipped 1 tests.
bzrlib.tests.blackbox.test_branch.TestBranchStacked.test_branch_stacked_from_smart_server
is leaking threads among 31 leaking tests.
17 non-main threads were left active in the end.
[no-chown-if-bzrlog-exists]% ./bzr selftest -s bt.test_plugins
Unable to load plugin 'hg'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib/__init__.pyc'>
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
Unable to load plugin 'svn'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib/__init__.pyc'>
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
Unable to load plugin 'git'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib/__init__.pyc'>
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
bzr selftest: /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzr
   /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib
   bzr-2.2.0dev1 python-2.6.4
Linux-2.6.31-20-generic-i686-with-Ubuntu-9.10-karmic

----------------------------------------------------------------------
Ran 72 tests in 1.313s

OK
[no-chown-if-bzrlog-exists]% ./bzr --no-plugins selftest -s bt.test_selftest
bzr sel...

[Parth Malwankar (parthm)]

Its been about an hour since I emailed on the test but for some reason that hasn't shown up so I just paste it here for now. Sorry if it shows up twice.

On Wed, Apr 21, 2010 at 1:11 PM, Robert Collins
<email address hidden> wrote:
> Some failures:
>
> ======================================================================
> FAIL: bzrlib.tests.test_decorators.TestDecoratorDocs.test_read_lock_passthrough
> ----------------------------------------------------------------------
...
> Which is pretty strange. Its possible that I've got the wrong mp here, but tomorrow we should have the reporting code all fixed up in pqm anyhow. Have you tried a full test run?
> Its very odd to see these failures on this proposal, as I'm sure you can guess ;).
>

These results do look surprising. I am able to run all the failing
tests without any problems. I ran bt.test_decorators on WinXP
+ Linux and others on linux.

----------------------------------------------------------------------
Ran 3 tests in 0.142s

OK
[no-chown-if-bzrlog-exists]% ./bzr --no-plugins selftest -s
bt.test_decorators
bzr selftest: /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzr
  /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib
  bzr-2.2.0dev1 python-2.6.4
Linux-2.6.31-20-generic-i686-with-Ubuntu-9.10-karmic

----------------------------------------------------------------------
Ran 34 tests in 0.121s

OK
[no-chown-if-bzrlog-exists]% ./bzr --no-plugins selftest -s bb
bzr selftest: /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzr
  /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib
  bzr-2.2.0dev1 python-2.6.4
Linux-2.6.31-20-generic-i686-with-Ubuntu-9.10-karmic

----------------------------------------------------------------------
Ran 1214 tests in 326.842s

OK (known_failures=2)
Missing feature 'case-insensitive case-preserving filesystem' skipped 18 tests.
Missing feature 'case-insensitive filesystem' skipped 1 tests.
bzrlib.tests.blackbox.test_branch.TestBranchStacked.test_branch_stacked_from_smart_server
is leaking threads among 31 leaking tests.
17 non-main threads were left active in the end.
[no-chown-if-bzrlog-exists]% ./bzr selftest -s bt.test_plugins
Unable to load plugin 'hg'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib/__init__.pyc'>
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
Unable to load plugin 'svn'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib/__init__.pyc'>
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
Unable to load plugin 'git'. It requested API version (2, 1, 0) of
module <module 'bzrlib' from
'/storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib/__init__.pyc'>
but the minimum exported version is (2, 2, 0), and the maximum is (2,
2, 0)
bzr selftest: /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzr
  /storage/parth/src/bzr.dev/no-chown-if-bzrlog-exists/bzrlib
  bzr-2.2.0dev1 python-2.6.4
Linux-2.6.31-20-generic-i686-with-Ubuntu-9.10-karmic

---------------------------------------------...

[Martin Packman (gz)]

Those all look like they're from my <lp:~gz/bzr/support_OO_flag> branch, and seem to just be a dumb error on my part, I'll handle over there.

[Vincent Ladeuil (vila)]

It looks like we had one more failure without feedback on pqm here... no mail, no comment in the mp... I'll look into it first thing monday morning unless lifeless (nudge nudge, what ? still not unpacked ? :-P) beats me to it

[Vincent Ladeuil (vila)]

Haa, submitting by mail revealed a conflict in NEWS, new submission sent.

[Vincent Ladeuil (vila)]

..which also failed due to test failing (some renaming was still needed for copy copy_ownership_from_path from copy_ownership).

[lp:~parthm/bzr/no-chown-if-bzrlog-exists]

5127. By Parth Malwankar on 2010-04-24

fixed test failure in bt.test_osutils

[Parth Malwankar (parthm)]

> ..which also failed due to test failing (some renaming was still needed for
> copy copy_ownership_from_path from copy_ownership).

Ouch. This is embarrassing. I don't know how I missed something this obvious. Sorry about that.
I have fixed it and pushed the changes.

[lp:~parthm/bzr/no-chown-if-bzrlog-exists]

5128. By Parth Malwankar on 2010-04-24

merged in changes from trunk

5129. By Parth Malwankar on 2010-04-24

moved NEWS entry to 2.2b3

[Vincent Ladeuil (vila)]

>>>>> Parth Malwankar <email address hidden> writes:

    >> ..which also failed due to test failing (some renaming was still needed for
    >> copy copy_ownership_from_path from copy_ownership).

    > Ouch. This is embarrassing.

Nah, don't worry about it. The fact that *pqm* didn't tell me that is
more embarassing...

    > I don't know how I missed something this obvious. Sorry about
    > that. I have fixed it and pushed the changes.

I had already sent the mp to pqm, but thanks all the same ;)

=== modified file 'NEWS'
--- NEWS	2010-04-23 14:15:23 +0000
+++ NEWS	2010-04-24 15:10:01 +0000
@@ -29,6 +29,11 @@
 Bug Fixes
 *********
 
+* ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and
+  group ownership from the containing directory. This allow bzr to work
+  better with sudo.
+  (Martin <[e-mail]>, Parth Malwankar, #376388)
+
 * ``bzr selftest --parallel=fork`` wait for its children avoiding zombies.
   (Vincent Ladeuil, #566670)
 
@@ -369,11 +374,6 @@
   mainline (i.e. it supports dotted revisions).
   (Parth Malwankar, #517800)
 
-* ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and
-  group ownership from the containing directory. This allow bzr to work
-  better with sudo.
-  (Parth Malwankar, #376388)
-
 * Use first apparent author not committer in GNU Changelog format.
   (Martin von Gagern, #513322)
 

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2010-04-23 08:51:52 +0000
+++ bzrlib/config.py	2010-04-24 15:10:01 +0000
@@ -519,7 +519,8 @@
 
     def _write_config_file(self):
         path = self._get_filename()
-        f = osutils.open_with_ownership(path, 'wb')
+        f = open(path, 'wb')
+        osutils.copy_ownership_from_path(path)
         self._get_parser().write(f)
         f.close()
 
@@ -818,7 +819,8 @@
                 trace.mutter('creating config parent directory: %r', parent_dir)
             os.mkdir(parent_dir)
         trace.mutter('creating config directory: %r', path)
-        osutils.mkdir_with_ownership(path)
+        os.mkdir(path)
+        osutils.copy_ownership_from_path(path)
 
 
 def config_dir():

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2010-04-23 09:28:29 +0000
+++ bzrlib/osutils.py	2010-04-24 15:10:01 +0000
@@ -1827,7 +1827,7 @@
             real_handlers[kind](abspath, relpath)
 
 
-def copy_ownership(dst, src=None):
+def copy_ownership_from_path(dst, src=None):
     """Copy usr/grp ownership from src file/dir to dst file/dir.
 
     If src is None, the containing directory is used as source. If chown
@@ -1849,30 +1849,6 @@
         trace.warning("Unable to copy ownership from '%s' to '%s': IOError: %s." % (src, dst, e))
 
 
-def mkdir_with_ownership(path, ownership_src=None):
-    """Create the directory 'path' with specified ownership.
-
-    If ownership_src is given, copies (chown) usr/grp ownership
-    from 'ownership_src' to 'path'. If ownership_src is None, use the
-    containing dir ownership.
-    """
-    os.mkdir(path)
-    copy_ownership(path, ownership_src)
-
-
-def open_with_ownership(filename, mode='r', bufsize=-1, ownership_src=None):
-    """Open the file 'filename' with the specified ownership.
-
-    If ownership_src is specified, copy usr/grp ownership from ownership_src
-    to filename. If ownership_src is None, copy ownership from containing
-    directory.
-    Returns the opened file object.
-    """
-    f = open(filename, mode, bufsize)
-    copy_ownership(filename, ownership_src)
-    return f
-
-
 def path_prefix_key(path):
     """Generate a prefix-order path key for path.
 

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2010-03-11 06:24:00 +0000
+++ bzrlib/tests/test_osutils.py	2010-04-24 15:10:01 +0000
@@ -1990,29 +1990,26 @@
     def _dummy_chown(self, path, uid, gid):
         self.path, self.uid, self.gid = path, uid, gid
 
-    def test_mkdir_with_ownership_chown(self):
-        """Ensure that osutils.mkdir_with_ownership chowns correctly with ownership_src.
-        """
-        ownsrc = '/'
-        osutils.mkdir_with_ownership('foo', ownsrc)
-
-        s = os.stat(ownsrc)
-        self.assertEquals(self.path, 'foo')
-        self.assertEquals(self.uid, s.st_uid)
-        self.assertEquals(self.gid, s.st_gid)
-
-    def test_open_with_ownership_chown(self):
-        """Ensure that osutils.open_with_ownership chowns correctly with ownership_src.
-        """
-        ownsrc = '/'
-        f = osutils.open_with_ownership('foo', 'w', ownership_src=ownsrc)
-
-        # do a test write and close
-        f.write('hello')
-        f.close()
-
-        s = os.stat(ownsrc)
-        self.assertEquals(self.path, 'foo')
-        self.assertEquals(self.uid, s.st_uid)
-        self.assertEquals(self.gid, s.st_gid)
-
+    def test_copy_ownership_from_path(self):
+        """copy_ownership_from_path test with specified src.
+        """
+        ownsrc = '/'
+        f = open('test_file', 'wt')
+        osutils.copy_ownership_from_path('test_file', ownsrc)
+
+        s = os.stat(ownsrc)
+        self.assertEquals(self.path, 'test_file')
+        self.assertEquals(self.uid, s.st_uid)
+        self.assertEquals(self.gid, s.st_gid)
+
+    def test_copy_ownership_nonesrc(self):
+        """copy_ownership_from_path test with src=None.
+        """
+        f = open('test_file', 'wt')
+        # should use parent dir for permissions
+        osutils.copy_ownership_from_path('test_file')
+
+        s = os.stat('..')
+        self.assertEquals(self.path, 'test_file')
+        self.assertEquals(self.uid, s.st_uid)
+        self.assertEquals(self.gid, s.st_gid)

=== modified file 'bzrlib/trace.py'
--- bzrlib/trace.py	2010-03-25 11:08:55 +0000
+++ bzrlib/trace.py	2010-04-24 15:10:01 +0000
@@ -238,12 +238,37 @@
     This sets the global _bzr_log_filename.
     """
     global _bzr_log_filename
+
+    def _open_or_create_log_file(filename):
+        """Open existing log file, or create with ownership and permissions
+
+        It inherits the ownership and permissions (masked by umask) from
+        the containing directory to cope better with being run under sudo
+        with $HOME still set to the user's homedir.
+        """
+        flags = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT
+        while True:
+            try:
+                fd = os.open(filename, flags)
+                break
+            except OSError, e:
+                if e.errno != errno.ENOENT:
+                    raise
+            try:
+                fd = os.open(filename, flags | os.O_CREAT | os.O_EXCL, 0666)
+            except OSError, e:
+                if e.errno != errno.EEXIST:
+                    raise
+            else:
+                osutils.copy_ownership_from_path(filename)
+                break
+        return os.fdopen(fd, 'at', 0) # unbuffered
+
+
     _bzr_log_filename = _get_bzr_log_filename()
     _rollover_trace_maybe(_bzr_log_filename)
     try:
-        buffering = 0 # unbuffered
-        bzr_log_file = osutils.open_with_ownership(_bzr_log_filename, 'at', buffering)
-        # bzr_log_file.tell() on windows always return 0 until some writing done
+        bzr_log_file = _open_or_create_log_file(_bzr_log_filename)
         bzr_log_file.write('\n')
         if bzr_log_file.tell() <= 2:
             bzr_log_file.write("this is a debug log for diagnosing/reporting problems in bzr\n")
@@ -252,7 +277,7 @@
 
         return bzr_log_file
 
-    except IOError, e:
+    except EnvironmentError, e:
         # If we are failing to open the log, then most likely logging has not
         # been set up yet. So we just write to stderr rather than using
         # 'warning()'. If we using warning(), users get the unhelpful 'no