{"thread":{"id":"30606","subject":"[PATCH] progress: don't print if !isatty(2).","startedAt":"2012-05-24T05:18:52Z","lastAt":"2012-05-24T21:46:41Z","messageCount":11,"participants":["Avery Pennarun","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"192046","messageId":"1337836732-26778-1-git-send-email-apenwarr@gmail.com","threadId":"30606","inReplyTo":null,"subject":"[PATCH] progress: don't print if !isatty(2).","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2012-05-24T05:18:52Z","receivedAt":"2012-05-24T05:18:52Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"If stderr isn't a tty, we shouldn't be printing incremental progress\nmessages. In particular, this affected 'git checkout -f . >&logfile' unless\nyou provided -q. And git-new-workdir has no way to provide -q.\n\nSigned-off-by: Avery Pennarun \n---\n progress.c | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 3971f49..4d9f416 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -211,9 +211,11 @@ int display_progress(struct progress *progress, unsigned n)\n struct progress *start_progress_delay(const char *title, unsigned total,\n \t\t\t\t unsigned percent_treshold, unsigned delay)\n {\n-\tstruct progress *progress = malloc(sizeof(*progress));\n+\tstruct progress *progress = NULL;\n+\tif (isatty(2))\n+\t\tprogress = malloc(sizeof(*progress));\n \tif (!progress) {\n-\t\t/* unlikely, but here's a good fallback */\n+\t\t/* use a simple fallback */\n \t\tfprintf(stderr, \"%s...\\n\", title);\n \t\tfflush(stderr);\n \t\treturn NULL;\n-- \n1.7.9.dirty\n"},{"id":"192048","messageId":"20120524054506.GA3440@sigill.intra.peff.net","threadId":"30606","inReplyTo":"1337836732-26778-1-git-send-email-apenwarr@gmail.com","subject":"Re: [PATCH] progress: don't print if !isatty(2).","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T05:45:06Z","receivedAt":"2012-05-24T05:45:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 24, 2012 at 01:18:52AM -0400, Avery Pennarun wrote:\n\n> If stderr isn't a tty, we shouldn't be printing incremental progress\n> messages. In particular, this affected 'git checkout -f . >&logfile' unless\n> you provided -q. And git-new-workdir has no way to provide -q.\n\nMakes sense to fix checkout, but...\n\n> diff --git a/progress.c b/progress.c\n> index 3971f49..4d9f416 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -211,9 +211,11 @@ int display_progress(struct progress *progress, unsigned n)\n> struct progress *start_progress_delay(const char *title, unsigned total,\n> \t\t\t\t unsigned percent_treshold, unsigned delay)\n> {\n> -\tstruct progress *progress = malloc(sizeof(*progress));\n> +\tstruct progress *progress = NULL;\n> +\tif (isatty(2))\n> +\t\tprogress = malloc(sizeof(*progress));\n\nThis is the wrong place to put the fix. The user might have asked git to\noverride the isatty(2) check and show progress anyway (e.g., \"git push\n--progress\"), and this would break that case.\n\nThe fix has to go in builtin/checkout.c, and probably looks like this:\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3ddda34..e8c1b1f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -343,7 +343,7 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)\n \topts.reset = 1;\n \topts.merge = 1;\n \topts.fn = oneway_merge;\n-\topts.verbose_update = !o->quiet;\n+\topts.verbose_update = !o->quiet && isatty(2);\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n \tparse_tree(tree);\n@@ -420,7 +420,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\ttopts.update = 1;\n \t\ttopts.merge = 1;\n \t\ttopts.gently = opts->merge && old->commit;\n-\t\ttopts.verbose_update = !opts->quiet;\n+\t\ttopts.verbose_update = !opts->quiet && isatty(2);\n \t\ttopts.fn = twoway_merge;\n \t\tif (opts->overwrite_ignore) {\n \t\t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n\nbut I did not test it.\n\n-Peff\n"},{"id":"192051","messageId":"1337839534-7760-1-git-send-email-apenwarr@gmail.com","threadId":"30606","inReplyTo":"20120524054506.GA3440@sigill.intra.peff.net","subject":"[PATCH] checkout: default to quiet if !isatty(2).","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2012-05-24T06:05:34Z","receivedAt":"2012-05-24T06:05:34Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"It would probably be better to have progress.c check isatty(2) all the time,\nbut that wouldn't allow things like 'git push --progress' to force progress\nreporting to on, so I won't try to solve the general case right now.\n\nActual fix suggested by Jeff King.\n\nSigned-off-by: Avery Pennarun \n---\n builtin/checkout.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f1984d9..4ee833a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -343,7 +343,7 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)\n \topts.reset = 1;\n \topts.merge = 1;\n \topts.fn = oneway_merge;\n-\topts.verbose_update = !o->quiet;\n+\topts.verbose_update = !o->quiet && isatty(2);\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n \tparse_tree(tree);\n@@ -420,7 +420,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\ttopts.update = 1;\n \t\ttopts.merge = 1;\n \t\ttopts.gently = opts->merge && old->commit;\n-\t\ttopts.verbose_update = !opts->quiet;\n+\t\ttopts.verbose_update = !opts->quiet && isatty(2);\n \t\ttopts.fn = twoway_merge;\n \t\tif (opts->overwrite_ignore) {\n \t\t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n-- \n1.7.9.dirty\n"},{"id":"192052","messageId":"20120524061000.GA14035@sigill.intra.peff.net","threadId":"30606","inReplyTo":"1337839534-7760-1-git-send-email-apenwarr@gmail.com","subject":"Re: [PATCH] checkout: default to quiet if !isatty(2).","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T06:10:00Z","receivedAt":"2012-05-24T06:10:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 24, 2012 at 02:05:34AM -0400, Avery Pennarun wrote:\n\n> It would probably be better to have progress.c check isatty(2) all the time,\n> but that wouldn't allow things like 'git push --progress' to force progress\n> reporting to on, so I won't try to solve the general case right now.\n\nThis looks better. There is a slight inaccuracy in your subject line,\nthough. We are not defaulting to quiet if !isatty(2) in all cases, but\nrather only when we call into unpack_trees, which generates the progress\noutput. We will still print the ahead/behind line, detached HEAD info,\netc.\n\nWhich I think is the right behavior, but is not quite what is advertised\nby your commit message.\n\n-Peff\n"},{"id":"192053","messageId":"1337839944-4651-1-git-send-email-apenwarr@gmail.com","threadId":"30606","inReplyTo":"20120524061000.GA14035@sigill.intra.peff.net","subject":"[PATCH v2] checkout: no progress messages if !isatty(2).","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2012-05-24T06:12:24Z","receivedAt":"2012-05-24T06:12:24Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"It would probably be better to have progress.c check isatty(2) all the time,\nbut that wouldn't allow things like 'git push --progress' to force progress\nreporting to on, so I won't try to solve the general case right now.\n\nActual fix suggested by Jeff King.\n\nSigned-off-by: Avery Pennarun \n---\n builtin/checkout.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f1984d9..4ee833a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -343,7 +343,7 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)\n \topts.reset = 1;\n \topts.merge = 1;\n \topts.fn = oneway_merge;\n-\topts.verbose_update = !o->quiet;\n+\topts.verbose_update = !o->quiet && isatty(2);\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n \tparse_tree(tree);\n@@ -420,7 +420,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\ttopts.update = 1;\n \t\ttopts.merge = 1;\n \t\ttopts.gently = opts->merge && old->commit;\n-\t\ttopts.verbose_update = !opts->quiet;\n+\t\ttopts.verbose_update = !opts->quiet && isatty(2);\n \t\ttopts.fn = twoway_merge;\n \t\tif (opts->overwrite_ignore) {\n \t\t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n-- \n1.7.9.dirty\n"},{"id":"192054","messageId":"CAHqTa-3UAgSXEH3XA7fdJBt+6t33xdbiaPcxAEqiOP_vwgVd3g@mail.gmail.com","threadId":"30606","inReplyTo":"20120524061000.GA14035@sigill.intra.peff.net","subject":"Re: [PATCH] checkout: default to quiet if !isatty(2).","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2012-05-24T06:12:56Z","receivedAt":"2012-05-24T06:12:56Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Thu, May 24, 2012 at 2:10 AM, Jeff King wrote:\n> On Thu, May 24, 2012 at 02:05:34AM -0400, Avery Pennarun wrote:\n>> It would probably be better to have progress.c check isatty(2) all the time,\n>> but that wouldn't allow things like 'git push --progress' to force progress\n>> reporting to on, so I won't try to solve the general case right now.\n>\n> This looks better. There is a slight inaccuracy in your subject line,\n> though. We are not defaulting to quiet if !isatty(2) in all cases, but\n> rather only when we call into unpack_trees, which generates the progress\n> output. We will still print the ahead/behind line, detached HEAD info,\n> etc.\n>\n> Which I think is the right behavior, but is not quite what is advertised\n> by your commit message.\n\nGood point, fixed.\n\nThanks!\n\nAvery\n"},{"id":"192095","messageId":"7vy5ohwhy7.fsf@alter.siamese.dyndns.org","threadId":"30606","inReplyTo":"1337839944-4651-1-git-send-email-apenwarr@gmail.com","subject":"Re: [PATCH v2] checkout: no progress messages if !isatty(2).","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-24T18:29:52Z","receivedAt":"2012-05-24T18:29:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Avery Pennarun writes:\n\n> It would probably be better to have progress.c check isatty(2) all the time,\n> but that wouldn't allow things like 'git push --progress' to force progress\n> reporting to on, so I won't try to solve the general case right now.\n\nBefore that \"It would probably be better\" comment to give your opinion,\nyou need to describe what problem you wanted to solve in the first place.\nI'll lift it from your original version of the patch:\n\n If stderr isn't a tty, we shouldn't be printing incremental progress\n messages. In particular, this affected 'git checkout -f . >&logfile'\n unless you provided -q. And git-new-workdir has no way to provide -q.\n\nI do not seem to find a sane justification for\n\n\tgit $cmd --progress 2>output\n\nuse case and I do not immediately see how that \"output\" file can be\nuseful. But we've allowed it for a long time, so probably this version is\nsafer. Besides, it is more explicit.\n\nThanks.\n"},{"id":"192097","messageId":"20120524183457.GA11841@sigill.intra.peff.net","threadId":"30606","inReplyTo":"7vy5ohwhy7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] checkout: no progress messages if !isatty(2).","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-24T18:34:57Z","receivedAt":"2012-05-24T18:34:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 24, 2012 at 11:29:52AM -0700, Junio C Hamano wrote:\n\n> I do not seem to find a sane justification for\n> \n> \tgit $cmd --progress 2>output\n> \n> use case and I do not immediately see how that \"output\" file can be\n> useful. But we've allowed it for a long time, so probably this version is\n> safer. Besides, it is more explicit.\n\nActually, I ran across a case of this just recently. If you are writing\na graphical interface that wraps git, scraping \"--progress\" output from\na pipe is the only way you can provide a progress meter within your\ninterface. That is what the \"GitHub for {Mac,Windows}\" interfaces do\n(they also use libgit2 where possible, but it is far from feature\ncomplete).\n\n-Peff\n"},{"id":"192098","messageId":"CAHqTa-3QUsW_AP67NWjc-Gu5FZ7xQZyOOM-=zea+vwZeT79=0A@mail.gmail.com","threadId":"30606","inReplyTo":"7vy5ohwhy7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] checkout: no progress messages if !isatty(2).","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2012-05-24T18:46:42Z","receivedAt":"2012-05-24T18:46:42Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Thu, May 24, 2012 at 2:29 PM, Junio C Hamano wrote:\n> Avery Pennarun writes:\n>> It would probably be better to have progress.c check isatty(2) all the time,\n>> but that wouldn't allow things like 'git push --progress' to force progress\n>> reporting to on, so I won't try to solve the general case right now.\n>\n> Before that \"It would probably be better\" comment to give your opinion,\n> you need to describe what problem you wanted to solve in the first place.\n> I'll lift it from your original version of the patch:\n>\n>    If stderr isn't a tty, we shouldn't be printing incremental progress\n>    messages.  In particular, this affected 'git checkout -f . >&logfile'\n>    unless you provided -q.  And git-new-workdir has no way to provide -q.\n\nDo you want me to rephrase the commit message and resend?\n\n> I do not seem to find a sane justification for\n>\n>        git $cmd --progress 2>output\n>\n> use case and I do not immediately see how that \"output\" file can be\n> useful.  But we've allowed it for a long time, so probably this version is\n> safer.  Besides, it is more explicit.\n\nYeah, I have nothing against allowing --progress to work. If I were\nto clarify my comment above, it would be to say that I'm worried about\nhow *many* places we keep calling isatty(). It is (as we can see from\nthe need for this patch) error prone, since I think most naive coders\nwould expect the progress stuff to act correctly by default if\n!isatty(2).\n\nSo maybe the \"right\" fix is to add a flag to start_progress_delay() to\n\"force\" verbose mode; if it's not set, start_progress_delay() would\ncheck isatty(2) and decide automatically what to do. This wouldn't\nsave much code, but would make sure developers think about their\nintentions.\n\nHave fun,\n\nAvery\n"},{"id":"192099","messageId":"CAHqTa-0y0ETeq2G2FPMWgAwkXF_LSLN=VqnijF-m3q2YT0z4mQ@mail.gmail.com","threadId":"30606","inReplyTo":"20120524183457.GA11841@sigill.intra.peff.net","subject":"Re: [PATCH v2] checkout: no progress messages if !isatty(2).","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2012-05-24T18:49:40Z","receivedAt":"2012-05-24T18:49:40Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Thu, May 24, 2012 at 2:34 PM, Jeff King wrote:\n> On Thu, May 24, 2012 at 11:29:52AM -0700, Junio C Hamano wrote:\n>> I do not seem to find a sane justification for\n>>\n>>       git $cmd --progress 2>output\n>>\n>> use case and I do not immediately see how that \"output\" file can be\n>> useful.  But we've allowed it for a long time, so probably this version is\n>> safer.  Besides, it is more explicit.\n>\n> Actually, I ran across a case of this just recently. If you are writing\n> a graphical interface that wraps git, scraping \"--progress\" output from\n> a pipe is the only way you can provide a progress meter within your\n> interface. That is what the \"GitHub for {Mac,Windows}\" interfaces do\n> (they also use libgit2 where possible, but it is far from feature\n> complete).\n\nThis is why we have ptys, isn't it? :)\n\n\n\nFWIW, in bup we use environment variables for this. bup's main\nprogram automatically redirects stderr to a pipe (to keep overlapping\nstatus messages from interfering with each other) and the subcommands\nneed to know that stderr \"was\" a tty. Arguably, an environment\nvariable is a better place for this since a script would presumably\nwant progress messages or not, globally. It would also have solved\nthe problem where git-new-worktree doesn't have a --quiet option.\n\nHave fun,\n\nAvery\n"},{"id":"192126","messageId":"7v62blw8u6.fsf@alter.siamese.dyndns.org","threadId":"30606","inReplyTo":"CAHqTa-3QUsW_AP67NWjc-Gu5FZ7xQZyOOM-=zea+vwZeT79=0A@mail.gmail.com","subject":"Re: [PATCH v2] checkout: no progress messages if !isatty(2).","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-24T21:46:41Z","receivedAt":"2012-05-24T21:46:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Avery Pennarun writes:\n\n>> I'll lift it from your original version of the patch:\n>>\n>>    If stderr isn't a tty, we shouldn't be printing incremental progress\n>>    messages.  In particular, this affected 'git checkout -f . >&logfile'\n>>    unless you provided -q.  And git-new-workdir has no way to provide -q.\n>\n> Do you want me to rephrase the commit message and resend?\n\nNo need, unless you want to say something vastly different from the above.\n\nThanks.\n"}]}