{"thread":{"id":"40695","subject":"[PATCH v4] checkout: add --progress option","startedAt":"2015-11-01T17:47:56Z","lastAt":"2015-11-01T20:26:58Z","messageCount":8,"participants":["Edmundo Carmona Antoranz","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"272676","messageId":"1446400076-9983-1-git-send-email-eantoranz@gmail.com","threadId":"40695","inReplyTo":null,"subject":"[PATCH v4] checkout: add --progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-01T17:47:56Z","receivedAt":"2015-11-01T17:47:56Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Under normal circumstances, and like other git commands,\ngit checkout will write progress info to stderr if\nattached to a terminal. This option allows progress\nto be forced even if not using a terminal. Also,\nprogress can be skipped if using option --no-progress.\n\nSigned-off-by: Edmundo Carmona Antoranz \n---\n Documentation/git-checkout.txt | 6 ++++++\n builtin/checkout.c | 26 ++++++++++++++++++++++++--\n 2 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\nindex e269fb1..93ba35a 100644\n--- a/Documentation/git-checkout.txt\n+++ b/Documentation/git-checkout.txt\n@@ -107,6 +107,12 @@ OPTIONS\n --quiet::\n \tQuiet, suppress feedback messages.\n \n+--progress::\n+\tProgress status is reported on the standard error stream\n+\tby default when it is attached to a terminal, unless -q\n+\tis specified. This flag forces progress status even if the\n+\tstandard error stream is not directed to a terminal.\n+\n -f::\n --force::\n \tWhen switching branches, proceed even if the index or the\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex bc703c0..c3b8e5d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -37,6 +37,7 @@ struct checkout_opts {\n \tint overwrite_ignore;\n \tint ignore_skipworktree;\n \tint ignore_other_worktrees;\n+\tint show_progress;\n \n \tconst char *new_branch;\n \tconst char *new_branch_force;\n@@ -417,7 +418,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n \topts.reset = 1;\n \topts.merge = 1;\n \topts.fn = oneway_merge;\n-\topts.verbose_update = !o->quiet && isatty(2);\n+\topts.verbose_update = o->show_progress;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n \tparse_tree(tree);\n@@ -501,7 +502,7 @@ static int merge_working_tree(const 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 && isatty(2);\n+\t\ttopts.verbose_update = opts->show_progress;\n \t\ttopts.fn = twoway_merge;\n \t\tif (opts->overwrite_ignore) {\n \t\t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n@@ -1156,6 +1157,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"second guess 'git checkout '\")),\n \t\tOPT_BOOL(0, \"ignore-other-worktrees\", &opts.ignore_other_worktrees,\n \t\t\t N_(\"do not check if another worktree is holding the given ref\")),\n+\t\tOPT_BOOL(0, \"progress\", &opts.show_progress, N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n \n@@ -1163,6 +1165,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tmemset(&new, 0, sizeof(new));\n \topts.overwrite_ignore = 1;\n \topts.prefix = prefix;\n+\topts.show_progress = -1;\n \n \tgitmodules_config();\n \tgit_config(git_checkout_config, &opts);\n@@ -1172,6 +1175,25 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, checkout_usage,\n \t\t\t PARSE_OPT_KEEP_DASHDASH);\n \n+\t/*\n+\t * Final processing of show_progress\n+\t * - User selected --progress: show progress\n+\t * - user selected --no-progress: skip progress\n+\t * - User didn't specify:\n+\t * (check rules in order till finding the first matching one)\n+\t * - user selected --quiet: skip progress\n+\t * - stderr is connected to a terminal: show progress\n+\t * - fallback: skip progress\n+\t */\n+\tif (opts.show_progress < 0) {\n+\t\t/* user didn't specify --[no-]progress */\n+\t\tif (opts.quiet) {\n+\t\t\topts.show_progress = 0;\n+\t\t} else {\n+\t\t\topts.show_progress = isatty(2);\n+\t\t}\n+\t}\n+\n \tif (conflict_style) {\n \t\topts.merge = 1; /* implied */\n \t\tgit_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n-- \n2.6.1\n"},{"id":"272679","messageId":"CAPig+cSLC_xkpXEznzPQdA=FE6GV5VSuwSrFy62Nzybv7jQf1g@mail.gmail.com","threadId":"40695","inReplyTo":"1446400076-9983-1-git-send-email-eantoranz@gmail.com","subject":"Re: [PATCH v4] checkout: add --progress option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-01T17:52:57Z","receivedAt":"2015-11-01T17:52:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 1, 2015 at 12:47 PM, Edmundo Carmona Antoranz\n wrote:\n> Under normal circumstances, and like other git commands,\n> git checkout will write progress info to stderr if\n> attached to a terminal. This option allows progress\n> to be forced even if not using a terminal. Also,\n> progress can be skipped if using option --no-progress.\n>\n> Signed-off-by: Edmundo Carmona Antoranz \n> ---\n> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\n> index e269fb1..93ba35a 100644\n> --- a/Documentation/git-checkout.txt\n> +++ b/Documentation/git-checkout.txt\n> @@ -107,6 +107,12 @@ OPTIONS\n> --quiet::\n> Quiet, suppress feedback messages.\n>\n> +--progress::\n> + Progress status is reported on the standard error stream\n> + by default when it is attached to a terminal, unless -q\n> + is specified. This flag forces progress status even if the\n> + standard error stream is not directed to a terminal.\n\nWhat this kind of implies, but neglects to say explicitly, is that the\nlogic implemented by this patch also overrides --quiet. It probably\nshould say so explicitly.\n\nI realize that this text was copied from elsewhere (likely from\ngit-clone.txt), but git-checkout.txt does try to do a bit better job\nwith formatting, so it might be a good idea to quote -q with backticks\n(`-q` or `--quiet`).\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index bc703c0..c3b8e5d 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1163,6 +1165,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n> memset(&new, 0, sizeof(new));\n> opts.overwrite_ignore = 1;\n> opts.prefix = prefix;\n> + opts.show_progress = -1;\n>\n> gitmodules_config();\n> git_config(git_checkout_config, &opts);\n> @@ -1172,6 +1175,25 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n> argc = parse_options(argc, argv, prefix, options, checkout_usage,\n> PARSE_OPT_KEEP_DASHDASH);\n>\n> + /*\n> + * Final processing of show_progress\n> + * - User selected --progress: show progress\n> + * - user selected --no-progress: skip progress\n> + * - User didn't specify:\n> + * (check rules in order till finding the first matching one)\n> + * - user selected --quiet: skip progress\n> + * - stderr is connected to a terminal: show progress\n> + * - fallback: skip progress\n> + */\n\nIt takes longer to read and digest this comment block than it does to\ncomprehend the actual logic in code, which is pretty clear in its\ncurrent form. Comment blocks which merely repeat easily digested code\nadd little, if any, value, so it might be worthwhile to drop the\ncomment altogether.\n\n> + if (opts.show_progress < 0) {\n> + /* user didn't specify --[no-]progress */\n\nHere, also, consider dropping the comment which merely repeats what\nthe code already states clearly.\n\n> + if (opts.quiet) {\n> + opts.show_progress = 0;\n> + } else {\n> + opts.show_progress = isatty(2);\n> + }\n\nStyle: drop unnecessary braces\n\n> + }\n> +\n> if (conflict_style) {\n> opts.merge = 1; /* implied */\n> git_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n> --\n> 2.6.1\n"},{"id":"272681","messageId":"CAOc6etbaQhE6tcS5m_3rvD7FFV9hTSdBD3RM5=OvqR_7VLqf0g@mail.gmail.com","threadId":"40695","inReplyTo":"CAPig+cSLC_xkpXEznzPQdA=FE6GV5VSuwSrFy62Nzybv7jQf1g@mail.gmail.com","subject":"Re: [PATCH v4] checkout: add --progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-01T17:58:30Z","receivedAt":"2015-11-01T17:58:30Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Sun, Nov 1, 2015 at 11:52 AM, Eric Sunshine wrote:\n>> + if (opts.quiet) {\n>> + opts.show_progress = 0;\n>> + } else {\n>> + opts.show_progress = isatty(2);\n>> + }\n>\n> Style: drop unnecessary braces\n>\n\nOk. WIll do!\n"},{"id":"272693","messageId":"20151101191902.GA19594@sigill.intra.peff.net","threadId":"40695","inReplyTo":"CAPig+cSLC_xkpXEznzPQdA=FE6GV5VSuwSrFy62Nzybv7jQf1g@mail.gmail.com","subject":"Re: [PATCH v4] checkout: add --progress option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-01T19:19:03Z","receivedAt":"2015-11-01T19:19:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 01, 2015 at 12:52:57PM -0500, Eric Sunshine wrote:\n\n> > diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\n> > index e269fb1..93ba35a 100644\n> > --- a/Documentation/git-checkout.txt\n> > +++ b/Documentation/git-checkout.txt\n> > @@ -107,6 +107,12 @@ OPTIONS\n> > --quiet::\n> > Quiet, suppress feedback messages.\n> >\n> > +--progress::\n> > + Progress status is reported on the standard error stream\n> > + by default when it is attached to a terminal, unless -q\n> > + is specified. This flag forces progress status even if the\n> > + standard error stream is not directed to a terminal.\n> \n> What this kind of implies, but neglects to say explicitly, is that the\n> logic implemented by this patch also overrides --quiet. It probably\n> should say so explicitly.\n> \n> I realize that this text was copied from elsewhere (likely from\n> git-clone.txt), but git-checkout.txt does try to do a bit better job\n> with formatting, so it might be a good idea to quote -q with backticks\n> (`-q` or `--quiet`).\n\nI was the one who suggested originally that --progress should override\n--quiet[1]. However, that was just based on what I thought was\nreasonable. I didn't look at what clone or other commands do.\nConsistency between commands is probably more important than any\nparticular behavior.\n\n[1] To be honest, this is kind of a crazy corner case anyway. It was\nmore just that it has to do _something_.\n\n> > + /*\n> > + * Final processing of show_progress\n> > + * - User selected --progress: show progress\n> > + * - user selected --no-progress: skip progress\n> > + * - User didn't specify:\n> > + * (check rules in order till finding the first matching one)\n> > + * - user selected --quiet: skip progress\n> > + * - stderr is connected to a terminal: show progress\n> > + * - fallback: skip progress\n> > + */\n> \n> It takes longer to read and digest this comment block than it does to\n> comprehend the actual logic in code, which is pretty clear in its\n> current form. Comment blocks which merely repeat easily digested code\n> add little, if any, value, so it might be worthwhile to drop the\n> comment altogether.\n\nYeah, I think I agree.\n\n-Peff\n"},{"id":"272694","messageId":"CAPig+cQvsXMqAzV_59sZdqDEh_SRfOMh+BY3XLmzSPdfWwugdg@mail.gmail.com","threadId":"40695","inReplyTo":"20151101191902.GA19594@sigill.intra.peff.net","subject":"Re: [PATCH v4] checkout: add --progress option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-01T19:29:16Z","receivedAt":"2015-11-01T19:29:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 1, 2015 at 2:19 PM, Jeff King wrote:\n> On Sun, Nov 01, 2015 at 12:52:57PM -0500, Eric Sunshine wrote:\n>> > +--progress::\n>> > + Progress status is reported on the standard error stream\n>> > + by default when it is attached to a terminal, unless -q\n>> > + is specified. This flag forces progress status even if the\n>> > + standard error stream is not directed to a terminal.\n>>\n>> What this kind of implies, but neglects to say explicitly, is that the\n>> logic implemented by this patch also overrides --quiet. It probably\n>> should say so explicitly.\n>\n> I was the one who suggested originally that --progress should override\n> --quiet[1]. However, that was just based on what I thought was\n> reasonable. I didn't look at what clone or other commands do.\n> Consistency between commands is probably more important than any\n> particular behavior.\n\nI did check git-clone during my v4 review, and --progress does\noverride --quiet (see transport.c:transport_set_verbosity()), so your\nsuggestion to make --progress override --quiet in git-checkout at\nleast keeps it consistent.\n\nAside: Specifying both --quiet and --progress with git-clone doesn't\ngive great results since (it seems) that no flushing is done until the\nnewline is emitted, thus you only see the end result rather than a\nrunning progress meter.\n\n> [1] To be honest, this is kind of a crazy corner case anyway. It was\n> more just that it has to do _something_.\n\nAn alternative would be to error out stating that --quiet and\n--progress are mutually exclusive.\n"},{"id":"272700","messageId":"CAOc6etb24owGX1KD9b-eYt5TtTKzEzinvnJ-81ev5QK_tKgLPQ@mail.gmail.com","threadId":"40695","inReplyTo":"CAPig+cQvsXMqAzV_59sZdqDEh_SRfOMh+BY3XLmzSPdfWwugdg@mail.gmail.com","subject":"Re: [PATCH v4] checkout: add --progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-01T19:35:16Z","receivedAt":"2015-11-01T19:35:16Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Sun, Nov 1, 2015 at 1:29 PM, Eric Sunshine wrote:\n>>> > +--progress::\n>>> > + Progress status is reported on the standard error stream\n>>> > + by default when it is attached to a terminal, unless -q\n>>> > + is specified. This flag forces progress status even if the\n>>> > + standard error stream is not directed to a terminal.\n\nBefore I send a new full patch, could you guys tell me if you find this ok?\n\n-q::\n--quiet::\nQuiet, suppress feedback messages. Progress can skip this option.\nRead the information about --progress\n\n--[no-]progress::\nProgress status is reported on the standard error stream\nby default when it is attached to a terminal, unless --quiet\nor -q is specified. This flag forces progress status even if the\nstandard error stream is not directed to a terminal and overrides\nthe --quiet or -q options.\n\nThank you all, guys!\n"},{"id":"272701","messageId":"CAPig+cT-Mbf0cs+5OpPv+za=yrjiqycG5EtZONSrZwy8xL5pSA@mail.gmail.com","threadId":"40695","inReplyTo":"CAOc6etb24owGX1KD9b-eYt5TtTKzEzinvnJ-81ev5QK_tKgLPQ@mail.gmail.com","subject":"Re: [PATCH v4] checkout: add --progress option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-01T20:15:21Z","receivedAt":"2015-11-01T20:15:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 1, 2015 at 2:35 PM, Edmundo Carmona Antoranz\n wrote:\n> On Sun, Nov 1, 2015 at 1:29 PM, Eric Sunshine wrote:\n>>>> > +--progress::\n>>>> > + Progress status is reported on the standard error stream\n>>>> > + by default when it is attached to a terminal, unless -q\n>>>> > + is specified. This flag forces progress status even if the\n>>>> > + standard error stream is not directed to a terminal.\n>\n> Before I send a new full patch, could you guys tell me if you find this ok?\n>\n> -q::\n> --quiet::\n> Quiet, suppress feedback messages. Progress can skip this option.\n> Read the information about --progress\n\nIMHO, this sort of corner case probably doesn't deserve being called\nout specially. Also, since --progress immediately follows --quiet in\nthe documentation, it's quite easy to discover --progress and read\nabout its behavior. Thus, I'd leave the description of --quiet as is.\n\n> --[no-]progress::\n> Progress status is reported on the standard error stream\n> by default when it is attached to a terminal, unless --quiet\n> or -q is specified. This flag forces progress status even if the\n> standard error stream is not directed to a terminal and overrides\n> the --quiet or -q options.\n\nNo need to make the description long by mentioning both -q and\n--quiet. (My v4 review suggested only that you quote -q or --quiet\nwith backticks; not that you mention both.) The reader can easily\ndiscover the alias of -q or --quiet, so mentioning one or the other\nshould be sufficient. A bit shorter and sweeter, perhaps:\n\n Progress status is reported on the standard error stream by\n default when it is attached to a terminal, unless `--quiet` is\n specified. This flag enables progress reporting even if not\n attached to a terminal, regardless of `--quiet`.\n"},{"id":"272703","messageId":"CAOc6etauvMjzjYxA2C7TVaHMnygYa3VNCeLzyPiesZH5CcqgcA@mail.gmail.com","threadId":"40695","inReplyTo":"CAPig+cT-Mbf0cs+5OpPv+za=yrjiqycG5EtZONSrZwy8xL5pSA@mail.gmail.com","subject":"Re: [PATCH v4] checkout: add --progress option","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-11-01T20:26:58Z","receivedAt":"2015-11-01T20:26:58Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Sun, Nov 1, 2015 at 2:15 PM, Eric Sunshine wrote:\n>\n> Progress status is reported on the standard error stream by\n> default when it is attached to a terminal, unless `--quiet` is\n> specified. This flag enables progress reporting even if not\n> attached to a terminal, regardless of `--quiet`.\n\nSounds good to me.\n\nSending another full patch.\n"}]}