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

[Laurent Goubet <laurent.goubet@xxxxxxx>]

Hi,

Okay, I've finally gotten back to this amidst the holidays and release/build/... tasks. I've tried to limit the size of all this by splitting the tasks :

- Factorizing the comparison code, making sure we use the same code for each of the 17 actions to launch comparisons. I have submitted a second patch set on https://git.eclipse.org/r/#/c/11966/ for review.
- Enabling restrained synchronizations : I am making these changes to support the logical model; this means that EGit will launch synchronizations not only on the whole repository as it does today, but also on smaller sets of files. I have rebased https://git.eclipse.org/r/#/c/11928/ (which makes sure EGit does not fetch "too much" from the repository when synchronizing) and submitted https://git.eclipse.org/r/#/c/13404/ that allows for finer-grained restrictions on the resources included in a synchronize operation.

This second action would also allow us to provide "synchronize" actions on folders or projects instead of the sole "synchronize workspace" we have now.

I have not modified the comparisons from the commit dialog since these open comparison dialogs instead of editors. I haven't made the changes to have less "magic" comparisons with the index (compare index with head, compare with index) for now either since those will need a much more intrusive change. I'll explore Robin's suggestion on that, but it will be another commit :p.

Hudson does not seem to have been triggered by these reviews. Has it been disabled? I could not launch the SWTBot tests on my machine, so was expecting its results :).

Laurent Goubet
Obeo

On 23/04/2013 10:40, Laurent Goubet wrote:

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

_______________________________________________
egit-dev mailing list
egit-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/egit-dev

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