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

)

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

We fix the postupgrade script by doing two things:

(1) Only remove files in the theme folder if it is not a submodule. The
default for the jambo init command is to have theme folders added as
submodules. Before we were removing the .git folder which made it so
that we could not see the history of the theme folder. We can now see
the full theme git history after an upgrade.

(2) Merge the correct global_config files. Before, we were merging the
theme global_config.json pre-upgrade and post-upgrade. That is, we did
not touch the user's global_config.json. This is not the desired
behavior, and instead, we would like to see new fields from any new theme
global_config.json file in the user-defined config/global_config.json.
So we change the files being merged.

Note: This means that on postupgrade, we update the user's
config/global_config.json with any new fields. We do not delete any
fields.

Note: This does not touch legacy upgrades.

Add tests for postupgrade merge JSON to make sure we keep certain
assumptions about this global_config merge.

First, it should always give precedence to the original config.
Second, it should not delete any custom fields added to the
global_config.
Third, it should be completely additive and add new fields if they exist
in any of the configuration parameters.

J=SLAP-578
TEST=manual

npm test

Test on a completely new `jambo init` site with the HH theme. Try `jambo
upgrade` and make sure there are no errors. The error we don't want to
see is below.
```
fatal: Path 'themes/answers-hitchhiker-theme/global_config.json' exists on disk, but not in 'HEAD'.
 Error: fatal: Path 'themes/answers-hitchhiker-theme/global_config.json' exists on disk, but not in 'HEAD'.

    at GitExecutorChain.onFatalException (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:60:87)
    at GitExecutorChain.<anonymous> (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:51:28)
    at Generator.throw (<anonymous>)
    at rejected (/Users/creotutar/test/jambo/test6/node_modules/simple-git/src/lib/runners/git-executor-chain.js:6:65)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
Error occurred on node version: v12.16.1
```

Used below command to clear the folder
```
rm -rf ./* && rm -rf .git* && jambo init && jambo import --theme
answers-hitchhiker-theme && npm install && git add -A && git commit -m
"init"
```

* PR fixes: syntax, remove trim, var name change

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);
+    });
+  });
+});