Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 317934

Summary:

Synchronize View does not use correct common ancestor

Product:

[Technology] EGit

Reporter:

Stefan Lay <stefan.lay>

Component:

Core

Assignee:

Chris Aniszczyk <caniszczyk>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

caniszczyk, dariusz.luksza, matthias.sohn

Version:

unspecified

Target Milestone:

0.9.0-M2

Hardware:

OS:

All

Whiteboard:

Attachments:

Description	Flags
Three way compare in synchronize view	none
Presentation of fixed three-way compare view	none

Description Stefan Lay

2010-06-25 05:10:08 EDT

The Synchronize View uses the current HEAD as the base for the comparison.

For explanation please see this simple example together with the screen shot of the three-way compare triggered from the synchronize view:

<example>
! [branch1] Change in branch1
 * [master] Change in master
--
 * [master] Change in master
+  [branch1] Change in branch1
+* [master^] Create class Fritz.java

I had added a comment to the class Fritz.java both in branch1 and in master. Then I synronized master with branch1. As base version I expect master^. However, as can be seen in the screen shot, the content of master is displayed there.
</example>

The reason seems to be that GitResourceVariantTreeSubscriber returns the content of the current HEAD in the method getBaseTree(). This is in my opinion not the correct implementation when we compare two branches.

I would suggest the following solution:

Return the content of the common base commit of the HEAD and the other branch here. The common base can be found using a RevWalk using RevFilter.MERGE_BASE. You can look at the class org.eclipse.jgit.revwalk.RevWalkMergeBaseTest for an example. We should discuss the (rather exotic) case of multiple merge bases with Shawn and the other JGit experts.

I think that with this approach a lot of coding in GitSyncInfo and GitResourceVariantComparator would be way easier. In GitInfo we could get rid of the strange traversal over the commit lists in calculateDescBasedOnGitCommits. I would even assume that the algorithm in calculateKind() of the base class SyncInfo would already do the most of the work.

Comment 1 Stefan Lay

2010-06-25 05:11:33 EDT

Created attachment 172726 [details]
Three way compare in synchronize view

The creation of the attachment had failed during bug creation.

Comment 2 Dariusz Luksza

2010-06-25 08:02:53 EDT

I'm currently working on this issue.My idea was to implement whole structure of IProject, IFile, IContainer, IFolder and IResource so that this implementation can read data from Git and return this when team framework calls roots() method on GitResourceVariantTreeSubscriber.

But Stefan's approach seams to be easier, therefore I'll switch and try to implement this as it is suggested.

Comment 3 Dariusz Luksza

2010-06-27 20:20:44 EDT

I think that I've manage to fix this issue. As Stefan suggested I used RevWalk with RevFilter.MERGE_BASE. Additionally I've added Git implementations for eclipse's resource interfaces (IResource, IContainer, IFile and so on) to be able return IResource[] in GitResourceVariantTree.members().

I think that this change will be ready to publish in two or three days. This code really need to be refactored ;), also tests need to be written.

Comment 4 Dariusz Luksza

2010-06-27 20:21:49 EDT

Created attachment 172865 [details]
Presentation of fixed three-way compare view

Comment 5 Dariusz Luksza

2010-06-29 20:04:45 EDT

I hit a race condition some how ... when synchronization stops on break points for debuging result is returned properly, but when I remove break points synchronization says "no changes found"; in some cases . This is quite huge problem because synchronization process is quite complicated :|

Comment 6 Dariusz Luksza

2010-06-30 21:07:55 EDT

I've manage to fix this issue (actually this was my mistake because remote resource variants had a reference to local resource content instead of content obtained from git).

My patch set desperately need some cleaning and refactoring (before I'll publish it). I'll continue working on it and push it as fast as I can.

Comment 7 Dariusz Luksza

2010-07-20 04:01:43 EDT

I've already pushed patch for this issue for code review:
http://egit.eclipse.org/r/#change,1064

Comment 8 Chris Aniszczyk

2010-08-02 10:25:41 EDT

I created a CQ for this change...
   https://dev.eclipse.org/ipzilla/show_bug.cgi?id=4390

Comment 9 Chris Aniszczyk

2010-08-03 10:42:26 EDT

Dariusz could you make the necessary confirmations for the IP review as a
comment to this bug:

a.  authored 100%
b.  has the rights to donate the content to Eclipse
c.  contributes the content under the EPL

Comment 10 Dariusz Luksza

2010-08-03 10:48:34 EDT

(In reply to comment #9)
> a.  authored 100%

Yes, this code is 100% authored by me

> b.  has the rights to donate the content to Eclipse

Yes, I have full right to donate this code to Eclipse

> c.  contributes the content under the EPL

Yes, it is contributed under EPL

Comment 11 Chris Aniszczyk

2010-08-05 15:31:53 EDT

Fixed with. 986e3823e1e3555bda426b3b09bd790d4595113a.

Thanks Dariusz!