support "keep this much disk space free" on Windows as well as other platforms #637

New issue

Closed

opened 2009-02-22 03:47:18 +00:00 by zooko · 35 comments

zooko commented

2009-02-22 03:47:18 +00:00

However, rather than having such fallbacks, I would really rather just ensure that Tahoe figures out how much free disk space it has on all supported platforms. For Windows (the only platform which currently does not work using statvfs), the way to do that is to require pywin32, import win32api, and call win32api.GetDiskFreeSpaceEx()

http://docs.activestate.com/activepython/2.5/pywin32/win32api__GetDiskFreeSpaceEx_meth.html
http://msdn.microsoft.com/en-us/library/aa364937(VS.85).aspx

Careful, don't call win32api.GetDiskFreeSpace() -- that's an old function that predates disks with more than 2^32^ bytes:
http://msdn.microsoft.com/en-us/library/aa364935.aspx

This patch changeset:2e45619844cb7c36 makes it so that if we don't know how to find out the free disk space on a platform, we fall back gracefully by display something like "I dunno" instead of the free disk space. We also already have code (before that patch) which falls back gracefully in the sense of emitting a warning at startup if the `reserved_space` feature has been configured (see src:doc/configuration.txt) and the current platform can't figure out how much free disk space it has. However, rather than having such fallbacks, I would really rather just ensure that Tahoe figures out how much free disk space it has on all supported platforms. For Windows (the only platform which currently does not work using `statvfs`), the way to do that is to require `pywin32`, `import win32api`, and call `win32api.GetDiskFreeSpaceEx()` <http://docs.activestate.com/activepython/2.5/pywin32/win32api__GetDiskFreeSpaceEx_meth.html> <http://msdn.microsoft.com/en-us/library/aa364937(VS.85).aspx> Careful, don't call `win32api.GetDiskFreeSpace()` -- that's an old function that predates disks with more than 2^32^ bytes: <http://msdn.microsoft.com/en-us/library/aa364935.aspx>

zooko added the

labels 2009-02-22 03:47:18 +00:00

zooko added this to the 1.4.1 milestone 2009-04-01 14:21:41 +00:00

zooko self-assigned this 2009-04-01 14:21:41 +00:00

zooko commented

2009-10-25 17:39:07 +00:00

Author

Any Windows hackers want to fix this? It really bothers me to see Windows getting second-class-citizen treatment, but I don't have convenient access to a Windows box right now to work on, and I'm prioritizing testing, packaging, and release-management for the imminent Tahoe-LAFS v1.6 release.

It really should be pretty easy. In fact, I'm marking it with the "easy" tag.

zooko modified the milestone from 1.6.0 to eventually

2009-10-25 17:39:07 +00:00

daira commented

2009-10-25 22:28:23 +00:00

I'll do this.

daira modified the milestone from eventually to 1.6.0

2009-10-25 22:28:23 +00:00

zooko was unassigned by daira

2009-10-25 22:28:23 +00:00

daira self-assigned this 2009-10-25 22:28:23 +00:00

daira commented

2009-10-25 22:37:36 +00:00

Hmm. It is possible that the call to get disk stats might fail, for some other reason than the API not being available. What should happen in that case?

