-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tasklog: don't log progress status when stdout is not a tty #3349
tasklog: don't log progress status when stdout is not a tty #3349
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks good. Thanks for the patch, and welcome to Git LFS!
There are a few things we should probably fix that I've mentioned below. I'm also interested to know how this will work on Windows both with and without Git Bash.
docs/man/git-lfs-config.5.ronn
Outdated
|
||
Controls whether Git LFS will suppress progress status when the | ||
standard output stream is not attached to a terminal. The default is `false` | ||
which make Git LFS detect whether stdout is a terminal and suppress progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: ”make“ instead of ”makes”
@@ -13,6 +14,8 @@ github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NH | |||
github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8= | |||
github.com/kr/pty v0.0.0-20150511174710-5cf931ef8f76 h1:i5TIRQpbCg4aJMUtVHIhkQnSw++Z405Z5pzqHqeNkdU= | |||
github.com/kr/pty v0.0.0-20150511174710-5cf931ef8f76/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= | |||
github.com/mattn/go-isatty v0.0.4 h1:bnP0vzxcAdeI1zdubAl5PjU6zsERjGZb7raWodagDYs= | |||
github.com/mattn/go-isatty v0.0.4/go.mod h1:M+lRXTBqGeGNdLjl/ufCoiOlB5xdOkqRJdNxMWT7Zi4= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally want to vendor these dependencies. Could you run make vendor
to pull those into the tree?
tasklog/log.go
Outdated
// environment variables | ||
type Environment interface { | ||
GetBool(key string, def bool) (val bool) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this used anywhere. Should it be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @steffengodskesen, thanks for sending this along -- and welcome to Git LFS! I agree with what @bk2204 has recommended thus far, and I have a couple of additional suggestions of my own. Let me know what you think of them, and I'd be happy to share my thoughts as well.
tasklog/log.go
Outdated
func NewLogger(sink io.Writer) *Logger { | ||
// current terminal width as the width of the line. Will log progress status if | ||
// stdout is a terminal or if forceProgress is true | ||
func NewLogger(sink io.Writer, forceProgress bool) *Logger { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it may be good to adopt the same pattern of option functions here, too, even though there is only one option that we'd be supporting. I suggest this, because I think the "extra baggage" of option functions is worth the tradeoff to remove a boolean argument. Perhaps:
type optionFn func(l *Logger)
var (
forced optionFn = func(l *Logger) {
l.forceProgress = true
}
)
// NB: this prevents mutating the underlying variable, which _can_ be changed at runtime.
func Forced() optionFn { return forced }
func NewLogger(sink io.Writer, fns ...optionFn) {
l := &Logger{...}
for _, fn := range fns {
fn(l)
}
// ...
}
Then to call this (with forcing) you would instead say:
l := tasklog.NewLogger(os.Stderr, tasklog.Forced())
I think that you would want a helper function that calls that ☝️, since it removes the convenience of simply passing along cfg.ForceProgress()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Nice pattern; hadn't seen that before. Thanks for the pointer.
I'm not sure I understand the "NB: this prevents mutating the underlying variable" comment, though.
@@ -214,6 +227,9 @@ func (l *Logger) logTask(task Task) { | |||
|
|||
var update *Update | |||
for update = range task.Updates() { | |||
if !isatty.IsTerminal(os.Stdout.Fd()) && !l.forceProgress { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does isatty
cache this result? If not, and if it's expensive to compute, we should cache it ourselves, since this loop is expected to run many times.
On another note, I don't think that this is quite what we want: indeed this checks whether we can write to stdout, but stdout is not always the io.Writer
we are writing to. Could you instead say:
type Logger struct {
ttyOnce sync.Once
tty bool
}
func (l *Logger) tty() bool {
l.ttyOnce.Do(func() {
if f, ok := l.sink.(interface{ // <- could use `*os.File`, too
Fd() uintptr
}); ok {
l.tty = isatty.IsTerminal(f.Fd())
return
}
})
return l.tty
}
And then rewrite this line to:
if !(l.tty() || l.forceProgress) {
continue
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the (possibly non-existing) Fd()
on the actual Writer
makes good sense. Won't work in the cases where the sink
is a io.MultiWriter
(command_fetch.go
and command_prune.go
does this), but I'll try to come up with a solution to that. Maybe wrapping the io.MultiWriter
in a type that exposes the Fd()
of its first writer.
The sink
won't change during the lifetime of the logger, so could we not cache result of the call to isatty
on instantiation of the logger?
But make it overridable by either - setting lfs.forceprogress 1 - setting GIT_LFS_FORCE_PROGRESS=1
695a1b1
to
b40c5cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great; thanks for addressing all of mine and @bk2204's changes!
To avoid filling logs with progress status when running git/git-lfs inside tools and services where the output does not go to a terminal.
The behaviour is overridable by either
lfs.forceprogress 1
GIT_LFS_FORCE_PROGRESS=1
Ideally this should be controllable by command line args (like
git push --progress
) but passing command line args down to filters and hooks is not really an option.