Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Conversation

@jmank88
Copy link
Collaborator

@jmank88 jmank88 commented Sep 4, 2017

What does this do / why do we need it?

This PR addresses a TODO fromunwrapVcsErr:

// TODO this is really dumb, lossy, and needs proper handling
func unwrapVcsErr(err error) error {
...

This function was returning errors like <message>: <command output>, and ignoring the available error.
Now it wraps up the error like we've been doing elsewhere: (errors.Wrap(err, out)), and then wraps that error with the message. This way nothing is missing from output, and we get a more complete errors.Wrap trail preserving underlying types, stack traces, etc.

case *vcs.RemoteError:
return fmt.Errorf("%s: %s", verr.Error(), verr.Out())
cause, out, msg := t.Original(), t.Out(), t.Error()
return errors.Wrap(errors.Wrap(cause, out), msg)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some significance to where Wrap is called from, so that wrapping somewhere other than where the error occurred is a bit strange, but I think that Masterminds/vcs would have to incorporate the errors package in order to avoid this, so it's likely nbd.

@sdboyer
Copy link
Member

sdboyer commented Sep 5, 2017

can you save me a minute and point me to a test where i can readily observe the difference in output?

@jmank88
Copy link
Collaborator Author

jmank88 commented Sep 5, 2017

can you save me a minute and point me to a test where i can readily observe the difference in output?

There isn't a test to validate the output format, if that is what you mean. I was just continuing in the spirit of #1102 and #1106, and then hard coded some errors to double check myself.

If you just wanted an example:
Given:

newVcsRemoteErrorOr("this is the error message", errors.New("this is the command error"), "this is some\n\nmessy\nmulti\n\tline\n\ncommand output")

Output diff:

this is the error message: this is some
		
		messy
		multi
			line
		
-		command output
+		command output: this is the command error

@sdboyer sdboyer merged commit a87f8cc into golang:master Sep 5, 2017
@sdboyer
Copy link
Member

sdboyer commented Sep 5, 2017

good enough. we've just gotten these errors wrong a few times before, and i'd like to avoid a repeat of that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants