Merge lp:~bialix/bzr/2.4-bug-967060 into lp:bzr/2.4

[Alexander Belchenko]

Provide st_uid and st_gid on _Win32Stat so it looks more like a normal stat object

_Win32Stat object provides members st_uid and st_gid, those are present in Python's os.stat object. These members required for external tools like bzr-git and dulwich. (Bug #967060).

I've based the fix on 2.4 branch to make sure we can merge it easily from 2.4 up to trunk (2.6). Actually we need this fix in bzr 2.5. I don't think there are plans to provide new windows installers for next bzr 2.4 releases.

[John A Meinel (jameinel)]

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

On 3/28/2012 4:11 PM, Alexander Belchenko wrote:
> Alexander Belchenko has proposed merging
> lp:~bialix/bzr/2.4-bug-967060 into lp:bzr/2.4.
>
> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #967060 in
> Bazaar: "[win32] `bzr add file` in git tree fails with traceback:
> _Win32Stat misses st_uid/st_gid members"
> https://bugs.launchpad.net/bzr/+bug/967060
>
> For more details, see:
> https://code.launchpad.net/~bialix/bzr/2.4-bug-967060/+merge/99737
>
> _Win32Stat object provides members st_uid and st_gid, those are
> present in Python's os.stat object. These members required for
> external tools like bzr-git and dulwich. (Bug #967060).
>
> I've based the fix on 2.4 branch to make sure we can merge it
> easily from 2.4 up to trunk (2.6). Actually we need this fix in bzr
> 2.5. I don't think there are plans to provide new windows
> installers for next bzr 2.4 releases.
>

I can confirm that 'os.stat()' on Python returns 0 for uid and gid on
regular files. So it seems great to me.

John
=:->

 review: approve

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

iEYEARECAAYFAk9zHg0ACgkQJdeBCYSNAAMw4QCfdRgnIS+ME0cCaHnA1z0h8GtU
v7QAnjY23NRh4MzObef1qfbeo4DNclF6
=txr6
-----END PGP SIGNATURE-----

review: Approve

[Martin Packman (gz)]

How about using properties returning 0 instead? This isn't a class designed for minimising memory usage, but as these values serve no purpose there seems no point storing them.

[Jelmer Vernooij (jelmer)]

> How about using properties returning 0 instead? This isn't a class designed
> for minimising memory usage, but as these values serve no purpose there seems
> no point storing them.
That seems slightly better to me too, but either change seems reasonable.

review: Approve

[Alexander Belchenko (bialix)]

Martin Packman пишет:
> How about using properties returning 0 instead? This isn't a class designed for minimising memory usage, but as these values serve no purpose there seems no point storing them.

I followed the approach John used for st_dev and st_ino. I can do it
either way. But then should I change st_dev and st_ino implementations
as well?

--
All the dude wanted was his rug back

[Alexander Belchenko (bialix)]

Martin Packman пишет:
> How about using properties returning 0 instead? This isn't a class designed for minimising memory usage, but as these values serve no purpose there seems no point storing them.

Well, thinking about it more I'd like to avoid properties because they
mean more code for no real benefit. To avoid duplication in explicit
initialization of those 4 members to zero, I'd prefer move them into
constructor (__init__ method). There is no constructor today, but we can
add it.

--
All the dude wanted was his rug back

[Jelmer Vernooij (jelmer)]

> Martin Packman пишет:
> > How about using properties returning 0 instead? This isn't a class designed
> for minimising memory usage, but as these values serve no purpose there seems
> no point storing them.
>
> Well, thinking about it more I'd like to avoid properties because they
> mean more code for no real benefit. To avoid duplication in explicit
> initialization of those 4 members to zero, I'd prefer move them into
> constructor (__init__ method). There is no constructor today, but we can
> add it.
Presumably the benefit in using a property would mean 4 fields less to store in memory per instance, and less time to spend assigning those member variables per stat object. I'm not sure how big the gains of that are in practice though.

I don't think that sort of change should be a requirement for this fix to land. But if you feel like fixing that, it would be great. :)

[Alexander Belchenko (bialix)]

Jelmer Vernooij пишет:
>> Martin Packman пишет:
>>> How about using properties returning 0 instead? This isn't a class designed
>> for minimising memory usage, but as these values serve no purpose there seems
>> no point storing them.
>>
>> Well, thinking about it more I'd like to avoid properties because they
>> mean more code for no real benefit. To avoid duplication in explicit
>> initialization of those 4 members to zero, I'd prefer move them into
>> constructor (__init__ method). There is no constructor today, but we can
>> add it.
> Presumably the benefit in using a property would mean 4 fields less to store in memory per instance, and less time to spend assigning those member variables per stat object. I'm not sure how big the gains of that are in practice though.
>
> I don't think that sort of change should be a requirement for this fix to land. But if you feel like fixing that, it would be great. :)

I'm not sure what is best here. I'd like to hear opinion of John Meinel.

--
All the dude wanted was his rug back

[John A Meinel (jameinel)]

I'm pretty sure writing "cdef public int st_dev" does create some sort of
accessor function for the attribute, since it has to turn the int into a
PyInt. So it is pretty much the same to write:

property st_dev:
  def __get__: return 0

So i would slight prefer the property approach. I don't think either is
going to be a measurable impact, though.
John
=:->
On Mar 28, 2012 6:19 PM, "Alexander Belchenko" <email address hidden> wrote:

> Jelmer Vernooij пишет:
> >> Martin Packman пишет:
> >>> How about using properties returning 0 instead? This isn't a class
> designed
> >> for minimising memory usage, but as these values serve no purpose there
> seems
> >> no point storing them.
> >>
> >> Well, thinking about it more I'd like to avoid properties because they
> >> mean more code for no real benefit. To avoid duplication in explicit
> >> initialization of those 4 members to zero, I'd prefer move them into
> >> constructor (__init__ method). There is no constructor today, but we can
> >> add it.
> > Presumably the benefit in using a property would mean 4 fields less to
> store in memory per instance, and less time to spend assigning those member
> variables per stat object. I'm not sure how big the gains of that are in
> practice though.
> >
> > I don't think that sort of change should be a requirement for this fix
> to land. But if you feel like fixing that, it would be great. :)
>
> I'm not sure what is best here. I'd like to hear opinion of John Meinel.
>
> --
> All the dude wanted was his rug back
>
> https://code.launchpad.net/~bialix/bzr/2.4-bug-967060/+merge/99737
> You are reviewing the proposed merge of lp:~bialix/bzr/2.4-bug-967060 into
> lp:bzr/2.4.
>

