Bug 525558 - IdenticalResourceMinimizer only consider the name of the resource and not the full path
Summary: IdenticalResourceMinimizer only consider the name of the resource and not the...
Status: NEW
Alias: None
Product: EMFCompare
Classification: Modeling
Component: UI (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows 7
: P3 minor (vote)
Target Milestone: ---   Edit
Assignee: EMF Compare CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-04 07:42 EDT by Antonio Campesino CLA
Modified: 2017-10-09 08:41 EDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Campesino CLA 2017-10-04 07:42:33 EDT
The IdenticalResourceMinimizer when looking for candidates to binary compare in order to reduce the scope of the comparison and conflict detection only looks for files with the same name, leaving out the path (normally relative to the workspace / repository root). Probably it doesn't have much effect at all, but it may cause some issues in very concrete cases where files has the same names but differenciated by forlders.
Comment 1 Eclipse Genie CLA 2017-10-04 07:56:18 EDT
New Gerrit change created: https://git.eclipse.org/r/106202
Comment 2 Laurent Goubet CLA 2017-10-09 04:31:55 EDT
This was the behavior we intended: the URIs themselves will not match for local comparisons (as you've realized when you needed to "fix" a test case that explicitely tested that), and they often won't match either for remote comparisons since a local URI (file: or platform:/resource) will not match a remote URI (http, https, git).

If you have a specific use case for which you need the URIs to match instead of the file names, please add this to this bugzilla so that I can know what you're trying to achieve.
Comment 3 Philip Langer CLA 2017-10-09 05:48:33 EDT
Thanks for your feedback, Laurent.

Well, I agree with what you say in general, however, for normal git comparisons, the remote storage is a WorkspaceGitBlobStorage adapting to an IStoragePathProvider for which oeec.ide.utils.ResourceUtil.createURIFor(IStorage) will be able to create the corresponding platform resource URI. This change hence positively affects the normal Git compare scenarios.

For local comparisons, you are certainly right. It won't work as the compared files can't have the same URI. However, one could argue that the simple name check also doesn't necessarily work -- the compared files could have a different name as well.

Anyways, if we fail to match two resources correctly, the impact is rather minimal: the IdenticalResourceMinimizer "just" fails to binary-compare the correct files with each other and thus fails to remove them so that we might to some unnecessary comparison work in the worst case.

If we however match wrong files with each other based on their simple file names (e.g., those files just happen to have the same name but are in different folders), there is a slight risk that we do something that we'd like to avoid. If those equally-named files happen to be binary identical, which has a rather low probability I admit, we remove them from the storage even though we shouldn't.

In summary, I'd argue we should be more precise here as the cost of matches that fall through the cracks is lower (we just do unnecessary work) than the one for having false-positives (we may remove files that we shouldn't). Would you agree? I might misunderstand your argument, of course.
Comment 4 Laurent Goubet CLA 2017-10-09 08:41:58 EDT
No, you're right on that. The number of "local" comparisons as compared to remote ones should be pretty low anyway, so arguing to keep the behavior for that reason seems petty.

I cannot, however, take the time right now to properly test the suggested change for URI matching against git comparisons. The addition of the IStoragePathProvider we did back then may indeed have fixed the issues I had with the matching for binary identical resources. If you think that's enough, feel free to +2 and push, I had no problem with the propsed patch itself.