# [PATCH] progress: don't print if !isatty(2). 11 messages from 2012-05-24 to 2012-05-24. Participants: Avery Pennarun, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/30606 ## Avery Pennarun, 2012-05-24 05:18 Subject: [PATCH] progress: don't print if !isatty(2). Message-ID: <1337836732-26778-1-git-send-email-apenwarr@gmail.com> URL: https://gitlist.dev/e/1337836732-26778-1-git-send-email-apenwarr%40gmail.com ``` If stderr isn't a tty, we shouldn't be printing incremental progress messages. In particular, this affected 'git checkout -f . >&logfile' unless you provided -q. And git-new-workdir has no way to provide -q. Signed-off-by: Avery Pennarun --- progress.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/progress.c b/progress.c index 3971f49..4d9f416 100644 --- a/progress.c +++ b/progress.c @@ -211,9 +211,11 @@ int display_progress(struct progress *progress, unsigned n) struct progress *start_progress_delay(const char *title, unsigned total, unsigned percent_treshold, unsigned delay) { - struct progress *progress = malloc(sizeof(*progress)); + struct progress *progress = NULL; + if (isatty(2)) + progress = malloc(sizeof(*progress)); if (!progress) { - /* unlikely, but here's a good fallback */ + /* use a simple fallback */ fprintf(stderr, "%s...\n", title); fflush(stderr); return NULL; -- 1.7.9.dirty ``` ## Jeff King, 2012-05-24 05:45 Subject: Re: [PATCH] progress: don't print if !isatty(2). Message-ID: <20120524054506.GA3440@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120524054506.GA3440%40sigill.intra.peff.net In-Reply-To: <1337836732-26778-1-git-send-email-apenwarr@gmail.com> ``` On Thu, May 24, 2012 at 01:18:52AM -0400, Avery Pennarun wrote: > If stderr isn't a tty, we shouldn't be printing incremental progress > messages. In particular, this affected 'git checkout -f . >&logfile' unless > you provided -q. And git-new-workdir has no way to provide -q. Makes sense to fix checkout, but... > diff --git a/progress.c b/progress.c > index 3971f49..4d9f416 100644 > --- a/progress.c > +++ b/progress.c > @@ -211,9 +211,11 @@ int display_progress(struct progress *progress, unsigned n) > struct progress *start_progress_delay(const char *title, unsigned total, > unsigned percent_treshold, unsigned delay) > { > - struct progress *progress = malloc(sizeof(*progress)); > + struct progress *progress = NULL; > + if (isatty(2)) > + progress = malloc(sizeof(*progress)); This is the wrong place to put the fix. The user might have asked git to override the isatty(2) check and show progress anyway (e.g., "git push --progress"), and this would break that case. The fix has to go in builtin/checkout.c, and probably looks like this: diff --git a/builtin/checkout.c b/builtin/checkout.c index 3ddda34..e8c1b1f 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -343,7 +343,7 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree) opts.reset = 1; opts.merge = 1; opts.fn = oneway_merge; - opts.verbose_update = !o->quiet; + opts.verbose_update = !o->quiet && isatty(2); opts.src_index = &the_index; opts.dst_index = &the_index; parse_tree(tree); @@ -420,7 +420,7 @@ static int merge_working_tree(struct checkout_opts *opts, topts.update = 1; topts.merge = 1; topts.gently = opts->merge && old->commit; - topts.verbose_update = !opts->quiet; + topts.verbose_update = !opts->quiet && isatty(2); topts.fn = twoway_merge; if (opts->overwrite_ignore) { topts.dir = xcalloc(1, sizeof(*topts.dir)); but I did not test it. -Peff ``` ## Avery Pennarun, 2012-05-24 06:05 Subject: [PATCH] checkout: default to quiet if !isatty(2). Message-ID: <1337839534-7760-1-git-send-email-apenwarr@gmail.com> URL: https://gitlist.dev/e/1337839534-7760-1-git-send-email-apenwarr%40gmail.com In-Reply-To: <20120524054506.GA3440@sigill.intra.peff.net> ``` It would probably be better to have progress.c check isatty(2) all the time, but that wouldn't allow things like 'git push --progress' to force progress reporting to on, so I won't try to solve the general case right now. Actual fix suggested by Jeff King. Signed-off-by: Avery Pennarun --- builtin/checkout.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index f1984d9..4ee833a 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -343,7 +343,7 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree) opts.reset = 1; opts.merge = 1; opts.fn = oneway_merge; - opts.verbose_update = !o->quiet; + opts.verbose_update = !o->quiet && isatty(2); opts.src_index = &the_index; opts.dst_index = &the_index; parse_tree(tree); @@ -420,7 +420,7 @@ static int merge_working_tree(struct checkout_opts *opts, topts.update = 1; topts.merge = 1; topts.gently = opts->merge && old->commit; - topts.verbose_update = !opts->quiet; + topts.verbose_update = !opts->quiet && isatty(2); topts.fn = twoway_merge; if (opts->overwrite_ignore) { topts.dir = xcalloc(1, sizeof(*topts.dir)); -- 1.7.9.dirty ``` ## Jeff King, 2012-05-24 06:10 Subject: Re: [PATCH] checkout: default to quiet if !isatty(2). Message-ID: <20120524061000.GA14035@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120524061000.GA14035%40sigill.intra.peff.net In-Reply-To: <1337839534-7760-1-git-send-email-apenwarr@gmail.com> ``` On Thu, May 24, 2012 at 02:05:34AM -0400, Avery Pennarun wrote: > It would probably be better to have progress.c check isatty(2) all the time, > but that wouldn't allow things like 'git push --progress' to force progress > reporting to on, so I won't try to solve the general case right now. This looks better. There is a slight inaccuracy in your subject line, though. We are not defaulting to quiet if !isatty(2) in all cases, but rather only when we call into unpack_trees, which generates the progress output. We will still print the ahead/behind line, detached HEAD info, etc. Which I think is the right behavior, but is not quite what is advertised by your commit message. -Peff ``` ## Avery Pennarun, 2012-05-24 06:12 Subject: [PATCH v2] checkout: no progress messages if !isatty(2). Message-ID: <1337839944-4651-1-git-send-email-apenwarr@gmail.com> URL: https://gitlist.dev/e/1337839944-4651-1-git-send-email-apenwarr%40gmail.com In-Reply-To: <20120524061000.GA14035@sigill.intra.peff.net> ``` It would probably be better to have progress.c check isatty(2) all the time, but that wouldn't allow things like 'git push --progress' to force progress reporting to on, so I won't try to solve the general case right now. Actual fix suggested by Jeff King. Signed-off-by: Avery Pennarun --- builtin/checkout.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index f1984d9..4ee833a 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -343,7 +343,7 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree) opts.reset = 1; opts.merge = 1; opts.fn = oneway_merge; - opts.verbose_update = !o->quiet; + opts.verbose_update = !o->quiet && isatty(2); opts.src_index = &the_index; opts.dst_index = &the_index; parse_tree(tree); @@ -420,7 +420,7 @@ static int merge_working_tree(struct checkout_opts *opts, topts.update = 1; topts.merge = 1; topts.gently = opts->merge && old->commit; - topts.verbose_update = !opts->quiet; + topts.verbose_update = !opts->quiet && isatty(2); topts.fn = twoway_merge; if (opts->overwrite_ignore) { topts.dir = xcalloc(1, sizeof(*topts.dir)); -- 1.7.9.dirty ``` ## Avery Pennarun, 2012-05-24 06:12 Subject: Re: [PATCH] checkout: default to quiet if !isatty(2). Message-ID: URL: https://gitlist.dev/e/CAHqTa-3UAgSXEH3XA7fdJBt%2B6t33xdbiaPcxAEqiOP_vwgVd3g%40mail.gmail.com In-Reply-To: <20120524061000.GA14035@sigill.intra.peff.net> ``` On Thu, May 24, 2012 at 2:10 AM, Jeff King wrote: > On Thu, May 24, 2012 at 02:05:34AM -0400, Avery Pennarun wrote: >> It would probably be better to have progress.c check isatty(2) all the time, >> but that wouldn't allow things like 'git push --progress' to force progress >> reporting to on, so I won't try to solve the general case right now. > > This looks better. There is a slight inaccuracy in your subject line, > though. We are not defaulting to quiet if !isatty(2) in all cases, but > rather only when we call into unpack_trees, which generates the progress > output. We will still print the ahead/behind line, detached HEAD info, > etc. > > Which I think is the right behavior, but is not quite what is advertised > by your commit message. Good point, fixed. Thanks! Avery ``` ## Junio C Hamano, 2012-05-24 18:29 Subject: Re: [PATCH v2] checkout: no progress messages if !isatty(2). Message-ID: <7vy5ohwhy7.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy5ohwhy7.fsf%40alter.siamese.dyndns.org In-Reply-To: <1337839944-4651-1-git-send-email-apenwarr@gmail.com> ``` Avery Pennarun writes: > It would probably be better to have progress.c check isatty(2) all the time, > but that wouldn't allow things like 'git push --progress' to force progress > reporting to on, so I won't try to solve the general case right now. Before that "It would probably be better" comment to give your opinion, you need to describe what problem you wanted to solve in the first place. I'll lift it from your original version of the patch: If stderr isn't a tty, we shouldn't be printing incremental progress messages. In particular, this affected 'git checkout -f . >&logfile' unless you provided -q. And git-new-workdir has no way to provide -q. I do not seem to find a sane justification for git $cmd --progress 2>output use case and I do not immediately see how that "output" file can be useful. But we've allowed it for a long time, so probably this version is safer. Besides, it is more explicit. Thanks. ``` ## Jeff King, 2012-05-24 18:34 Subject: Re: [PATCH v2] checkout: no progress messages if !isatty(2). Message-ID: <20120524183457.GA11841@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120524183457.GA11841%40sigill.intra.peff.net In-Reply-To: <7vy5ohwhy7.fsf@alter.siamese.dyndns.org> ``` On Thu, May 24, 2012 at 11:29:52AM -0700, Junio C Hamano wrote: > I do not seem to find a sane justification for > > git $cmd --progress 2>output > > use case and I do not immediately see how that "output" file can be > useful. But we've allowed it for a long time, so probably this version is > safer. Besides, it is more explicit. Actually, I ran across a case of this just recently. If you are writing a graphical interface that wraps git, scraping "--progress" output from a pipe is the only way you can provide a progress meter within your interface. That is what the "GitHub for {Mac,Windows}" interfaces do (they also use libgit2 where possible, but it is far from feature complete). -Peff ``` ## Avery Pennarun, 2012-05-24 18:46 Subject: Re: [PATCH v2] checkout: no progress messages if !isatty(2). Message-ID: URL: https://gitlist.dev/e/CAHqTa-3QUsW_AP67NWjc-Gu5FZ7xQZyOOM-%3Dzea%2BvwZeT79%3D0A%40mail.gmail.com In-Reply-To: <7vy5ohwhy7.fsf@alter.siamese.dyndns.org> ``` On Thu, May 24, 2012 at 2:29 PM, Junio C Hamano wrote: > Avery Pennarun writes: >> It would probably be better to have progress.c check isatty(2) all the time, >> but that wouldn't allow things like 'git push --progress' to force progress >> reporting to on, so I won't try to solve the general case right now. > > Before that "It would probably be better" comment to give your opinion, > you need to describe what problem you wanted to solve in the first place. > I'll lift it from your original version of the patch: > >    If stderr isn't a tty, we shouldn't be printing incremental progress >    messages.  In particular, this affected 'git checkout -f . >&logfile' >    unless you provided -q.  And git-new-workdir has no way to provide -q. Do you want me to rephrase the commit message and resend? > I do not seem to find a sane justification for > >        git $cmd --progress 2>output > > use case and I do not immediately see how that "output" file can be > useful.  But we've allowed it for a long time, so probably this version is > safer.  Besides, it is more explicit. Yeah, I have nothing against allowing --progress to work. If I were to clarify my comment above, it would be to say that I'm worried about how *many* places we keep calling isatty(). It is (as we can see from the need for this patch) error prone, since I think most naive coders would expect the progress stuff to act correctly by default if !isatty(2). So maybe the "right" fix is to add a flag to start_progress_delay() to "force" verbose mode; if it's not set, start_progress_delay() would check isatty(2) and decide automatically what to do. This wouldn't save much code, but would make sure developers think about their intentions. Have fun, Avery ``` ## Avery Pennarun, 2012-05-24 18:49 Subject: Re: [PATCH v2] checkout: no progress messages if !isatty(2). Message-ID: URL: https://gitlist.dev/e/CAHqTa-0y0ETeq2G2FPMWgAwkXF_LSLN%3DVqnijF-m3q2YT0z4mQ%40mail.gmail.com In-Reply-To: <20120524183457.GA11841@sigill.intra.peff.net> ``` On Thu, May 24, 2012 at 2:34 PM, Jeff King wrote: > On Thu, May 24, 2012 at 11:29:52AM -0700, Junio C Hamano wrote: >> I do not seem to find a sane justification for >> >>       git $cmd --progress 2>output >> >> use case and I do not immediately see how that "output" file can be >> useful.  But we've allowed it for a long time, so probably this version is >> safer.  Besides, it is more explicit. > > Actually, I ran across a case of this just recently. If you are writing > a graphical interface that wraps git, scraping "--progress" output from > a pipe is the only way you can provide a progress meter within your > interface. That is what the "GitHub for {Mac,Windows}" interfaces do > (they also use libgit2 where possible, but it is far from feature > complete). This is why we have ptys, isn't it? :) FWIW, in bup we use environment variables for this. bup's main program automatically redirects stderr to a pipe (to keep overlapping status messages from interfering with each other) and the subcommands need to know that stderr "was" a tty. Arguably, an environment variable is a better place for this since a script would presumably want progress messages or not, globally. It would also have solved the problem where git-new-worktree doesn't have a --quiet option. Have fun, Avery ``` ## Junio C Hamano, 2012-05-24 21:46 Subject: Re: [PATCH v2] checkout: no progress messages if !isatty(2). Message-ID: <7v62blw8u6.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v62blw8u6.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Avery Pennarun writes: >> I'll lift it from your original version of the patch: >> >>    If stderr isn't a tty, we shouldn't be printing incremental progress >>    messages.  In particular, this affected 'git checkout -f . >&logfile' >>    unless you provided -q.  And git-new-workdir has no way to provide -q. > > Do you want me to rephrase the commit message and resend? No need, unless you want to say something vastly different from the above. Thanks. ```