Bazaar

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

  1. no-chown-if-bzrlog-exists
  2. Merge into bzr.dev
Proposed by Parth Malwankar
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: 5126
Merge reported by: Vincent Ladeuil
Merged at revision: not available
Proposed branch: lp:~parthm/bzr/no-chown-if-bzrlog-exists
Merge into: lp:bzr
Diff against target: 206 lines (+62/-62)
5 files modified
NEWS (+5/-5)
bzrlib/config.py (+4/-2)
bzrlib/osutils.py (+1/-25)
bzrlib/tests/test_osutils.py (+23/-26)
bzrlib/trace.py (+29/-4)
To merge this branch: bzr merge lp:~parthm/bzr/no-chown-if-bzrlog-exists
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Martin Packman (community) Approve
Martin Pool Needs Fixing
John A Meinel Approve
Review via email: mp+22197@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Parth Malwankar (parthm) 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.

Revision history for this message
John A Meinel (jameinel) wrote :

-----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-----

Revision history for this message
John A Meinel (jameinel) wrote :

-----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
Revision history for this message
Martin Packman (gz) wrote :

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.

Revision history for this message
Parth Malwankar (parthm) wrote :

> 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

Revision history for this message
Martin Packman (gz) wrote :

> 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.

Revision history for this message
Parth Malwankar (parthm) wrote :

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

Revision history for this message
Parth Malwankar (parthm) 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.

Revision history for this message
John A Meinel (jameinel) wrote :

-----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-----

Revision history for this message
Martin Pool (mbp) wrote :

+ """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
Revision history for this message
Parth Malwankar (parthm) wrote :

> + """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.

Revision history for this message
Martin Pool (mbp) wrote :

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/>

Revision history for this message
Parth Malwankar (parthm) wrote :

> 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.

Revision history for this message
Martin Packman (gz) wrote :

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
Revision history for this message
Parth Malwankar (parthm) wrote :

> 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.

Revision history for this message
Martin Packman (gz) wrote :

Passes tests here.

review: Approve
Revision history for this message
Parth Malwankar (parthm) wrote :

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

Revision history for this message
Robert Collins (lifeless) wrote :

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
Revision history for this message
Parth Malwankar (parthm) wrote :

> 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.

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Robert Collins (lifeless) wrote :
Download full text (8.0 KiB)

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
------------...

Read more...

Revision history for this message
Parth Malwankar (parthm) wrote :
Download full text (3.6 KiB)

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...

Read more...

Revision history for this message
Parth Malwankar (parthm) wrote :
Download full text (3.7 KiB)

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

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

Read more...

Revision history for this message
Martin Packman (gz) wrote :

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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

..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 updated
5127. By Parth Malwankar

fixed test failure in bt.test_osutils

Revision history for this message
Parth Malwankar (parthm) wrote :

> ..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 updated
5128. By Parth Malwankar

merged in changes from trunk

5129. By Parth Malwankar

