[PATCH v4] checkout: add --progress option

[PATCH v4] checkout: add --progress option

[Edmundo Carmona Antoranz]

Under normal circumstances, and like other git commands,
git checkout will write progress info to stderr if
attached to a terminal. This option allows progress
to be forced even if not using a terminal. Also,
progress can be skipped if using option --no-progress.

Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
---
 Documentation/git-checkout.txt |  6 ++++++
 builtin/checkout.c             | 26 ++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index e269fb1..93ba35a 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -107,6 +107,12 @@ OPTIONS
 --quiet::
 	Quiet, suppress feedback messages.
 
+--progress::
+	Progress status is reported on the standard error stream
+	by default when it is attached to a terminal, unless -q
+	is specified. This flag forces progress status even if the
+	standard error stream is not directed to a terminal.
+
 -f::
 --force::
 	When switching branches, proceed even if the index or the
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bc703c0..c3b8e5d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -37,6 +37,7 @@ struct checkout_opts {
 	int overwrite_ignore;
 	int ignore_skipworktree;
 	int ignore_other_worktrees;
+	int show_progress;
 
 	const char *new_branch;
 	const char *new_branch_force;
@@ -417,7 +418,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 	opts.reset = 1;
 	opts.merge = 1;
 	opts.fn = oneway_merge;
-	opts.verbose_update = !o->quiet && isatty(2);
+	opts.verbose_update = o->show_progress;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 	parse_tree(tree);
@@ -501,7 +502,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		topts.update = 1;
 		topts.merge = 1;
 		topts.gently = opts->merge && old->commit;
-		topts.verbose_update = !opts->quiet && isatty(2);
+		topts.verbose_update = opts->show_progress;
 		topts.fn = twoway_merge;
 		if (opts->overwrite_ignore) {
 			topts.dir = xcalloc(1, sizeof(*topts.dir));
@@ -1156,6 +1157,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 				N_("second guess 'git checkout <no-such-branch>'")),
 		OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
 			 N_("do not check if another worktree is holding the given ref")),
+		OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
 		OPT_END(),
 	};
 
@@ -1163,6 +1165,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	memset(&new, 0, sizeof(new));
 	opts.overwrite_ignore = 1;
 	opts.prefix = prefix;
+	opts.show_progress = -1;
 
 	gitmodules_config();
 	git_config(git_checkout_config, &opts);
@@ -1172,6 +1175,25 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
 			     PARSE_OPT_KEEP_DASHDASH);
 
