fixes

Author: ecraig12345

change/change-bdba81ff-9645-4804-9b52-917255f03803.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "type": "patch",
+      "comment": "Refactor internals for getting changed packages",
+      "packageName": "beachball",
+      "email": "elcraig@microsoft.com",
+      "dependentChangeType": "patch"
+    }
+  ]
+}

packages/beachball/src/__fixtures__/repository.ts

@@ -167,21 +167,21 @@ ${gitResult.stderr.toString()}`);
 
   /**
    * Update the content of a JSON file that already exists in the repo.
-   * The updates will be merged with the original.
+   * The updates will be shallowly merged with the original. Throws if the file doesn't exist.
    *
    * This is useful if you'd like to mostly use a built-in fixture but change one package,
    * such as making it private.
    */
-  updateJsonFile(filename: string, updates: object, options?: { mustExist?: boolean; commit?: boolean }): void {
+  updateJsonFile(filename: string, updates: object, options?: { commit?: boolean }): void {
     if (!filename.endsWith('.json')) {
       throw new Error('This method only works with json files');
     }
 
     const fullPath = this.pathTo(filename);
-    const oldContent = readJson<object>(fullPath);
-    if (options?.mustExist && !oldContent) {
+    if (!fs.existsSync(fullPath)) {
       throw new Error(`JSON file does not exist: ${filename}`);
     }
+    const oldContent = readJson<object>(fullPath);
     writeJson(fullPath, { ...oldContent, ...updates });
 
     if (options?.commit) {

packages/beachball/src/__functional__/git/getAllChangedPackages.test.ts

@@ -199,9 +199,9 @@ describe('getAllChangedPackages', () => {
     repo = getReusedRepoWithBranch('monorepo');
 
     // Update packages so they'll be ignored
-    repo.updateJsonFile('packages/foo/package.json', { private: true }, { mustExist: true });
+    repo.updateJsonFile('packages/foo/package.json', { private: true });
     repo.writeFile('packages/foo/foo.js');
-    repo.updateJsonFile('packages/bar/package.json', { beachball: { shouldPublish: false } }, { mustExist: true });
+    repo.updateJsonFile('packages/bar/package.json', { beachball: { shouldPublish: false } });
     repo.writeFile('packages/bar/bar.js');
     // baz is not ignored
     repo.writeFile('packages/baz/baz.js');

packages/beachball/src/changefile/getChangedPackages.ts

@@ -9,15 +9,21 @@ import { readJson } from '../object/readJson';
 import { bulletedList } from '../logging/bulletedList';
 import { getAllChangedPackages } from './getAllChangedPackages';
 import { getIncludedLoggers, isPackageIncluded } from './isPackageIncluded';
-import { ensureSharedHistory, hasCommonCommit } from '../git/ensureSharedHistory';
+import { ensureSharedHistory } from '../git/ensureSharedHistory';
 
 /**
  * Gets all the changed packages which **do not already have a change file** and are in scope.
+ * This is only used by the `change` and `check` commands, not the bump/publish process.
  *
- * Exceptions:
+ * Special cases:
  * - If `options.package` is provided, use that as-is (skipping all git operations).
  * - If `options.all` is true, gets all the packages in scope regardless of whether they've changed
- *   (skipping git diff of files), filtered by packages that already have change files.
+ *   (skipping git diff of files), omitting packages that already have change files.
+ *
+ * Usually (without `options.package`) this has the side effect of calling `ensureSharedHistory` to
+ * verify that enough git history is available to check for changes between `HEAD` and
+ * `options.branch` (only an issue for shallow clones), and deepens the history if needed.
+ * Unless `options.fetch` is `false`, it will also fetch from the remote.
  */
 export function getChangedPackages(
   options: BeachballOptions,
@@ -33,10 +39,10 @@ export function getChangedPackages(
 
   console.log(`Checking for changes against "${options.branch}"`);
 
-  // We should fetch shared history even with --all for accurate change file checks later
-  if (!hasCommonCommit(branch, options.path)) {
-    ensureSharedHistory(options);
-  }
+  // Ensure the current branch and target branch have a common shared commit. This has the side
+  // effect of fetching from the remote (unless disabled), which should be done even if there's
+  // already shared history (and even with --all) for accurate added change file checks later.
+  ensureSharedHistory(options);
 
   let changedPackages: string[];
 

packages/beachball/src/git/ensureSharedHistory.ts

@@ -197,6 +197,6 @@ function isShallowRepository(cwd: string): boolean {
 }
 
 /** Returns whether `branch` and HEAD have a common commit anywhere in their history */
-export function hasCommonCommit(branch: string, cwd: string): boolean {
+function hasCommonCommit(branch: string, cwd: string): boolean {
   return git(['merge-base', branch, 'HEAD'], { cwd }).success;
 }