moved NEWS entry to 2.2b3

Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> 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 ;)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-04-23 14:15:23 +0000
+++ NEWS 2010-04-24 15:10:01 +0000
@@ -29,6 +29,11 @@
29Bug Fixes29Bug Fixes
30*********30*********
3131
32* ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and
33 group ownership from the containing directory. This allow bzr to work
34 better with sudo.
35 (Martin <gzlist@googlemail.com>, Parth Malwankar, #376388)
36
32* ``bzr selftest --parallel=fork`` wait for its children avoiding zombies.37* ``bzr selftest --parallel=fork`` wait for its children avoiding zombies.
33 (Vincent Ladeuil, #566670)38 (Vincent Ladeuil, #566670)
3439
@@ -369,11 +374,6 @@
369 mainline (i.e. it supports dotted revisions).374 mainline (i.e. it supports dotted revisions).
370 (Parth Malwankar, #517800)375 (Parth Malwankar, #517800)
371376
372* ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and
373 group ownership from the containing directory. This allow bzr to work
374 better with sudo.
375 (Parth Malwankar, #376388)
376
377* Use first apparent author not committer in GNU Changelog format.377* Use first apparent author not committer in GNU Changelog format.
378 (Martin von Gagern, #513322)378 (Martin von Gagern, #513322)
379379
380380
=== 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 @@
519519
520 def _write_config_file(self):520 def _write_config_file(self):
521 path = self._get_filename()521 path = self._get_filename()
522 f = osutils.open_with_ownership(path, 'wb')522 f = open(path, 'wb')
523 osutils.copy_ownership_from_path(path)
523 self._get_parser().write(f)524 self._get_parser().write(f)
524 f.close()525 f.close()
525526
@@ -818,7 +819,8 @@
818 trace.mutter('creating config parent directory: %r', parent_dir)819 trace.mutter('creating config parent directory: %r', parent_dir)
819 os.mkdir(parent_dir)820 os.mkdir(parent_dir)
820 trace.mutter('creating config directory: %r', path)821 trace.mutter('creating config directory: %r', path)
821 osutils.mkdir_with_ownership(path)822 os.mkdir(path)
823 osutils.copy_ownership_from_path(path)
822824
823825
824def config_dir():826def config_dir():
825827
=== 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 @@
1827 real_handlers[kind](abspath, relpath)1827 real_handlers[kind](abspath, relpath)
18281828
18291829
1830def copy_ownership(dst, src=None):1830def copy_ownership_from_path(dst, src=None):
1831 """Copy usr/grp ownership from src file/dir to dst file/dir.1831 """Copy usr/grp ownership from src file/dir to dst file/dir.
18321832
1833 If src is None, the containing directory is used as source. If chown1833 If src is None, the containing directory is used as source. If chown
@@ -1849,30 +1849,6 @@
1849 trace.warning("Unable to copy ownership from '%s' to '%s': IOError: %s." % (src, dst, e))1849 trace.warning("Unable to copy ownership from '%s' to '%s': IOError: %s." % (src, dst, e))
18501850
18511851
1852def mkdir_with_ownership(path, ownership_src=None):
1853 """Create the directory 'path' with specified ownership.
1854
1855 If ownership_src is given, copies (chown) usr/grp ownership
1856 from 'ownership_src' to 'path'. If ownership_src is None, use the
1857 containing dir ownership.
1858 """
1859 os.mkdir(path)
1860 copy_ownership(path, ownership_src)
1861
1862
1863def open_with_ownership(filename, mode='r', bufsize=-1, ownership_src=None):
1864 """Open the file 'filename' with the specified ownership.
1865
1866 If ownership_src is specified, copy usr/grp ownership from ownership_src
1867 to filename. If ownership_src is None, copy ownership from containing
1868 directory.
1869 Returns the opened file object.
1870 """
1871 f = open(filename, mode, bufsize)
1872 copy_ownership(filename, ownership_src)
1873 return f
1874
1875
1876def path_prefix_key(path):1852def path_prefix_key(path):
1877 """Generate a prefix-order path key for path.1853 """Generate a prefix-order path key for path.
18781854
18791855
=== 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 @@
1990 def _dummy_chown(self, path, uid, gid):1990 def _dummy_chown(self, path, uid, gid):
1991 self.path, self.uid, self.gid = path, uid, gid1991 self.path, self.uid, self.gid = path, uid, gid
19921992
1993 def test_mkdir_with_ownership_chown(self):1993 def test_copy_ownership_from_path(self):
1994 """Ensure that osutils.mkdir_with_ownership chowns correctly with ownership_src.1994 """copy_ownership_from_path test with specified src.
1995 """1995 """
1996 ownsrc = '/'1996 ownsrc = '/'
1997 osutils.mkdir_with_ownership('foo', ownsrc)1997 f = open('test_file', 'wt')
19981998 osutils.copy_ownership_from_path('test_file', ownsrc)
1999 s = os.stat(ownsrc)1999
2000 self.assertEquals(self.path, 'foo')2000 s = os.stat(ownsrc)
2001 self.assertEquals(self.uid, s.st_uid)2001 self.assertEquals(self.path, 'test_file')
2002 self.assertEquals(self.gid, s.st_gid)2002 self.assertEquals(self.uid, s.st_uid)
20032003 self.assertEquals(self.gid, s.st_gid)
2004 def test_open_with_ownership_chown(self):2004
2005 """Ensure that osutils.open_with_ownership chowns correctly with ownership_src.2005 def test_copy_ownership_nonesrc(self):
2006 """2006 """copy_ownership_from_path test with src=None.
2007 ownsrc = '/'2007 """
2008 f = osutils.open_with_ownership('foo', 'w', ownership_src=ownsrc)2008 f = open('test_file', 'wt')
20092009 # should use parent dir for permissions
2010 # do a test write and close2010 osutils.copy_ownership_from_path('test_file')
2011 f.write('hello')2011
2012 f.close()2012 s = os.stat('..')
20132013 self.assertEquals(self.path, 'test_file')
2014 s = os.stat(ownsrc)2014 self.assertEquals(self.uid, s.st_uid)
2015 self.assertEquals(self.path, 'foo')2015 self.assertEquals(self.gid, s.st_gid)
2016 self.assertEquals(self.uid, s.st_uid)
2017 self.assertEquals(self.gid, s.st_gid)
2018
20192016
=== 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 @@
238 This sets the global _bzr_log_filename.238 This sets the global _bzr_log_filename.
239 """239 """
240 global _bzr_log_filename240 global _bzr_log_filename
241
242 def _open_or_create_log_file(filename):
243 """Open existing log file, or create with ownership and permissions
244
245 It inherits the ownership and permissions (masked by umask) from
246 the containing directory to cope better with being run under sudo
247 with $HOME still set to the user's homedir.
248 """
249 flags = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT
250 while True:
251 try:
252 fd = os.open(filename, flags)
253 break
254 except OSError, e:
255 if e.errno != errno.ENOENT:
256 raise
257 try:
258 fd = os.open(filename, flags | os.O_CREAT | os.O_EXCL, 0666)
259 except OSError, e:
260 if e.errno != errno.EEXIST:
261 raise
262 else:
263 osutils.copy_ownership_from_path(filename)
264 break
265 return os.fdopen(fd, 'at', 0) # unbuffered
266
267
241 _bzr_log_filename = _get_bzr_log_filename()268 _bzr_log_filename = _get_bzr_log_filename()
242 _rollover_trace_maybe(_bzr_log_filename)269 _rollover_trace_maybe(_bzr_log_filename)
243 try:270 try:
244 buffering = 0 # unbuffered271 bzr_log_file = _open_or_create_log_file(_bzr_log_filename)
245 bzr_log_file = osutils.open_with_ownership(_bzr_log_filename, 'at', buffering)
246 # bzr_log_file.tell() on windows always return 0 until some writing done
247 bzr_log_file.write('\n')272 bzr_log_file.write('\n')
248 if bzr_log_file.tell() <= 2:273 if bzr_log_file.tell() <= 2:
249 bzr_log_file.write("this is a debug log for diagnosing/reporting problems in bzr\n")274 bzr_log_file.write("this is a debug log for diagnosing/reporting problems in bzr\n")
@@ -252,7 +277,7 @@
252277
253 return bzr_log_file278 return bzr_log_file
254279
255 except IOError, e:280 except EnvironmentError, e:
256 # If we are failing to open the log, then most likely logging has not281 # If we are failing to open the log, then most likely logging has not
257 # been set up yet. So we just write to stderr rather than using282 # been set up yet. So we just write to stderr rather than using
258 # 'warning()'. If we using warning(), users get the unhelpful 'no283 # 'warning()'. If we using warning(), users get the unhelpful 'no