Re: Re: Confusing code in ObjectManager manage_FTPstat

On Mon, Oct 31, 2005 at 10:05:45AM -0500, Jim Fulton wrote:
| Sidnei da Silva wrote:
| >Found some lovely piece of code deep into the FTP parts of Zope 2 last
| >saturday, one of them is truely ugly. It's listing the contents of
| >the current and parent folders for no apparent reason (or at least, it
| >didn't make sense either to me or Chris McDonough).
| 
| There's a comment stating the intent.

Yes, but the original code does the check in a truely obscure way, at
least to me. I've thought of spelling:

        # check to see if we are acquiring our objectValues or not
        if not (len(REQUEST.PARENTS) > 1 and
                self.objectValues() == REQUEST.PARENTS[1].objectValues()):

As:

        if self.objectValues.im_self is not self:

However I'm not sure that would be the be the correct
replacement. There doesn't seem to exist any test for this.

| >The code in question is in the ``manage_FTPstat`` method of
| >``OFS.ObjectManager``. Tracing back the source of this code, it seems
| >to have been (surprisingly) introduced by Jim Fulton, back in
| >1999. [1]
| >
| >Later on, Amos Latteier changed part of the code (namely
| >``manage_FTPlist``) to use a slightly more friendlier, yet equally
| >confusing ``is_acquired`` method. [2]
| >
| >I'm now sitting here, trying to make sense of this code and wondering
| >what was the original intention in the first place. Would anyone have
| >a clue?
| 
| I think the comments make this pretyu clear.  The intent is to avoid
| providing listings of acquired objects.  In fact, the intent of both
| bits of code, I think is to prevent FTP access to acquired objects.

Wouldn't that be better implemented by making the same check as it's
done for WebDAV and set 'maybe_webdav_client' (abusing the name)
so that it sets the 'no_acquire_flag'?

| I suspect that there is a clearer way to implement the intent.

I have three suggestions right now:

 1. change manage_FTPstat to use the same thing as manage_FTPlist
    (using App.Common.is_acquired, and possibly rewriting is_acquired
    to a simpler check)

OR

 2. Throw away the `is_acquired` check and replace it by a much
    simpler check

OR

 3. Setting 'maybe_webdav_client' in the request object so the object
    is never acquired for FTP, the same way as it's done for WebDAV.

-- 
Sidnei da Silva
Enfold Systems, LLC.
http://enfoldsystems.com