{"action":"created","issue":{"url":"https://api.github.com/repos/vanadium/issues/issues/1128","repository_url":"https://api.github.com/repos/vanadium/issues","labels_url":"https://api.github.com/repos/vanadium/issues/issues/1128/labels{/name}","comments_url":"https://api.github.com/repos/vanadium/issues/issues/1128/comments","events_url":"https://api.github.com/repos/vanadium/issues/issues/1128/events","html_url":"https://github.com/vanadium/issues/issues/1128","id":131514278,"number":1128,"title":"gosh: passing t.Fatalf to Shell.Opts.Fatalf gives unintuitive output","user":{"login":"asadovsky","id":538767,"avatar_url":"https://avatars.githubusercontent.com/u/538767?v=3","gravatar_id":"","url":"https://api.github.com/users/asadovsky","html_url":"https://github.com/asadovsky","followers_url":"https://api.github.com/users/asadovsky/followers","following_url":"https://api.github.com/users/asadovsky/following{/other_user}","gists_url":"https://api.github.com/users/asadovsky/gists{/gist_id}","starred_url":"https://api.github.com/users/asadovsky/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/asadovsky/subscriptions","organizations_url":"https://api.github.com/users/asadovsky/orgs","repos_url":"https://api.github.com/users/asadovsky/repos","events_url":"https://api.github.com/users/asadovsky/events{/privacy}","received_events_url":"https://api.github.com/users/asadovsky/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"asadovsky","id":538767,"avatar_url":"https://avatars.githubusercontent.com/u/538767?v=3","gravatar_id":"","url":"https://api.github.com/users/asadovsky","html_url":"https://github.com/asadovsky","followers_url":"https://api.github.com/users/asadovsky/followers","following_url":"https://api.github.com/users/asadovsky/following{/other_user}","gists_url":"https://api.github.com/users/asadovsky/gists{/gist_id}","starred_url":"https://api.github.com/users/asadovsky/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/asadovsky/subscriptions","organizations_url":"https://api.github.com/users/asadovsky/orgs","repos_url":"https://api.github.com/users/asadovsky/repos","events_url":"https://api.github.com/users/asadovsky/events{/privacy}","received_events_url":"https://api.github.com/users/asadovsky/received_events","type":"User","site_admin":false},"milestone":null,"comments":1,"created_at":"2016-02-05T00:13:11Z","updated_at":"2016-02-19T20:06:21Z","closed_at":null,"body":"The line number that gets printed is misleading (it's the place where t.Fatalf was passed, not where it's called), you don't get a stack trace, etc. We need to improve usability here somehow."},"comment":{"url":"https://api.github.com/repos/vanadium/issues/issues/comments/186382818","html_url":"https://github.com/vanadium/issues/issues/1128#issuecomment-186382818","issue_url":"https://api.github.com/repos/vanadium/issues/issues/1128","id":186382818,"user":{"login":"asadovsky","id":538767,"avatar_url":"https://avatars.githubusercontent.com/u/538767?v=3","gravatar_id":"","url":"https://api.github.com/users/asadovsky","html_url":"https://github.com/asadovsky","followers_url":"https://api.github.com/users/asadovsky/followers","following_url":"https://api.github.com/users/asadovsky/following{/other_user}","gists_url":"https://api.github.com/users/asadovsky/gists{/gist_id}","starred_url":"https://api.github.com/users/asadovsky/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/asadovsky/subscriptions","organizations_url":"https://api.github.com/users/asadovsky/orgs","repos_url":"https://api.github.com/users/asadovsky/repos","events_url":"https://api.github.com/users/asadovsky/events{/privacy}","received_events_url":"https://api.github.com/users/asadovsky/received_events","type":"User","site_admin":false},"created_at":"2016-02-19T20:06:21Z","updated_at":"2016-02-19T20:06:21Z","body":"+cc @tatatodd @cosnicolaou \r\n\r\nSo, the problem with `Fatalf` and `Logf` is that the intuitive use of these in tests, `gosh.NewShell(gosh.Opts{Fatalf: t.Fatalf, Logf: t.Logf})`, results in incorrect line numbers, which is really bad for usability.\r\n\r\nI filed this Go issue, but I am not confident that it's fixable: https://github.com/golang/go/issues/14415\r\n\r\nI propose that we switch the API from taking `Fatalf` and `Logf` to taking `FailNow` and `LogLevel` arguments (exact names to be discussed), with the following semantics:\r\n- If `FailNow` is not specified, it defaults to `panic`, or perhaps to `runtime.Goexit` (https://golang.org/pkg/runtime/#Goexit). If it's overridden to `nil` after constructing a `gosh.Shell`, failures do not panic. For tests, clients would typically set `FailNow: t.FailNow` (https://golang.org/pkg/testing/#T.FailNow).\r\n- `LogLevel` is an enum with allowed values `Info`, `Fatal`, and `Silent`, defaulting to `Info` (since gosh intends to default to usability over performance, while providing an escape hatch for performance).\r\n  + In `Silent` mode, we log nothing.\r\n  + In `Fatal` mode, we log the current runtime stack before calling `FailNow`, and that's it. \r\n  + In `Info` mode, we log everything from `Fatal`, along with that's currently logged in gosh (which isn't much), along with the complete stdout/stderr of any failed command (or perhaps just the first and last N bytes), as per #1127.\r\n\r\nThoughts?"}}