Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [egit-dev] Logical model support in EGit 
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
>

Back to the top