Fix missing author in git commit (#386)

src/lib/__snapshots__/cherrypickAndCreateTargetPullRequest.test.ts.snap

@@ -100,9 +100,9 @@ Array [
     "git",
     Array [
       "-c",
-      "user.name=\\"Søren Louv-Jansen\\"",
+      "user.name=\\"Soren L\\"",
       "-c",
-      "user.email=\\"soren[email protected]\\"",
+      "user.email=\\"soren@louv.dk\\"",
       "cherry-pick",
       "-x",
       "mySha",
@@ -112,6 +112,10 @@ Array [
   Array [
     "git",
     Array [
+      "-c",
+      "user.name=\\"Soren L\\"",
+      "-c",
+      "user.email=\\"[email protected]\\"",
       "commit",
       "--no-edit",
     ],
@@ -121,9 +125,9 @@ Array [
     "git",
     Array [
       "-c",
-      "user.name=\\"Søren Louv-Jansen\\"",
+      "user.name=\\"Soren L\\"",
       "-c",
-      "user.email=\\"soren[email protected]\\"",
+      "user.email=\\"soren@louv.dk\\"",
       "cherry-pick",
       "-x",
       "mySha2",
@@ -133,6 +137,10 @@ Array [
   Array [
     "git",
     Array [
+      "-c",
+      "user.name=\\"Soren L\\"",
+      "-c",
+      "user.email=\\"[email protected]\\"",
       "commit",
       "--no-edit",
     ],

src/lib/author.ts

@@ -0,0 +1,42 @@
+import { Commit } from '../entrypoint.module';
+import { ValidConfigOptions } from '../options/options';
+import { getGitConfig, getLocalSourceRepoPath } from './git';
+
+export type GitConfigAuthor = { name?: string; email?: string };
+export async function getGitConfigAuthor(
+  options: ValidConfigOptions
+): Promise<GitConfigAuthor | undefined> {
+  const localRepoPath = await getLocalSourceRepoPath(options);
+  if (!localRepoPath) {
+    return;
+  }
+
+  return {
+    name: await getGitConfig({ dir: localRepoPath, key: 'user.name' }),
+    email: await getGitConfig({ dir: localRepoPath, key: 'user.email' }),
+  };
+}
+
+export type CommitAuthor = Required<GitConfigAuthor>;
+export function getCommitAuthor({
+  options,
+  commit,
+  gitConfigAuthor,
+}: {
+  options: ValidConfigOptions;
+  commit: Commit;
+  gitConfigAuthor?: GitConfigAuthor;
+}): CommitAuthor {
+  if (options.resetAuthor) {
+    return {
+      name: options.authenticatedUsername,
+      email: `<${options.authenticatedUsername}@users.noreply.github.com>`,
+    };
+  }
+
+  return {
+    name: options.gitAuthorName ?? gitConfigAuthor?.name ?? commit.author.name,
+    email:
+      options.gitAuthorEmail ?? gitConfigAuthor?.email ?? commit.author.email,
+  };
+}

src/lib/cherrypickAndCreateTargetPullRequest.test.ts

@@ -47,6 +47,8 @@ describe('cherrypickAndCreateTargetPullRequest', () => {
       const options = {
         assignees: [] as string[],
         authenticatedUsername: 'sqren_authenticated',
+        gitAuthorName: 'Soren L',
+        gitAuthorEmail: '[email protected]',
         author: 'sqren',
         fork: true,
         interactive: true,

src/lib/cherrypickAndCreateTargetPullRequest.ts

@@ -2,10 +2,15 @@ import chalk from 'chalk';
 import { isEmpty, difference, flatten } from 'lodash';
 import { ValidConfigOptions } from '../options/options';
 import { BackportError } from './BackportError';
+import {
+  CommitAuthor,
+  getCommitAuthor,
+  getGitConfigAuthor,
+  GitConfigAuthor,
+} from './author';
 import { spawnPromise } from './child-process-promisified';
 import { getRepoPath } from './env';
 import { getCommitsWithoutBackports } from './getCommitsWithoutBackports';
-import { getGitConfigAuthor, GitConfigAuthor } from './getGitConfigAuthor';
 import {
   cherrypick,
   createBackportBranch,
@@ -200,38 +205,44 @@ async function waitForCherrypick(
     getFirstLine(commit.sourceCommit.message)
   )}`;
   const cherrypickSpinner = ora(options.interactive, spinnerText).start();
+  const commitAuthor = getCommitAuthor({ options, commit, gitConfigAuthor });
 
-  await cherrypickAndHandleConflicts(
+  await cherrypickAndHandleConflicts({
     options,
     commit,
+    commitAuthor,
     targetBranch,
     cherrypickSpinner,
-    gitConfigAuthor
-  );
+  });
 
   // Conflicts should be resolved and files staged at this point
 
   try {
-    // Run `git commit`
-    await commitChanges(commit, options);
+    // Run `git commit` in case conflicts were not manually committed
+    await commitChanges({ options, commit, commitAuthor });
     cherrypickSpinner.succeed();
   } catch (e) {
     cherrypickSpinner.fail();
     throw e;
   }
 }
 
-async function cherrypickAndHandleConflicts(
-  options: ValidConfigOptions,
-  commit: Commit,
-  targetBranch: string,
-  cherrypickSpinner: Ora,
-  gitConfigAuthor?: GitConfigAuthor
-) {
+async function cherrypickAndHandleConflicts({
+  options,
+  commit,
+  commitAuthor,
+  targetBranch,
+  cherrypickSpinner,
+}: {
+  options: ValidConfigOptions;
+  commit: Commit;
+  commitAuthor: CommitAuthor;
+  targetBranch: string;
+  cherrypickSpinner: Ora;
+}) {
   const mergedTargetPullRequest = commit.expectedTargetPullRequests.find(
     (pr) => pr.state === 'MERGED' && pr.branch === targetBranch
   );
-  const commitAuthor = getCommitAuthor({ options, gitConfigAuthor, commit });
 
   let conflictingFiles: ConflictingFiles;
   let unstagedFiles: string[];
@@ -398,26 +409,3 @@ async function listConflictingAndUnstagedFiles({
     unstagedFiles: _unstagedFiles,
   });
 }
-
-function getCommitAuthor({
-  options,
-  gitConfigAuthor,
-  commit,
-}: {
-  options: ValidConfigOptions;
-  gitConfigAuthor?: GitConfigAuthor;
-  commit: Commit;
-}) {
-  if (options.resetAuthor) {
-    return {
-      name: options.authenticatedUsername,
-      email: `<${options.authenticatedUsername}@users.noreply.github.com>`,
-    };
-  }
-
-  return {
-    name: options.gitAuthorName ?? gitConfigAuthor?.name ?? commit.author.name,
-    email:
-      options.gitAuthorEmail ?? gitConfigAuthor?.email ?? commit.author.email,
-  };
-}

src/lib/git.private.test.ts

@@ -354,7 +354,7 @@ describe('git.integration', () => {
       });
 
       expect(async () => {
-        return await commitChanges(commit, options);
+        return await commitChanges({ commit, commitAuthor, options });
       }).not.toThrowError();
 
       const message = await getMostRecentCommitMessage(cwd);
@@ -374,7 +374,7 @@ describe('git.integration', () => {
         cwd,
       });
 
-      await commitChanges(commit, options);
+      await commitChanges({ commit, commitAuthor, options });
 
       const message = await getMostRecentCommitMessage(cwd);
       expect(message).toBe('my fallback commit message');
@@ -407,7 +407,7 @@ describe('git.integration', () => {
       // disregard conflicts and stage all files
       await childProcess.exec('git add -A', { cwd });
 
-      await commitChanges(commit, options);
+      await commitChanges({ commit, commitAuthor, options });
 
       const message = await getMostRecentCommitMessage(cwd);
       expect(message).toMatchInlineSnapshot(`

src/lib/git.test.ts

@@ -596,7 +596,9 @@ describe('commitChanges', () => {
       .spyOn(childProcess, 'spawnPromise')
       .mockResolvedValueOnce({ stderr: '', stdout: '', code: 0, cmdArgs: [] });
 
-    await expect(await commitChanges(commit, options)).toBe(undefined);
+    await expect(await commitChanges({ commit, commitAuthor, options })).toBe(
+      undefined
+    );
   });
 
   it('should swallow error if changes have already been committed manaully', async () => {
@@ -609,7 +611,9 @@ describe('commitChanges', () => {
     });
 
     jest.spyOn(childProcess, 'spawnPromise').mockRejectedValueOnce(err);
-    await expect(await commitChanges(commit, options)).toBe(undefined);
+    await expect(await commitChanges({ commit, commitAuthor, options })).toBe(
+      undefined
+    );
   });
 
   describe('when commit fails due to empty message', () => {
@@ -633,21 +637,36 @@ describe('commitChanges', () => {
           cmdArgs: [],
         });
 
-      res = await commitChanges(commit, options);
+      res = await commitChanges({ commit, commitAuthor, options });
     });
 
     it('should manually set the commit message', () => {
       expect(spy).toHaveBeenCalledTimes(2);
-      expect(spy).toHaveBeenCalledWith(
+      expect(spy.mock.calls[0]).toEqual([
         'git',
-        ['commit', '--no-edit'],
-        expect.any(String)
-      );
-      expect(spy).toHaveBeenCalledWith(
+        [
+          '-c',
+          'user.name="Soren L"',
+          '-c',
+          'user.email="[email protected]"',
+          'commit',
+          '--no-edit',
+        ],
+        expect.any(String),
+      ]);
+
+      expect(spy.mock.calls[1]).toEqual([
         'git',
-        ['commit', '--message=The original commit message'],
-        expect.any(String)
-      );
+        [
+          '-c',
+          'user.name="Soren L"',
+          '-c',
+          'user.email="[email protected]"',
+          'commit',
+          '--message=The original commit message',
+        ],
+        expect.any(String),
+      ]);
     });
 
     it('should handle the error and resolve successfully', async () => {
@@ -661,7 +680,7 @@ describe('commitChanges', () => {
     expect.assertions(1);
 
     await expect(
-      commitChanges(commit, options)
+      commitChanges({ commit, commitAuthor, options })
     ).rejects.toThrowErrorMatchingInlineSnapshot(`"another error"`);
   });
 });

src/lib/git.ts

@@ -4,6 +4,7 @@ import { ora } from '../lib/ora';
 import { ValidConfigOptions } from '../options/options';
 import { filterNil } from '../utils/filterEmpty';
 import { BackportError } from './BackportError';
+import { CommitAuthor } from './author';
 import {
   spawnPromise,
   SpawnError,
@@ -288,16 +289,16 @@ export async function cherrypick({
   options: ValidConfigOptions;
   sha: string;
   mergedTargetPullRequest?: ExpectedTargetPullRequest;
-  commitAuthor: { name: string; email: string };
+  commitAuthor: CommitAuthor;
 }): Promise<{
   conflictingFiles: { absolute: string; relative: string }[];
   unstagedFiles: string[];
   needsResolving: boolean;
 }> {
   const cmdArgs = [
-    '-c',
+    `-c`,
     `user.name="${commitAuthor.name}"`,
-    '-c',
+    `-c`,
     `user.email="${commitAuthor.email}"`,
     `cherry-pick`,
     ...(options.mainline != undefined
@@ -359,17 +360,26 @@ export async function cherrypick({
   }
 }
 
-export async function commitChanges(
-  commit: Commit,
-  options: ValidConfigOptions
-) {
+export async function commitChanges({
+  options,
+  commit,
+  commitAuthor,
+}: {
+  options: ValidConfigOptions;
+  commit: Commit;
+  commitAuthor: CommitAuthor;
+}) {
   const noVerifyFlag = options.noVerify ? ['--no-verify'] : [];
   const cwd = getRepoPath(options);
 
   try {
     await spawnPromise(
       'git',
       [
+        `-c`,
+        `user.name="${commitAuthor.name}"`,
+        `-c`,
+        `user.email="${commitAuthor.email}"`,
         'commit',
         '--no-edit', // Use the selected commit message without launching an editor.
         ...noVerifyFlag, // bypass pre-commit and commit-msg hooks
@@ -396,6 +406,10 @@ export async function commitChanges(
         await spawnPromise(
           'git',
           [
+            `-c`,
+            `user.name="${commitAuthor.name}"`,
+            `-c`,
+            `user.email="${commitAuthor.email}"`,
             'commit',
             `--message=${commit.sourceCommit.message}`,
             ...noVerifyFlag, // bypass pre-commit and commit-msg hooks