{"thread":{"id":"27727","subject":"[PATCH/RFC] Encapsulating isatty(2) calls into new progress_is_desired()","startedAt":"2011-06-29T11:28:25Z","lastAt":"2011-06-30T20:41:21Z","messageCount":3,"participants":["Steffen Daode Nurpmeso","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"170664","messageId":"3d6b6818cfc542a3acf7a7a43ac6842fd74ddcee.1309342095.git.sdaoden@gmail.com","threadId":"27727","inReplyTo":null,"subject":"[PATCH/RFC] Encapsulating isatty(2) calls into new progress_is_desired()","fromName":"Steffen Daode Nurpmeso","fromEmail":"sdaoden@googlemail.com","sentAt":"2011-06-29T11:28:25Z","receivedAt":"2011-06-29T11:28:25Z","isPatch":true,"sender":{"key":"sdaoden@googlemail.com","avatar":null},"body":"Adds a progress_is_desired() function and replaces most calls of\nisatty(2) in the codebase with it.\n\nSigned-off-by: Steffen Daode Nurpmeso <sdaoden@gmail.com>\n---\nOk, i yelled at it and here you have a patch which performs the\nencapsulation, just in case you agree.\nI'm using static init, the short glance i had at the code made me\nthink thread safety is not an issue once this is called?\nI think caching the value is worth an .align; it's a syscall\nAFAIK.  (It's not used yet, however.)\n\nAnd regardless of the fact that i'm stupid  Jeff King is right,\nfrontend parsing is important, and then CR plays an important role.\nIt's painful when you try to get some usable info out of these\nclosed proprietary status messages.  Sorry, really sorry.\nI *really* love 'git add -i' and 'git rebase -i', these are\n*fantastic* features!  Not scriptable through cron(8) though. :-)\nLine tagging is also nice for parsing, by the way, as in\n\n> wx-account: message 42/1 (6837/6837 bytes) .. sync\n@ xy-account: xy@z.com -> pop.gmail.com/74.125.39.109:995\n- xy-account: 0 messages (0 bytes) [action=seen]\n< org.kernel.git: 22 tickets ok (0 errors).\n< org.openbsd.misc: 20 tickets ok (0 errors).\n= Dispatched 42 tickets to 2 targets.\n\n builtin/merge.c          |    5 +++--\n builtin/pack-objects.c   |    2 +-\n builtin/prune-packed.c   |    2 +-\n builtin/unpack-objects.c |    2 +-\n progress.c               |    8 ++++++++\n progress.h               |    1 +\n remote-curl.c            |    2 +-\n transport.c              |    7 ++++---\n transport.h              |    2 +-\n 9 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 325891e..51fe030 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -695,8 +695,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\to.subtree_shift = \"\";\n \n \t\to.renormalize = option_renormalize;\n-\t\to.show_rename_progress =\n-\t\t\tshow_progress == -1 ? isatty(2) : show_progress;\n+\t\to.show_rename_progress = ((show_progress == -1)\n+\t\t\t\t\t\t? progress_is_desired()\n+\t\t\t\t\t\t: show_progress);\n \n \t\tfor (x = 0; x < xopts_nr; x++)\n \t\t\tif (parse_merge_opt(&o, xopts[x]))\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f402a84..f3ee648 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2138,7 +2138,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!pack_compression_seen && core_compression_seen)\n \t\tpack_compression_level = core_compression_level;\n \n-\tprogress = isatty(2);\n+\tprogress = progress_is_desired();\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \ndiff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\nindex f9463de..cc48807 100644\n--- a/builtin/prune-packed.c\n+++ b/builtin/prune-packed.c\n@@ -71,7 +71,7 @@ void prune_packed_objects(int opts)\n \n int cmd_prune_packed(int argc, const char **argv, const char *prefix)\n {\n-\tint opts = isatty(2) ? VERBOSE : 0;\n+\tint opts = progress_is_desired() ? VERBOSE : 0;\n \tconst struct option prune_packed_options[] = {\n \t\tOPT_BIT('n', \"dry-run\", &opts, \"dry run\", DRY_RUN),\n \t\tOPT_NEGBIT('q', \"quiet\", &opts, \"be quiet\", VERBOSE),\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex f63973c..14cea36 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -501,7 +501,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_default_config, NULL);\n \n-\tquiet = !isatty(2);\n+\tquiet = !progress_is_desired();\n \n \tfor (i = 1 ; i < argc; i++) {\n \t\tconst char *arg = argv[i];\ndiff --git a/progress.c b/progress.c\nindex c548de4..6baadd6 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -209,6 +209,14 @@ int display_progress(struct progress *progress, unsigned n)\n \treturn progress ? display(progress, n, NULL) : 0;\n }\n \n+int progress_is_desired(void)\n+{\n+\tstatic long eitty /*= 0*/;\n+\tif (!eitty)\n+\t\teitty = isatty(2) ? 1 : -1;\n+\treturn (eitty == 1);\n+}\n+\n struct progress *start_progress_delay(const char *title, unsigned total,\n \t\t\t\t       unsigned percent_treshold, unsigned delay)\n {\ndiff --git a/progress.h b/progress.h\nindex 611e4c4..d7020d3 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -5,6 +5,7 @@ struct progress;\n \n void display_throughput(struct progress *progress, off_t total);\n int display_progress(struct progress *progress, unsigned n);\n+int progress_is_desired(void);\n struct progress *start_progress(const char *title, unsigned total);\n struct progress *start_progress_delay(const char *title, unsigned total,\n \t\t\t\t       unsigned percent_treshold, unsigned delay);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex b5be25c..b47097a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -841,7 +841,7 @@ int main(int argc, const char **argv)\n \t}\n \n \toptions.verbosity = 1;\n-\toptions.progress = !!isatty(2);\n+\toptions.progress = progress_is_desired();\n \toptions.thin = 1;\n \n \tremote = remote_get(argv[1]);\ndiff --git a/transport.c b/transport.c\nindex c9c8056..8c67cfa 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -883,7 +883,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \tconst char *helper;\n \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n \n-\tret->progress = isatty(2);\n+\tret->progress = progress_is_desired();\n \n \tif (!remote)\n \t\tdie(\"No remote provided to transport_get()\");\n@@ -998,9 +998,10 @@ void transport_set_verbosity(struct transport *transport, int verbosity,\n \t *\n \t *   1. Report progress, if force_progress is 1 (ie. --progress).\n \t *   2. Don't report progress, if verbosity < 0 (ie. -q/--quiet ).\n-\t *   3. Report progress if isatty(2) is 1.\n+\t *   3. Report progress if progress default policies apply.\n \t **/\n-\ttransport->progress = force_progress || (verbosity >= 0 && isatty(2));\n+\ttransport->progress = (force_progress ||\n+\t\t\t\t(verbosity >= 0 && progress_is_desired()));\n }\n \n int transport_push(struct transport *transport,\ndiff --git a/transport.h b/transport.h\nindex 161d724..6b73240 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -82,7 +82,7 @@ struct transport {\n \tsigned verbose : 3;\n \t/**\n \t * Transports should not set this directly, and should use this\n-\t * value without having to check isatty(2), -q/--quiet\n+\t * value without having to check progress_is_desired(), -q/--quiet\n \t * (transport->verbose < 0), etc. - checking has already been done\n \t * in transport_set_verbosity().\n \t **/\n-- \n1.7.6.1.ge79e.dirty\n"},{"id":"170686","messageId":"7vboxgrzwt.fsf@alter.siamese.dyndns.org","threadId":"27727","inReplyTo":"3d6b6818cfc542a3acf7a7a43ac6842fd74ddcee.1309342095.git.sdaoden@gmail.com","subject":"Re: [PATCH/RFC] Encapsulating isatty(2) calls into new progress_is_desired()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-29T23:17:06Z","receivedAt":"2011-06-29T23:17:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Daode Nurpmeso <sdaoden@googlemail.com> writes:\n\n> Adds a progress_is_desired() function and replaces most calls of\n> isatty(2) in the codebase with it.\n\nTwo comments, and perhaps a half.\n\nOne is trivial: want_progress() might be shorter and nicer.\n\nThe other is that we have isatty() for different file descriptors, and I\nwonder if we want to consolidate them further and/or give them names, in a\nseparate patch. Here is a quick analysis.\n\n\n* Most of the isatty(2) calls are indeed \"want_progress()\", except for one.\n\nbuiltin/merge.c:\tshow_progress == -1 ? isatty(2) : show_progress;\n\n\tWhen \"show_progress\" is unset (denoted by -1), take the hint from\n\tstderr to decide if we show the progress output.\n\nbuiltin/pack-objects.c:\tprogress = isatty(2);\n\n\tDefault \"progress\" to 0 (no progress output) or 1 (give progress\n\tfor the write-out phase only when sending the pack to\n\tfilesystem). Later, command line parsing may update this variable.\n\nbuiltin/unpack-objects.c:\tquiet = !isatty(2);\n\n\tDefault to give progress when connected to a tty.\n\nremote-curl.c:\toptions.progress = !!isatty(2);\n\n\tDefault to give progress when connected to a tty.\n\ntransport.c:\tret->progress = isatty(2);\n\n\tDefault to give progress when connected to a tty.\n\ntransport.c:\ttransport->progress = force_progress || (verbosity >= 0 && isatty(2));\ntransport.c-}\n\n\tDefault to give progress when connected to a tty.\n\npager.c:\tif (isatty(2))\npager.c-\t\tdup2(pager_process.in, 2);\npager.c-\tclose(pager_process.in);\n\n\tThis should not be renamed to want_progress() even though it is\n\tisatty(2).\n\n\n* Many places ask \"Are we in a keyboard-interactive session?\" using\n  isatty(0) and change their behaviour accordingly. We might want a\n  symbolic name for them.\n\nbuiltin/commit.c:\t\tif (isatty(0))\nbuiltin/commit.c-\t\t\tfprintf(stderr, _(\"(reading log message from standard input)\\n\"));\n\n        This is to warn against \"git commit -F -\" that forgot to redirect\n        an upstream pipe into the command;\n        i.e. \"script-to-generate-message | git commit -F -\" is what the\n        user probably wanted to.\n\nbuiltin/pack-redundant.c:\tif (!isatty(0)) {\nbuiltin/pack-redundant.c-\t\twhile (fgets(buf, sizeof(buf), stdin)) {\nbuiltin/pack-redundant.c-\t\t\tsha1 = xmalloc(20);\n\n\tThe command _can_ be fed a list of object names from the standard\n\tinput, but we do not expect anybody typing the object names from\n\tthe keyboard.\n\n\tSide note: I may be more consistent to do the \"warn\" thing commit\n\tdoes (see above) instead; I also suspect this command outlived its\n\tusefulness.\n\nbuiltin/prune-packed.c:\tint opts = isatty(2) ? VERBOSE : 0;\n\n\tDefault to give progress when connected to a tty.\n\nbuiltin/revert.c:\tif (isatty(0))\nbuiltin/revert.c-\t\tedit = 1;\n\n\t\"git revert\" was designed to force you to edit the message to\n\tjustify why reverting the given commit is the right thing, but\n\tin a non-interactive session, opening an editor is not a good\n\tthing to do.\n\nbuiltin/shortlog.c:\tif (!nongit && !rev.pending.nr && isatty(0))\nbuiltin/shortlog.c-\t\tadd_head_to_pending(&rev);\nbuiltin/shortlog.c-\tif (rev.pending.nr == 0) {\nbuiltin/shortlog.c:\t\tif (isatty(0))\nbuiltin/shortlog.c-\t\t\tfprintf(stderr, _(\"(reading log message from standard input)\\n\"));\nbuiltin/shortlog.c-\t\tread_from_stdin(&log);\n\n\t\"git shortlog\" (without any revisions argument) can work as a\n\tfilter to read \"log\" output (e.g. \"git log ... | git shortlog\")\n\tand condense it down, in which case it will not be reading from\n\tthe terminal .  Without such an upstream process, \"git shortlog\"\n\tinternally runs \"git log HEAD\" and gives its output.\n\n* We decide to color the output when spitting out to a terminal using\n  isatty(1).\n\nbuiltin/config.c:\t\t\tstdout_is_tty = isatty(1);\nbuiltin/config.c-\t\treturn get_colorbool(argc != 0);\n\n\tThis is to default for color output on terminal output, and is\n\tconsistent with the one in color.c below.\n\ncolor.c-\tif (stdout_is_tty < 0)\ncolor.c:\t\tstdout_is_tty = isatty(1);\ncolor.c-\tif (stdout_is_tty || (pager_in_use() && pager_use_color)) {\ncolor.c-\t\tchar *term = getenv(\"TERM\");\n\n\tDefault to give color output on a capable terminal when\n\tconfigruation says auto.\n\npager.c:\tconst char *pager = git_pager(isatty(1));\n\n\tUse pager on tty by default.\n\n\nThe remaining half is about various ways the \"progress\", \"verbose\" and\n\"quiet\" are all mixed up (obviously \"progress\" is part of being \"verbose\",\nbut \"verbose\" can and do mean giving more information other than\nprogress), and they are different ways in which the values to give to the\ninternal variables are determined (some use isatty(2) to initialize the\ndefault and then let option parser to update it, some run option parser\nfirst and if the variable is unset use isatty(2) to give it default). We\nmight want to think about making them more uniform (or we may not).\n"},{"id":"170730","messageId":"20110630204121.GB63317@sherwood.local","threadId":"27727","inReplyTo":"7vboxgrzwt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] Encapsulating isatty(2) calls into new progress_is_desired()","fromName":"Steffen Daode Nurpmeso","fromEmail":"sdaoden@googlemail.com","sentAt":"2011-06-30T20:41:21Z","receivedAt":"2011-06-30T20:41:21Z","isPatch":true,"sender":{"key":"sdaoden@googlemail.com","avatar":null},"body":"@ Junio C Hamano <gitster@pobox.com> wrote (2011-06-30 01:17+0200):\n> Two comments, and perhaps a half.\n\nGit looks pretty transparent through your eyes, so thanks for your\nmail.  I really have appreciated to read it.\n(The very idea itself is a shallow no-go that way.)\n\n--\nCiao, Steffen\nsdaoden(*)(gmail.com)\n() ascii ribbon campaign - against html e-mail\n/\\ www.asciiribbon.org - against proprietary attachments\n"}]}