Is it OK to delete redundant or now-misnamed helper methods like do_statvfs and stat_disk in storage/server.py? (I'll keep get_available_space.)

Hmm. It is possible that the call to get disk stats might fail, for some other reason than the API not being available. What should happen in that case? Is it OK to delete redundant or now-misnamed helper methods like do_statvfs and stat_disk in storage/server.py? (I'll keep get_available_space.)

zooko commented

2009-10-25 23:09:54 +00:00

Author

Hm, if the call to get disk stats were to fail, what should we do? I think I would suggest logging an error message and returning that there is zero disk space free.

It is okay (indeed encouraged) to refactor code to be cleaner whenever you make a change. (Part of the reason that this is okay is that each change is expected to have full test coverage.)

Hm, if the call to get disk stats were to fail, what should we do? I think I would suggest logging an error message and returning that there is zero disk space free. It is okay (indeed encouraged) to refactor code to be cleaner whenever you make a change. (Part of the reason that this is okay is that each change is expected to have full test coverage.)

daira commented

2009-10-26 00:54:03 +00:00

In _auto_deps.py:

## The following block is commented-out because there is not currently a pywin32 package which
## can be easy_install'ed and also which actually makes "import win32api" succeed.  Users have
## to manually install pywin32 on Windows before installing Tahoe.
##import platform
##if platform.system() == "Windows":
##    # Twisted requires pywin32 if it is going to offer process management functionality, or if
##    # it is going to offer iocp reactor.  We currently require process management.  It would be
##    # better if Twisted would declare that it requires pywin32 if it is going to offer process
##    # management.  Then the specification and the evolution of Twisted's reliance on pywin32 can
##    # be confined to the Twisted setup data, and Tahoe can remain blissfully ignorant about such
##    # things as if a future version of Twisted requires a different version of pywin32, or if a
##    # future version of Twisted implements process management without using pywin32 at all,
##    # etc..  That is twisted ticket #3238 -- http://twistedmatrix.com/trac/ticket/3238 .  But
##    # until Twisted does that, Tahoe needs to be non-ignorant of the following requirement:
##    install_requires.append('pywin32')

Is this still a problem? Will things break if it is uncommented?

In _auto_deps.py: ``` ## The following block is commented-out because there is not currently a pywin32 package which ## can be easy_install'ed and also which actually makes "import win32api" succeed. Users have ## to manually install pywin32 on Windows before installing Tahoe. ##import platform ##if platform.system() == "Windows": ## # Twisted requires pywin32 if it is going to offer process management functionality, or if ## # it is going to offer iocp reactor. We currently require process management. It would be ## # better if Twisted would declare that it requires pywin32 if it is going to offer process ## # management. Then the specification and the evolution of Twisted's reliance on pywin32 can ## # be confined to the Twisted setup data, and Tahoe can remain blissfully ignorant about such ## # things as if a future version of Twisted requires a different version of pywin32, or if a ## # future version of Twisted implements process management without using pywin32 at all, ## # etc.. That is twisted ticket #3238 -- http://twistedmatrix.com/trac/ticket/3238 . But ## # until Twisted does that, Tahoe needs to be non-ignorant of the following requirement: ## install_requires.append('pywin32') ``` Is this still a problem? Will things break if it is uncommented?

daira commented

2009-10-26 01:02:23 +00:00

The easy_install bug against pywin32:
http://sourceforge.net/tracker/index.php?func=detail&aid=1799934&group_id=78018&atid=551954
isn't fixed. So I'll leave this as it is; the Windows build instructions at http://allmydata.org/trac/tahoe/wiki/InstallDetails already say to manually install pywin32.

The easy_install bug against pywin32: <http://sourceforge.net/tracker/index.php?func=detail&aid=1799934&group_id=78018&atid=551954> isn't fixed. So I'll leave this as it is; the Windows build instructions at <http://allmydata.org/trac/tahoe/wiki/InstallDetails> already say to manually install pywin32.

daira commented

2009-10-26 01:06:34 +00:00

Gack, I'm suffering from random Vista brokenness (I think it is this issue: http://cygwin.com/ml/cygwin/2009-04/msg00252.html ) that's stopping me from testing this patch. Might be a couple of days before I can get round to it.

Gack, I'm suffering from random Vista brokenness (I think it is this issue: <http://cygwin.com/ml/cygwin/2009-04/msg00252.html> ) that's stopping me from testing this patch. Might be a couple of days before I can get round to it.

zooko commented

2009-10-26 01:28:09 +00:00

Author

Sigh. Waitaminute, why do you need cygwin to test your patch?

You are right to leave the pywin32 requirement commented-out for now.

Sigh. Waitaminute, why do you need cygwin to test your patch? You are right to leave the `pywin32` requirement commented-out for now.

daira commented

2009-10-26 02:07:38 +00:00

Replying to zooko:

Hm, if the call to get disk stats were to fail, what should we do? I think I would suggest logging an error message and returning that there is zero disk space free.

Done, and added a unit test for this.

Waitaminute, why do you need cygwin to test your patch?

Python depends on cygwin. Actually I'm not getting the same error any more, even though I all I did was build in a different bash prompt. (Doesn't it worry you when computers work nondeterministically?)

Replying to [zooko](/tahoe-lafs/trac/issues/637#issuecomment-370684): > Hm, if the call to get disk stats were to fail, what should we do? I think I would suggest logging an error message and returning that there is zero disk space free. Done, and added a unit test for this. > Waitaminute, why do you need cygwin to test your patch? Python depends on cygwin. Actually I'm not getting the same error any more, even though I all I did was build in a different bash prompt. (Doesn't it worry you when computers work nondeterministically?)

daira commented

2009-10-26 02:08:21 +00:00

Attachment ticket637-untested.darcspatch.txt (35279 bytes) added

improved patch for ticket 637

**Attachment** ticket637-untested.darcspatch.txt (35279 bytes) added improved patch for ticket 637

ticket637-untested.darcspatch.txt

34 KiB

daira commented

2009-10-26 02:15:10 +00:00

Ah, I have two different Python installations (cygwin Python 2.5 and native-Windows Python 2.6). That explains it.

zooko commented

2009-10-26 20:07:01 +00:00

Author

Okay so the current status is that we're waiting for unit tests, right?

zooko commented

2009-10-27 02:33:35 +00:00

Author

If you like this ticket you may also like #814 (v1.4.1 storage servers sending a negative number for maximum-immutable-share-size?).

zooko commented

2009-10-29 18:19:46 +00:00

Author

David-Sarah, are you planning to write a unit test for this?

daira commented

2009-10-29 22:00:20 +00:00

Yes. It would just be a simple sanity check that get_disk_stats doesn't fail and returns values greater than zero on all platforms.

I've been a bit swamped and haven't had time to debug the current patch yet; currently it is causing other storage server tests to fail. Should get round to it later today.

Yes. It would just be a simple sanity check that get_disk_stats doesn't fail and returns values greater than zero on all platforms. I've been a bit swamped and haven't had time to debug the current patch yet; currently it is causing other storage server tests to fail. Should get round to it later today.

daira commented

2009-11-06 07:27:45 +00:00

Attachment ticket637-tested.darcspatch.txt (13354 bytes) added

Tested fix for #637 and #814

**Attachment** ticket637-tested.darcspatch.txt (13354 bytes) added Tested fix for #637 and #814

ticket637-tested.darcspatch.txt

13 KiB

daira commented

2009-11-06 07:29:38 +00:00

Sorry that took so long. The new patch is only tested on Windows (native); it needs review and testing on Unix and cygwin.

daira removed their assignment 2009-11-08 03:34:37 +00:00

kevan commented

2009-11-18 06:19:54 +00:00

Owner

I'll take a stab at reviewing this (I can at least test it on OS X). ticket637-tested.darcspatch.txt looks like the output of darcs diff, though -- darcs apply barfs on it, at least. Would you mind updating that to be something from darcs send?

zooko commented

2009-11-18 16:46:08 +00:00

Author

Here are instructions for how to generate patches: http://allmydata.org/trac/tahoe/wiki/Patches . Also if the patch submitter doesn't provide a darcs send-style patch but just a diff-style patch, you can always just apply it with patch. That loses some metadata such as the original patch author's name, and it doesn't work well for patches which mv files, rename tokens or otherwise do something fancy, but if it works it works.

Kevan: if you're planning to review this maybe "accept" the ticket so that it is assigned to you.

Here are instructions for how to generate patches: <http://allmydata.org/trac/tahoe/wiki/Patches> . Also if the patch submitter doesn't provide a `darcs send`-style patch but just a `diff`-style patch, you can always just apply it with `patch`. That loses some metadata such as the original patch author's name, and it doesn't work well for patches which mv files, rename tokens or otherwise do something fancy, but if it works it works. Kevan: if you're planning to review this maybe "accept" the ticket so that it is assigned to you.

kevan commented

2009-11-20 21:28:40 +00:00

Owner

The tests work okay on my Mac.

I'm not sure how closely we adhere to PEP-8 (the CodingStandards page mentions it, but lots of code in tahoe-lafs ignores it), but a number of places in the patch violate it. In particular,

The stuff in _auto_deps.py
Line 45 of src/allmydata/storage/server.py
The docstring starting on line 160 of src/allmydata/storage/server.py.
The docstring starting on line 243 of src/allmydata/storage/server.py.

On lines 42 and 46 of src/allmydata/storage/server.py, you have Windows line endings instead of unix line endings. I'm not sure how big a deal this is, but most of the other code seems to use unix line endings.

I don't understand why we treat a failure in the win32api system call the same way as not having an API to determine disk usage. In the former case, we make the server read-only and report that it has no free space. In the latter case, we make the server writable and report that it has an arbitrary amount of free space. To me, these errors seem like two different expressions of the same problem -- that the server can't figure out how much free space there is. Why not treat them the same?

Or, put another way: If the user specifies a reserved space requirement, and we can't honor it (for whatever reason), we should make the server read only (or at least that seems more right to me).

(this isn't so much a critique of the patch -- indeed, this is existing behavior -- as a question that the patch makes me want to ask. Perhaps I should open another ticket for it?)

In any case, expressions of this issue are on lines 225 and 249 of src/allmydata/storage/server.py.

I like the tests -- I don't see any problems with them.

The tests work okay on my Mac. I'm not sure how closely we adhere to PEP-8 (the [CodingStandards](http://allmydata.org/trac/tahoe/wiki/CodingStandards) page mentions it, but lots of code in tahoe-lafs ignores it), but a number of places in the patch violate it. In particular, * The stuff in _auto_deps.py * Line 45 of src/allmydata/storage/server.py * The docstring starting on line 160 of src/allmydata/storage/server.py. * The docstring starting on line 243 of src/allmydata/storage/server.py. On lines 42 and 46 of src/allmydata/storage/server.py, you have Windows line endings instead of unix line endings. I'm not sure how big a deal this is, but most of the other code seems to use unix line endings. I don't understand why we treat a failure in the win32api system call the same way as not having an API to determine disk usage. In the former case, we make the server read-only and report that it has no free space. In the latter case, we make the server writable and report that it has an arbitrary amount of free space. To me, these errors seem like two different expressions of the same problem -- that the server can't figure out how much free space there is. Why not treat them the same? Or, put another way: If the user specifies a reserved space requirement, and we can't honor it (for whatever reason), we should make the server read only (or at least that seems more right to me). (this isn't so much a critique of the patch -- indeed, this is existing behavior -- as a question that the patch makes me want to ask. Perhaps I should open another ticket for it?) In any case, expressions of this issue are on lines 225 and 249 of src/allmydata/storage/server.py. I like the tests -- I don't see any problems with them.

daira commented

2009-11-21 00:18:53 +00:00

For _auto_deps.py and the docstrings in server.py, I just followed the conventions already used in those files.

The line endings should indeed be fixed.

import win32api, win32con also violates PEP 8, although I can't actually see the reason for that style constraint.

Re: error handling -- I assume you mean, why '''don't''' we treat a failure in the system call in the same way as not having an API? The reason is that not having an API is not an indication that there is anything wrong; just that the platform has a limitation that means we can't implement space checking.

If the platform does have the API, OTOH, then we're supposed to be enforcing space checking, and an admin may be expecting that we do. So a system call failure should be treated as an error in that case, as suggested by Zooko in comment:6.

For _auto_deps.py and the docstrings in server.py, I just followed the conventions already used in those files. The line endings should indeed be fixed. `import win32api, win32con` also violates PEP 8, although I can't actually see the reason for that style constraint. Re: error handling -- I assume you mean, why '''don't''' we treat a failure in the system call in the same way as not having an API? The reason is that not having an API is not an indication that there is anything wrong; just that the platform has a limitation that means we can't implement space checking. If the platform does have the API, OTOH, then we're supposed to be enforcing space checking, and an admin may be expecting that we do. So a system call failure should be treated as an error in that case, as suggested by Zooko in comment:6.

kevan commented

2009-11-21 02:44:50 +00:00

Owner

That's right -- why aren't they treated the same. Sorry for the confusion.

I think I have a reply for your comment on error handling, but it is off topic -- your patch is consistent with the existing behavior of not enforcing space reservations when an API isn't available, and attempting to strongly enforce them on platforms that have an API. If I can articulate that into a good argument, I'll post it in another ticket.

In summary: with fixed windows line endings, and if the PEP8 leeway is okay, then this patch looks okay.

That's right -- why aren't they treated the same. Sorry for the confusion. I think I have a reply for your comment on error handling, but it is off topic -- your patch is consistent with the existing behavior of not enforcing space reservations when an API isn't available, and attempting to strongly enforce them on platforms that have an API. If I can articulate that into a good argument, I'll post it in another ticket. In summary: with fixed windows line endings, and if the PEP8 leeway is okay, then this patch looks okay.

zooko commented

2009-11-21 04:30:39 +00:00

Author

Thanks for the code review, Kevan! What about _auto_deps.py violates PEP-8?

I updated CodingStandards to explicitly reject PEP-8's rule about importing multiple modules on a single import line and PEP-257's rule about closing multi-line docstrings with \n\n"""\n.

David-Sarah: The patch description currently says:



q
ticket637

I.e., the first line is a blank line, the second line is 'q', and the third line is 'ticket637'. Please change it (by running darcs amend) to something like:

storage server: detect disk space usage on Windows too (fixes #637)

Then re-attach the patch with the new description. Actually if you prefer I can make that change when I commit your patch.

Thanks for the code review, Kevan! What about `_auto_deps.py` violates PEP-8? I updated [CodingStandards](wiki/CodingStandards) to explicitly reject PEP-8's rule about importing multiple modules on a single `import` line and PEP-257's rule about closing multi-line docstrings with `\n\n"""\n`. David-Sarah: The patch description currently says: ``` q ticket637 ``` I.e., the first line is a blank line, the second line is 'q', and the third line is 'ticket637'. Please change it (by running `darcs amend`) to something like: ``` storage server: detect disk space usage on Windows too (fixes #637) ``` Then re-attach the patch with the new description. Actually if you prefer I can make that change when I commit your patch.

kevan commented

2009-11-21 04:38:34 +00:00

Owner

From http://www.python.org/dev/peps/pep-0008/:

Maximum Line Length

Limit all lines to a maximum of 79 characters.

and

For flowing long blocks of text (docstrings or comments),
limiting the length to 72 characters is recommended.

From <http://www.python.org/dev/peps/pep-0008/>: Maximum Line Length Limit all lines to a maximum of 79 characters. and For flowing long blocks of text (docstrings or comments), limiting the length to 72 characters is recommended.

zooko commented

2009-11-21 05:05:49 +00:00

Author

Okay I've updated CodingStandards to add this pearl of wisdom:

Ignore the part of PEP-8 which specifes 79- or 72- char line widths. Use whatever line-widths look best to you in your editor on your screen. Please don't change other people's line-widths unnecessarily, making it look better on your display and worse on theirs. That's not nice. (Note: unless you are Brian. Brian can do anything he wants!)

Therefore David-Sarah's original patch is now in compliance with our coding standards. I'll amend it to have a nice patch description and I'll commit it.

Okay I've updated [CodingStandards](wiki/CodingStandards) to add this pearl of wisdom: * Ignore the part of PEP-8 which specifes 79- or 72- char line widths. Use whatever line-widths look best to you in your editor on your screen. Please don't change other people's line-widths unnecessarily, making it look better on your display and worse on theirs. That's not nice. (Note: unless you are Brian. Brian can do anything he wants!) Therefore David-Sarah's original patch is now in compliance with our coding standards. I'll amend it to have a nice patch description and I'll commit it.

daira commented

2009-11-21 05:13:19 +00:00

We're already way over 79 characters for many lines in the existing source.

Actually I think (although this is probably the wrong place to have the discussion) that the 79-character limit is completely obsolete for modern screens and programming languages. Consider lines such as

stats['storage_server.disk_free_for_nonroot'] = disk['free_for_nonroot']

storagedir = os.path.join(self.sharedir, storage_index_to_dir(storage_index))

Wrapping them would just make them less readable, not more.

OTOH, 100 characters is a reasonable limit, so I'll try to reformat the patch to satisfy that.

Apologies for the darcs patch description; darcs dumped me into an editor that was using the wrong code page, so I got a screenful of mojibake and had to type the description blind.

We're already way over 79 characters for many lines in the existing source. Actually I think (although this is probably the wrong place to have the discussion) that the 79-character limit is completely obsolete for modern screens and programming languages. Consider lines such as ``` stats['storage_server.disk_free_for_nonroot'] = disk['free_for_nonroot'] ``` or ``` storagedir = os.path.join(self.sharedir, storage_index_to_dir(storage_index)) ``` Wrapping them would just make them less readable, not more. OTOH, 100 characters is a reasonable limit, so I'll try to reformat the patch to satisfy that. Apologies for the darcs patch description; darcs dumped me into an editor that was using the wrong code page, so I got a screenful of mojibake and had to type the description blind.

kevan commented

2009-11-21 05:18:39 +00:00

Owner

Okay, cool. I don't really care either way about the line widths; it was intended as an informational item more than anything else. :)

daira commented

2009-11-21 06:02:13 +00:00

Attachment ticket637.darcspatch.txt (35270 bytes) added

storage server: detect disk space usage on Windows too (fixes #637)

**Attachment** ticket637.darcspatch.txt (35270 bytes) added storage server: detect disk space usage on Windows too (fixes #637)

ticket637.darcspatch.txt

34 KiB

daira commented

2009-11-21 06:03:01 +00:00

Line widths fixed, like it or not :-) Also the line endings and patch description.

warner commented

2009-11-28 21:55:28 +00:00

It looks like the clause at line 229 (in get_stats) will put a negative number into storage_server.disk_avail, while I think it should be clamped at zero. Likewise, get_available_space and thus remote_get_version will see negative numbers. If #814 is supposed to be subsumed by this ticket, we'll need to fix that.

Maybe get_disk_stats should clamp the avail value at zero before returning anything?

It looks like the clause at line 229 (in `get_stats`) will put a negative number into `storage_server.disk_avail`, while I think it should be clamped at zero. Likewise, `get_available_space` and thus `remote_get_version` will see negative numbers. If #814 is supposed to be subsumed by this ticket, we'll need to fix that. Maybe `get_disk_stats` should clamp the `avail` value at zero before returning anything?

daira commented

2009-11-29 00:47:39 +00:00

Replying to warner:

Maybe get_disk_stats should clamp the avail value at zero before returning anything?

It already does; see server.py line 205.

Also see test_disk_stats in test_storage.py, line 481, although that obviously only tests the result of one call.

Replying to [warner](/tahoe-lafs/trac/issues/637#issuecomment-370708): > Maybe `get_disk_stats` should clamp the `avail` value at zero before returning anything? It already does; see `server.py` line 205. Also see `test_disk_stats` in `test_storage.py`, line 481, although that obviously only tests the result of one call.

daira commented

2009-11-29 00:58:03 +00:00

Replying to [davidsarah]comment:35:

Also see test_disk_stats in test_storage.py, line 481, although that obviously only tests the result of one call.

Actually this doesn't test the clamp on avail because it uses reserved_space=0. But test_disk_stats_avail_nonnegative just after it does test that avail is clamped to zero.

Replying to [davidsarah]comment:35: > Also see `test_disk_stats` in `test_storage.py`, line 481, although that obviously only tests the result of one call. Actually this doesn't test the clamp on `avail` because it uses `reserved_space=0`. But `test_disk_stats_avail_nonnegative` just after it does test that `avail` is clamped to zero.

warner commented

2009-11-30 20:05:01 +00:00

Doh, I don't know how I missed that.. must have been sleepy.

Excellent patch! Committed, in changeset:ef002c935a15eb76. The only change I made (in a second patch) was to the comment about readonly_storage causing avail<=0. The original was correct: it always causes avail=0.

thanks!

Doh, I don't know how I missed that.. must have been sleepy. Excellent patch! Committed, in changeset:ef002c935a15eb76. The only change I made (in a second patch) was to the comment about readonly_storage causing `avail<=0`. The original was correct: it always causes `avail=0`. thanks!

warner added the

r/fixed

label 2009-11-30 20:05:01 +00:00

warner closed this issue

2009-11-30 20:05:01 +00:00

daira commented

2009-11-30 23:45:15 +00:00

Replying to warner:

The only change I made (in a second patch) was to the comment about readonly_storage causing avail<=0. The original was correct: it always causes avail=0.

You mean server.py line 326? It was remaining_space <= 0, and my change was correct. remaining_space is >= 0 or None at line 309, but notice line 315, remaining_space -= self.allocated_size().

Replying to [warner](/tahoe-lafs/trac/issues/637#issuecomment-370711): > The only change I made (in a second patch) was to the comment about readonly_storage causing `avail<=0`. The original was correct: it always causes `avail=0`. You mean `server.py` line 326? It was `remaining_space <= 0`, and my change was correct. `remaining_space` is `>= 0` or `None` at line 309, but notice line 315, `remaining_space -= self.allocated_size()`.

warner commented

2009-12-01 02:53:32 +00:00

Gahh.. you're absolutely right. I've reverted to your version. And I'm going to stop trying to fix other people's fixes now.

(actually, I recently learned a term for these inevitable bugs in correcting-someone-else patches: Muphry's Law)

Gahh.. you're absolutely right. I've reverted to your version. And I'm going to stop trying to fix other people's fixes now. (actually, I recently learned a term for these inevitable bugs in correcting-someone-else patches: [Muphry's Law](http://en.wikipedia.org/wiki/Muphry%27s_law))