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

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.


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


Back to the top