feat: add gh.ExecInteractive()

[stemar94]

For #114

[stemar94]

Could also be nice to not wrap the error, if it's a proper exec.ExitError.

[samcoe]

@stemar94 This is looking good so far. What would be the reason not to wrap an exec.ExitError? Go provides mechanisms to unwrap it if the user wants to.

[mislav]

Thanks for this!

[mislav]

I understand that this basically copies the error message from Exec(), but we could maybe improve this error message in both functions.

The original error from exec.LookPath (which safeexec uses under the hood) will be something like:

exec: "gh": executable file not found in $PATH

After being wrapped, the full message will be:

could not find gh executable in PATH. error: exec: "gh": executable file not found in $PATH

I don't think the extra explanation added by wrapping adds any value here. I'd vote to avoid wrapping this error message altogether (and in the Exec() function too).

[stemar94]

Done

[mislav]

I don't mind that the original ExitError is wrapped here. The extra context makes it clear that it was the gh command that failed, and not code from the extension itself. Perhaps we can just make it a lighter wrapper:

Suggested change

	err = fmt.Errorf("failed to run gh. error: %w", err)
	err = fmt.Errorf("gh: %w", err)

[stemar94]

I went for fmt.Errorf("gh execution failed: %w", err) for both Execs.

[mislav]

Since this is meant to spawn a potentially longer-running process, this function could accept a context.Context instance so that the caller has an option to kill the gh process when it needs to.

[stemar94]

Done, also added a test.

[stemar94]

@stemar94 This is looking good so far. What would be the reason not to wrap an exec.ExitError? Go provides mechanisms to unwrap it if the user wants to.

I like for the ExecInteractive to happen transparently, where I like to hand over responsibility to the actual gh.
So it would be nice, if I could easily forward the exit code aswell.

Without unwrapping:

if execErr := gh.ExecInteractive(); execErr != nil {
  if exitErr, ok := execErr.(*exec.ExitError); ok {
    os.Exit(exitErr.ExitCode())
  }
}

With unwrapping:

if err := gh.ExecInteractive(); err != nil {
  if execErr := errors.Unwrap(err); execErr != nil {
    if exiterr, ok := execErr.(*exec.ExitError); ok {
      os.Exit(exiterr.ExitCode())
    }
  }
}

Also what does the one layer of wrapping give us in value?

[mislav]

With unwrapping:

if err := gh.ExecInteractive(); err != nil {
  if execErr := errors.Unwrap(err); execErr != nil {
    if exiterr, ok := execErr.(*exec.ExitError); ok {
      os.Exit(exiterr.ExitCode())
    }
  }
}

Go callers that handle specific error types should neither assume that the error value directly returned is a specific type nor manually call Unwrap to get to the desired type. Instead, they should use errors.As() to match a specific type:

if err := gh.ExecInteractive(); err != nil {
  var exitError *exec.ExitError
  if errors.As(err, &exitError) {
    os.Exit(exitError.ExitCode())
  }
}

This way, it doesn't matter whether err is literally a *exec.ExitError or a wrapped *exec.ExitError. It will just work.

[mislav]

This looks good! Feel free to mark your PR as "ready" when you get the test right so we can proceed to the final review. Thank you

[mislav]

Pushed some tweaks, including a new ExecContext function (to complement Exec) and added support for the GH_PATH environment variable that the parent gh process can set to inform extensions about the absolute location of gh on disk, in case gh isn't in PATH.