[Alexander Belchenko (bialix)]

John A Meinel пишет:
> I'm pretty sure writing "cdef public int st_dev" does create some sort of
> accessor function for the attribute, since it has to turn the int into a
> PyInt. So it is pretty much the same to write:
>
> property st_dev:
> def __get__: return 0

Well, actually access to int variable performed by internal Python
function, so Pyrex does not generate any special explicit accessor
function, but only creates an entry in tp_members structure. But for
property Pyrex does generate special (although trivial) code to convert
C zero to proper Python object.

> So i would slight prefer the property approach. I don't think either is
> going to be a measurable impact, though.

OK, I'll change the code to use properties. That way we can avoid
duplication in setting those variables to zero and save 16 bytes for
each stat object instance.

[Vincent Ladeuil (vila)]

Thanks fixing that Alexander !

I'll put this proposal into a wip one, pign here or on IRC when you feel it can be landed or if you want a final review.

[Alexander Belchenko (bialix)]

Ping-pong.

I've changed the code to use properties, and added small test to check that all properties are present and return expected value. Please, take a look. I think it's ready to land.

[Jelmer Vernooij (jelmer)]

Looks good to me, thanks.

review: Approve

[Martin Packman (gz)]

test__walkdirs_win32.Test_Win32Stat.test_zero_members_present OK 0ms

review: Approve

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/_walkdirs_win32.pyx'
--- bzrlib/_walkdirs_win32.pyx	2011-04-05 12:15:34 +0000
+++ bzrlib/_walkdirs_win32.pyx	2012-03-29 08:38:20 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008-2011 Canonical Ltd
+# Copyright (C) 2008-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -95,8 +95,19 @@
             return self._st_size
 
     # os.stat always returns 0, so we hard code it here
-    cdef readonly int st_dev
-    cdef readonly int st_ino
+    property st_dev:
+        def __get__(self):
+            return 0
+    property st_ino:
+        def __get__(self):
+            return 0
+    # st_uid and st_gid required for some external tools like bzr-git & dulwich
+    property st_uid:
+        def __get__(self):
+            return 0
+    property st_gid:
+        def __get__(self):
+            return 0
 
     def __repr__(self):
         """Repr is the same as a Stat object.
@@ -195,8 +206,6 @@
         statvalue.st_mtime = _ftime_to_timestamp(&data.ftLastWriteTime)
         statvalue.st_atime = _ftime_to_timestamp(&data.ftLastAccessTime)
         statvalue._st_size = _get_size(data)
-        statvalue.st_ino = 0
-        statvalue.st_dev = 0
         return statvalue
 
     def read_dir(self, prefix, top):
@@ -288,6 +297,4 @@
     statvalue.st_mtime = st.st_mtime
     statvalue.st_atime = st.st_atime
     statvalue._st_size = st.st_size
-    statvalue.st_ino = 0
-    statvalue.st_dev = 0
     return statvalue

=== modified file 'bzrlib/tests/test__walkdirs_win32.py'
--- bzrlib/tests/test__walkdirs_win32.py	2010-05-16 12:21:31 +0000
+++ bzrlib/tests/test__walkdirs_win32.py	2012-03-29 08:38:20 +0000
@@ -93,3 +93,22 @@
         self.assertEqual(errno.ENOENT, e.errno)
         self.assertEqual(3, e.winerror)
         self.assertEqual((3, u'no_such_dir/*'), e.args)
+
+
+class Test_Win32Stat(tests.TestCaseInTempDir):
+
+    _test_needs_features = [win32_readdir_feature]
+
+    def setUp(self):
+        super(Test_Win32Stat, self).setUp()
+        from bzrlib._walkdirs_win32 import lstat
+        self.win32_lstat = lstat
+
+    def test_zero_members_present(self):
+        self.build_tree(['foo'])
+        st = self.win32_lstat('foo')
+        # we only want to ensure that some members are present
+        self.assertEqual(0, st.st_dev)
+        self.assertEqual(0, st.st_ino)
+        self.assertEqual(0, st.st_uid)
+        self.assertEqual(0, st.st_gid)

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2012-02-26 19:56:15 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2012-03-29 08:38:20 +0000
@@ -55,6 +55,10 @@
 * Prevent a traceback being printed to stderr when logging has problems and
   accept utf-8 byte string without breaking. (Martin Packman, #714449)
 
+* _Win32Stat object provides members st_uid and st_gid, those are present
+  in Python's os.stat object. These members required for external tools like
+  bzr-git and dulwich. (Alexander Belchenko, #967060)
+
 Documentation
 *************