Include the command and args used to call it in the error object

[ysabri]

This is in reference to my comment on #144.

I feel that the Git command + the args used to call the command should be included in the error object. I find it very useful sometimes when debugging to know what args were used to call the command that errored out, especially for common errors (#144 explains what a common error is).

For example if I'm iterating over a list of branches to get their logs and one of them errors out with a common error. The only way for me to find out which branch caused the error is to catch the error in the function and print out the branch name right there. This can get annoying since I can handle all errors in one place at a higher level if the information was included already.

Another thing I can think of, this can also be helpful in logging errors given that exec is asynchronous.

[shiftkey]

@ysabri i'm curious to explore this, but I'd love an example snippet to start from that's dugite focused.

GitProcess.exec will happily return you a failing Git operation (with the exit code set), so doing something like this doesn't feel that compelling, based on what you've provided:

const testRepoPath = temp.mkdirSync('desktop-git-test-blank')
const args = ['branch', 'branch-does-not-exist']

const result = await GitProcess.exec(args, testRepoPath)

expect(result.args).toEqual(args)
expect(result.path).toBe(testRepoPath)

But maybe we could generalize some of the error handling infrastructure in GitHub Desktop and pull that into dugite.

For example, we have this error type in GitHub Desktop:

export class GitError extends Error {
  /** The result from the failed command. */
  public readonly result: IGitResult

  /** The args for the failed command. */
  public readonly args: ReadonlyArray<string>

  public constructor(result: IGitResult, args: ReadonlyArray<string>) {
    super(getResultMessage(result))

    this.name = 'GitError'
    this.result = result
    this.args = args
  }
}

Which we use to throw if we aren't able to convert the error code into a meaningful app-specific error code: https://github.com/desktop/desktop/blob/1b69c2f04bd55afbe363821dccd0a3966c40e509/app/src/lib/git/core.ts#L166

But we also have a bunch of additional infrastructure about expected exit codes and you might not need the full experience here...

[ysabri]

Yes, that is exactly what I was talking about. I like how you guys have the args in GitError type in GitHub Desktop.

In fact I really liked what you guys did in GitHub Desktop, so for my project I did something very similar but a little more general for my use case.

Keep in mind the code below is still work in progress :)
Here: https://github.com/ysabri/GitDrive/blob/master/src/git-drive/git/core-git.ts#L12
And here: https://github.com/ysabri/GitDrive/blob/master/src/git-drive/git/core-git.ts#L51

If you want to generalize the approach I think Dugite should also include resolving where the error came from. The problem is Git returns errors either in stdout or stderr, which explains the additional infrastructure you guys did in GitHub Desktop.

What I don't like in Dugite is that the responsibility of figuring out where the error is falls on the caller. This can be handled/abstracted by Dugite based on regex matching (like you do already) or based on matching error codes to operations.

I hope this helps and excuse any misunderstanding, it has been a while since I have worked on this

[niik]

I can see this argued either way but our current philosophy for dugite is to move towards less abstractions and not more. Frankly I'm not convinced that the error parsing regexes in dugite even belong here. We don't have any decent test coverage to ensure that the expressions still match the errors that Git throws. The only chance we have to detect it is if GitHub Desktop's test suite notices it and Desktop doesn't use half of the errors defined in dugite.

So for now our call here is that we won't be adding any higher level error abstractions to dugite.

[niik]

Okay so I'm not one to not admit when I'm full of crap. When adding support for AbortSignal in #590 introducing a new error which encapsulates details of the execution (stderr, stdout, signal etc) was the only way to go. You were right, and I was wrong though I stand by the general principle that dugite should be doing less, not more 😄

[ysabri]

@niik Thanks for coming back to this and mentioning your experience. I see both approaches come down to the needs of Dugite users. The GitHub Desktop and my project needed what I talked about, so I thought it was worth introducing into the library.