tbzr code for discussion.

Tue Jun 10 22:56:33 BST 2008

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

Mark Hammond wrote:
| Hi all,
|   I've pushed my recent Tortoise work to Launchpad
| (lp:~tortoisebzr-developers/tortoisebzr/tbzr2-proto).  The intention is for
| this code to be reviewed and discussed, agreement reached on a number of
| issues, and then we can start moving towards a first version.  My hope is
| that many of these can be resolved (or even implemented ;) after the
| mini-sprint we are having in Sydney next week, but I welcome all discussion
| before then.  I've included the README.txt from that branch for your
| convenience.
|
| Cheers,
|
| Mark
|

Thanks for letting us get a preview. Some comments:

1) It is actually tbzrlib/pipe/client.py not "cache".

2) no win32pipe.TransactNamedPipe - consider upgrading pywin32to build 212

I get this when running a query. But I wasn't able to find a __version__ string,
or a build string in my pywin32 install. Usually I would expect to be able to do:

|>> import win32com
|>> print win32com.__version__

or at least some variant of it. I'm sure I have at least build 210, because
there is a warning that says "XX has been deprecated since 210".

Maybe I'm just missing something. (Also, your 'license.txt' still only says
1999-2001, I assume you want copyright through 2008).

I know I just installed win32 in the last few months, so is 212 rather new?

3) I find it a bit odd to have the scripts in a subdirectory of the library.
(Running tbzrlib/app.py imports 'tbzrlib' to run.) Maybe that is just me. But it
does make running out of the working directory more difficult than it has to.
(Even if you want to put it into a 'scripts/' directory, you can add a quick
tree-level shim which shoves the right path into sys.path to make it easy to run
from source.)

One thing that is really nice about the bzr layout, is that you can just symlink
'bzr' into your path, and it just works.

4) Doing client.py file_status . gives a random error about not enough
arguments. Doing 'client.py file_status . 1' gives an ERROR with a traceback. I
assume because '.' is a directory and not a file.
But certainly the error is confusing as the final error is:
~    (val,) = struct.unpack("H", buf[ndx:ndx+int_size])
TypeError: 'NoneType' object is unsubscriptable

5) If you need to run 'tbzrlib/app.py' directly, why do you need to run 'hello'
as well?

| This is very early - bits of it work, but lots of it does not.
|
| To get started:
|
| * set PYTHONPATH so bzrlib, and the 'tbzrlib' in this package are
| referenced.
|
| * Execute 'python tbzrlib\app.py' - this will start the 'cache process'
| which
|   clients connect to.  The cache process doesn't auto-start upon request.
|
| * To test things: execute 'python tbzrlib\cache\client.py hello'.  The
|   command 'goodbye' will shut the server down while the command
|   'file_status c:\path\to\directory 1' should print an integer status
|   of the filename.
|
| * To test the shell extension:
|   - change to the 'shellext\python' directory.
|   - execute 'python tbzr.py' - you should see a message telling you things
|     were registered.
|   - Install the latest TSVN - that will ensure the shared 'TortoiseOverlays'
|     are installed which we depend on (our installer will arrange for this to
|     be installed, but source code users are on their own!)
|   - Execute a new instance of (say) notepad.  Select File->Open.  You should
|     see the notepad process connect to the pipe server, and if you are very
|     lucky, you may see icon overlays for files and directories under version
|     control.
|   - To see debug output from the python shell extension, register the
|     extension using 'python tbzr.py --debug', then open Pythonwin's
|     'Remote Trace Collector Tool' to see output from the Python code.
|
| There are many many things which are still up-for-grabs and not set in
| stone.
| Many things have "quick-and-easy" implementations assuming that a better
| strategy will be put in place.  Discussion of these is invited:
|
| * Source directory layout.
|
|   - Created a new library called 'tbzrlib' containing all server Python
|     functionality, including the "entry point" for the server
|     process.
|
|   - I've attempted to keep it VCS agnostic where possible and have
|     largely succeeded - only 'app.py' (which is the above entry point)
|     knows about bzr.  However, this means the name 'tbzrlib' is misleading
|     - maybe it should be 'tvcslib' and we move app.py out to 'bzrapp.py' -
|     but where would 'bzrapp.py' live in the source tree then?

