[egit-dev] locking mechanism

[Laurent Goubet <laurent.goubet@xxxxxxx>]

Hi all,

I am not familiar with the locking mechanism that's been used in JGit, I think it is too limited, but I don't really know if it would be possible (and how) to make it a little friendlier. Consider the very simple test I attach to this mail (I had it as org.eclipse.egit.ui.test\src\org\eclipse\egit\ui\DeleteTest.java ... but its location is somewhat irrelevant).

The gist of it is : if I lock a repository's dir cache, I can no longer manipulate the workspace files supervised by it, even from sub-calls "under" my lock, but that's inconsistent :

DirCache dirCache = repo.lockDirCache();

IFile file1 = iProject.getFile(new Path("file1"));
file1.create(new ByteArrayInputStream(new byte[0]), false, new NullProgressMonitor()); // <- I can "create" a file just fine

file1.delete(false, new NullProgressMonitor()); // <- but tring to delete it fails miserably

dirCache.unlock();

I understand that what happens here is that EGit fails because of a workspace hook to automatically add "deleted" files to the index, but that hook cannot lock the index since I've already taken that lock beforehand.

My questions would be :

- Why are we only "automatically adding to the index" when the user deletes files, but not when he adds some? I think that if we do one, we should do the other. Not to mention that here, the hook fails to automatically stage the deletion of a file which creation had yet to be staged (i.e. a no-op).
- Since I call "delete" in a place where I _do_ hold the DirCache's lock, should this really fail? There should be a way for the delete hook to get a hold of the lock its thread holds, but how could I go about making this lock thread local?

This is a very simple test case, but it actually mimics a behavior I have seen from within a rebase operation (I am currently looking at how to make the rebase/cherry-pick/merge/... aware of logical models). Basically, this locking mechanism fails when there are multiple "worlds" in effect : when I make EGit interact with Team or other Eclipse plugins, there is a chance that that "other plugin" will try and make changes to the workspace. Eclipse handles that gracefully : I am within a WorkspaceOperation, I hold the lock for the containing IProject, I don't need to re-lock the file I need to change. JGit does not fare as good : I hold the index lock, yet I still try to re-lock it from a callee.

We should be able to share locks with callees, and I think the "current Thread" is a good context for this sharing... Yet I wonder if there were other reasons for this in JGit other than mimicking the cgit behavior?

Laurent Goubet
Obeo

package org.eclipse.egit.ui;

import java.io.ByteArrayInputStream;

import org.eclipse.core.resources.IFile;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.runtime.NullProgressMonitor;
import org.eclipse.core.runtime.Path;
import org.eclipse.egit.core.project.RepositoryMapping;
import org.eclipse.egit.core.test.GitTestCase;
import org.eclipse.egit.core.test.TestRepository;
import org.eclipse.jgit.dircache.DirCache;
import org.eclipse.jgit.lib.Repository;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

public class DeleteTest extends GitTestCase {
	private Repository repo;

	private IProject iProject;

	private TestRepository testRepo;

	@Before
	public void setUp() throws Exception {
		super.setUp();
		iProject = project.project;
		testRepo = new TestRepository(gitDir);
		testRepo.connect(iProject);
		repo = RepositoryMapping.getMapping(iProject).getRepository();
	}

	@After
	public void clearGitResources() throws Exception {
		testRepo.disconnect(iProject);
		testRepo.dispose();
		repo = null;
		super.tearDown();
	}

	@Test
	public void deleteTest() throws Exception {
		DirCache dirCache = repo.lockDirCache();

		IFile file1 = iProject.getFile(new Path("file1"));
		file1.create(new ByteArrayInputStream(new byte[0]), false,
				new NullProgressMonitor());

		try {
			file1.delete(false, new NullProgressMonitor());
		} catch (Exception e) {
			// tearDown will fail too if we don't unlock now, preventing us to
			// see this failure message
			dirCache.unlock();
			throw e;
		}
	}
}

begin:vcard
fn:Laurent Goubet
n:Goubet;Laurent
org:<a href="http://www.obeo.fr";>Obeo</a>
email;internet:laurent.goubet@xxxxxxx
url:http://www.obeo.fr
version:2.1
end:vcard