+	/*
+	 * Final processing of show_progress
+	 * - User selected --progress: show progress
+	 * - user selected --no-progress: skip progress
+	 * - User didn't specify:
+	 *     (check rules in order till finding the first matching one)
+	 *     - user selected --quiet: skip progress
+	 *     - stderr is connected to a terminal: show progress
+	 *     - fallback: skip progress
+	 */
+	if (opts.show_progress < 0) {
+		/* user didn't specify --[no-]progress */
+		if (opts.quiet) {
+			opts.show_progress = 0;
+		} else {
+			opts.show_progress = isatty(2);
+		}
+	}
+
 	if (conflict_style) {
 		opts.merge = 1; /* implied */
 		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
-- 
2.6.1

Re: [PATCH v4] checkout: add --progress option

[Eric Sunshine]

On Sun, Nov 1, 2015 at 12:47 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> Under normal circumstances, and like other git commands,
> git checkout will write progress info to stderr if
> attached to a terminal. This option allows progress
> to be forced even if not using a terminal. Also,
> progress can be skipped if using option --no-progress.
>
> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
> ---
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index e269fb1..93ba35a 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -107,6 +107,12 @@ OPTIONS
>  --quiet::
>         Quiet, suppress feedback messages.
>
> +--progress::
> +       Progress status is reported on the standard error stream
> +       by default when it is attached to a terminal, unless -q
> +       is specified. This flag forces progress status even if the
> +       standard error stream is not directed to a terminal.

What this kind of implies, but neglects to say explicitly, is that the
logic implemented by this patch also overrides --quiet. It probably
should say so explicitly.

I realize that this text was copied from elsewhere (likely from
git-clone.txt), but git-checkout.txt does try to do a bit better job
with formatting, so it might be a good idea to quote -q with backticks
(`-q` or `--quiet`).

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bc703c0..c3b8e5d 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1163,6 +1165,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>         memset(&new, 0, sizeof(new));
>         opts.overwrite_ignore = 1;
>         opts.prefix = prefix;
> +       opts.show_progress = -1;
>
>         gitmodules_config();
>         git_config(git_checkout_config, &opts);
> @@ -1172,6 +1175,25 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>         argc = parse_options(argc, argv, prefix, options, checkout_usage,
>                              PARSE_OPT_KEEP_DASHDASH);
>
> +       /*
> +        * Final processing of show_progress
> +        * - User selected --progress: show progress
> +        * - user selected --no-progress: skip progress
> +        * - User didn't specify:
> +        *     (check rules in order till finding the first matching one)
> +        *     - user selected --quiet: skip progress
> +        *     - stderr is connected to a terminal: show progress
> +        *     - fallback: skip progress
> +        */

It takes longer to read and digest this comment block than it does to
comprehend the actual logic in code, which is pretty clear in its
current form. Comment blocks which merely repeat easily digested code
add little, if any, value, so it might be worthwhile to drop the
comment altogether.

> +       if (opts.show_progress < 0) {
> +               /* user didn't specify --[no-]progress */

Here, also, consider dropping the comment which merely repeats what
the code already states clearly.

> +               if (opts.quiet) {
> +                       opts.show_progress = 0;
> +               } else {
> +                       opts.show_progress = isatty(2);
> +               }

Style: drop unnecessary braces

> +       }
> +
>         if (conflict_style) {
>                 opts.merge = 1; /* implied */
>                 git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
> --
> 2.6.1

Re: [PATCH v4] checkout: add --progress option

[Edmundo Carmona Antoranz]

On Sun, Nov 1, 2015 at 11:52 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> +               if (opts.quiet) {
>> +                       opts.show_progress = 0;
>> +               } else {
>> +                       opts.show_progress = isatty(2);
>> +               }
>
> Style: drop unnecessary braces
>

Ok. WIll do!

Re: [PATCH v4] checkout: add --progress option

[Jeff King]

On Sun, Nov 01, 2015 at 12:52:57PM -0500, Eric Sunshine wrote:

> > diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> > index e269fb1..93ba35a 100644
> > --- a/Documentation/git-checkout.txt
> > +++ b/Documentation/git-checkout.txt
> > @@ -107,6 +107,12 @@ OPTIONS
> >  --quiet::
> >         Quiet, suppress feedback messages.
> >
> > +--progress::
> > +       Progress status is reported on the standard error stream
> > +       by default when it is attached to a terminal, unless -q
> > +       is specified. This flag forces progress status even if the
> > +       standard error stream is not directed to a terminal.
> 
> What this kind of implies, but neglects to say explicitly, is that the
> logic implemented by this patch also overrides --quiet. It probably
> should say so explicitly.
> 
> I realize that this text was copied from elsewhere (likely from
> git-clone.txt), but git-checkout.txt does try to do a bit better job
> with formatting, so it might be a good idea to quote -q with backticks
> (`-q` or `--quiet`).

I was the one who suggested originally that --progress should override
--quiet[1]. However, that was just based on what I thought was
reasonable. I didn't look at what clone or other commands do.
Consistency between commands is probably more important than any
particular behavior.

[1] To be honest, this is kind of a crazy corner case anyway. It was
more just that it has to do _something_.

> > +       /*
> > +        * Final processing of show_progress
> > +        * - User selected --progress: show progress
> > +        * - user selected --no-progress: skip progress
> > +        * - User didn't specify:
> > +        *     (check rules in order till finding the first matching one)
> > +        *     - user selected --quiet: skip progress
> > +        *     - stderr is connected to a terminal: show progress
> > +        *     - fallback: skip progress
> > +        */
> 
> It takes longer to read and digest this comment block than it does to
> comprehend the actual logic in code, which is pretty clear in its
> current form. Comment blocks which merely repeat easily digested code
> add little, if any, value, so it might be worthwhile to drop the
> comment altogether.

Yeah, I think I agree.

-Peff

Re: [PATCH v4] checkout: add --progress option

[Eric Sunshine]

On Sun, Nov 1, 2015 at 2:19 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Nov 01, 2015 at 12:52:57PM -0500, Eric Sunshine wrote:
>> > +--progress::
>> > +       Progress status is reported on the standard error stream
>> > +       by default when it is attached to a terminal, unless -q
>> > +       is specified. This flag forces progress status even if the
>> > +       standard error stream is not directed to a terminal.
>>
>> What this kind of implies, but neglects to say explicitly, is that the
>> logic implemented by this patch also overrides --quiet. It probably
>> should say so explicitly.
>
> I was the one who suggested originally that --progress should override
> --quiet[1]. However, that was just based on what I thought was
> reasonable. I didn't look at what clone or other commands do.
> Consistency between commands is probably more important than any
> particular behavior.

I did check git-clone during my v4 review, and --progress does
override --quiet (see transport.c:transport_set_verbosity()), so your
suggestion to make --progress override --quiet in git-checkout at
least keeps it consistent.

Aside: Specifying both --quiet and --progress with git-clone doesn't
give great results since (it seems) that no flushing is done until the
newline is emitted, thus you only see the end result rather than a
running progress meter.

> [1] To be honest, this is kind of a crazy corner case anyway. It was
> more just that it has to do _something_.

An alternative would be to error out stating that --quiet and
--progress are mutually exclusive.

Re: [PATCH v4] checkout: add --progress option

[Edmundo Carmona Antoranz]

On Sun, Nov 1, 2015 at 1:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> > +--progress::
>>> > +       Progress status is reported on the standard error stream
>>> > +       by default when it is attached to a terminal, unless -q
>>> > +       is specified. This flag forces progress status even if the
>>> > +       standard error stream is not directed to a terminal.

Before I send a new full patch, could you guys tell me if you find this ok?

-q::
--quiet::
Quiet, suppress feedback messages. Progress can skip this option.
Read the information about --progress

--[no-]progress::
Progress status is reported on the standard error stream
by default when it is attached to a terminal, unless --quiet
or -q is specified. This flag forces progress status even if the
standard error stream is not directed to a terminal and overrides
the --quiet or -q options.

Thank you all, guys!

Re: [PATCH v4] checkout: add --progress option

[Eric Sunshine]

On Sun, Nov 1, 2015 at 2:35 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> On Sun, Nov 1, 2015 at 1:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> > +--progress::
>>>> > +       Progress status is reported on the standard error stream
>>>> > +       by default when it is attached to a terminal, unless -q
>>>> > +       is specified. This flag forces progress status even if the
>>>> > +       standard error stream is not directed to a terminal.
>
> Before I send a new full patch, could you guys tell me if you find this ok?
>
> -q::
> --quiet::
> Quiet, suppress feedback messages. Progress can skip this option.
> Read the information about --progress

IMHO, this sort of corner case probably doesn't deserve being called
out specially. Also, since --progress immediately follows --quiet in
the documentation, it's quite easy to discover --progress and read
about its behavior. Thus, I'd leave the description of --quiet as is.

> --[no-]progress::
> Progress status is reported on the standard error stream
> by default when it is attached to a terminal, unless --quiet
> or -q is specified. This flag forces progress status even if the
> standard error stream is not directed to a terminal and overrides
> the --quiet or -q options.

No need to make the description long by mentioning both -q and
--quiet. (My v4 review suggested only that you quote -q or --quiet
with backticks; not that you mention both.) The reader can easily
discover the alias of -q or --quiet, so mentioning one or the other
should be sufficient. A bit shorter and sweeter, perhaps:

    Progress status is reported on the standard error stream by
    default when it is attached to a terminal, unless `--quiet` is
    specified. This flag enables progress reporting even if not
    attached to a terminal, regardless of `--quiet`.

Re: [PATCH v4] checkout: add --progress option

[Edmundo Carmona Antoranz]

On Sun, Nov 1, 2015 at 2:15 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
>     Progress status is reported on the standard error stream by
>     default when it is attached to a terminal, unless `--quiet` is
>     specified. This flag enables progress reporting even if not
>     attached to a terminal, regardless of `--quiet`.

Sounds good to me.

Sending another full patch.