threads / patch / 30606

patchprogress: don't print if !isatty(2).

Subject: [PATCH] progress: don't print if !isatty(2).

## tl;dr

11 messages between May 24, 2012 and May 24, 2012. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Avery Pennarun· May 24, 2012, 05:18 UTC · lore

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 <apenwarr@gmail.com>
---
 progress.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Show changes to progress.c +4 −2
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· May 24, 2012, 05:45 UTC · re: Avery Pennarun · lore

Re: [PATCH] progress: don't print if !isatty(2).

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...
Show 12 quoted lines
> 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:
Show changes to builtin/checkout.c +2 −3
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· May 24, 2012, 06:05 UTC · re: Jeff King · lore

[PATCH] checkout: default to quiet if !isatty(2).

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 <apenwarr@gmail.com>
---
 builtin/checkout.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/checkout.c +2 −2
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· May 24, 2012, 06:10 UTC · re: Avery Pennarun · lore

Re: [PATCH] checkout: default to quiet if !isatty(2).

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· May 24, 2012, 06:12 UTC · re: Jeff King · lore

[PATCH v2] checkout: no progress messages if !isatty(2).

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 <apenwarr@gmail.com>
---
 builtin/checkout.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/checkout.c +2 −2
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
Junio C Hamano· May 24, 2012, 18:29 UTC · re: Avery Pennarun · lore

Re: [PATCH v2] checkout: no progress messages if !isatty(2).

Avery Pennarun <apenwarr@gmail.com> 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· May 24, 2012, 18:34 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] checkout: no progress messages if !isatty(2).

On Thu, May 24, 2012 at 11:29:52AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> 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· May 24, 2012, 18:49 UTC · re: Jeff King · lore

Re: [PATCH v2] checkout: no progress messages if !isatty(2).

On Thu, May 24, 2012 at 2:34 PM, Jeff King <peff@peff.net> wrote:
Show 15 quoted lines
> 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? :)
</halfkidding>

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
Avery Pennarun· May 24, 2012, 18:46 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] checkout: no progress messages if !isatty(2).

On Thu, May 24, 2012 at 2:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> Avery Pennarun <apenwarr@gmail.com> 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?
Show 7 quoted lines
> 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
Junio C Hamano· May 24, 2012, 21:46 UTC · re: Avery Pennarun · lore

Re: [PATCH v2] checkout: no progress messages if !isatty(2).

Avery Pennarun <apenwarr@gmail.com> writes:
Show 7 quoted lines
>> 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.
Avery Pennarun· May 24, 2012, 06:12 UTC · re: Jeff King · lore

Re: [PATCH] checkout: default to quiet if !isatty(2).

On Thu, May 24, 2012 at 2:10 AM, Jeff King <peff@peff.net> wrote:
Show 13 quoted lines
> 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

← back to recent threads