CodingSpectator's watched directory may get locked

[reprogrammer]

If the user closes Eclipse while CodingSpectator is uploading its data, the svn working directory remains locked and future attempts to upload the data will fail until the user performs svn cleanup. CodingSpectator should perform svn cleanup whenever needed.

[vazexqi]

Steps to reproduce:

1. Put a big file (couple of MBs) into the watched directory i.e. ${RUNNING_ECLIPSE_INSTANCE}/.metadata/.plugins/org.eclipse.ltk.core.refactoring
2. Perform an upload through the preference page.
3. Just as it is about to upload, forcibly terminate the running Eclipse instance.
4. Do a 'svn status' on ${RUNNING_ECLIPSE_INSTANCE}/.metadata/.plugins/org.eclipse.ltk.core.refactoring and notice that it informs you that the files are locked.

   edu.illinois.codingspectator.monitor.ui.submission.Submitter$InitializationException: org.tmatesoft.svn.core.SVNException: svn: Working copy '/Users/vazexqi/Documents/runtime-EclipseApplication-CLEAN/.metadata/.plugins/org.eclipse.ltk.core.refactoring' locked; try performing 'cleanup'
   at edu.illinois.codingspectator.monitor.ui.submission.Submitter.authenticateAndInitialize(Submitter.java:73)
   at edu.illinois.codingspectator.monitor.ui.submission.Submitter.promptUntilValidCredentialsOrCanceled(Submitter.java:140)
   at edu.illinois.codingspectator.monitor.ui.Uploader.promptUntilValidCredentialsOrCanceled(Uploader.java:39)
   at ...  
   

The next time you try to upload data it fails, warning that the directory is locked (and suggesting to do a cleanup).

[vazexqi]

Resolved in a4ed8a7de87883dc11f4f2c3c50765014205f2d1

[reprogrammer]

I suggest the following refacgtorings.

1. Use SVNException.getErrorMessage().getErrorCode() instead of Exception.getMessage().contains("locked; try performing 'cleanup'").
2. Remove the argument to edu.illinois.codingspectator.monitor.core.submission.SVNManager.doCleanup(String)and make it use the field edu.illinois.codingspectator.monitor.core.submission.SVNManager.svnWorkingCopyDirectory instead.
3. Make edu.illinois.codingspectator.monitor.ui.submission.Submitter.tryCleanup() reuse edu.illinois.codingspectator.monitor.ui.submission.Submitter.svnManager rather than creating a new instance of SVNManager.

[vazexqi]

See 73a1743171ed8b2f031c75cb2859d59d208fb3d2

I've incorporated suggestion 1 and 2. I am not sure if it is beneficial to incorporate 3. I think it's easier to read if we just create a new SVNManager on the cleaning up. If we want to reuse, we have to be sure that a SVNManager is always available before calling tryCleanUp which seems like an implicit requirement that we might forget in the future. So creating a new instance is easier – everything is localized in that method.

[reprogrammer]

I think SVNManager and Submitter smell a little bit. The SVNManager class seems to have taken two different responsibilities: performing remote and local SVN operations. This is why it has two different constructors. As a result, Submitter constructs an instance of SVNManager that knows about the user credentials and shares it among its methods for remote operations. But, some methods of Submitter create their own instances of SVNManager because they just invoke local SVN operations. The problem with this setup is that if someone invokes the remote operations on an instance of SVNManager that lacks user credentials, he/she won't get any errors statically.

I suggest the following refactorings to remove the above code smell.

1. Provide only one constructor for SVNManager that takes both the path to the working directory and the user credentials. And, make Submitter create a single instance of SVNManager to be used by all of its methods.
2. Split SVNManager into two classes: RemoteSVNManager and LocalSVNManager .

[reprogrammer]

The plugin name edu.illinois.codingspectator.monitor.core.submitter is duplicated in Submitter.lookupExtensions(). I suggest to extract it either as a local variable or field.

[reprogrammer]

I suggest to simplify the API of SVNManager by removing the parameters of the two methods doAdd(String) and doCommit(String), and using the field svnWorkingCopyDirectory as other methods.

[reprogrammer]

There is a small code duplication in the methods of SVNManager because all of them create the instance new File(svnWorkingCopyDirectory). I suggest to remove this duplication by extracting it to a method or field.

[vazexqi]

Resolved in c464a0c7eef5da7b50b135a25773427dce8f1d13.

[reprogrammer]

The code now looks much better. But, there is still a small code smell. The constructor of LocalSVNManager takes username and password as its parameters. The problem is that LocalSVNManager doesn't need these two values at all. These two parameters are provided only because RemoteSVNManager, which is a subclass of LocalSVNManager, needs them. I think this is a sign that inheritance is not the right tool here. I suggest to use composition instead of inheritance to reuse code here. That is, let's make LocalSVNManager and RemoteSVNManager subclass a common class AbstractSVNManager, and introduce a class called SVNManager that is composed of instances of LocalSVNManager and RemoteSVNManager.

[vazexqi]

The creation of LocalSVNManager and RemoteSVNManager are encapsulated through SVNManager which hides the extra parameters.

I'm not sure about this last change – it seems like over-engineering. We don't have enough actual clients/use cases using this code so I can't evaluate realistically how much of a smell this is. Our goal is not to create an API but to get things done so I won't worry too much about this. This code should be marked as internal anyway.

For now I am not going to implement this change.

[reprogrammer]

I implemented the change in 84f6b20875b04d3c65a434e8a148aaf16d105dde.