| Hi!
I've managed to make the changes as you were telling in your previous mail. All tests (the new and the old ones) pass. The new patchset has been uploaded to https://git.eclipse.org/r/#/c/14968/
Best regards, Mikael Hey Nibor,
Thank you for you answer and your reviews. Laurent is currently on vacation and will be back in the beginning of September. I will try to take over during this time lapse.
See my comments below.
Le 5 août 2013 à 16:39, Robin Stocker <robin@xxxxxxxxx> a écrit :
Hey Laurent,
See inline replies below.
Hi guys,
Any news about these reviews? I've just updated the existing reviews
to rebase them and better reflect their dependencies (I had one
backwards, which is why I still had some test failures on
https://git.eclipse.org/r/#/c/11966/ ).
I think I'll have some time in the next weeks to look at the reviews.
I'd like to get some help with https://git.eclipse.org/r/14968 .
Basically, the GitSyncCache is updated a little too eagerly, and I
tend to end with a cache that's far from reflecting the true state
of my workspace files. The test I provided along with this review
shows an obviously wrong behavior :
----------
public void test() {
[...]
GitSubscriberResourceMappingContext context = ...;
assertTrue(context.hasRemoteChange(iFile1, new
NullProgressMonitor())); // works as expected (iFile1 has remote
change, the method returns true)
assertTrue(context.hasRemoteChange(iFile2, new
NullProgressMonitor())); // works as expected (iFile2 has remote
change, the method returns true)
assertTrue(context.hasRemoteChange(iFile1, new
NullProgressMonitor())); // Unexpectedly fails. iFile1 has remote
changes, we just checked. Bu the method still returns false
}
----------
This is due to GitSyncObjectCache#merge() resetting the state of any
files not being refreshed to "IN-SYNC" (so the second assertion
above resets the state of "iFile1" to "IN-SYNC", whatever its real
state). Removing this (as does the change on review) will make that
test pass... but two others will fail (as can be seen on the report
from the automatic hudson job for this change :
https://hudson.eclipse.org/sandbox/job/egit.gerrit/4742/ ).
Could anyone take a look (the original code was from Dariusz Luksza
back in 2011 if that helps, https://git.eclipse.org/r/#/c/3891/ )
and provide me with any lead as to how I could go and fix that
properly?
I undid the change to GitSyncObjectCache#merge and put a breakpoint in
there, then ran your test.
An interesting place is in GitResourceVariantTreeSubscriber.refresh.
As can be seen from the local variables, the refresh is done just on
the resource /Project-1/file1.sample.
Because it's not a full refresh, GitSyncCache.getAllData is called
with the updateRequests. The resulting cache looks like this:
/home/robin/junit-workspace/.git: entry: ThreeDiffEntry[null null]
members: entry: ThreeDiffEntry[MODIFY Project-1]
members: entry: ThreeDiffEntry[MODIFY Project-1/file1.sample]
The existing cache looks like this:
/home/robin/junit-workspace/.git: entry: ThreeDiffEntry[null null]
members: entry: ThreeDiffEntry[MODIFY Project-1]
members: entry: ThreeDiffEntry[ADD Project-1/.classpath]
entry: ThreeDiffEntry[ADD Project-1/.project]
entry: ThreeDiffEntry[MODIFY Project-1/file1.sample]
entry: ThreeDiffEntry[ADD Project-1/bin]
entry: ThreeDiffEntry[MODIFY Project-1/file2.sample]
// etc.
Now the existing cache is merged with newCache.
The problem is that the merge method in GitSyncObjectCache
assumes that the passed cache is complete, but in reality it was
filtered to only the specified resource(s).
To fix this, I think the updateRequests have to be passed to merge
and merge should only change the nodes whose path is included in the
filtered paths.
Do you think you can prepare a fix like that?
I will try to do that very soon.
By the way, is there a bug report about this, or could you create one?
Seems like this could be the cause for some different user-visible
problems, a bug report would help with tracking/referencing.
Yes. Here is the bug report: https://bugs.eclipse.org/bugs/show_bug.cgi?id=415430. I hope it does not bother you I copied pasted your comments on this bug.
For the five other changes I own, very briefly :
https://git.eclipse.org/r/11928 should now be good.
https://git.eclipse.org/r/13404 , https://git.eclipse.org/r/11966 and
https://git.eclipse.org/r/13405 all aim at solving the same issue of
launching comparisons with the same code from everywhere,
considering models along the way.
https://git.eclipse.org/r/14967 and https://git.eclipse.org/r/14966
are probably easier, since they're just the fix for an NPE observed
on some configurations and the addition of new tests.
As a side note, I am mostly familiar with the comparison side of
things (compare and synchronize), as well as with the corresponding
Team's APIs; I'd be willing to become a commiter if that could help
in maintaining or helping with the reviews in that part of the code
in EGit.
Regards,
Laurent Goubet
Obeo
On 04/06/2013 19:42, Matthias Sohn wrote:
On Tue, Jun 4, 2013 at 1:18 PM, Laurent Goubet <
laurent.goubet@xxxxxxx > wrote:
Thanks Matthias,
I've finally managed to get these SWTBot tests working locally, and
my latest patch sets "should" solve the test failures.
The builds triggered by these have all failed, but... :
- https://git.eclipse.org/r/#/c/13405/ : only one unrelated failure,
a wrong dependency to hamcrest?
this is caused by
https://bugs.eclipse.org/bugs/show_bug.cgi?id=404346
- https://git.eclipse.org/r/#/c/13404/ : three unrelated failures,
two of which seem to be random SWTBot errors.
- https://git.eclipse.org/r/#/c/11966/ : This one should be
re-triggered. The failures are related, but wrong : since the two
aforementioned changes depend on that one, and they didn't fail,
this one should not either (and the previous patch set was actually
working)
These changes are no longer drafts and can be reviewed, thanks for
the time :).
I'll have a look soon
--
Matthias
_______________________________________________
egit-dev mailing list
egit-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/egit-dev
_______________________________________________
egit-dev mailing list
egit-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/egit-dev
_______________________________________________
egit-dev mailing list
egit-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/egit-dev
|
