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