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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Nov 1, 2015 at 12:47 PM, Edmundo Carmona Antoranz
<eantoranz@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]