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

[Robin Stocker <robin@xxxxxxxxx>]

Hi Laurent,

> Continuing on this effort, a number of patches that make progress in
> this direction have been contributed and merged. For the most part,
> they were patches I made as preparative work : take ancestor into
> account, use accurate revision instead of local data for the "left"
> side... I have now started to work on the real need; using the
> models when comparing. This requires me to modify all of the
> comparison action handlers ... and there are a lot.
> 
> All of the things that can launch a comparison (action handlers,
> history view, commit viewer...) use a different code to launch the
> comparison. I plan on factorizing all of this into a single entry
> point, removing duplicated code in the process.

Good idea!

> However, I have some
> trouble with the comparisons "with index" (either as left or right
> side of the comparison). I just pushed a draft version of this work
> on https://git.eclipse.org/r/11966 . This is not in a state where it
> can be properly reviewed (some actions are not yet refactored... and
> the tests need to be changed to take the new behavior into account),
> but I'd like some input on two points in particular :
> 
> CompareUtils (
> https://git.eclipse.org/r/#/c/11966/1/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/CompareUtils.java
> ) line 554 :
> I placed a TODO there... since I really don't know : is there a point
> in trying to find a common ancestor between "Index" and ....
> something? The two comparison actions we have is "compare Index with
> HEAD" and "Compare with Index". So the two only things we can get
> there with are HEAD or the working tree. IIUC, there is no real
> meaning in trying to find a common ancestor there, which would be
> either HEAD (compare index with HEAD) or the Index (compare with
> Index)... since the common ancestor is equal to one of the two
> compared versions, there are no possible conflicts, and thus no
> reason to use 3-way rather than 2-way comparison. Am I right in
> these assumptions?

Yes, I think so. For conflicting files, depending on the compare
being done, the ancestor is represented by the "base" index entry.

But this shouldn't be a concern right now as comparing ours/theirs
in case of conflicts is not yet implemented, see here:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=391856

> GitModelSynchronize (
> https://git.eclipse.org/r/#/c/11966/1/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/synchronize/GitModelSynchronize.java
> ) line 110 :
> 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?

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

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

> Am I even
> right in assuming that there is no RevCommit or special identifier
> that corresponds to the index and could be used?

Yes, see Matthias' reply.

Cheers,
  Robin Stocker

> Thanks in advance for any input on this.
> 
> Laurent Goubet
> Obeo
> 
> On 15/02/2013 10:09, Laurent Goubet wrote:
> 
> 
> Hi all,
> 
> I am currently working on the integration with EGit and the model
> providers from Eclipse Team. The basic need is that some actions
> executed on the repository content need to be aware of logical
> models provided by the model provider(s) and retrieve multiple files
> instead of a single one.
> 
> Indeed, there are times when a "physical" file is part of a bigger
> picture. For example, an html file depends on its associated CSS
> file to be properly rendered, a split zip file depends on the
> presence of all of its fragments to be opened, a java file requires
> all of its imports in order to be compiled... For most of these, it
> is "harmless" (sort of) to ignore the "whole picture" and consider
> only a single file when applying actions. Java, for one, will still
> allow you to open the erroneous file and edit its "import" section
> to remove the parts that were not properly updated. An html file
> with wrong links will still be openable in a browser, even though it
> will be missing the rendering layed out in its CSS file. For others,
> the "bigger picture" will be broken if we only consider a single
> file. The zip file will be corrupted and totally unusable if I
> replace one of its fragments by an older version from the git
> history.
> 
> The model providers API is designed to tackle such needs and allow
> third-party plugins to provide their own logic to resolve the
> logical model, from a given "starting point" resource. The starting
> point is usually the resource selected in the workspace for an
> action (such as "replace", "compare", "commit" ...). In my case, the
> logic I am contributing to Team is aimed at resolving EMF models.
> One model depends on its fragments as well as all of its referenced
> resources to be present, lest it be corrupted and unloadable (you
> may want to look at
> http://wiki.eclipse.org/EMF_Compare/Logical_Model for an in-depth
> description of the use case).
> 
> Benjamin Muskalla had worked back in the end of 2011 to make the
> "Replace With" action aware of model providers (through
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=354932 ). The "commit"
> action still ignores them (trying to commit a file that is part of a
> model *should* prompt the user to commit the rest of the logical
> model). My priority though is to get all of the "comparison" actions
> to properly call the Team APIs (see bug
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=354474 ). Some
> improvements towards this have been made through bug
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=393225 and the
> "compare with > reference" action now properly calls the model
> providers for additional input. However, all of the remaining
> actions are still ignoring this API :
> 
> 
>     * Compare With Menu
> 
> 
>         * Commit
>         * Git Index with HEAD
>         * HEAD Revision
>         * Previous Revision
>         * Branch, Tag or Reference << as outlined above, this one is
>         actually working thanks to bug 354932
>         * Git Index
>     * History View
> 
>         * double-click a commit
>         * right-click > compare with workspace
>         * double-click a file (bottom-right pane)
>         * right-click a file > compare with version in ancestor
>         * right-click a file > compare with version in workspace
>     * Commit Viewer
> 
>         * double-click a file
>         * right-click a file > compare with version in ancestor
>         * right-click a file > compare with version in workspace
>     * Git Staging view
> 
>         * double-click a file (staged or unstaged)
> 
> 
> We'd like to know if the EGit team has any plans to make all of these
> actions support extensions to the model providers, and how we could
> best contribute to this effort.
> 
> 
> We also meet some problems in other parts of EGit. We do not have an
> explicit dependency on EGit, JGit, CVS, Subversive .... or any other
> Team Provider. We target "Team" as a whole and are independent from
> the actual provider that's behind a given project (we don't even
> care whether there _is_ a Team provider in the first place). As
> such, we are querying the file history and revisions through the
> Team APIs (IFileRevision, IFileHistoryProvider, ...) and we need
> EGit to properly fill all information even when used through this
> "proxy". We've started looking at what's missing and providing
> patches ( https://bugs.eclipse.org/bugs/show_bug.cgi?id=398982 ),
> please let us know if there are things we can do to make these
> contributions better.
> Laurent Goubet
> Obeo
> 
> _______________________________________________
> 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
>