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
|