Re: CVSRoot

Ojares Rami EINT wrote:
> 
> I wrote for my self a new CVSRoot class because at the time it was faster for me.
> Now I would like to discuss a few thoughts before submitting obscure patches.

O.K.

> First the distinction between local and server connection types.
> Now both use ServerConnection that does not use sockets but both
> execute cvs server on local machine.

Yes, this is somewhat confusing.

> 
> Should it be so that there are two different classes
> ServerConnection: opens socket to host and starts cvs server
> LocalConnection: starts cvs server on local machine.

Well, as Milos explained, :local: connection method is not a "connection to"
something, but rather client+server together. It directly acts on the
repository without any client-server communication. The JavaCVS does not
implement the server and thus is not capable of :local: connections. As a
workaround for that, ServerConnection was used, which connects to the "cvs
server".

But I agree, that ServerConnection should be able to connect to remote servers.
We can then implement a separate LocalConnection, that would connect to the
"cvs server" as a workaround for the real :local: mode.
I've never used/tested ServerConnection. IMHO :server: method is not much used
anyway and for local connection it does not make much sense to use JavaCVS,
since you need to use the cvs.exe as a server anyway. In NetBeans we use
JavaCVS only for :pserver: method, but :server: mode can be handy as well.

> 
> There are few aspects to CVSRoot
> 1. recognizing existing cvsroots correctly

There's a simple test case for that at
javacvs/test/unit/library/org/netbeans/lib/cvsclient/CVSRootTest.java
If you find some cvsroot for which the test fails, please publish them.

> 2. writing new cvsroots correctly

This is not solved in the CVSRoot class, it was not needed yet. But it's not
hard to implement I guess.

> 3. comparing cvsroots for equality

The same as above.

> 
> Let's review LOCAL connection type
> 
> It should recognize urls with format
> [:local:[username:][password:]]repositoryPath
> So if :local: does not exist username and password are not allowed.

Hmm, this is a mistake in the CVSRoot class. Following CVSROOT is not
considered as a bad one:
:local:user@hostname:/path/to/repository

> Since local connection never uses username or password
> it should always create urls of format
> :local:repositoryPath
> Of course it could also create url
> repositoryPath
> but it is maybe less clear because it omits the connection type.
> And it would be more clear if it would tell the user what kind of
> cvsroots it creates.

I agree. Currently CVSRoot just returns the original CVSROOT that you gave to
it to parse.

> 
> Then equality should only compare the equality of repository
> for LOCAL connection type.

O.K.

> 
> Here is an implementation of CVSROOT that tries to be clear of those
> issues and after it ConnectionFactory that implements the distinction between
> LOCAL and SERVER.
> 
> public class CVSRoot {
> 
> [snip]
> 
> }

I've skimmed the code, and it looks good. One difference is, that the original
CVSRoot can parse CVSROOTs of unknown method names, therefore the parsing as
such finish correctly and the method name is compared later to the known once.
It is therefore more generic. If you introduce integer constants for connection
method types, you need to update the CVSRoot class each time you add a new
method. But otherwise it works in a similar way I guess. You have an extra
constructor with Properties (where do you use this?). Since you change
equals(), we need to implement hashCode() as well.
Instead of
if (new File(this.repository)).equals(new File(compared.repository)))
one can use:
if (this.repository.equals(compared.repository))
with the same result I believe. You probably meant to write:
if (new File(this.repository)).getCanonicalFile().equals(new
File(compared.repository).getCanonicalFile()))


> 
> And then the ConnectionFactory's getConnection method
> 
> // Note the parameter is CVSRoot instead of string (allows us to create CVSRoot the way we want.
> public static Connection getConnection(CVSRoot root) throws IllegalArgumentException {
> 
>     if (root.getConnectionType() == CVSRoot.LOCAL) {
>         return new LocalConnection(root);
>     }
>     if (root.getConnectionType() == CVSRoot.SERVER) {
>         return new ServerConnection(root);
>     }
>     if (root.getConnectionType() == CVSRoot.PSERVER) {
>         return new PServerConnection(root);
>     }
>     if (root.getConnectionType() == CVSRoot.EXT) {
>         return new SSHConnection(root);
>     }
> 
>     throw new IllegalArgumentException("Unrecognized CvsRoot: " + cvsRoot);
> }

You'll never reach the throw clause when CVSRoot can parse only the four
pre-defined connection methods.

I would rather have the CVSRoot more abstract and have the String connection
type constants. That's only my personal preference ;-)

> 
> The PServerConnection has to be changed a bit to accommodate the changes in CVSRoot
> but that is easy.
> 
> I can change a diff with the changes + SSHConnection addition.
> J2SSH library requires commons-logging package so that would have to be included
> too (which is not so nice). But maybe javacvs could use that logging package too
> instead of obscure System properties.

How does it differ from java.util.logging? But since it's apache license, there
should not be much problems.

> 
> - rami
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]