Sounds reasonable, I would put it up a level, or possibly in a scripts/
directory. (As mentioned above.)

|
|   - Is it too nested (eg 'cache' and 'pipe' subdirs, each with their own
|     'tests' directory)

Nesting is great for me, as long as the tests all chain properly. We took the
approach of nesting tests to mimic the library layout, but that isn't the only
way. (Specifically, a file bzrlib/subdir/foo.py should have a test as something
like bzrlib/tests/subdir/test_foo.py, though we don't do that as much as we
probably should.)

|
|   - shellext\python is for the 'temporary' Python implementation, largely
|     copied and hacked from "TortoiseBZR", alhough it doesn't talk to bzrlib
|     directly, but instead connects to a pipe..  Eventually this will be
|     replaced with a .cpp implementation in 'shell\???' (shell\src ?)
|
| * User prefs: No framework for user-prefs are in place.  Registry doesn't
|   really seem appropriate.  Reuse any of bzrlib appropriate here?  If so,
|   what? :)

We have ConfigObj for ini-style configuration. You would want per-user
configuration, and since this is really win32-only, I don't really see why you
wouldn't use the Registry.

Otherwise you could use something like 'bzrlib.config.get_config_dir()' which
gives you the AppData\Bazaar\2.0 directory, (yeah yeah, a bit of cruft there.)

|
| * User extensibility: Similar to user-prefs, but we picture that in the
|   future users will be able to define their own commands for the context
|   menu, for example.  Alternatively, they may be able to enable or disable
|   selected builtin commands.  How to accomodate these?  Do we need/want a
|   "plugins" concept?
|
| * Too much logic in the pipe server: The pipe.server package should only be
|   used for getting the bits off the wire, and leave the processing to
| another
|   module - but currently this is all in the bowels of the pipe itself.
|   Possibility is to create a TCP server for testing and non-windows users,
|   which would be a testbed to refactor comms from logic.
|
| * Logging:  In some places I've used 'note' and 'mutter' from bzrlib, but
|   in others I've used the logging module directly.  I'm not sure it is
|   appropriate to log to the bzr log: is it worth trying to reuse the bzrlib
|   logging, or should we roll our own based around the logging module that
|   is appropriate to us? (eg, the ability to have different log levels for
|   different logs would probably help debugging the server app)
|

logging.py is actually a bit of a thorn in bzrlib's side. It is rather slow for
an app that starts and stops a lot. Probably it would be quite good for
tortoise's server, but bad for a client.

| * Tests: test coverage is reasonable and uses the bzrlib.tests package.
|   However, it isn't integrated into the 'test runner' - how can I
|   tell the bzr test runner to run the tests from my various tests dirs?
|

So that it runs when you run "bzr selftest"? You install a plugin and then give
it either a "load_tests()" member or a "test_suite()" member. (load_tests is
preferred, because it allows the function to skip loading if they aren't going
to be run, while test_suite() assumes you always return a fully populated test
suite.)

| * Command-line options: use optparse module?
|

That would be my recommendation.

| * Recursive status requests: currently we request the full recursive status
|   of a tree as that is our only option.  While this works OK with the
| current
|   code, its not optimized for that case (ie, the code is optimized for only
|   one directories worth of info coming back at a time.)  If we need to stick
|   with this technique, the DirectoryCache should change to take better
|   advantage of it (eg, the cached unit would become the entire tree, not
|   a single directory.)

See massive discussion on the mailing list.

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

iEYEARECAAYFAkhO+JEACgkQJdeBCYSNAAMxoACfWEB94fWWTljjZlYvSt9D4W2q
aWIAn2gMLOMprptRgVjQ/JzwiByH9q9J
=+WBh
-----END PGP SIGNATURE-----