threads / patch / 40695

v4checkout: add --progress option

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

## tl;dr

8 messages between Nov 1, 2015 and Nov 1, 2015. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Edmundo Carmona Antoranz· Nov 1, 2015, 17:47 UTC · lore

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(-)
Show changes to 2 files +30 −2

Documentation/git-checkout.txt, builtin/checkout.c

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
Eric Sunshine· Nov 1, 2015, 17:52 UTC · re: Edmundo Carmona Antoranz · lore

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

On Sun, Nov 1, 2015 at 12:47 PM, Edmundo Carmona Antoranz <eantoranz@gmail.com> wrote:

Show 21 quoted lines
> 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`).

Show 26 quoted lines
> 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.

Show 5 quoted lines
> +               if (opts.quiet) {
> +                       opts.show_progress = 0;
> +               } else {
> +                       opts.show_progress = isatty(2);
> +               }
Style: drop unnecessary braces
Show 7 quoted lines
> +       }
> +
>         if (conflict_style) {
>                 opts.merge = 1; /* implied */
>                 git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
> --
> 2.6.1
Edmundo Carmona Antoranz· Nov 1, 2015, 17:58 UTC · re: Eric Sunshine · lore

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

On Sun, Nov 1, 2015 at 11:52 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 8 quoted lines
>> +               if (opts.quiet) {
>> +                       opts.show_progress = 0;
>> +               } else {
>> +                       opts.show_progress = isatty(2);
>> +               }
>
> Style: drop unnecessary braces
>
Ok. WIll do!
Jeff King· Nov 1, 2015, 19:19 UTC · re: Eric Sunshine · lore

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

On Sun, Nov 01, 2015 at 12:52:57PM -0500, Eric Sunshine wrote:
Show 22 quoted lines
> > 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_.

Show 16 quoted lines
> > +       /*
> > +        * 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
Eric Sunshine· Nov 1, 2015, 19:29 UTC · re: Jeff King · lore

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

On Sun, Nov 1, 2015 at 2:19 PM, Jeff King <peff@peff.net> wrote:
Show 16 quoted lines
> 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.

Edmundo Carmona Antoranz· Nov 1, 2015, 19:35 UTC · re: Eric Sunshine · lore

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

On Sun, Nov 1, 2015 at 1:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 5 quoted lines
>>> > +--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!
Eric Sunshine· Nov 1, 2015, 20:15 UTC · re: Edmundo Carmona Antoranz · lore

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

On Sun, Nov 1, 2015 at 2:35 PM, Edmundo Carmona Antoranz <eantoranz@gmail.com> wrote:

Show 13 quoted lines
> 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.

Show 6 quoted lines
> --[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`.
Edmundo Carmona Antoranz· Nov 1, 2015, 20:26 UTC · re: Eric Sunshine · lore

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

On Sun, Nov 1, 2015 at 2:15 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 5 quoted lines
>
>     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.

← back to recent threads