Handle terminfo envvar #2
Conversation
Signed-off-by: Ian Campbell <[email protected]>
- Checking if the file is unreadable is not needed, just try to open and read and handle the error. - Upon success just return the result rather than breaking from the loop and falling through, which requires more complex error handling. - Upon overall failure return the error directly. There is no need to fmt.Sprintf the static string argument to errors.New() This does not yet correctly handle the case where $TERMINFO contains a value. Signed-off-by: Ian Campbell <[email protected]>
Signed-off-by: Ian Campbell <[email protected]>
https://linux.die.net/man/5/terminfo says: > If the environment variable TERMINFO is set, it is interpreted as the > pathname of a directory containing the compiled description you are working > on. Only that directory is searched. I'd have preferred a more authoritative/OS neutral reference (such as http://pubs.opengroup.org/onlinepubs/7908799/xcurses/terminfo.html) but was unable to find one. Signed-off-by: Ian Campbell <[email protected]>
gotty.go
Outdated
| termloc := os.Getenv("TERMINFO") | ||
| if len(termloc) == 0 { | ||
| if termloc := os.Getenv("TERMINFO"); len(termloc) > 0 { | ||
| path := filepath.Join(termloc, string(termName[0]), termName) |
There was a problem hiding this comment.
This is only used on Linux, so perhaps path.Join() instead of filepath.Join()?
There was a problem hiding this comment.
I think using the thing which is semantically correct everywhere is the right thing to do even if it isn't strictly necessary for a given case. Unless there is some massive performance overhead of filepath vs path but I doubt that.
There was a problem hiding this comment.
My worry is that when running in, e.g. git bash, I think it would pick back-slashes as a separator, but the environment expects forward slashes. It's a change in behaviour at least (existing code hard-codes /, and everything else assumes forward slashes)
@jhowardmsft @rneugeba to the rescue on what is used in that situation 😊
There was a problem hiding this comment.
One more thing; OpenTermInfo("") will panic. Given that this function is exported, I suggest to add a check for this
if termName == "" {
nil, errors.New("No terminfo file(-location) found")
}There was a problem hiding this comment.
Ah, because of the termName[0]. Will push another fixup, thanks
Using filepath switches from `/` to `\` separators on some platforms which might be unexpected, so defer such a change to another time/discussion. Signed-off-by: Ian Campbell <[email protected]>
|
See you just pushed; I just added another comment #2 (comment) 😂 |
Signed-off-by: Ian Campbell <[email protected]>
|
I'll leave squashing until @Nvveen requests it. |
|
@ijc @thaJeztah @Nvveen any luck to merge this? |
|
For Docker/Moby, we replaced this dependency with https://github.com/morikuni/aec, which is slightly more maintained, and was already in use by some other tools (see moby/moby#38398) |
As reported in moby/moby#32400 (née docker/for-mac#1515) setting
$TERMINFOleads to a panic becauseOpenTermInfohas no code to handle this case.This PR cleans up a bunch of stuff in that function and adds the missing case for when
$TERMINFOis set.NB I also include ijc@f565047 which is also in #1 so that
go fmt -sis clean.