Partial fixes to #211, NPE fixes, and normalized path view.

Bug #211 can only be partially fixed, due to a problem with the IDE (see the bug report for details). There were some additional NPEs discovered that came with the refactoring of the configuration passing, and with infrequently encoutnered logging. The connection view pane now has the root paths shown with the right path separator character on Windows.
groboclown · Mar 2, 2020 · 835b505 · 835b505
1 parent 5499d09
commit 835b505
Show file tree

Hide file tree

Showing 26 changed files with 212 additions and 108 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -1,6 +1,20 @@
 # IDEA Community VCS Integration for Perforce
 
 
+## ::v0.10.15::
+
+### Overview
+
+* Bug fixes
+
+### Details
+
+* Bug fixes
+    * Fixed part of a bug where the VCS root configuration shared options between root directories (#211).  This is only a partial fix, as there's an underlying bug in the IDE that causes this problem.  The resolved properties panel shows the path that's being set, so if the UI seems to show the wrong path, it's because of this underlying bug.
+    * Fixed some NPEs caused by certain error and logging conditions.
+    * Normalized the path view in the connections panel for Windows.
+
+
 ## ::v0.10.13:: and ::v0.10.14::
 
 ### Overview

diff --git a/idea-p4server/api/src/main/java/net/groboclown/p4/server/api/ProjectConfigRegistry.java b/idea-p4server/api/src/main/java/net/groboclown/p4/server/api/ProjectConfigRegistry.java
@@ -120,7 +120,7 @@ public ClientConfigRoot getClientFor(@Nullable FilePath file) {
             }
         }
         if (LOG.isDebugEnabled()) {
-            LOG.debug("Using client root " + closest.getClientRootDir() + " for " + file);
+            LOG.debug("Using client root " + (closest == null ? null : closest.getClientRootDir()) + " for " + file);
         }
 
         return closest;

diff --git a/idea-p4server/api/src/main/java/net/groboclown/p4/server/api/config/part/ConfigPart.java b/idea-p4server/api/src/main/java/net/groboclown/p4/server/api/config/part/ConfigPart.java
@@ -202,4 +202,12 @@ public interface ConfigPart {
      * @return the raw port string value, before parsing as a name.
      */
     String getRawPort();
+
+    /**
+     * Make a copy of this configuration part.
+     *
+     * @return a copy
+     */
+    @NotNull
+    ConfigPart copy();
 }
diff --git a/...4server/api/src/main/java/net/groboclown/p4/server/api/config/part/ConfigPartAdapter.java b/...4server/api/src/main/java/net/groboclown/p4/server/api/config/part/ConfigPartAdapter.java
@@ -196,6 +196,12 @@ public String getRawPort() {
         return null;
     }
 
