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

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

[Avery Pennarun]

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(-)

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

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

[Jeff King]

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

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

[Avery Pennarun]

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(-)

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

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

[Jeff King]

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

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

[Avery Pennarun]

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(-)

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

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

[Junio C Hamano]

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.

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

[Jeff King]

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

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

[Avery Pennarun]

On Thu, May 24, 2012 at 2:34 PM, Jeff King <peff@peff.net> 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? :)

</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

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

[Avery Pennarun]

On Thu, May 24, 2012 at 2:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

> 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

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

[Junio C Hamano]

Avery Pennarun <apenwarr@gmail.com> 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.

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

[Avery Pennarun]

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