[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [stellation-res] Revised version of Jonathan Gossage's workspace audit
|
I enclose my responses to the revised workspace audit done by Jonathan
Gossage. I have done something for each entry, as follows:
Done - code has been changed as suggested.
TODO - a comment that begins with /* TODO JG.n
has been inserted in the corresponding source file, where n is the
issue number.
I plan to check in my latest revisions soon. These include the work I
did over the weekend, much of which involved changing the handling of
Project signature to be consistent with the way it's now done for
artifacts, including the use of MD5 checksum for the project
signature.
I propose to then freeze the API while Jim Wright gets the GUI working
using the new workspace code, working only on Javadoc while this is
being done (and continuing until fully up to date Javadoc available
thereafter).
Once GUI conversion complete, Jonathan and I can work to sort out
the remaining issues by taking care of each TODO.
dave
On Tue, Sep 03, 2002 at 10:32:47AM -0400, Dave Shields wrote:
>
> org.eclipse.stellation.workspace.Change
>
> 1. Should use the default platform directory separator instead of a hard
> coded '/'.
>
TODO
>
> 2. The method "fileNotFound" should log through log4J. If the user
> needs this output on sydout than use the appropriate appender. Logging
> directly to stdout is problamatic since stdout will not be usefully
> available in all environments (such as within Eclipse).
>
TODO
> 3. The method "inform(String") needs a Javadoc entry.
>
> Is stdout an appropriate destination if this method invoked from
> within Eclipse? Would an optional callback mechanism make sense?
>
TODO
> 4.After discovering and reporting that the file does not exist in the
> workspace should either an exception be thrown or a silent exit
> taken. It does not make sense to proceed.
>
TODO
> 5. The method "synchronize" should use symbolic return codes. It
> should also use the return codes from the lower level "rm" and "add"
> calls. At the least it should accurately return the overall status of
> the operation.
>
Done.
> 5A.Should the synchronize operation terminate if errors are encountered?
> If so it should rollback any changes and I don't see that the current
> environment has the bookeeping to do this.
>
No. Svc does 'save' before synchronize, so user can do their own rollback.
>
> 5B.This method does not honour return codes from lower level
> methods.Should it do so and should a rollback mechanism be
> implemented?
>
Return codes have been eliminated.
> org.eclipse.stellation.workspace.Checkin
>
> 6. The method "markExecutables" is now a nop and has only one
> invocation from in this class. Should this both be removed?
>
markExecutables deleted.
> org.eclipse.stellation.workspace.Checkout
>
> 7. The call to the Unix utility "sleep" seems a bit heavy-weight when
> you can easily use the Java Object.Wait(time) calls. Incidentally
> Windows has the same problem with file modified time, so this should
> not be Unix specific anyway.
>
TODO. I tried to do this but ran into problem so I left in old code
and commented-out the new code I had just added.
> org.eclipse.stellation.workspace.Configuration
>
> 8. In method "addTypes" the Javadoc parameters have a spelling error
> in "patterm".
>
Done.
>
> 9. In method "defaultSuffixes" there are a number of common Windows
> suffixes that should be added, including the following:
>
> The following should be data types
> "exe", "sys", "lib"
>
> The following should be ignore type
> "obj"
>
> The following should be text type
> "cxx", "hpp", "bat", "bas", "xml" (maybe, not sure of the implications of embedded CDATA)
> "txt", "def"
>
TODO
> 10. The method "defaultTypes" should use symbolic names for default
> pattern names. These names are used elsewhere.
>
No names assigned. Left code as is.
>
> org.eclipse.stellation.workspace.Configure
>
>
> 11. Also is the character '/' in the constant "/.*" intended to be
> interpreted as a path component separator? If so it should be
> represented as the default platorm separator.
>
TODO
>
> org.eclipse.stellation.workspace.Context
>
> 12. The "getComment" method needs updated Javadoc with @param, @return
> and @throws specs.
>
> If you prompt for a comment in command line mode, you should tell the
> user how to end the comment. Many users have forgotten or do not know
> how to type an EOF from the keyboard. As a quick check I asked our
> Unix developers where I work and not one could remember how to do
> this. I couldn't either!
>
TODO. It's control-D, though as I recall one can use stty to set this
character to some other value so you can't assume that control-D will work.
>
> org.eclipse.stellation.workspace.Create
>
> 13. "create" method Should this not be made a static method of
> workspace.Project? As it stands it simply looks like a kuldgy way of
> getting a non-class method.
>
> Given that all the necessary functionality is in Project is there
> really any need for a "do nothing" class just to support the create
> command. If you were going to add the ability to create more types of
> objects than simply projects than this class would make sense.
>
TODO. I agree, but left code as is for now. We can sort out later.
>
> org.eclipse.stellation.workspace.Differ
>
>
> 15. The Javadoc comment for the "compare" method is a little
> misleading. It should make clear that what is being compared is the
> text artifacts in the current workspace with a branch of the same
> project in the repository. Part of the confusion rises from the fact
> that a project object represents a branch and it's materialization in
> a workspace, not an entire project. Also it is missing a @param spec
> for the second project.
>
TODO
>
> 16. The method "getRevisionProject" looks as if it belongs in the
> Project class. It offers no functionality that is specific to the
> differ command but simply implements some functionality that uses the
> Project class.
>
TODO (I agree this should be done).
> 17. Method "getStateProject" nedds a Javadoc update - needs @throws
> spec. Again this method really looks like it belongs in the Project
> class.
>
TODO (I agree this should be done).
>
> org.eclipse.stellation.workspace.Export
>
> 18. Should use platform default dpath component separator rather than hard
> coded constant.
>
TODO
>
> org.eclipse.stellation.workspace.Files
>
> 19. Javadoc comment for "closeContentDirectories" is misleading as
> perhaps is the method name. The content directores are being deleted,
> not closed.
Renamed closeContentDirectories() to eraseContentDirectories().
>
> 20. The two file copy methods will benefit significantly from an
> increase in the buffer size used from 4096 to 65536. The reason for
> this has to do with the caching strategies used with both Windows and
> Linux will get a strong hint to read ahead aggressively which is
> exactly what you want when doing file copies. The only cost is a
> larger tan necessary buffer with small files, but that cost is less
> than the overhaed involved in discovering the file size and tuning the
> buffer size.
>
Done.
> 21. In the method "emptyArchive" should it be an error to encounter a
> sub-directory in the archive directory? If not is it intentional to
> leave any discovered sub-directories alone.
>
TODO
> 22. The method "find" is not going to work properly on Windows systems
> unless Cygwin and the GNU utilities are installed. For the moment,
> this is acceptable, but over the long term it seems a bit much to have
> to install Cygwin simply to get access to "find" and "diff", and at
> some point alternates for these utilities should be found or
> developed.
>
"find" intended only for use on Unix. We need to revisit all the
platform-specific code in Files (and elsewhere, though any such code
should be in Files) as part of win32 port.
> 23. The method "findFileInfo" can be given an alternative
> implementation using Java IO only in a Windows environment. The reason
> for this is that in Windows you are only interested in regular files
> and directories and do not have to worry about executable files or
> symbolic links. This would remove the need for the "find" utility in
> Windows since the only use Stellation seems to make of it is to
> classify the workspace files. Of course, if you are willing to allow
> JNI, then there is no need for the "find" utility in a Unix
> environment either.
>
See comment for 22 above.
>
> 24. The method "getCanonicalPath" will not work correctly as it stands
> since it will not properly normalize all Windows paths. The problem is
> that in Windows both '\' and '/' are allowed as path component
> separators in all places except the command line and can be intermixed
> in the same path. The easiest solution would be to add a first pass
> that converts all instances of path separators into the default path
> separator for the platform. This will also fix numerous downstream
> problems where other methods try to parse path strings to identify
> directories.
>
> This routine also needs to deal with UNC prefixes and/or drive letters
> on Windows
>
TODO. The intent is that early on, as arguments are processed, paths
are put into a canonical form consisting of the full pathname, so that
most code in the system need not further worry about canonical form
and can use the path directly to access the associated file.
> 25. The method "getContentDirectory" can easily be modified to not
> require looping to determine the next directory name to
> allocate. Simply get a list of the files and directories in the
> context root directory and use the list size as the file name for the
> next directory. This will be more efficient than testing files one at
> a time.
>
>
TODO
> 26. Should the method "getCurrentDirectory" throw an exception if it
> is not defined. This represents a serious software error and I doubt
> that any of this methods consumers are prepared to handle a null
> pointer.
>
TODO
> 27. The method "getExecutables" should return an empty StringSet
> immediately on Windows since the Unix concept of executable file
> attribute is not applicable. I don't think that it is necessary to
> simulate this because even if a Windows executable gets stored on a
> Unix file system it is not likely to be executed!
>
Done.
> 28. The method "getLinkTarget" should return a null immediately on a
> Windows platform since soft links are not available at application
> level.
>
Done.
> 29. The method "getMembers" should check against the project path
> symbolically rather than using hard coded path.
>
The intent is to use 'SVC' for the project directory now and forever,
so no symbolic name needed.
> 30. The method "getName" is not guaranteed to work on Windows unless
> the path is in the form of an absolute canonical path. This is because
> if the path has not been normalized you cannot guarantee what path
> separator is present. For safety's sake all paths should be normalized
> to use the default path component separator for the active platform.
>
See comment for 24
> 31. The method "getParentName" is not guaranteed to work on Windows
> unless the path is in the form of an absolute canonical path. This is
> because if the path has not been normalized you cannot guarantee what
> path separator is present. For safety's sake all paths should be
> normalized to use the default path component separator for the active
> platform.
>
See comment for JG.24
> 32. The method getTime can be refactored a bit to eliminate redundant
> code when the Java File class can be used.
>
TODO
> 33. The ASCII tests in "getTypeFromContents" are only valid for basic
> ASCII chaacter sets. Ultimately, Stellation should be able to deal
> with files that use any encoding and any character set, but this is a
> substantial area that is better dealt with later.
>
TODO
> 34. In the method "getWorkspaceFiles" the direcctory name for the
> project directory should not be hard coded.
>
This method has been deleted.
> 34A.
> The Perl compiler should be configured to generate case insensitive
> patterns on a Windows platform.
>
TODO
> 35. "isWorkspacePath" will only work reliably under Windows if path
> component separators have been normalized to the platform
> default. Javadoc needs upgrading to add @param spec.
>
See comment for JG.24
>
> 35A.If the call "renameTo" fails should revert to doing a file copy/delete
> sequence. There are too many legitimate cases on both Windows and Unix
> to give up without a final try.
>
TODO
> 36. The method "normalizeText" neds the Javadoc entry revised to
> reflect new implementation.
>
> I have serious doubts about the usefulness of this operation. It is
> costly and given that all internal operations are done on text
> artifacts where line termination characters have been removed during
> the construction process I don't see the benefit.
>
It is costly, but it is needed. For example, I have encountered
unnormalized files in the Linux kernel, and several times after Jim
Wright has checked in code he created under Eclipse and forgot to put
in "normal" form.
> 37. The method "removeFile" returns one of a set of small integers as
> a return code. This seems to be a fairly common idiom in
> Stellation. Should these values not be given symbolic names and used
> consistently throughout the code base?
>
Done. Use of return codes eliminated.
>
> org.eclipse.stellation.workspace.GetProperty
>
>
> 38. The method "getProperty" should use symbolic values for exit codes.
> Also the comments indicate an uncertainty over the return code set
> when a named property is not present. This should be resolved but
> given that this method is called from the command line it is probably
> better to leave things as they are so external scripts can know what
> is happening.
>
Use of return codes eliminated.
> org.eclipse.stellation.workspace.Location
>
> 39. The constructor Javadoc should be updated with @param spec. Also,
> database names should be referenced symbolically and other location
> option names should also be symbolic. For the longer term this method
> probably needs to be initialized with static tables defining protocol
> specific options that may be present in the location string. This can
> be done during database protocol loading.
>
TODO
> 40. The Javadoc for method "getHost" needs updating since this call is
> valid for some local protocols such as MySQL. However, it is probably
> better to replace the parameter specific calls "getHost" or "getPort"
> with generic calls that take the name of the option as a parameter and
> always return a string. This mechanism will fit with the table driven
> parameter identification mechanism.
>
TODO
> 41. A specific variable to identify a local protocol is now required
> since some local protocols may use socket connections and need host
> and port parameters.
>
TODO
>
> org.eclipse.stellation.workspace.Merge
>
>
> 42. The method "cleanCompound" should use the platform default path
> component separator in the logging statements for tidiness and
> consistency when normalized path names are used.
>
TODO
> org.eclipse.stellation.workspace.Project
>
>
> 43. The method "buildPaths(String)" needs a Javadoc entry. The
> variable _pathMap is problamatic as it stands. Since it is going to
> contain file paths, it needs a custom comparator to ensure that
> compares on the paths are case sensitive on Unix systems and case
> insensitive on Windows systems.
>
TODO.
>
> 44. The method "getProjectSignature" need the Javadoc updated with a
> @throws spec. It should also use symbolic names for attribute
> keys. There is a problem with the computation of the project
> signature. It uses the Java algorithm to hash a string. The problem
> with this is that hash production algorithms are only required to
> provide a reasonable expectation that a collision will not occur. In
> other places you use the MD5 hash which has much better behaviour in
> this respect. For strong one-way hash algorithms, the probability that
> two input streams will generate the same hash is roughly equivalent to
> 1 in 2^number of hash bits. So an MD5 hash will give you odds of 1 in
> 2^64. Probably the best hash algorithm currently available is the
> SHA-1 hash. It uses a 20 byte hash and so yields a 1 in 2^160
> probability of generating a collision. The donside of course is that
> it requires 20 bytes of storage as opposed to 8 bytes for your current
> hashes.
>
> If you are interested in using this algorithm, there is an excellent
> open source implementation in the Bouncy Castle cryptographic library.
>
> This method should also use symbolic names for attribute keys.
>
>
TODO. I converted Project signature to be md5 checksum this weekend.
I used MD5 only since I didn't want to run into any US export
regulations re cryptographic software, and a quick search via Google
indicated MD5 was NOT subject to any such restrictions.
It's easy to convert to SHA if it is not subject to any export
restrictions.
> 45. The method "setProjectBranch" has an editing errorcheckError" in
> the Javadoc.
>
Done.
> 46. The method "setProperty" needs the Javadoc updated for a changed
> calling sequence. Also would it not be prudent to add a second to the
> "propertyModificationTime" to ensure that it is always different to
> the oroginal time.
>
TODO
> 47. In the method "update" the discussion on hashes in point 26 for
> this class is relevant. Note that the code for signature computation
> can be significantly simplified if you use SHA_1 which is guaranteed
> to produce a non-zero hash.
>
update() has been removed. See comment for 44 re use of SHA.
> Also the discussion in the Files class point 37 on the need for text
> file normalization is relevant.
>
> 48. in method "write(OutputStream)" there is a comment about
> PrintWriter error handling. Although PrintWriter does not throw
> exceptions it does provide a "checkError" method that allows errors to
> be detected and if so then an exception can be thrown.
>
TODO
>
> org.eclipse.stellation.workspace.Svc
>
>
> 49. Also there is a "TODO" comment on one statement in this method. Is
> this still relevant and if so what needs to be done?
>
Done.
> 50. The method "main" breaks conventional usage in the Javadoc
> entry. The method does not return a value, instead it calls a system
> function to set the exit code. This should be documented but in the
> description, not in a @returns tag.
>
TODO
>
> 51. The Javadoc description is misleading for method
> "printException". The method operates on an exception period. It knows
> nothing about throw or catch operations in other methods and in fact
> may be called before an exception is first thrown in some
> circumstances.
>
TODO
> 52. Also there seems to be a bit of fragility in the use of the
> "simple" option type. Currently the only option that has this type is
> "help". The semantics of the type are not specified but the code uses
> the type as a signal to issue the help message rather than directly
> recognizing the option. If the "simple" type has real meaning than it
> is possible that other options might be added in the future with this
> type and the existing code would then break;
>
TODO
>
> org.eclipse.stellation.workspace.Workspace
>
> 53. In method "getProjectDocumentFile" the XML file suffix should be
> symbolic.
>
TODO. This may be a case where a literal name is appropriate.
--
Dave Shields, IBM Research, shields@xxxxxxxxxxxxxx.
