From b39c458cfbac78b331617c30a631ce450cf5a3b3 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Fri, 14 Nov 2025 02:28:45 +0100 Subject: [PATCH 1/4] Fix incorrect SubMonitor usage patterns This commit addresses several SubMonitor anti-patterns identified throughout the codebase, based on best practices from the Eclipse article on Progress Monitors. Changes made: 1. Remove unnecessary null checks before SubMonitor.convert() - SubMonitor.convert() can handle null monitors - Fixed in team Policy.java files (core and ui) 2. Remove unnecessary SubMonitor.done() calls - SubMonitor.done() is a no-op and should not be called - Removed 27 instances across resources and team bundles: * org.eclipse.core.filesystem: LocalFile.java (2 calls) * org.eclipse.core.resources: File.java (6), Folder.java (1), Project.java (3), SaveManager.java (4), Workspace.java (7) * org.eclipse.compare.core: RangeComparatorLCS.java (1) * org.eclipse.compare: AddFromHistoryAction.java (1) * org.eclipse.team.core: BundleImporterDelegate.java (1), Subscriber.java (1) 3. Remove redundant cancellation checks before split() - SubMonitor.split() already checks for cancellation - Fixed in AutoBuildJob.java Reference: https://www.eclipse.org/articles/Article-Progress-Monitors/article.html --- .../internal/filesystem/local/LocalFile.java | 88 ++++++------- .../core/internal/events/AutoBuildJob.java | 2 +- .../eclipse/core/internal/resources/File.java | 100 +++++++-------- .../core/internal/resources/Folder.java | 1 - .../core/internal/resources/Project.java | 60 ++++----- .../core/internal/resources/SaveManager.java | 118 ++++++++---------- .../core/internal/resources/Workspace.java | 60 ++++----- .../rangedifferencer/RangeComparatorLCS.java | 110 ++++++++-------- .../internal/AddFromHistoryAction.java | 6 +- .../provisional/BundleImporterDelegate.java | 1 - .../team/core/subscribers/Subscriber.java | 1 - .../eclipse/team/internal/core/Policy.java | 3 - .../org/eclipse/team/internal/ui/Policy.java | 3 - 13 files changed, 244 insertions(+), 309 deletions(-) diff --git a/resources/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/LocalFile.java b/resources/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/LocalFile.java index f2bda5e0584..83ce799d59a 100644 --- a/resources/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/LocalFile.java +++ b/resources/bundles/org.eclipse.core.filesystem/src/org/eclipse/core/internal/filesystem/local/LocalFile.java @@ -179,8 +179,6 @@ protected void copyFile(IFileInfo sourceInfo, IFileStore destination, int option Policy.error(EFS.ERROR_EXISTS, NLS.bind(Messages.fileExists, target.filePath), e); } catch (IOException e) { Policy.error(EFS.ERROR_WRITE, NLS.bind(Messages.failedCopy, this.filePath, target.filePath), e); - } finally { - subMonitor.done(); } } else { super.copyFile(sourceInfo, destination, options, monitor); @@ -457,55 +455,51 @@ public void move(IFileStore destFile, int options, IProgressMonitor monitor) thr File destination = destinationFile.file; boolean overwrite = (options & EFS.OVERWRITE) != 0; SubMonitor subMonitor = SubMonitor.convert(monitor, NLS.bind(Messages.moving, source.getAbsolutePath()), 1); + //this flag captures case renaming on a case-insensitive OS, or moving + //two equivalent files in an environment that supports symbolic links. + //in these cases we NEVER want to delete anything + boolean sourceEqualsDest = false; try { - //this flag captures case renaming on a case-insensitive OS, or moving - //two equivalent files in an environment that supports symbolic links. - //in these cases we NEVER want to delete anything - boolean sourceEqualsDest = false; - try { - sourceEqualsDest = isSameFile(source, destination); - } catch (IOException e) { - String message = NLS.bind(Messages.couldNotMove, source.getAbsolutePath()); - Policy.error(EFS.ERROR_WRITE, message, e); - } - if (!sourceEqualsDest && !overwrite && destination.exists()) { - String message = NLS.bind(Messages.fileExists, destination.getAbsolutePath()); - Policy.error(EFS.ERROR_EXISTS, message); - } - if (source.renameTo(destination)) { - // double-check to ensure we really did move - // since java.io.File#renameTo sometimes lies - if (!sourceEqualsDest && source.exists()) { - // XXX: document when this occurs - if (destination.exists()) { - // couldn't delete the source so remove the destination and throw an error - // XXX: if we fail deleting the destination, the destination (root) may still exist - new LocalFile(destination).delete(EFS.NONE, null); - String message = NLS.bind(Messages.couldnotDelete, source.getAbsolutePath()); - Policy.error(EFS.ERROR_DELETE, message); - } - // source exists but destination doesn't so try to copy below - } else { - // destination.exists() returns false for broken links, this has to be handled explicitly - if (!destination.exists() && !destFile.fetchInfo().getAttribute(EFS.ATTRIBUTE_SYMLINK)) { - // neither the source nor the destination exist. this is REALLY bad - String message = NLS.bind(Messages.failedMove, source.getAbsolutePath(), destination.getAbsolutePath()); - Policy.error(EFS.ERROR_WRITE, message); - } - // the move was successful - return; + sourceEqualsDest = isSameFile(source, destination); + } catch (IOException e) { + String message = NLS.bind(Messages.couldNotMove, source.getAbsolutePath()); + Policy.error(EFS.ERROR_WRITE, message, e); + } + if (!sourceEqualsDest && !overwrite && destination.exists()) { + String message = NLS.bind(Messages.fileExists, destination.getAbsolutePath()); + Policy.error(EFS.ERROR_EXISTS, message); + } + if (source.renameTo(destination)) { + // double-check to ensure we really did move + // since java.io.File#renameTo sometimes lies + if (!sourceEqualsDest && source.exists()) { + // XXX: document when this occurs + if (destination.exists()) { + // couldn't delete the source so remove the destination and throw an error + // XXX: if we fail deleting the destination, the destination (root) may still exist + new LocalFile(destination).delete(EFS.NONE, null); + String message = NLS.bind(Messages.couldnotDelete, source.getAbsolutePath()); + Policy.error(EFS.ERROR_DELETE, message); } + // source exists but destination doesn't so try to copy below + } else { + // destination.exists() returns false for broken links, this has to be handled explicitly + if (!destination.exists() && !destFile.fetchInfo().getAttribute(EFS.ATTRIBUTE_SYMLINK)) { + // neither the source nor the destination exist. this is REALLY bad + String message = NLS.bind(Messages.failedMove, source.getAbsolutePath(), destination.getAbsolutePath()); + Policy.error(EFS.ERROR_WRITE, message); + } + // the move was successful + return; } - // for some reason renameTo didn't work - if (sourceEqualsDest) { - String message = NLS.bind(Messages.couldNotMove, source.getAbsolutePath()); - Policy.error(EFS.ERROR_WRITE, message, null); - } - // fall back to default implementation - super.move(destFile, options, subMonitor.newChild(1)); - } finally { - subMonitor.done(); } + // for some reason renameTo didn't work + if (sourceEqualsDest) { + String message = NLS.bind(Messages.couldNotMove, source.getAbsolutePath()); + Policy.error(EFS.ERROR_WRITE, message, null); + } + // fall back to default implementation + super.move(destFile, options, subMonitor.newChild(1)); } private boolean isSameFile(File source, File destination) throws IOException { diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/AutoBuildJob.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/AutoBuildJob.java index 1978fa637bb..c27de8935f0 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/AutoBuildJob.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/AutoBuildJob.java @@ -297,7 +297,7 @@ public void preferenceChange(PreferenceChangeEvent event) { public IStatus run(IProgressMonitor monitor) { SubMonitor subMonitor = SubMonitor.convert(monitor, 1); synchronized (this) { - if (subMonitor.isCanceled() || isInterrupted()) { + if (isInterrupted()) { return canceled(); } } diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/File.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/File.java index 54a72e30207..34b81a3f1bb 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/File.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/File.java @@ -91,8 +91,6 @@ public void appendContents(InputStream content, int updateFlags, IProgressMonito } } catch (IOException streamCloseIgnored) { // ignore; - } finally { - subMonitor.done(); } } @@ -163,8 +161,6 @@ public void create(InputStream content, int updateFlags, IProgressMonitor monito } } catch (IOException streamCloseIgnored) { // ignore; - } finally { - subMonitor.done(); } } @@ -177,38 +173,34 @@ public void create(InputStream content, boolean force, IProgressMonitor monitor) @Override public void create(byte[] content, int updateFlags, IProgressMonitor monitor) throws CoreException { SubMonitor subMonitor = SubMonitor.convert(monitor, NLS.bind(Messages.resources_creating, getFullPath()), 100); + checkValidPath(path, FILE, true); + final ISchedulingRule rule = workspace.getRuleFactory().createRule(this); try { - checkValidPath(path, FILE, true); - final ISchedulingRule rule = workspace.getRuleFactory().createRule(this); - try { - workspace.prepareOperation(rule, subMonitor.newChild(1)); - checkCreatable(); - workspace.beginOperation(true); - IFileStore store = getStore(); - IFileInfo localInfo = create(updateFlags, subMonitor.newChild(40), store); - boolean local = content != null; - if (local) { - try { - internalSetContents(content, localInfo, updateFlags, false, subMonitor.newChild(59)); - } catch (CoreException | OperationCanceledException e) { - // CoreException when a problem happened creating the file on disk - // OperationCanceledException when the operation of setting contents has been - // canceled - // In either case delete from the workspace and disk - workspace.deleteResource(this); - store.delete(EFS.NONE, null); - throw e; - } + workspace.prepareOperation(rule, subMonitor.newChild(1)); + checkCreatable(); + workspace.beginOperation(true); + IFileStore store = getStore(); + IFileInfo localInfo = create(updateFlags, subMonitor.newChild(40), store); + boolean local = content != null; + if (local) { + try { + internalSetContents(content, localInfo, updateFlags, false, subMonitor.newChild(59)); + } catch (CoreException | OperationCanceledException e) { + // CoreException when a problem happened creating the file on disk + // OperationCanceledException when the operation of setting contents has been + // canceled + // In either case delete from the workspace and disk + workspace.deleteResource(this); + store.delete(EFS.NONE, null); + throw e; } - setLocal(local); - } catch (OperationCanceledException e) { - workspace.getWorkManager().operationCanceled(); - throw e; - } finally { - workspace.endOperation(rule, true); } + setLocal(local); + } catch (OperationCanceledException e) { + workspace.getWorkManager().operationCanceled(); + throw e; } finally { - subMonitor.done(); + workspace.endOperation(rule, true); } } @@ -244,7 +236,6 @@ IFileInfo create(int updateFlags, IProgressMonitor subMonitor, IFileStore store) throw new ResourceException(IResourceStatus.FAILED_WRITE_LOCAL, getFullPath(), message, null); } } - subMonitor.done(); workspace.createResource(this, updateFlags); return localInfo; @@ -510,39 +501,33 @@ public void setContents(InputStream content, int updateFlags, IProgressMonitor m } } catch (IOException streamCloseIgnored) { // ignore; - } finally { - subMonitor.done(); } } @Override public void setContents(byte[] content, int updateFlags, IProgressMonitor monitor) throws CoreException { String message = NLS.bind(Messages.resources_settingContents, getFullPath()); SubMonitor subMonitor = SubMonitor.convert(monitor, message, 100); + if (workspace.shouldValidate) { + workspace.validateSave(this); + } + final ISchedulingRule rule = workspace.getRuleFactory().modifyRule(this); + SubMonitor newChild = subMonitor.newChild(1); try { - if (workspace.shouldValidate) { - workspace.validateSave(this); - } - final ISchedulingRule rule = workspace.getRuleFactory().modifyRule(this); - SubMonitor newChild = subMonitor.newChild(1); - try { - workspace.prepareOperation(rule, newChild); - ResourceInfo info = getResourceInfo(false, false); - checkAccessible(getFlags(info)); - workspace.beginOperation(true); - IFileInfo fileInfo = getStore().fetchInfo(); - if (BitMask.isSet(updateFlags, IResource.DERIVED)) { - // update of derived flag during IFile.write: - info.set(ICoreConstants.M_DERIVED); - } - internalSetContents(content, fileInfo, updateFlags, false, subMonitor.newChild(99)); - } catch (OperationCanceledException e) { - workspace.getWorkManager().operationCanceled(); - throw e; - } finally { - workspace.endOperation(rule, true); + workspace.prepareOperation(rule, newChild); + ResourceInfo info = getResourceInfo(false, false); + checkAccessible(getFlags(info)); + workspace.beginOperation(true); + IFileInfo fileInfo = getStore().fetchInfo(); + if (BitMask.isSet(updateFlags, IResource.DERIVED)) { + // update of derived flag during IFile.write: + info.set(ICoreConstants.M_DERIVED); } + internalSetContents(content, fileInfo, updateFlags, false, subMonitor.newChild(99)); + } catch (OperationCanceledException e) { + workspace.getWorkManager().operationCanceled(); + throw e; } finally { - subMonitor.done(); + workspace.endOperation(rule, true); } } @@ -616,7 +601,6 @@ public void setCharset(String newCharset, IProgressMonitor monitor) throws CoreE workspace.getWorkManager().operationCanceled(); throw e; } finally { - subMonitor.done(); workspace.endOperation(rule, true); } } diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Folder.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Folder.java index d6fb7271f75..ffb4052e975 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Folder.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Folder.java @@ -118,7 +118,6 @@ public void create(int updateFlags, boolean local, IProgressMonitor monitor) thr workspace.getWorkManager().operationCanceled(); throw e; } finally { - subMonitor.done(); workspace.endOperation(rule, true); } } diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Project.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Project.java index ea8835f2e9f..eeadafad2b2 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Project.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Project.java @@ -252,7 +252,6 @@ public void close(IProgressMonitor monitor) throws CoreException { workspace.getWorkManager().operationCanceled(); throw e; } finally { - subMonitor.done(); workspace.endOperation(rule, true); } } @@ -360,7 +359,6 @@ public void create(IProjectDescription description, int updateFlags, IProgressMo workspace.getWorkManager().operationCanceled(); throw e; } finally { - subMonitor.done(); workspace.endOperation(rule, true); } } @@ -612,43 +610,39 @@ public void run(IProgressMonitor innerMonitor) throws CoreException { final ISchedulingRule notificationsRule = relaxed ? null : workspace.getRoot(); SubMonitor subMonitor = SubMonitor.convert(innerMonitor, 100); try { - try { - workspace.prepareOperation(notificationsRule, innerMonitor); - if (!shouldBuild()) { - return; - } - workspace.beginOperation(true); - workspace.aboutToBuild(Project.this, trigger); - } finally { - workspace.endOperation(notificationsRule, false); + workspace.prepareOperation(notificationsRule, innerMonitor); + if (!shouldBuild()) { + return; } + workspace.beginOperation(true); + workspace.aboutToBuild(Project.this, trigger); + } finally { + workspace.endOperation(notificationsRule, false); + } + try { + IStatus result; + workspace.prepareOperation(projectBuildRule, innerMonitor); + // don't open the tree eagerly because it will be wasted if no build occurs + workspace.beginOperation(false); + result = workspace.getBuildManager().build(config, trigger, builderName, args, + subMonitor.split(100)); + if (!result.isOK()) { + throw new ResourceException(result); + } + } finally { + workspace.endOperation(projectBuildRule, false); try { - IStatus result; - workspace.prepareOperation(projectBuildRule, innerMonitor); - // don't open the tree eagerly because it will be wasted if no build occurs + workspace.prepareOperation(notificationsRule, innerMonitor); + // don't open the tree eagerly because it will be wasted if no change occurs workspace.beginOperation(false); - result = workspace.getBuildManager().build(config, trigger, builderName, args, - subMonitor.split(100)); - if (!result.isOK()) { - throw new ResourceException(result); + workspace.broadcastBuildEvent(Project.this, IResourceChangeEvent.POST_BUILD, trigger); + // building may close the tree, so open it + if (workspace.getElementTree().isImmutable()) { + workspace.newWorkingTree(); } } finally { - workspace.endOperation(projectBuildRule, false); - try { - workspace.prepareOperation(notificationsRule, innerMonitor); - // don't open the tree eagerly because it will be wasted if no change occurs - workspace.beginOperation(false); - workspace.broadcastBuildEvent(Project.this, IResourceChangeEvent.POST_BUILD, trigger); - // building may close the tree, so open it - if (workspace.getElementTree().isImmutable()) { - workspace.newWorkingTree(); - } - } finally { - workspace.endOperation(notificationsRule, false); - } + workspace.endOperation(notificationsRule, false); } - } finally { - subMonitor.done(); } } diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/SaveManager.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/SaveManager.java index b666609579b..a7f8c251985 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/SaveManager.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/SaveManager.java @@ -241,41 +241,37 @@ public ISavedState addParticipant(String pluginId, ISaveParticipant participant) protected void broadcastLifecycle(final int lifecycle, Map contexts, final MultiStatus warnings, IProgressMonitor monitor) { SubMonitor subMonitor = SubMonitor.convert(monitor, contexts.size()); - try { - for (final Iterator> it = contexts.entrySet().iterator(); it.hasNext();) { - Map.Entry entry = it.next(); - String pluginId = entry.getKey(); - final ISaveParticipant participant = saveParticipants.get(pluginId); - // save participants can be removed concurrently - if (participant == null) { - subMonitor.worked(1); - continue; - } - final SaveContext context = entry.getValue(); - /* Be extra careful when calling lifecycle method on arbitrary plugin */ - ISafeRunnable code = new ISafeRunnable() { - - @Override - public void handleException(Throwable e) { - String message = Messages.resources_saveProblem; - IStatus status = new Status(IStatus.WARNING, ResourcesPlugin.PI_RESOURCES, - IResourceStatus.INTERNAL_ERROR, message, e); - warnings.add(status); - - /* Remove entry for defective plug-in from this save operation */ - it.remove(); - } - - @Override - public void run() throws Exception { - executeLifecycle(lifecycle, participant, context); - } - }; - SafeRunner.run(code); + for (final Iterator> it = contexts.entrySet().iterator(); it.hasNext();) { + Map.Entry entry = it.next(); + String pluginId = entry.getKey(); + final ISaveParticipant participant = saveParticipants.get(pluginId); + // save participants can be removed concurrently + if (participant == null) { subMonitor.worked(1); + continue; } - } finally { - subMonitor.done(); + final SaveContext context = entry.getValue(); + /* Be extra careful when calling lifecycle method on arbitrary plugin */ + ISafeRunnable code = new ISafeRunnable() { + + @Override + public void handleException(Throwable e) { + String message = Messages.resources_saveProblem; + IStatus status = new Status(IStatus.WARNING, ResourcesPlugin.PI_RESOURCES, + IResourceStatus.INTERNAL_ERROR, message, e); + warnings.add(status); + + /* Remove entry for defective plug-in from this save operation */ + it.remove(); + } + + @Override + public void run() throws Exception { + executeLifecycle(lifecycle, participant, context); + } + }; + SafeRunner.run(code); + subMonitor.worked(1); } } @@ -1574,37 +1570,33 @@ protected void snapTree(ElementTree tree, IProgressMonitor monitor) throws CoreE long start = System.currentTimeMillis(); String message; SubMonitor subMonitor = SubMonitor.convert(monitor, Policy.totalWork); + // the tree must be immutable + tree.immutable(); + // don't need to snapshot if there are no changes + if (tree == lastSnap) { + return; + } + operationCount = 0; + IPath snapPath = workspace.getMetaArea().getSnapshotLocationFor(workspace.getRoot()); + ElementTreeWriter writer = new ElementTreeWriter(this); + java.io.File localFile = snapPath.toFile(); try { - // the tree must be immutable - tree.immutable(); - // don't need to snapshot if there are no changes - if (tree == lastSnap) { - return; + SafeChunkyOutputStream safeStream = new SafeChunkyOutputStream(localFile); + try (DataOutputStream out = new DataOutputStream(safeStream);) { + out.writeInt(ICoreConstants.WORKSPACE_TREE_VERSION_2); + writeWorkspaceFields(out, subMonitor); + writer.writeDelta(tree, lastSnap, IPath.ROOT, ElementTreeWriter.D_INFINITE, out, + ResourceComparator.getSaveComparator()); + safeStream.succeed(); + out.close(); } - operationCount = 0; - IPath snapPath = workspace.getMetaArea().getSnapshotLocationFor(workspace.getRoot()); - ElementTreeWriter writer = new ElementTreeWriter(this); - java.io.File localFile = snapPath.toFile(); - try { - SafeChunkyOutputStream safeStream = new SafeChunkyOutputStream(localFile); - try (DataOutputStream out = new DataOutputStream(safeStream);) { - out.writeInt(ICoreConstants.WORKSPACE_TREE_VERSION_2); - writeWorkspaceFields(out, subMonitor); - writer.writeDelta(tree, lastSnap, IPath.ROOT, ElementTreeWriter.D_INFINITE, out, - ResourceComparator.getSaveComparator()); - safeStream.succeed(); - out.close(); - } - } catch (IOException e) { - message = NLS.bind(Messages.resources_writeWorkspaceMeta, localFile.getAbsolutePath()); - throw new ResourceException(IResourceStatus.FAILED_WRITE_METADATA, IPath.ROOT, message, e); - } - lastSnap = tree; - if (Policy.DEBUG_SAVE_TREE) { - Policy.debug("Snapshot Workspace Tree: " + (System.currentTimeMillis() - start) + "ms"); //$NON-NLS-1$ //$NON-NLS-2$ - } - } finally { - subMonitor.done(); + } catch (IOException e) { + message = NLS.bind(Messages.resources_writeWorkspaceMeta, localFile.getAbsolutePath()); + throw new ResourceException(IResourceStatus.FAILED_WRITE_METADATA, IPath.ROOT, message, e); + } + lastSnap = tree; + if (Policy.DEBUG_SAVE_TREE) { + Policy.debug("Snapshot Workspace Tree: " + (System.currentTimeMillis() - start) + "ms"); //$NON-NLS-1$ //$NON-NLS-2$ } } @@ -2181,7 +2173,6 @@ protected void writeTree(Map statesToSave, DataOutputStream output.writeUTF(string); } } finally { - subMonitor.done(); if (!wasImmutable) { workspace.newWorkingTree(); } @@ -2257,7 +2248,6 @@ protected void writeTree(Project project, DataOutputStream output, IProgressMoni output.writeUTF(string); } } finally { - subMonitor.done(); if (!wasImmutable) { workspace.newWorkingTree(); } diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java index 690cb8fe72e..23f957470cf 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java @@ -632,7 +632,6 @@ private void buildInternal(IBuildConfiguration[] requestedConfigs, int trigger, throw new ResourceException(result); } } finally { - subMonitor.done(); // building may close the tree, but we are still inside an operation so open it if (tree.isImmutable()) { newWorkingTree(); @@ -1135,7 +1134,6 @@ public IStatus copy(IResource[] resources, IPath destination, int updateFlags, I getWorkManager().operationCanceled(); throw e; } finally { - subMonitor.done(); endOperation(getRoot(), true); } if (status.matches(IStatus.ERROR)) { @@ -1529,7 +1527,6 @@ public IStatus delete(IResource[] resources, int updateFlags, IProgressMonitor m getWorkManager().operationCanceled(); throw e; } finally { - subMonitor.done(); endOperation(getRoot(), true); } } @@ -2206,7 +2203,6 @@ public IStatus move(IResource[] resources, IPath destination, int updateFlags, I getWorkManager().operationCanceled(); throw e; } finally { - subMonitor.done(); endOperation(getRoot(), true); } if (status.matches(IStatus.ERROR)) { @@ -2512,7 +2508,6 @@ public void run(ICoreRunnable action, ISchedulingRule rule, int options, IProgre } throw e; } finally { - subMonitor.done(); if (avoidNotification) { notificationManager.endAvoidNotify(); } @@ -2641,7 +2636,6 @@ protected void shutdown(IProgressMonitor monitor) throws CoreException { throw new CoreException(status); } } finally { - subMonitor.done(); } } @@ -2906,43 +2900,39 @@ private void createMultiple(ConcurrentMap filesToCreate, int updat IPath name = files.iterator().next().getFullPath(); // XXX any name SubMonitor subMonitor = SubMonitor.convert(monitor, NLS.bind(Messages.resources_creating, name), 1); + ISchedulingRule rule = MultiRule + .combine(files.stream().map(getRuleFactory()::createRule).toArray(ISchedulingRule[]::new)); + NullProgressMonitor npm = new NullProgressMonitor(); try { - ISchedulingRule rule = MultiRule - .combine(files.stream().map(getRuleFactory()::createRule).toArray(ISchedulingRule[]::new)); - NullProgressMonitor npm = new NullProgressMonitor(); + prepareOperation(rule, npm); + for (File file : files) { + file.checkCreatable(); + } + beginOperation(true); try { - prepareOperation(rule, npm); + File.internalSetMultipleContents(filesToCreate, updateFlags, false, subMonitor.newChild(1), + executorService); + } catch (CoreException | OperationCanceledException e) { + // CoreException when a problem happened creating a file on disk + // OperationCanceledException when the operation of setting contents has been + // canceled + // In either case delete from the workspace and disk for (File file : files) { - file.checkCreatable(); - } - beginOperation(true); - try { - File.internalSetMultipleContents(filesToCreate, updateFlags, false, subMonitor.newChild(1), - executorService); - } catch (CoreException | OperationCanceledException e) { - // CoreException when a problem happened creating a file on disk - // OperationCanceledException when the operation of setting contents has been - // canceled - // In either case delete from the workspace and disk - for (File file : files) { - try { - deleteResource(file); - IFileStore store = file.getStore(); - store.delete(EFS.NONE, null); - } catch (Exception e2) { - e.addSuppressed(e); - } + try { + deleteResource(file); + IFileStore store = file.getStore(); + store.delete(EFS.NONE, null); + } catch (Exception e2) { + e.addSuppressed(e); } - throw e; } - } catch (OperationCanceledException e) { - getWorkManager().operationCanceled(); throw e; - } finally { - endOperation(rule, true); } + } catch (OperationCanceledException e) { + getWorkManager().operationCanceled(); + throw e; } finally { - subMonitor.done(); + endOperation(rule, true); } } } diff --git a/team/bundles/org.eclipse.compare.core/src/org/eclipse/compare/rangedifferencer/RangeComparatorLCS.java b/team/bundles/org.eclipse.compare.core/src/org/eclipse/compare/rangedifferencer/RangeComparatorLCS.java index 4cdbeaf790d..c6f083b430f 100644 --- a/team/bundles/org.eclipse.compare.core/src/org/eclipse/compare/rangedifferencer/RangeComparatorLCS.java +++ b/team/bundles/org.eclipse.compare.core/src/org/eclipse/compare/rangedifferencer/RangeComparatorLCS.java @@ -71,73 +71,69 @@ protected void setLcs(int sl1, int sl2) { } public RangeDifference[] getDifferences(SubMonitor subMonitor, AbstractRangeDifferenceFactory factory) { - try { - List differences = new ArrayList<>(); - int length = getLength(); - if (length == 0) { - differences.add(factory.createRangeDifference(RangeDifference.CHANGE, 0, this.comparator2.getRangeCount(), 0, this.comparator1.getRangeCount())); - } else { - subMonitor.beginTask(null, length); - int index1, index2; - index1 = index2 = 0; - int l1, l2; - int s1 = -1; - int s2 = -1; - while(index1 < this.lcs[0].length && index2 < this.lcs[1].length) { - // Move both LCS lists to the next occupied slot - while ((l1= this.lcs[0][index1]) == 0) { - index1++; - if (index1 >= this.lcs[0].length) { - break; - } - } + List differences = new ArrayList<>(); + int length = getLength(); + if (length == 0) { + differences.add(factory.createRangeDifference(RangeDifference.CHANGE, 0, this.comparator2.getRangeCount(), 0, this.comparator1.getRangeCount())); + } else { + subMonitor.beginTask(null, length); + int index1, index2; + index1 = index2 = 0; + int l1, l2; + int s1 = -1; + int s2 = -1; + while(index1 < this.lcs[0].length && index2 < this.lcs[1].length) { + // Move both LCS lists to the next occupied slot + while ((l1= this.lcs[0][index1]) == 0) { + index1++; if (index1 >= this.lcs[0].length) { break; } - while ((l2= this.lcs[1][index2]) == 0) { - index2++; - if (index2 >= this.lcs[1].length) { - break; - } - } + } + if (index1 >= this.lcs[0].length) { + break; + } + while ((l2= this.lcs[1][index2]) == 0) { + index2++; if (index2 >= this.lcs[1].length) { break; } - // Convert the entry to an array index (see setLcs(int, int)) - int end1 = l1 - 1; - int end2 = l2 - 1; - if (s1 == -1 && (end1 != 0 || end2 != 0)) { - // There is a diff at the beginning - // TODO: We need to conform that this is the proper order - differences.add(factory.createRangeDifference(RangeDifference.CHANGE, 0, end2, 0, end1)); - } else if (end1 != s1 + 1 || end2 != s2 + 1) { - // A diff was found on one of the sides - int leftStart = s1 + 1; - int leftLength = end1 - leftStart; - int rightStart = s2 + 1; - int rightLength = end2 - rightStart; - // TODO: We need to conform that this is the proper order - differences.add(factory.createRangeDifference(RangeDifference.CHANGE, rightStart, rightLength, leftStart, leftLength)); - } - s1 = end1; - s2 = end2; - index1++; - index2++; - worked(subMonitor, 1); } - if (s1 != -1 && (s1 + 1 < this.comparator1.getRangeCount() || s2 + 1 < this.comparator2.getRangeCount())) { - // TODO: we need to find the proper way of representing an append - int leftStart = s1 < this.comparator1.getRangeCount() ? s1 + 1 : s1; - int rightStart = s2 < this.comparator2.getRangeCount() ? s2 + 1 : s2; - // TODO: We need to confirm that this is the proper order - differences.add(factory.createRangeDifference(RangeDifference.CHANGE, rightStart, this.comparator2.getRangeCount() - (s2 + 1), leftStart, this.comparator1.getRangeCount() - (s1 + 1))); + if (index2 >= this.lcs[1].length) { + break; } - + // Convert the entry to an array index (see setLcs(int, int)) + int end1 = l1 - 1; + int end2 = l2 - 1; + if (s1 == -1 && (end1 != 0 || end2 != 0)) { + // There is a diff at the beginning + // TODO: We need to conform that this is the proper order + differences.add(factory.createRangeDifference(RangeDifference.CHANGE, 0, end2, 0, end1)); + } else if (end1 != s1 + 1 || end2 != s2 + 1) { + // A diff was found on one of the sides + int leftStart = s1 + 1; + int leftLength = end1 - leftStart; + int rightStart = s2 + 1; + int rightLength = end2 - rightStart; + // TODO: We need to conform that this is the proper order + differences.add(factory.createRangeDifference(RangeDifference.CHANGE, rightStart, rightLength, leftStart, leftLength)); + } + s1 = end1; + s2 = end2; + index1++; + index2++; + worked(subMonitor, 1); } - return differences.toArray(new RangeDifference[differences.size()]); - } finally { - subMonitor.done(); + if (s1 != -1 && (s1 + 1 < this.comparator1.getRangeCount() || s2 + 1 < this.comparator2.getRangeCount())) { + // TODO: we need to find the proper way of representing an append + int leftStart = s1 < this.comparator1.getRangeCount() ? s1 + 1 : s1; + int rightStart = s2 < this.comparator2.getRangeCount() ? s2 + 1 : s2; + // TODO: We need to confirm that this is the proper order + differences.add(factory.createRangeDifference(RangeDifference.CHANGE, rightStart, this.comparator2.getRangeCount() - (s2 + 1), leftStart, this.comparator1.getRangeCount() - (s1 + 1))); + } + } + return differences.toArray(new RangeDifference[differences.size()]); } private void worked(SubMonitor subMonitor, int work) { diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/AddFromHistoryAction.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/AddFromHistoryAction.java index d37e2847594..dd4817ce25c 100644 --- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/AddFromHistoryAction.java +++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/AddFromHistoryAction.java @@ -130,11 +130,7 @@ public void execute(IProgressMonitor pm) throws InvocationTargetException { IFileState fileState = s.fFileState; createContainers(file); SubMonitor subMonitor= SubMonitor.convert(pm, 1); - try { - file.create(fileState.getContents(), false, subMonitor); - } finally { - subMonitor.done(); - } + file.create(fileState.getContents(), false, subMonitor); } } catch (CoreException e) { throw new InvocationTargetException(e); diff --git a/team/bundles/org.eclipse.team.core/src/org/eclipse/team/core/importing/provisional/BundleImporterDelegate.java b/team/bundles/org.eclipse.team.core/src/org/eclipse/team/core/importing/provisional/BundleImporterDelegate.java index ebc8a92e8af..3d4c1ab928f 100644 --- a/team/bundles/org.eclipse.team.core/src/org/eclipse/team/core/importing/provisional/BundleImporterDelegate.java +++ b/team/bundles/org.eclipse.team.core/src/org/eclipse/team/core/importing/provisional/BundleImporterDelegate.java @@ -103,7 +103,6 @@ public IProject[] performImport(ScmUrlImportDescription[] descriptions, IProgres if (!references.isEmpty()) { SubMonitor subMonitor = SubMonitor.convert(monitor, references.size()); result = psfCapability.addToWorkspace(references.toArray(new String[references.size()]), new ProjectSetSerializationContext(), subMonitor); - subMonitor.done(); } } return result; diff --git a/team/bundles/org.eclipse.team.core/src/org/eclipse/team/core/subscribers/Subscriber.java b/team/bundles/org.eclipse.team.core/src/org/eclipse/team/core/subscribers/Subscriber.java index b4b5326d1a0..c6aec1511cf 100644 --- a/team/bundles/org.eclipse.team.core/src/org/eclipse/team/core/subscribers/Subscriber.java +++ b/team/bundles/org.eclipse.team.core/src/org/eclipse/team/core/subscribers/Subscriber.java @@ -269,7 +269,6 @@ public void collectOutOfSync(IResource[] resources, int depth, SyncInfoSet set, IProgressMonitor subMonitor = Policy.subMonitorFor(monitor, 100); subMonitor.beginTask(null, IProgressMonitor.UNKNOWN); collect(resource, depth, set, subMonitor); - subMonitor.done(); } } finally { monitor.done(); diff --git a/team/bundles/org.eclipse.team.core/src/org/eclipse/team/internal/core/Policy.java b/team/bundles/org.eclipse.team.core/src/org/eclipse/team/internal/core/Policy.java index 02f924fb1ba..36ab4860797 100644 --- a/team/bundles/org.eclipse.team.core/src/org/eclipse/team/internal/core/Policy.java +++ b/team/bundles/org.eclipse.team.core/src/org/eclipse/team/internal/core/Policy.java @@ -54,9 +54,6 @@ public static IProgressMonitor monitorFor(IProgressMonitor monitor) { } public static IProgressMonitor subMonitorFor(IProgressMonitor monitor, int ticks) { - if (monitor == null) { - return new NullProgressMonitor(); - } if (monitor instanceof NullProgressMonitor) { return monitor; } diff --git a/team/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/Policy.java b/team/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/Policy.java index 934cfae4f1b..73342182be5 100644 --- a/team/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/Policy.java +++ b/team/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/Policy.java @@ -69,9 +69,6 @@ public static void checkCanceled(IProgressMonitor monitor) { } public static IProgressMonitor subMonitorFor(IProgressMonitor monitor, int ticks) { - if (monitor == null) { - return new NullProgressMonitor(); - } if (monitor instanceof NullProgressMonitor) { return monitor; } From 403841b942f1de7d875ba3a2534a92b0f48f6baa Mon Sep 17 00:00:00 2001 From: Eclipse Platform Bot Date: Fri, 14 Nov 2025 08:04:31 +0000 Subject: [PATCH 2/4] Version bump(s) for 4.38 stream --- team/bundles/org.eclipse.compare.core/META-INF/MANIFEST.MF | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/team/bundles/org.eclipse.compare.core/META-INF/MANIFEST.MF b/team/bundles/org.eclipse.compare.core/META-INF/MANIFEST.MF index b887b385803..6405505e8e1 100644 --- a/team/bundles/org.eclipse.compare.core/META-INF/MANIFEST.MF +++ b/team/bundles/org.eclipse.compare.core/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: %pluginName Bundle-SymbolicName: org.eclipse.compare.core -Bundle-Version: 3.8.800.qualifier +Bundle-Version: 3.8.900.qualifier Bundle-Vendor: %providerName Require-Bundle: org.eclipse.core.runtime;bundle-version="[3.29.0,4.0.0)" Bundle-RequiredExecutionEnvironment: JavaSE-17 From 917c3109a0b9b6f795065e3544ccddc96f102b74 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Fri, 14 Nov 2025 09:05:01 +0100 Subject: [PATCH 3/4] Fix SubMonitor.convert() usage in Policy.subMonitorFor() methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous implementation used SubMonitor.convert(monitor, ticks) which internally calls beginTask(), causing "beginTask may only be called once" errors when the returned monitor is passed to code that also calls beginTask(). The correct pattern is SubMonitor.convert(monitor).split(ticks), which creates a child monitor without calling beginTask on it, allowing the caller to call beginTask as needed. This fixes test failures where FussyProgressMonitor detected multiple beginTask calls (e.g., "beginTask may only be called once (old name=Opening 'MyProject'.)"). Files changed: - resources/bundles/org.eclipse.core.resources/.../Policy.java - team/bundles/org.eclipse.team.core/.../Policy.java - team/bundles/org.eclipse.team.ui/.../Policy.java - team/bundles/org.eclipse.jsch.core/.../Policy.java 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../src/org/eclipse/core/internal/utils/Policy.java | 2 +- .../src/org/eclipse/jsch/internal/core/Policy.java | 2 +- .../src/org/eclipse/team/internal/core/Policy.java | 2 +- .../src/org/eclipse/team/internal/ui/Policy.java | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/utils/Policy.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/utils/Policy.java index 97a3c093cff..c7d961aadce 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/utils/Policy.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/utils/Policy.java @@ -189,6 +189,6 @@ public static IProgressMonitor subMonitorFor(IProgressMonitor monitor, int ticks if (monitor instanceof NullProgressMonitor) { return monitor; } - return new SubProgressMonitor(monitor, ticks); + return SubMonitor.convert(monitor).split(ticks); } } diff --git a/team/bundles/org.eclipse.jsch.core/src/org/eclipse/jsch/internal/core/Policy.java b/team/bundles/org.eclipse.jsch.core/src/org/eclipse/jsch/internal/core/Policy.java index dd069acbb74..7924e6703ff 100644 --- a/team/bundles/org.eclipse.jsch.core/src/org/eclipse/jsch/internal/core/Policy.java +++ b/team/bundles/org.eclipse.jsch.core/src/org/eclipse/jsch/internal/core/Policy.java @@ -38,7 +38,7 @@ public static IProgressMonitor subMonitorFor(IProgressMonitor monitor, if(monitor instanceof NullProgressMonitor){ return monitor; } - return SubMonitor.convert(monitor, ticks); + return SubMonitor.convert(monitor).split(ticks); } } diff --git a/team/bundles/org.eclipse.team.core/src/org/eclipse/team/internal/core/Policy.java b/team/bundles/org.eclipse.team.core/src/org/eclipse/team/internal/core/Policy.java index 36ab4860797..2f38b188b47 100644 --- a/team/bundles/org.eclipse.team.core/src/org/eclipse/team/internal/core/Policy.java +++ b/team/bundles/org.eclipse.team.core/src/org/eclipse/team/internal/core/Policy.java @@ -57,7 +57,7 @@ public static IProgressMonitor subMonitorFor(IProgressMonitor monitor, int ticks if (monitor instanceof NullProgressMonitor) { return monitor; } - return SubMonitor.convert(monitor, ticks); + return SubMonitor.convert(monitor).split(ticks); } public static IProgressMonitor infiniteSubMonitorFor(IProgressMonitor monitor, int ticks) { diff --git a/team/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/Policy.java b/team/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/Policy.java index 73342182be5..db750a89cfc 100644 --- a/team/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/Policy.java +++ b/team/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/Policy.java @@ -72,7 +72,7 @@ public static IProgressMonitor subMonitorFor(IProgressMonitor monitor, int ticks if (monitor instanceof NullProgressMonitor) { return monitor; } - return SubMonitor.convert(monitor, ticks); + return SubMonitor.convert(monitor).split(ticks); } public static IProgressMonitor monitorFor(IProgressMonitor monitor) { From e39b98b0c3c27b189a8a5c47d7fce94aeb7234c5 Mon Sep 17 00:00:00 2001 From: Eclipse Platform Bot Date: Fri, 14 Nov 2025 08:10:08 +0000 Subject: [PATCH 4/4] Version bump(s) for 4.38 stream --- team/bundles/org.eclipse.jsch.core/META-INF/MANIFEST.MF | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/team/bundles/org.eclipse.jsch.core/META-INF/MANIFEST.MF b/team/bundles/org.eclipse.jsch.core/META-INF/MANIFEST.MF index 82861fbee2b..83de036ad6d 100644 --- a/team/bundles/org.eclipse.jsch.core/META-INF/MANIFEST.MF +++ b/team/bundles/org.eclipse.jsch.core/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: %pluginName Bundle-SymbolicName: org.eclipse.jsch.core;singleton:=true -Bundle-Version: 1.5.700.qualifier +Bundle-Version: 1.5.800.qualifier Bundle-Activator: org.eclipse.jsch.internal.core.JSchCorePlugin Bundle-Vendor: %providerName Bundle-Localization: plugin