Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [egit-dev] locking mechanism

Comments inlined Below

On 18/12/2013 16:06, Lay, Stefan wrote:
See my comments below

From: Laurent Goubet <laurent.goubet@xxxxxxx>
Reply-To: "laurent.goubet@xxxxxxx" <laurent.goubet@xxxxxxx>
Date: Mittwoch, 18. Dezember 2013 10:52
To: EGit discussion <egit-dev@xxxxxxxxxxx>
Subject: [egit-dev] locking mechanism

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).

 I also don't understand why deletions are automatically added to the index. Maybe this was necessary at the very beginning of the Egit/Jgit project. I would rather remove it for the deletion of files than add it for adding files.

I would also prefer it that way (removing the delete hook altogether). I wonder whether this was initially made to detect "moves" though?

- 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?

I think the intention of the DirCache's lock is to use it on low-level operations on the git index. In that case it should not happen that another code is called which also wants to lock the DirCache. So what is the use-case for locking the DirCache and inside the lock calling an operation on an Eclipse-Resource?
Actually, this would not bother me if the delete didn't have a specific hook trying to lock the index. Sorry for the long-winded introduction, but I think it will be more comprehensible if I talk about the context beforehand :

Eclipse allows plugins to define what is a "model" to them, as well as how the model's components (a set of distinct IFiles) should be handled. Eclipse Team builds upon that to allow model providers to define how their own files (components of their models) should be merged (see also org.eclipse.team.core.mapping.IResourceMappingMerger). This means that the merging of files shouldn't be done through JGit's own algorithms when faced with files that define their own merger.

Adding support for custom merge drivers is one step towards that (I still need to complete the changes that were asked for on the related review)... but it cannot handle all cases. In the end, a merge driver is only called when there are "textual" conflicts (as opposed to "logical" conflicts) detected on the file to merge... on a single file. On the other hand, "we" (model implementers) have our own ways to detect conflicts, and we do so on multiple files at once. There may very well exists "textual" conflicts that do not translate into logical conflicts (i.e. we can perfectly merge the files without any hiccup)... and reversely : logical conflicts may very well exist within files that _do not_ textually conflict with each other.

In order to correctly handle these cases we need to have a merging strategy that is aware of the workspace, its IResources and the ModelProviders that may be defined on these resources. I can get to the point were EGit provides its own subclass of the org.eclipse.jgit.merge.RecursiveMerger to a strategy registry. This allows me to properly take precedence over the low-level merge drivers used by JGit on the files composing a larger logical model. This is still pretty low level though : it will be used by rebases, merges, cherry-picks...

And there is my use case : the merge strategies take the DirCache lock pretty early on (and that's expected, they know they'll have to modify the index). For the recursive merger, this is done at the very beginning of the merge operation, in org.eclipse.jgit.merge.ResolveMerger.mergeImpl() . However, for every file that is part of the operation, I'll try and see whether a model provider whishes to handle the merging for that file and its model. If it does, then it is up to it to determine what operations are needed on the workspace files, then tell us what's to be "marked as merged" (i.e. "added to the index")... and if it decides that one of the files needs to be deleted, the deletion hook coughs up an exception when it does not manage to lock the already locked index.

I may be able to sneak deep enough inside the model mergers to "catch" deletion requests and avoid the hook's interference... but all this really made me wonder "why" we are trying to automagically change the index in some cases of user-workspace intercations.

Laurent Goubet
Obeo
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


Back to the top