+    @NotNull
+    @Override
+    public ConfigPart copy() {
+        return new ConfigPartAdapter(sourceName);
+    }
+
     @Override
     public boolean requiresUserEnteredPassword() {
         return false;

diff --git a/...server/api/src/main/java/net/groboclown/p4/server/api/config/part/MultipleConfigPart.java b/...server/api/src/main/java/net/groboclown/p4/server/api/config/part/MultipleConfigPart.java
@@ -29,6 +29,7 @@
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
 
 public class MultipleConfigPart
         implements ConfigPart {
@@ -134,6 +135,15 @@ public String getRawPort() {
         return null;
     }
 
+    @NotNull
+    @Override
+    public ConfigPart copy() {
+        MultipleConfigPart ret = new MultipleConfigPart(sourceName,
+                parts.stream().map(ConfigPart::copy).collect(Collectors.toList()));
+        ret.extraProblems.addAll(extraProblems);
+        return ret;
+    }
+
     @Override
     public boolean hasServerNameSet() {
         for (ConfigPart part : parts) {
@@ -414,6 +424,12 @@ public List<ConfigPart> getChildren() {
         return new ArrayList<>(parts);
     }
 
+
+    @NotNull
+    public List<ConfigPart> cloneChildren() {
+        return parts.stream().map(ConfigPart::copy).collect(Collectors.toList());
+    }
+
     public void addAdditionalProblem(@NotNull ConfigProblem problem) {
         this.extraProblems.add(problem);
     }

diff --git a/idea-p4server/api/src/test/java/net/groboclown/p4/server/api/MockConfigPart.java b/idea-p4server/api/src/test/java/net/groboclown/p4/server/api/MockConfigPart.java
@@ -44,7 +44,8 @@ public class MockConfigPart
     private boolean requiresUserEnteredPassword;
     private Collection<ConfigProblem> configProblems = new ArrayList<>();
 
-    public MockConfigPart copy() {
+    @NotNull
+    public MockConfigPart copyMock() {
         MockConfigPart ret = new MockConfigPart()
                 .withSourceName(sourceName)
                 .withP4ServerName(serverName)
@@ -308,6 +309,12 @@ public String getRawPort() {
         return serverName.getFullPort();
     }
 
+    @NotNull
+    @Override
+    public ConfigPart copy() {
+        return copyMock();
+    }
+
     public MockConfigPart withConfigProblems(ConfigProblem... problems) {
         configProblems = Arrays.asList(problems);
         return this;

diff --git a/...ver/api/src/test/java/net/groboclown/p4/server/api/config/part/ConfigPartAdapterTest.java b/...ver/api/src/test/java/net/groboclown/p4/server/api/config/part/ConfigPartAdapterTest.java
@@ -14,9 +14,11 @@
 package net.groboclown.p4.server.api.config.part;
 
 import net.groboclown.p4.server.api.config.ConfigProblem;
+import org.jetbrains.annotations.NotNull;
 import org.junit.jupiter.api.Test;
 
 import javax.annotation.Nonnull;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
@@ -224,10 +226,21 @@ private ProblemConfigPartAdapter(ConfigProblem... problems) {
             this.problems = Arrays.asList(problems);
         }
 
+        public ProblemConfigPartAdapter(List<ConfigProblem> problems) {
+            super("Blah");
+            this.problems = new ArrayList<>(problems);
+        }
+
         @Nonnull
         @Override
         public Collection<ConfigProblem> getConfigProblems() {
             return problems;
         }
+
+        @NotNull
+        @Override
+        public ConfigPart copy() {
+            return new ProblemConfigPartAdapter(problems);
+        }
     }
-}
+}
diff --git a/...-p4server/impl/src/main/java/net/groboclown/p4/server/impl/ProjectConfigRegistryImpl.java b/...-p4server/impl/src/main/java/net/groboclown/p4/server/impl/ProjectConfigRegistryImpl.java
@@ -37,7 +37,6 @@
 import net.groboclown.p4.server.api.util.FilteredIterable;
 import net.groboclown.p4.server.impl.cache.ClientConfigRootImpl;
 import net.groboclown.p4.server.impl.cache.ServerStatusImpl;
-import net.groboclown.p4.server.impl.util.DirectoryMappingUtil;
 import net.groboclown.p4.server.impl.util.RootSettingsUtil;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -274,10 +273,9 @@ protected void updateVcsRoots() {
                     continue;
                 }
                 if (settings instanceof P4VcsRootSettings) {
-                    oldRoots.remove(DirectoryMappingUtil.getDirectory(getProject(), directoryMapping));
+                    oldRoots.remove(RootSettingsUtil.getDirectory(getProject(), directoryMapping));
                     updateRoot(
-                            RootSettingsUtil.getFixedRootSettings(getProject(), directoryMapping,
-                                DirectoryMappingUtil.getDirectory(getProject(), directoryMapping)),
+                            RootSettingsUtil.getFixedRootSettings(getProject(), directoryMapping),
                             directoryMapping);
                 } else {
                     LOG.warn("P4Vcs root mapping has non-vcs settings " + settings.getClass());
@@ -303,10 +301,8 @@ private void updateRoot(@NotNull P4VcsRootSettings settings,
                     addClientConfig(clientConfig, root);
                     return;
                 }
-                if (LOG.isDebugEnabled()) {
-                    LOG.debug(root + ": skipping invalid config " + parentPart);
-                }
-            } else if (LOG.isDebugEnabled()) {
+            }
+            if (LOG.isDebugEnabled()) {
                 LOG.debug(root + ": skipping invalid config " + parentPart);
             }
         } catch (IllegalArgumentException e) {

diff --git a/...mpl/src/main/java/net/groboclown/p4/server/impl/config/PersistentRootConfigComponent.java b/...mpl/src/main/java/net/groboclown/p4/server/impl/config/PersistentRootConfigComponent.java
@@ -129,8 +129,9 @@ public Element getState() {
         Element ret = new Element("all-root-configs");
         for (Map.Entry<VirtualFile, List<ConfigPart>> entry : rootPartMap.entrySet()) {
             if (!isValidRoot(entry.getKey())) {
-                LOG.info("Skipped writing root " + entry.getKey() +
-                        " because it does not appear to be a valid Perforce VCS root");
+                LOG.info("Skipped persisting VCS root " + entry.getKey() +
+                        " because it does not appear to be a valid Perforce root");
+                continue;
             }
             VcsRootCacheStore store = new VcsRootCacheStore(entry.getKey());
             store.setConfigParts(entry.getValue());
@@ -199,6 +200,8 @@ private boolean isValidRoot(final VirtualFile file) {
                 return true;
             }
         }
+        // This can happen if the passed-in file is not yet registered in
+        // the VCS root path.
         return false;
     }
 }
diff --git a/...er/impl/src/main/java/net/groboclown/p4/server/impl/config/part/ClientNameConfigPart.java b/...er/impl/src/main/java/net/groboclown/p4/server/impl/config/part/ClientNameConfigPart.java
@@ -17,6 +17,7 @@
 import com.intellij.openapi.util.text.StringUtil;
 import com.intellij.openapi.vfs.VirtualFile;
 import net.groboclown.p4.server.api.config.ConfigProblem;
+import net.groboclown.p4.server.api.config.part.ConfigPart;
 import net.groboclown.p4.server.api.config.part.ConfigPartAdapter;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -43,12 +44,19 @@ public ClientNameConfigPart(@NotNull String sourceName) {
         super(sourceName);
     }
 
+    // require this constructor
+    @SuppressWarnings("unused")
     public ClientNameConfigPart(@NotNull String sourceName, @NotNull VirtualFile root,
             @NotNull Map<String, String> stateValues) {
         super(sourceName);
         this.clientName = stateValues.get("c");
     }
 
+    private ClientNameConfigPart(@NotNull String sourceName, @Nullable String clientName) {
+        super(sourceName);
+        this.clientName = clientName;
+    }
+
     @Override
     public boolean reload() {
         // Nothing to do
@@ -69,6 +77,14 @@ public String getRawPort() {
         return null;
     }
 
+    @NotNull
+    @Override
+    public ConfigPart copy() {
+        ClientNameConfigPart ret = new ClientNameConfigPart(getSourceName(), clientName);
+        ret.additionalProblems.addAll(additionalProblems);
+        return ret;
+    }
+
 
     @Override
     public boolean hasClientnameSet() {
@@ -89,15 +105,6 @@ public void setClientname(@Nullable String clientName) {
         }
     }
 
-
-    public void addAdditionalProblem(@NotNull ConfigProblem problem) {
-        additionalProblems.add(problem);
-    }
-
-    public void clearAdditionalProblems() {
-        additionalProblems.clear();
-    }
-
     @Override
     public boolean equals(Object o) {
         if (o == null || ! getClass().equals(o.getClass())) {

diff --git a/...server/impl/src/main/java/net/groboclown/p4/server/impl/config/part/EnvCompositePart.java b/...server/impl/src/main/java/net/groboclown/p4/server/impl/config/part/EnvCompositePart.java
@@ -85,6 +85,12 @@ public String getRawPort() {
         return configParts.getRawPort();
     }
 
+    @NotNull
+    @Override
+    public ConfigPart copy() {
+        return new EnvCompositePart(vcsRoot);
+    }
+
     @Override
     public boolean hasServerNameSet() {
         return configParts.hasServerNameSet();
@@ -424,6 +430,12 @@ public boolean reload() {
         public Collection<ConfigProblem> getConfigProblems() {
             return Collections.emptyList();
         }
+
+        @NotNull
+        @Override
+        public ConfigPart copy() {
+            return new EnvPassword();
+        }
     }
 
 

diff --git a/...p4server/impl/src/main/java/net/groboclown/p4/server/impl/config/part/FileConfigPart.java b/...p4server/impl/src/main/java/net/groboclown/p4/server/impl/config/part/FileConfigPart.java
@@ -216,6 +216,12 @@ public String getRawPort() {
         return rawPort;
     }
 
+    @NotNull
+    @Override
+    public ConfigPart copy() {
+        return new FileConfigPart(vcsRoot, filePath);
+    }
+
     @Nls
     @NotNull
     @Override

diff --git a/...impl/src/main/java/net/groboclown/p4/server/impl/config/part/RequirePasswordDataPart.java b/...impl/src/main/java/net/groboclown/p4/server/impl/config/part/RequirePasswordDataPart.java
@@ -15,6 +15,7 @@
 package net.groboclown.p4.server.impl.config.part;
 
 import com.intellij.openapi.vfs.VirtualFile;
+import net.groboclown.p4.server.api.config.part.ConfigPart;
 import net.groboclown.p4.server.api.config.part.ConfigPartAdapter;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -30,6 +31,7 @@ public RequirePasswordDataPart() {
     }
 
     // for ConfigStateProvider
+    @SuppressWarnings("unused")
     public RequirePasswordDataPart(String sourceName, VirtualFile vcsRoot, Map<String, String> values) {
         super(sourceName);
     }
@@ -82,4 +84,10 @@ public int hashCode() {
     public Map<String, String> getState() {
         return Collections.emptyMap();
     }
+
+    @NotNull
+    @Override
+    public ConfigPart copy() {
+        return new RequirePasswordDataPart();
+    }
 }
diff --git a/...pl/src/main/java/net/groboclown/p4/server/impl/config/part/ServerFingerprintDataPart.java b/...pl/src/main/java/net/groboclown/p4/server/impl/config/part/ServerFingerprintDataPart.java
@@ -17,6 +17,7 @@
 import com.intellij.openapi.util.text.StringUtil;
 import com.intellij.openapi.vfs.VirtualFile;
 import net.groboclown.p4.server.api.config.ConfigProblem;
+import net.groboclown.p4.server.api.config.part.ConfigPart;
 import net.groboclown.p4.server.api.config.part.ConfigPartAdapter;
 import org.jetbrains.annotations.Nls;
 import org.jetbrains.annotations.NotNull;
@@ -35,6 +36,7 @@ public ServerFingerprintDataPart(@Nls @NotNull String sourceName) {
     }
 
     // for ConfigStateProvider
+    @SuppressWarnings("unused")
     public ServerFingerprintDataPart(@NotNull String sourceName, VirtualFile root,
             @NotNull Map<String, String> values) {
         super(sourceName);
@@ -96,4 +98,10 @@ public Map<String, String> getState() {
         ret.put("f", fingerprint);
         return ret;
     }
+
+    @NotNull
+    @Override
+    public ConfigPart copy() {
+        return new ServerFingerprintDataPart(fingerprint);
+    }
 }
diff --git a/...p4server/impl/src/main/java/net/groboclown/p4/server/impl/config/part/SimpleDataPart.java b/...p4server/impl/src/main/java/net/groboclown/p4/server/impl/config/part/SimpleDataPart.java
@@ -79,6 +79,12 @@ public String getRawPort() {
         return trimmedProperty(PORT_KEY);
     }
 
+    @NotNull
+    @Override
+    public ConfigPart copy() {
+        return new SimpleDataPart(vcsRoot, sourceName, properties);
+    }
+
     // Password must be carefully stored.
 
     @Override