Re: [egit-dev] Logical model support in EGit

[Laurent Goubet <laurent.goubet@xxxxxxx>]

Hi,

Thank you both for your answers.

I'll remove the TODO concerning the search for a common ancestor (3-way 
comparison) for comparisons "with index". We'll see during reviews 
whether I should leave a comment in the code to point to the reason we 
bypass that lookup in these cases.

> > This one has been much more of a headache... How could I make it
> > possible to launch a synchronization between "Index" and any other
> > revision? Synchronization uses a GitSynchronizeData as input. The
> > GitSynchronizeData uses RevCommits to describe the revisions to
> > compare. However... there are no RevCommit or commit ID
> > corresponding to the index version. In CompareUtils I have used
> > GitFileRevision.INDEX as the "magic" string to use in order to
> > compare with the index (though that in itself is a problem since the
> > user could have a branch named "Index"...
> > CompareTreeView.INDEX_VERSION is probably a better choice), but that
> > isn't an option in the GitSynchronizeData. What would be the better
> > approach?
> I think it would be good to raise the abstraction and don't pass around
> strings for describing what to compare.
>
> Just thinking out loud a bit:
>
> A base class, e.g. GitCompareSource
> Subclasses for different types, e.g. WorkspaceSource, IndexSource,
> CommitSource (by ID), BranchSource (by name)
>
> And the code using these should delegate to a method on the base class,
> with the subclasses implementing the logic where they differ.
>
> The abstraction should be made usable by both compare and synchronize.
>
> What do you think?

I was trying to keep as close as possible to the existing code, but yes, 
that does sound pretty good (at least, much better than my boolean 
proposition :p). This would be a much more intrusive change than I 
originally planned for though (and all of my duplicate removing is 
already making for a large change).

I'll try and see how this could be put into action once I have 
factorized everything.

> > Add a "useIndex" boolean such as
> > GitSynchronizeData.includeLocal and change every place that uses the
> > GitSynchronizeData?
> IMO we should try not to add another round of ifs, but instead also
> get rid of includeLocal.

We completely agree here. A boolean to check everywhere we use the 
synchronize data is already too much.


> > Or is there a less intrusive way?
> I don't think it will be easy to change to the above, but hopefully
> it should not be too much.

This will require changes in a good number of places... and a lot of 
unit tests to check for correctness and adapt to the behavior. Which is 
why I originally planned to keep close to the existing.

Maybe a better plan for this would be to split this in two, first 
factorize every comparison actions into a single entry point, except for 
the two "index" comparisons... then switch to using comparison source 
(or whatever we'll call them), and finally change the last two actions 
to use these new "sources" (or the reverse).

I'll be in holidays for a while, but I'll try to complete this asap when 
I return.

Laurent Goubet
Obeo

begin:vcard
fn:Laurent Goubet
n:Goubet;Laurent
org:<a href="http://www.obeo.fr";>Obeo</a>
email;internet:laurent.goubet@xxxxxxx
url:http://www.obeo.fr
version:2.1
end:vcard