From f46237a20462b5451035c2a83e6177e2e7682cfd Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Thu, 11 May 2017 22:17:54 +0200 Subject: [PATCH] Fix synchronize with deleted resources ResourceVariantTreeSubscriber.members() must return *all* resources known to the subscriber, whether they exist in the "workspace" or not. Otherwise resources that were deleted will be missed. Git is a bit special in that we don't want to report .gitignored resources, and untracked resources only if the user so wishes. Thus our logic is a bit different and governed basically by what our 3-way diff cache contains, but in any case we must include files that only exist in the remote (i.e., in those caches). Furthermore, isSupervised() must also work for IContainers, not just for files. That was limited in commit 3fa9329 [1] to files to prevent folders showing up with wrong markers. However, this kind of fix is just plain wrong -- the new test could never work with that. But if we re-consider folders as supervised, they'll show up as modified even when they have no changes below. Our cache should contain IN_SYNC entries for such folders. ThreeWayDifFEntry.scan() produces MODIFIED entries, though. Since I'm unsure about the interactions between scans with path filters to partially update the EGit synchronization caches, I have not touched that scan--it would be non-trivial to make it produce IN_SYNC entries anyway, and then the cache update logic would probably also have to be non-trivially adapted. Instead I've chosen to perform a top-down check in GitSyncInfo and return IN_SYNC for any folder that does not contain at least one file that is itself not IN_SYNC. [1] Ib25bf43721ef4cc87dba33aa01909e50a4cbef48; bug 324604 Bug: 516426 Change-Id: Ia12309f52e5140ff058b4fee17b9c7d48d435290 Signed-off-by: Thomas Wolf --- .../GitSubscriberResourceMappingContextTest.java | 100 +++++++++++++++++++++ .../GitResourceVariantTreeSubscriber.java | 76 +++++++++++----- .../eclipse/egit/core/synchronize/GitSyncInfo.java | 31 +++++++ 3 files changed, 183 insertions(+), 24 deletions(-) diff --git a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/synchronize/GitSubscriberResourceMappingContextTest.java b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/synchronize/GitSubscriberResourceMappingContextTest.java index c8a5107d9..6e090f2b0 100644 --- a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/synchronize/GitSubscriberResourceMappingContextTest.java +++ b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/synchronize/GitSubscriberResourceMappingContextTest.java @@ -7,6 +7,7 @@ *******************************************************************************/ package org.eclipse.egit.core.synchronize; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -14,6 +15,7 @@ import java.io.ByteArrayInputStream; import java.io.File; import java.io.UnsupportedEncodingException; +import org.eclipse.core.resources.IContainer; import org.eclipse.core.resources.IFile; import org.eclipse.core.resources.IFolder; import org.eclipse.core.resources.IProject; @@ -292,6 +294,104 @@ public class GitSubscriberResourceMappingContextTest extends GitTestCase { assertFalse(context.hasLocalChange(file, new NullProgressMonitor())); } + @Test + public void hasDeletion() throws Exception { + File file1 = testRepo.createFile(iProject, "file1.sample"); + testRepo.appendContentAndCommit(iProject, file1, "initial content", + "first commit in master"); + + IFile iFile1 = testRepo.getIFile(iProject, file1); + + testRepo.createAndCheckoutBranch(MASTER, BRANCH); + iFile1.delete(true, new NullProgressMonitor()); + try (Git git = new Git(testRepo.getRepository())) { + git.add() + .addFilepattern(iProject.getName() + '/' + iFile1.getName()) + .setUpdate(true) + .call(); + } + testRepo.commit("Deleted file1.sample"); + + RemoteResourceMappingContext context = prepareContext(BRANCH, MASTER); + boolean hasFile1 = false; + for (IResource member : context.fetchMembers(iProject, + new NullProgressMonitor())) { + if (iFile1.getName().equals(member.getName())) { + hasFile1 = true; + break; + } + } + assertTrue(hasFile1); + assertFalse(context.hasRemoteChange(iFile1, new NullProgressMonitor())); + assertTrue(context.hasLocalChange(iFile1, new NullProgressMonitor())); + } + + @Test + public void hasNestedDeletion() throws Exception { + File file1 = testRepo.createFile(iProject, + "sub/subfolder/file1.sample"); + testRepo.appendContentAndCommit(iProject, file1, "initial content", + "first commit in master"); + + IFile iFile1 = testRepo.getIFile(iProject, file1); + + assertTrue(iFile1.exists()); + + IContainer subfolder = iFile1.getParent(); + + assertTrue(subfolder instanceof IFolder); + assertEquals("subfolder", subfolder.getName()); + + IContainer sub = subfolder.getParent(); + + assertTrue(sub instanceof IFolder); + assertEquals("sub", sub.getName()); + + testRepo.createAndCheckoutBranch(MASTER, BRANCH); + iFile1.delete(true, new NullProgressMonitor()); + subfolder.delete(true, new NullProgressMonitor()); + sub.delete(true, new NullProgressMonitor()); + try (Git git = new Git(testRepo.getRepository())) { + git.add() + .addFilepattern( + iProject.getName() + "/sub/subfolder/file1.sample") + .setUpdate(true).call(); + } + testRepo.commit("Deleted sub/subfolder/file1.sample"); + + assertFalse(iFile1.exists()); + assertFalse(subfolder.exists()); + assertFalse(sub.exists()); + + RemoteResourceMappingContext context = prepareContext(BRANCH, MASTER); + boolean hasFile1 = false; + for (IResource member : context.fetchMembers(iProject, + new NullProgressMonitor())) { + if (sub.getName().equals(member.getName())) { + for (IResource child : context.fetchMembers(sub, + new NullProgressMonitor())) { + if (subfolder.getName().equals(child.getName())) { + for (IResource grandchild : context.fetchMembers( + subfolder, new NullProgressMonitor())) { + if (iFile1.getName().equals(grandchild.getName())) { + hasFile1 = true; + break; + } + } + break; + } + } + break; + } + } + assertTrue(hasFile1); + assertFalse(context.hasRemoteChange(iFile1, new NullProgressMonitor())); + assertTrue(context.hasLocalChange(iFile1, new NullProgressMonitor())); + assertTrue( + context.hasLocalChange(subfolder, new NullProgressMonitor())); + assertTrue(context.hasLocalChange(sub, new NullProgressMonitor())); + } + private RevCommit setContentsAndCommit(IFile targetFile, String newContents, String commitMessage) throws Exception { diff --git a/org.eclipse.egit.core/src/org/eclipse/egit/core/synchronize/GitResourceVariantTreeSubscriber.java b/org.eclipse.egit.core/src/org/eclipse/egit/core/synchronize/GitResourceVariantTreeSubscriber.java index da3b9346b..4b58b965e 100644 --- a/org.eclipse.egit.core/src/org/eclipse/egit/core/synchronize/GitResourceVariantTreeSubscriber.java +++ b/org.eclipse.egit.core/src/org/eclipse/egit/core/synchronize/GitResourceVariantTreeSubscriber.java @@ -18,16 +18,16 @@ import static org.eclipse.jgit.lib.Repository.stripWorkDir; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; -import java.util.Set; import org.eclipse.core.resources.IContainer; import org.eclipse.core.resources.IFile; import org.eclipse.core.resources.IProject; import org.eclipse.core.resources.IResource; import org.eclipse.core.runtime.CoreException; +import org.eclipse.core.runtime.IPath; import org.eclipse.core.runtime.IProgressMonitor; +import org.eclipse.core.runtime.Path; import org.eclipse.egit.core.Activator; import org.eclipse.egit.core.internal.CoreText; import org.eclipse.egit.core.internal.storage.WorkspaceFileRevision; @@ -107,51 +107,79 @@ public class GitResourceVariantTreeSubscriber extends @Override public boolean isSupervised(IResource res) throws TeamException { - return IResource.FILE == res.getType() - && gsds.contains(res.getProject()) - && gsds.shouldBeIncluded(res); + return gsds.contains(res.getProject()) && gsds.shouldBeIncluded(res); } /** - * Returns all members of git repository (including those that are not - * imported into workspace) + * Returns all members of the given resource as recorded by git. Resources + * ignored by git via .gitignore will not be returned, even if they exist in + * the workspace. * * @param res + * the resource to get the members of + * @return the resources, which may or may not exist in the workspace */ @Override public IResource[] members(IResource res) throws TeamException { - if (res.getType() == IResource.FILE || !gsds.shouldBeIncluded(res)) + if (res.getType() == IResource.FILE || !gsds.shouldBeIncluded(res)) { return new IResource[0]; - + } GitSynchronizeData gsd = gsds.getData(res.getProject()); Repository repo = gsd.getRepository(); GitSyncObjectCache repoCache = cache.get(repo); - Set gitMembers = new HashSet(); - Map allMembers = new HashMap(); + Collection allMembers = new ArrayList<>(); + Map existingMembers = new HashMap<>(); - Set gitCachedMembers = new HashSet(); String path = stripWorkDir(repo.getWorkTree(), res.getLocation().toFile()); GitSyncObjectCache cachedMembers = repoCache.get(path); - if (cachedMembers != null) { - Collection members = cachedMembers.members(); - if (members != null) - gitCachedMembers.addAll(members); - } + // A normal synchronizer would just return the union of existing + // resources and non-existing ones that exist only in git. For git, + // however, we want to ignore .gitignored resources completely, and + // include untracked files only if the preference to do so is set + // (in which case the cache will contain them already). So we add + // only the existing ones that are also recorded in the git 3-way + // cache, plus those recorded only in git, plus the git recorded + // one if it's a file vs.folder conflict. try { - for (IResource member : ((IContainer) res).members()) - allMembers.put(member.getName(), member); + IContainer container = (IContainer) res; + // Existing resources + if (container.exists()) { + for (IResource member : container.members()) { + existingMembers.put(member.getName(), member); + } + } - for (GitSyncObjectCache gitMember : gitCachedMembers) { - IResource member = allMembers.get(gitMember.getName()); - if (member != null) - gitMembers.add(member); + // Now add the ones from git + if (cachedMembers != null) { + Collection members = cachedMembers + .members(); + if (members != null) { + for (GitSyncObjectCache gitMember : members) { + String name = gitMember.getName(); + IResource existing = existingMembers.get(name); + if (existing != null) { + allMembers.add(existing); + } + if (existing == null || (existing + .getType() != IResource.FILE) != gitMember + .getDiffEntry().isTree()) { + // Non-existing, or file vs. folder + IPath localPath = new Path(name); + if (gitMember.getDiffEntry().isTree()) { + allMembers.add(container.getFolder(localPath)); + } else { + allMembers.add(container.getFile(localPath)); + } + } + } + } } } catch (CoreException e) { throw TeamException.asTeamException(e); } - return gitMembers.toArray(new IResource[gitMembers.size()]); + return allMembers.toArray(new IResource[allMembers.size()]); } @Override diff --git a/org.eclipse.egit.core/src/org/eclipse/egit/core/synchronize/GitSyncInfo.java b/org.eclipse.egit.core/src/org/eclipse/egit/core/synchronize/GitSyncInfo.java index bd6506201..4900301cf 100644 --- a/org.eclipse.egit.core/src/org/eclipse/egit/core/synchronize/GitSyncInfo.java +++ b/org.eclipse.egit.core/src/org/eclipse/egit/core/synchronize/GitSyncInfo.java @@ -10,6 +10,8 @@ package org.eclipse.egit.core.synchronize; import static org.eclipse.jgit.lib.Repository.stripWorkDir; +import java.util.Collection; + import org.eclipse.core.resources.IResource; import org.eclipse.egit.core.synchronize.ThreeWayDiffEntry.ChangeType; import org.eclipse.egit.core.synchronize.ThreeWayDiffEntry.Direction; @@ -51,6 +53,12 @@ class GitSyncInfo extends SyncInfo { if (obj == null) return IN_SYNC; + if (obj.getDiffEntry().isTree()) { + // Check that we do have at least one descendant that is a file + if (!hasNonSyncFile(obj)) { + return IN_SYNC; + } + } int direction; Direction gitDirection = obj.getDiffEntry().getDirection(); if (gitDirection == Direction.INCOMING) @@ -72,6 +80,29 @@ class GitSyncInfo extends SyncInfo { return IN_SYNC; } + private boolean hasNonSyncFile(GitSyncObjectCache obj) { + Collection children = obj.members(); + if (children == null) { + return false; + } + for (GitSyncObjectCache child : children) { + if (!child.getDiffEntry().isTree()) { + if (child.getDiffEntry() + .getChangeType() != ThreeWayDiffEntry.ChangeType.IN_SYNC) { + return true; + } + } + } + for (GitSyncObjectCache child : children) { + if (child.getDiffEntry().isTree()) { + if (hasNonSyncFile(child)) { + return true; + } + } + } + return false; + } + @Override public boolean equals(Object other) { return super.equals(other); -- 2.11.4.GIT