postupgrade: Fix script for submodules, merge correctly, keep .git

postupgrade/PostUpgradeHandler.js

@@ -1,7 +1,7 @@
 const fs = require('fs');
 const fsExtra = require('fs-extra');
 const path = require('path');
-const { mergeJson } = require('./utils');
+const { mergeJson, isGitSubmodule } = require('./utils');
 const simpleGit = require('simple-git/promise')();
 
 /**
@@ -15,13 +15,19 @@ class PostUpgradeHandler {
   }
 
   async handlePostUpgrade() {
-    this.removeFromTheme('.git', '.gitignore', 'tests');
+    if (!isGitSubmodule(this.themeDir)) {
+      this.removeFromTheme('.git', '.gitignore', 'tests');
+    }
+
+    const userGlobalConfigPath = 
+      path.relative(process.cwd(), path.join(this.configDir, this.globalConfigFile));
     const themeGlobalConfigPath = 
       path.relative(process.cwd(), path.join(this.themeDir, this.globalConfigFile));
     if (fsExtra.pathExistsSync(themeGlobalConfigPath)) {
-      const mergedGlobalConfig = await this.mergeThemeGlobalConfig(themeGlobalConfigPath);
-      fs.writeFileSync(themeGlobalConfigPath, mergedGlobalConfig);
+      const mergedGlobalConfig = await this.mergeThemeGlobalConfig(userGlobalConfigPath, themeGlobalConfigPath);
+      fs.writeFileSync(userGlobalConfigPath, mergedGlobalConfig);
     }
+
     this.copyStaticFilesToTopLevel(
       'package.json', 'Gruntfile.js', 'webpack-config.js', 'package-lock.json');
   }
@@ -37,10 +43,13 @@ class PostUpgradeHandler {
 
   /**
    * Merges the theme's global_config with the user's current global_config.
+   * @param {string} userGlobalConfigPath The path to the user's current global config
+   * @param {string} themeGlobalConfigPath The path to the global config in the theme
+   * @return {string} The comment-json merged global config
    */
-  async mergeThemeGlobalConfig(themeGlobalConfigPath) {
+  async mergeThemeGlobalConfig(userGlobalConfigPath, themeGlobalConfigPath) {
     const updatedCommentJson = fs.readFileSync(themeGlobalConfigPath, 'utf-8');
-    const originalCommentJson = await simpleGit.show([`HEAD:${themeGlobalConfigPath}`]);
+    const originalCommentJson = fs.readFileSync(userGlobalConfigPath, 'utf-8');
     return mergeJson(updatedCommentJson, originalCommentJson);
   }
 

postupgrade/utils.js

@@ -1,6 +1,7 @@
 const fs = require('fs');
 const { parse, assign, stringify } = require('comment-json');
 const path = require('path');
+const simpleGit = require('simple-git/promise')();
 
 /**
  * Merges two comment-json files, with the original having higher priority.
@@ -52,3 +53,16 @@ function removeEmptyDirectoriesRecursively(dirpath) {
   }
 }
 exports.removeEmptyDirectoriesRecursively = removeEmptyDirectoriesRecursively;
+
+/**
+ * Returns whether the given file path is registered as a git submodule.
+ * @param {string} submodulePath
+ * @returns {boolean}
+ */
+async function isGitSubmodule(submodulePath) {
+  const submodulePaths = await simpleGit.subModule(['foreach', '--quiet', 'echo $sm_path']);
+  return !!submodulePaths
+    .split('\n')
+    .find(p => p === submodulePath)
+}
+exports.isGitSubmodule = isGitSubmodule;

tests/postupgrade/config1.json

@@ -0,0 +1,5 @@
+{
+  // Comment1
+  "one": "hi", // Inline comment
+  "onetwo": "hello"
+}

tests/postupgrade/config2.json

@@ -0,0 +1,5 @@
+{
+  "two": "bye",
+  "onetwo": "goodbye"
+  // Comment2
+}

tests/postupgrade/config3.json

@@ -0,0 +1,8 @@
+{
+  // Comment1
+  "one": "hi", // Inline comment
+  "onetwo": "goodbye"
+  // Comment2
+  ,
+  "two": "bye"
+}

tests/postupgrade/utils.js

@@ -0,0 +1,17 @@
+const { mergeJson } = require('postupgrade/utils.js');
+const fs = require('fs');
+
+describe('postupgrade utils', () => {
+  describe('mergeJson', () => {
+    it('puts together two comment JSON strings', () => {
+      const config1 = fs.readFileSync('./tests/postupgrade/config1.json', 'utf-8');
+      const config2 = fs.readFileSync('./tests/postupgrade/config2.json', 'utf-8');
+      const expected = fs.readFileSync('./tests/postupgrade/config3.json', 'utf-8');
+
+      // if this passes, then it is correctly not deleting any fields or comments
+      // and it is giving precedence to the second parameter config
+      const mergedConfig = mergeJson(config1, config2);
+      expect(mergedConfig).toEqual(expected);
+    });
+  });
+});