Re: [egit-dev] handling of linked resources: please review

[Robin Stocker <robin@xxxxxxxxx>]

Hi François,

> With EGit 2.2 approaching it would be nice to move forward with this
> change I've been working on for a while:
> 
> https://git.eclipse.org/r/#/c/3680/
> 
> However there are important points to review which is whyI'm writing
> on this mailing list.
> 
> The point of this change is to make egit gracefully handle/ignore
> linked resources. In earlier version of this change most action
> handlers included a check to make sure no linked resources were
> selected. Recently I've come to realize this may not be desirable as
> such, and that what really needs to be checked is whether the
> selected items all point to the same repository. The latest patch
> set reflects that decision.
> 
> The recent change from Robin Stocker (
> https://git.eclipse.org/r/#/c/7564/ ) already underline the fact
> that egit can operate on resources that may not be part of a project
> in the workspace. A classic example is the main pom.xml of the EGit
> project which is at the repository root but not within any project.
> One can use the repository view to access such files and operate on
> them. Another way would be to create a linked resource pointing to
> PROJECT_LOC/../pom.xml (entered as PARENT-1-PROJECT_LOC/pom.xml when
> creating the linked resource). The latter method is nice because
> it's a quicker shortcut: one can operate on the file directly from
> the explorer instead of having to search for the repository view.

Sounds good.

> Earlier version of the patch would have prevented such possibility.
> In the latest patch set action handlers now test
> selectionMapsToSingleRepository() instead of
> selectionContainsLinkedResources(). That's the basic idea for most
> handlers. There are however two handlers that currently work on
> multiple repositories:
> org.eclipse.egit.ui.internal.actions.AddTo/RemoveFromIndexActionHandler.isEnabled()
> {
> return getProjectsInRepositoryOfSelectedResources().length > 0;
> }
> Both add and remove from index handlers can work on a selection that
> spans multiple repos. However looking at how the underlying
> operations are implemented, only
> org.eclipse.egit.core.op.AddToIndexOperation actually supports
> multiple repos. The counterpart
> org.eclipse.egit.core.op.RemoveFromIndexOperation operates on one
> repo only, given in its constructor.

AddToIndexOperation does it in a pretty complex way, it could be much
simplified by using ResourceUtil.splitResourcesByRepository.

RemoveFromIndexOperation should be fixed. I prepared a change for this:

https://git.eclipse.org/r/8988

> There is another discrepancy between these two actions/operations:
> the add uses GitScopeUtil.getRelatedChanges() to include related
> model elements while the remove doesn't.
> So here are questions that need answers:
> Q1-Should AddToIndex operate on multiple repositories, as it does
> currently?

I'd say yes (why not).

> Q2-Should RemoveFromIndex operate on multiple repositories as well
> (it doesn't at present, the corresponding operation will also need
> to be changed)?

Yes, see change above.

> Q3-Should RemoveFromIndex also include related model elements (it
> doesn't at present, unlike Add)?

I don't use the modelling part myself, but I'd say yes, as Add/Remove
should be congruent.

But that is not directly related to linked resources, is it? Please
file a bug so we can do it separately to simplify things.

> Also, while checking all these actions/operations/handlers in search
> for places where we should handle linked resources properly, I tried
> to understand how their execution is guarded. For handlers, the
> function isEnabled() prevents users from triggering an action when
> the selected resources are not adequate. In the case of
> org.eclipse.egit.ui.internal.actions.RepositoryAction, which is just
> a wrapper around a RepositoryActionHandler, the function isEnabled()
> calls handler.isEnabled(). However execute() and run() are guarded
> by shouldRunAction(), which by default returns true, and which needs
> to be overriden by subclasses.
> Actually only one subclass overrides this function:
> org.eclipse.egit.ui.internal.actions.RebaseAction.shouldRunAction() {
> Repository repo = getRepository(); // Renamed to
> getSelectionRepository() in my patch
> return !isInRebasingState(repo);
> }

I think this is related to Rebase being a "pulldown" menu in the toolbar.
The above was introduced with this change:

https://git.eclipse.org/r/#/c/5321/3

Maybe Dariusz or Matthias can explain.

> The corresponding action handler guard looks like this:
> org.eclipse.egit.ui.internal.actions.RebaseActionHandler.isEnabled()
> {
> Repository repo = getRepository(); // Renamed to
> getSelectionRepository() in my patch
> return repo != null && isLocalBranchCheckedout(repo);
> }
> Q4-Shouldn't the above two function be the same, ie check the same
> things?
> More generally:
> Q5-Do we have situations where the handler/action/operation is
> executed without isEnabled() guarding the execution?

No, I don't think so in general.

> Q6-If so shouldn't we also perform the check just before execution?
> More precisely could we have the handler's execute perform the same
> check as in isEnabled(), or if there's a corresponding
> org.eclipse.egit.core.op.IEGitOperation, have it perform the same
> check?

No, I don't think it's necessary (and sometimes checking enablement
can be expensive).

> Finally, beyond actions/operations/handlers, there are other cases
> below that need to be considered. Feel free to comment and add other
> cases that you think need special attention with regards to linked
> resources.
> 
> org.eclipse.egit.core.ContainerTreeIterator,
> org.eclipse.egit.core.IteratorService,
> org.eclipse.egit.core.internal.indexdiff.GitResourceDeltaVisitor:
> Linked resources are now ignored by these classes in order to keep
> them as close as possible to the file system which is the only thing
> jgit treewalk knows about.

Yes, linked resources that are from the repository working directory
will be visited by AdaptableFileTreeIterator at their "real" location.

In ContainerTreeIterator: Why do you change isEntryIgnored? I'd do
the filtering in the entries() method directly.

> org.eclipse.egit.core.project.RepositoryFinder: Linked resources are
> now ignored by deault, but there's an optional boolean to specify
> otherwise. This class is used when sharing a project not yet
> connected to EGit provider, in which case the search for a
> repository to connect to should not traverse linked resources.

Ok.

> org.eclipse.egit.core.synchronize.GitResourceVariantTreeSubscriber:
> from what I understand this is used in comparison and merging, so
> now it ignores linked resources because the actual filesystem tree
> is what needs to be handled.

Ok.

> org.eclipse.egit.ui.internal.operations.GitScopeUtil: No change there
> because it's used by ActionHandlers which already decide whether to
> ignore or not linked resources, so no need to filter twice.

Ok.

> org.eclipse.egit.ui.internal.decorators.GitLightweightDecorator: No
> change, the linked resources check was already there, but it only
> avoided decorating linked resources that pointed to a resource
> outside the project's repository. If a linked resource points to a
> resource within the same repository, it gets decorated. In fact only
> the > gets visible when a tracked resource has changed, the arrow
> indicating a linked resource seems to take precedence over other
> decorations. In the case of a linked folder pointing to a folder
> within the same repository, its children get decorated because only
> the linked folder gets an arrow indicating it's a linked folder.
> 
> Comments are welcomed, preferably via gerrit as this mail is just a
> copy of the last comment:

Too late :).

I added some further comments on the review.

> https://git.eclipse.org/r/#/c/3680/
> 
> _______________________________________________
> egit-dev mailing list
> egit-dev@xxxxxxxxxxx
> https://dev.eclipse.org/mailman/listinfo/egit-dev
>