Merge pull request #582 from desktop/output-encoding

Support setting output encoding in exec/execTask

.github/workflows/ci.yml

@@ -25,7 +25,7 @@ jobs:
             friendlyName: macOS
           - os: windows-latest
             friendlyName: Windows
-          - os: windows-2019
+          - os: windows-latest
             friendlyName: Windows
             nodeVersion: 18
             arch: x86

examples/api-extensibility.ts

@@ -1,5 +1,4 @@
-import { GitProcess, IGitExecutionOptions } from '../lib/'
-import { ChildProcess } from 'child_process'
+import { GitProcess, IGitStringExecutionOptions } from '../lib/git-process'
 
 const byline = require('byline')
 const ProgressBar = require('progress')
@@ -51,12 +50,12 @@ function setResolvingProgress(percent: number) {
 async function performClone(): Promise<void> {
   const path = 'C:/some/path/on/disk'
 
-  const options: IGitExecutionOptions = {
+  const options: IGitStringExecutionOptions = {
     // enable diagnostics
     env: {
       GIT_HTTP_USER_AGENT: 'dugite/2.12.0',
     },
-    processCallback: (process: ChildProcess) => {
+    processCallback: process => {
       byline(process.stderr).on('data', (chunk: string) => {
         if (chunk.startsWith('Receiving objects: ')) {
           const percent = tryParse(chunk)

lib/git-process.ts

@@ -1,7 +1,7 @@
 import * as fs from 'fs'
 import { kill } from 'process'
 
-import { execFile, spawn, ExecOptionsWithStringEncoding } from 'child_process'
+import { execFile, spawn } from 'child_process'
 import {
   GitError,
   GitErrorRegexes,
@@ -12,18 +12,35 @@ import { ChildProcess } from 'child_process'
 
 import { setupEnvironment } from './git-environment'
 
-/** The result of shelling out to git. */
 export interface IGitResult {
   /** The standard output from git. */
-  readonly stdout: string
+  readonly stdout: string | Buffer
 
   /** The standard error output from git. */
-  readonly stderr: string
+  readonly stderr: string | Buffer
 
   /** The exit code of the git process. */
   readonly exitCode: number
 }
 
+/** The result of shelling out to git using a string encoding (default) */
+export interface IGitStringResult extends IGitResult {
+  /** The standard output from git. */
+  readonly stdout: string
+
+  /** The standard error output from git. */
+  readonly stderr: string
+}
+
+/** The result of shelling out to git using a buffer encoding */
+export interface IGitBufferResult extends IGitResult {
+  /** The standard output from git. */
+  readonly stdout: Buffer
+
+  /** The standard error output from git. */
+  readonly stderr: Buffer
+}
+
 /**
  * A set of configuration options that can be passed when
  * executing a streaming Git command.
@@ -62,6 +79,12 @@ export interface IGitExecutionOptions {
    */
   readonly stdinEncoding?: BufferEncoding
 
+  /**
+   * The encoding to use when decoding the stdout and stderr output. Defaults to
+   * 'utf8'.
+   */
+  readonly encoding?: BufferEncoding | 'buffer'
+
   /**
    * The size the output buffer to allocate to the spawned process. Set this
    * if you are anticipating a large amount of output.
@@ -81,6 +104,14 @@ export interface IGitExecutionOptions {
   readonly processCallback?: (process: ChildProcess) => void
 }
 
+export interface IGitStringExecutionOptions extends IGitExecutionOptions {
+  readonly encoding?: BufferEncoding
+}
+
+export interface IGitBufferExecutionOptions extends IGitExecutionOptions {
+  readonly encoding: 'buffer'
+}
+
 /**
  * The errors coming from `execFile` have a `code` and we wanna get at that
  * without resorting to `any` casts.
@@ -140,6 +171,16 @@ export class GitProcess {
    * See the result's `stderr` and `exitCode` for any potential git error
    * information.
    */
+  public static exec(
+    args: string[],
+    path: string,
+    options?: IGitStringExecutionOptions
+  ): Promise<IGitStringResult>
+  public static exec(
+    args: string[],
+    path: string,
+    options?: IGitBufferExecutionOptions
+  ): Promise<IGitBufferResult>
   public static exec(
     args: string[],
     path: string,
@@ -163,11 +204,26 @@ export class GitProcess {
    *
    * And `cancel()` will try to cancel the git process
    */
+  public static execTask(
+    args: string[],
+    path: string,
+    options?: IGitStringExecutionOptions
+  ): IGitTask<IGitStringResult>
+  public static execTask(
+    args: string[],
+    path: string,
+    options?: IGitBufferExecutionOptions
+  ): IGitTask<IGitBufferResult>
   public static execTask(
     args: string[],
     path: string,
     options?: IGitExecutionOptions
-  ): IGitTask {
+  ): IGitTask<IGitResult>
+  public static execTask(
+    args: string[],
+    path: string,
+    options?: IGitExecutionOptions
+  ): IGitTask<IGitResult> {
     let pidResolve: {
       (arg0: any): void
       (value: number | PromiseLike<number | undefined> | undefined): void
@@ -185,23 +241,28 @@ export class GitProcess {
 
         const { env, gitLocation } = setupEnvironment(customEnv)
 
-        // Explicitly annotate opts since typescript is unable to infer the correct
-        // signature for execFile when options is passed as an opaque hash. The type
-        // definition for execFile currently infers based on the encoding parameter
-        // which could change between declaration time and being passed to execFile.
-        // See https://git.io/vixyQ
-        const execOptions: ExecOptionsWithStringEncoding = {
-          cwd: path,
-          encoding: 'utf8',
-          maxBuffer: options ? options.maxBuffer : 10 * 1024 * 1024,
-          env,
-        }
+        // This is the saddest hack. There's a bug in the types for execFile
+        // (ExecFileOptionsWithBufferEncoding is the exact same as
+        // ExecFileOptionsWithStringEncoding) so we can't get TS to pick the
+        // execFile overload that types stdout/stderr as buffer by setting
+        // the encoding to 'buffer'. So we'll do this ugly where we pretend
+        // it'll only ever be a valid encoding or 'null' (which isn't true).
+        //
+        // This will trick TS to pick the ObjectEncodingOptions overload of
+        // ExecFile which correctly types stderr/stdout as Buffer | string.
+        //
+        // Some day someone with more patience than me will contribute an
+        // upstream fix to DefinitelyTyped and we can remove this. It's
+        // essentially https://github.com/DefinitelyTyped/DefinitelyTyped/pull/67202
+        // but for execFile.
+        const encoding = (options?.encoding ?? 'utf8') as BufferEncoding | null
+        const maxBuffer = options ? options.maxBuffer : 10 * 1024 * 1024
 
         const spawnedProcess = execFile(
           gitLocation,
           args,
-          execOptions,
-          function (err: Error | null, stdout, stderr) {
+          { cwd: path, encoding, maxBuffer, env },
+          function (err, stdout, stderr) {
             result.updateProcessEnded()
 
             if (!err) {
@@ -249,7 +310,7 @@ export class GitProcess {
             if (err.message === 'stdout maxBuffer exceeded') {
               reject(
                 new Error(
-                  `The output from the command could not fit into the allocated stdout buffer. Set options.maxBuffer to a larger value than ${execOptions.maxBuffer} bytes`
+                  `The output from the command could not fit into the allocated stdout buffer. Set options.maxBuffer to a larger value than ${maxBuffer} bytes`
                 )
               )
             } else {
@@ -389,15 +450,15 @@ export enum GitTaskCancelResult {
 }
 
 /** This interface represents a git task (process). */
-export interface IGitTask {
+export interface IGitTask<T> {
   /** Result of the git process. */
-  readonly result: Promise<IGitResult>
+  readonly result: Promise<T>
   /** Allows to cancel the process if it's running. Returns true if the process was killed. */
   readonly cancel: () => Promise<GitTaskCancelResult>
 }
 
-class GitTask implements IGitTask {
-  constructor(result: Promise<IGitResult>, pid: Promise<number | undefined>) {
+class GitTask<T> implements IGitTask<T> {
+  constructor(result: Promise<T>, pid: Promise<number | undefined>) {
     this.result = result
     this.pid = pid
     this.processEnded = false
@@ -407,7 +468,7 @@ class GitTask implements IGitTask {
   /** Process may end because process completed or process errored. Either way, we can no longer cancel it. */
   private processEnded: boolean
 
-  result: Promise<IGitResult>
+  result: Promise<T>
 
   public updateProcessEnded(): void {
     this.processEnded = true

test/helpers.ts

@@ -50,9 +50,9 @@ export async function initializeWithRemote(
   return { path: testRepoPath, remote: remotePath }
 }
 
-export function verify(
-  result: IGitResult,
-  callback: (result: IGitResult) => void
+export function verify<T extends IGitResult>(
+  result: T,
+  callback: (result: T) => void
 ) {
   try {
     callback(result)
@@ -61,8 +61,8 @@ export function verify(
       'error encountered while verifying; poking at response from Git:'
     )
     console.log(` - exitCode: ${result.exitCode}`)
-    console.log(` - stdout: ${result.stdout.trim()}`)
-    console.log(` - stderr: ${result.stderr.trim()}`)
+    console.log(` - stdout: ${result.stdout}`)
+    console.log(` - stderr: ${result.stderr}`)
     console.log()
     throw e
   }
@@ -87,9 +87,17 @@ function getFriendlyGitError(gitError: GitError): string {
 
 expect.extend({
   toHaveGitError(result: IGitResult, expectedError: GitError) {
-    let gitError = GitProcess.parseError(result.stderr)
+    let gitError = GitProcess.parseError(
+      Buffer.isBuffer(result.stderr)
+        ? result.stderr.toString('utf8')
+        : result.stderr
+    )
     if (gitError === null) {
-      gitError = GitProcess.parseError(result.stdout)
+      gitError = GitProcess.parseError(
+        Buffer.isBuffer(result.stdout)
+          ? result.stdout.toString('utf8')
+          : result.stdout
+      )
     }
 
     const message = () => {

test/slow/gcm-test.ts

@@ -2,12 +2,16 @@ import { GitProcess } from '../../lib'
 import { gitCredentialManagerVersion } from '../helpers'
 
 describe('git-credential-manager', () => {
-  it('matches the expected version', async () => {
-    const result = await GitProcess.exec(
-      ['credential-manager', '--version'],
-      process.cwd()
-    )
-    expect(result.exitCode).toBe(0)
-    expect(result.stdout).toContain(gitCredentialManagerVersion)
-  })
+  it(
+    'matches the expected version',
+    async () => {
+      const result = await GitProcess.exec(
+        ['credential-manager', '--version'],
+        process.cwd()
+      )
+      expect(result.exitCode).toBe(0)
+      expect(result.stdout).toContain(gitCredentialManagerVersion)
+    },
+    30 * 1000
+  )
 })