[PATCH] checkout: add --progress option

[PATCH] 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             | 17 ++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

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..e28c36b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -27,6 +27,8 @@ static const char * const checkout_usage[] = {
 	NULL,
 };
 
+static int option_progress = -1;
+
 struct checkout_opts {
 	int patch_mode;
 	int quiet;
@@ -417,7 +419,19 @@ 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);
+	/**
+	 * Rules to display progress:
+	 * -q is selected
+	 *      no verbiage
+	 * -q is _not_ selected and --no-progress _is_ selected,
+	 *      progress will be skipped
+	 * -q is _not_ selected and --progress _is_ selected,
+	 *      progress will be printed to stderr
+	 * -q is _not_ selected and --progress is 'undefined'
+	 *      progress will be printed to stderr _if_ working on a terminal
+	 */
+	opts.verbose_update = !o->quiet && (option_progress > 0 ||
+					   (option_progress < 0 && isatty(2)));
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 	parse_tree(tree);
@@ -1156,6 +1170,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", &option_progress, N_("force progress reporting")),
 		OPT_END(),
 	};
 
-- 
2.6.1

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

[Junio C Hamano]

Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:

> 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             | 17 ++++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> 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.
> +

Unlike some other codepaths like pack-objects (hence "fetch"), "git
checkout" uses start_progress_delay() to avoid showing the progress
when the operation finishes quickly.  I do not think the --progress
option should "force" the output in such a case, but the text reads
as if that is what happens.

Probably it is not a big deal, though.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bc703c0..e28c36b 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -27,6 +27,8 @@ static const char * const checkout_usage[] = {
>  	NULL,
>  };
>  
> +static int option_progress = -1;
> +
>  struct checkout_opts {
>  	int patch_mode;
>  	int quiet;
> @@ -417,7 +419,19 @@ 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);
> +	/**
> +	 * Rules to display progress:
> +	 * -q is selected
> +	 *      no verbiage

All the other say "progress will be..."; this only confuses readers
if "verbiage" is referring to the same "progress" or it is something
else (and if so at least two important things are left unsaid: (1)
if "progress" is also part of "verbiage", and (2) what kind of
output you are squelching).

And I cannot quite tell which among these possibilities (and there
may be others) you meant.

> +	 * -q is _not_ selected and --no-progress _is_ selected,
> +	 *      progress will be skipped
> +	 * -q is _not_ selected and --progress _is_ selected,
> +	 *      progress will be printed to stderr
> +	 * -q is _not_ selected and --progress is 'undefined'
> +	 *      progress will be printed to stderr _if_ working on a terminal
> +	 */
> +	opts.verbose_update = !o->quiet && (option_progress > 0 ||
> +					   (option_progress < 0 && isatty(2)));
>  	opts.src_index = &the_index;
>  	opts.dst_index = &the_index;
>  	parse_tree(tree);
> @@ -1156,6 +1170,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", &option_progress, N_("force progress reporting")),
>  		OPT_END(),
>  	};

I see some existing commands say "show progress" and some others say
"force progress reporting".  At some point we may want to pick one
and use it consistently (but that is not on a topic for this change).

Thanks.

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

[Eric Sunshine]

On Thu, Oct 29, 2015 at 9:23 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>
> ---
> -       opts.verbose_update = !o->quiet && isatty(2);
> +       /**
> +        * Rules to display progress:
> +        * -q is selected
> +        *      no verbiage
> +        * -q is _not_ selected and --no-progress _is_ selected,
> +        *      progress will be skipped
> +        * -q is _not_ selected and --progress _is_ selected,
> +        *      progress will be printed to stderr
> +        * -q is _not_ selected and --progress is 'undefined'
> +        *      progress will be printed to stderr _if_ working on a terminal
> +        */
> +       opts.verbose_update = !o->quiet && (option_progress > 0 ||
> +                                          (option_progress < 0 && isatty(2)));

Does this logic also need to be applied to the other instance where
isatty() is consulted in merge_working_tree()?

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Oct 29, 2015 at 9:23 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>
>> ---
>> -       opts.verbose_update = !o->quiet && isatty(2);
>> +       /**
>> +        * Rules to display progress:
>> +        * -q is selected
>> +        *      no verbiage
>> +        * -q is _not_ selected and --no-progress _is_ selected,
>> +        *      progress will be skipped
>> +        * -q is _not_ selected and --progress _is_ selected,
>> +        *      progress will be printed to stderr
>> +        * -q is _not_ selected and --progress is 'undefined'
>> +        *      progress will be printed to stderr _if_ working on a terminal
>> +        */
>> +       opts.verbose_update = !o->quiet && (option_progress > 0 ||
>> +                                          (option_progress < 0 && isatty(2)));
>
> Does this logic also need to be applied to the other instance where
> isatty() is consulted in merge_working_tree()?

This makes me wonder if option_progress and o->quiet change once
parse_options() finished doing its job.  If we know that these two
will never change once parse_options() is done, then we can
introduce a variable "show_progress", assign the value of that
variable to opts.verbose_update in these places and then arrange
that variable is set to the combination of quiet, option_progress
and isatty(2) just once after parse_options().

That is, something like this on top of Edmundo's patch.

 builtin/checkout.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e28c36b..53d7c49 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -27,7 +27,7 @@ static const char * const checkout_usage[] = {
 	NULL,
 };
 
-static int option_progress = -1;
+static int show_progress = 1;
 
 struct checkout_opts {
 	int patch_mode;
@@ -430,8 +430,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
 	 * -q is _not_ selected and --progress is 'undefined'
 	 *      progress will be printed to stderr _if_ working on a terminal
 	 */
-	opts.verbose_update = !o->quiet && (option_progress > 0 ||
-					   (option_progress < 0 && isatty(2)));
+	opts.verbose_update = show_progress;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 	parse_tree(tree);
@@ -515,7 +514,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 = show_progress;
 		topts.fn = twoway_merge;
 		if (opts->overwrite_ignore) {
 			topts.dir = xcalloc(1, sizeof(*topts.dir));
@@ -1143,6 +1142,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	struct branch_info new;
 	char *conflict_style = NULL;
 	int dwim_new_local_branch = 1;
+	int option_progress = -1;
 	struct option options[] = {
 		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
 		OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
@@ -1187,6 +1187,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
 			     PARSE_OPT_KEEP_DASHDASH);
 
+	show_progress = (!opts.quiet &&
+			 (0 < option_progress || (option_progress < 0 && isatty(2))));
+
 	if (conflict_style) {
 		opts.merge = 1; /* implied */
 		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);

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

[Eric Sunshine]

On Fri, Oct 30, 2015 at 2:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> On Thu, Oct 29, 2015 at 9:23 PM, Edmundo Carmona Antoranz
>> <eantoranz@gmail.com> wrote:
>>> +       opts.verbose_update = !o->quiet && (option_progress > 0 ||
>>> +                                          (option_progress < 0 && isatty(2)));
>>
>> Does this logic also need to be applied to the other instance where
>> isatty() is consulted in merge_working_tree()?
>
> This makes me wonder if option_progress and o->quiet change once
> parse_options() finished doing its job.  If we know that these two
> will never change once parse_options() is done, then we can
> introduce a variable "show_progress", assign the value of that
> variable to opts.verbose_update in these places and then arrange
> that variable is set to the combination of quiet, option_progress
> and isatty(2) just once after parse_options().
>
> That is, something like this on top of Edmundo's patch.
>
> @@ -27,7 +27,7 @@ static const char * const checkout_usage[] = {
> -static int option_progress = -1;
> +static int show_progress = 1;
> @@ -430,8 +430,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
> -       opts.verbose_update = !o->quiet && (option_progress > 0 ||
> -                                          (option_progress < 0 && isatty(2)));
> +       opts.verbose_update = show_progress;
> @@ -515,7 +514,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
> -               topts.verbose_update = !opts->quiet && isatty(2);
> +               topts.verbose_update = show_progress;
> @@ -1143,6 +1142,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> +       int option_progress = -1;
> @@ -1187,6 +1187,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>         argc = parse_options(argc, argv, prefix, options, checkout_usage,
>                              PARSE_OPT_KEEP_DASHDASH);
>
> +       show_progress = (!opts.quiet &&
> +                        (0 < option_progress || (option_progress < 0 && isatty(2))));

Yep, I was thinking that too. Also, Edmundo's explanatory comment
about how --quiet, --progress, and isatty() interact would move down
to this location (if it's deemed worth keeping).

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

[Jeff King]

On Fri, Oct 30, 2015 at 11:48:36AM -0700, Junio C Hamano wrote:

> > Does this logic also need to be applied to the other instance where
> > isatty() is consulted in merge_working_tree()?
> 
> This makes me wonder if option_progress and o->quiet change once
> parse_options() finished doing its job.  If we know that these two
> will never change once parse_options() is done, then we can
> introduce a variable "show_progress", assign the value of that
> variable to opts.verbose_update in these places and then arrange
> that variable is set to the combination of quiet, option_progress
> and isatty(2) just once after parse_options().
> 
> That is, something like this on top of Edmundo's patch.

Yeah, I like splitting it out like this.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index e28c36b..53d7c49 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -27,7 +27,7 @@ static const char * const checkout_usage[] = {
>  	NULL,
>  };
>  
> -static int option_progress = -1;
> +static int show_progress = 1;

You don't need this initialization, right? Looks like we assign
unconditionally to it in cmd_checkout.

> @@ -1143,6 +1142,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	struct branch_info new;
>  	char *conflict_style = NULL;
>  	int dwim_new_local_branch = 1;
> +	int option_progress = -1;
>  	struct option options[] = {
>  		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
>  		OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
> @@ -1187,6 +1187,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, options, checkout_usage,
>  			     PARSE_OPT_KEEP_DASHDASH);
>  
> +	show_progress = (!opts.quiet &&
> +			 (0 < option_progress || (option_progress < 0 && isatty(2))));
> +

I sometimes find it confusing when there are two variables with very
similar meanings (option_progress and show_progress here). I wonder if
we could use one variable, like:

  static int show_progress = -1;
  ...
  OPT_BOOL(0, "progress", &show_progress, ...);
  ...
  parse_options(...);
  if (show_progress < 0) {
	if (opts.quiet)
		show_progress = 0;
	else
		show_progress = isatty(2);
  }

That somehow is much clearer to me, especially around the behavior of
"-q --progress". Mine does the opposite of what you wrote above, but I
think it makes more sense.

I can live with it either way, though. :)

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I sometimes find it confusing when there are two variables with very
> similar meanings (option_progress and show_progress here). I wonder if
> we could use one variable, like:
>
>   static int show_progress = -1;
>   ...
>   OPT_BOOL(0, "progress", &show_progress, ...);
>   ...
>   parse_options(...);
>   if (show_progress < 0) {
> 	if (opts.quiet)
> 		show_progress = 0;
> 	else
> 		show_progress = isatty(2);
>   }
>
> That somehow is much clearer to me, especially around the behavior of
> "-q --progress". Mine does the opposite of what you wrote above, but I
> think it makes more sense.
>
> I can live with it either way, though. :)

Actually, using a single variable is my preference.  In this case I
wanted to illustrate that the value parsed by parse_options() does
not have to be the one that is used in the final location deep in
the callchain for educational purposes, and I found it clearer to
use two separate variables in the illustration.

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

[Edmundo Carmona Antoranz]

On Fri, Oct 30, 2015 at 1:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Actually, using a single variable is my preference.  In this case I
> wanted to illustrate that the value parsed by parse_options() does
> not have to be the one that is used in the final location deep in
> the callchain for educational purposes, and I found it clearer to
> use two separate variables in the illustration.

>From all your feedback, I think I got a working example not using the
static variable, also reworked the wording to explain in what cases
progress will be skipped.

So... on top of my patch, I think this sums it up:

 builtin/checkout.c | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e28c36b..a2e5cd3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -27,8 +27,6 @@ static const char * const checkout_usage[] = {
       NULL,
};

-static int option_progress = -1;
-
struct checkout_opts {
       int patch_mode;
       int quiet;
@@ -39,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;
@@ -406,6 +405,17 @@ static void describe_detached_head(const char
*msg, struct commit *commit)
       strbuf_release(&sb);
}

+/*
+ * Any of these conditions will make progress output be skipped:
+ * - selected --quiet
+ * - selected --no-progress
+ * - (didn't select --progress nor --no-progress) and not working on a terminal
+ */
+static int verbose_update(const struct checkout_opts *o)
+{
+       return !o->quiet && o->show_progress && (o->show_progress >= 0
|| isatty(2));
+}
+
static int reset_tree(struct tree *tree, const struct checkout_opts *o,
                     int worktree, int *writeout_error)
{
@@ -419,19 +429,7 @@ static int reset_tree(struct tree *tree, const
struct checkout_opts *o,
       opts.reset = 1;
       opts.merge = 1;
       opts.fn = oneway_merge;
-       /**
-        * Rules to display progress:
-        * -q is selected
-        *      no verbiage
-        * -q is _not_ selected and --no-progress _is_ selected,
-        *      progress will be skipped
-        * -q is _not_ selected and --progress _is_ selected,
-        *      progress will be printed to stderr
-        * -q is _not_ selected and --progress is 'undefined'
-        *      progress will be printed to stderr _if_ working on a terminal
-        */
-       opts.verbose_update = !o->quiet && (option_progress > 0 ||
-                                          (option_progress < 0 && isatty(2)));
+       opts.verbose_update = verbose_update(o);
       opts.src_index = &the_index;
       opts.dst_index = &the_index;
       parse_tree(tree);
@@ -515,7 +513,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 = verbose_update(opts);
               topts.fn = twoway_merge;
               if (opts->overwrite_ignore) {
                       topts.dir = xcalloc(1, sizeof(*topts.dir));
@@ -1170,7 +1168,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", &option_progress, N_("force
progress reporting")),
+               OPT_BOOL(0, "progress", &opts.show_progress, N_("force
progress reporting")),
               OPT_END(),
       };

@@ -1178,6 +1176,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);

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

[Edmundo Carmona Antoranz]

One line got wrapped around, hope that's not a problem to understand
the whole concept.

> +/*
> + * Any of these conditions will make progress output be skipped:
> + * - selected --quiet
> + * - selected --no-progress
> + * - (didn't select --progress nor --no-progress) and not working on a terminal
> + */
> +static int verbose_update(const struct checkout_opts *o)
> +{
> +       return !o->quiet && o->show_progress && (o->show_progress >= 0
> || isatty(2));
> +}
> +

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

[Edmundo Carmona Antoranz]

Ok.... this is like the previous patch (on top of my patch) but does
post_processing right after parse_options and so there's no need for
the function I had introduced before. Hope it can get your blessings
so I can send-email the _second_ patch for this feature (following the
one that has already being merged into pu, right?).

 builtin/checkout.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e28c36b..9e78835 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -27,8 +27,6 @@ static const char * const checkout_usage[] = {
       NULL,
};

-static int option_progress = -1;
-
struct checkout_opts {
       int patch_mode;
       int quiet;
@@ -39,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;
@@ -419,19 +418,7 @@ static int reset_tree(struct tree *tree, const
struct checkout_opts *o,
       opts.reset = 1;
       opts.merge = 1;
       opts.fn = oneway_merge;
-       /**
-        * Rules to display progress:
-        * -q is selected
-        *      no verbiage
-        * -q is _not_ selected and --no-progress _is_ selected,
-        *      progress will be skipped
-        * -q is _not_ selected and --progress _is_ selected,
-        *      progress will be printed to stderr
-        * -q is _not_ selected and --progress is 'undefined'
-        *      progress will be printed to stderr _if_ working on a terminal
-        */
-       opts.verbose_update = !o->quiet && (option_progress > 0 ||
-                                          (option_progress < 0 && isatty(2)));
+       opts.verbose_update = o->show_progress;
       opts.src_index = &the_index;
       opts.dst_index = &the_index;
       parse_tree(tree);
@@ -515,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));
@@ -1170,7 +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", &option_progress, N_("force
progress reporting")),
+               OPT_BOOL(0, "progress", &opts.show_progress, N_("force
progress reporting")),
               OPT_END(),
       };

@@ -1178,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);
@@ -1187,6 +1175,16 @@ 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
+        * Any of these 3 conditions will make progress output be skipped:
+        * - selected --quiet
+        * - selected --no-progress
+        * - didn't select --progress and not working on a terminal
+        */
+       opts.show_progress = !opts.quiet && opts.show_progress &&
+                            (opts.show_progress >= 0 || isatty(2));
+
       if (conflict_style) {
               opts.merge = 1; /* implied */
               git_xmerge_config("merge.conflictstyle", conflict_style, NULL);

On Fri, Oct 30, 2015 at 6:58 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> On Fri, Oct 30, 2015 at 1:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Actually, using a single variable is my preference.  In this case I
>> wanted to illustrate that the value parsed by parse_options() does
>> not have to be the one that is used in the final location deep in
>> the callchain for educational purposes, and I found it clearer to
>> use two separate variables in the illustration.
>
> From all your feedback, I think I got a working example not using the
> static variable, also reworked the wording to explain in what cases
> progress will be skipped.
>
> So... on top of my patch, I think this sums it up:
>
>  builtin/checkout.c | 33 ++++++++++++++++-----------------
> 1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index e28c36b..a2e5cd3 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -27,8 +27,6 @@ static const char * const checkout_usage[] = {
>        NULL,
> };
>
> -static int option_progress = -1;
> -
> struct checkout_opts {
>        int patch_mode;
>        int quiet;
> @@ -39,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;
> @@ -406,6 +405,17 @@ static void describe_detached_head(const char
> *msg, struct commit *commit)
>        strbuf_release(&sb);
> }
>
> +/*
> + * Any of these conditions will make progress output be skipped:
> + * - selected --quiet
> + * - selected --no-progress
> + * - (didn't select --progress nor --no-progress) and not working on a terminal
> + */
> +static int verbose_update(const struct checkout_opts *o)
> +{
> +       return !o->quiet && o->show_progress && (o->show_progress >= 0
> || isatty(2));
> +}
> +
> static int reset_tree(struct tree *tree, const struct checkout_opts *o,
>                      int worktree, int *writeout_error)
> {
> @@ -419,19 +429,7 @@ static int reset_tree(struct tree *tree, const
> struct checkout_opts *o,
>        opts.reset = 1;
>        opts.merge = 1;
>        opts.fn = oneway_merge;
> -       /**
> -        * Rules to display progress:
> -        * -q is selected
> -        *      no verbiage
> -        * -q is _not_ selected and --no-progress _is_ selected,
> -        *      progress will be skipped
> -        * -q is _not_ selected and --progress _is_ selected,
> -        *      progress will be printed to stderr
> -        * -q is _not_ selected and --progress is 'undefined'
> -        *      progress will be printed to stderr _if_ working on a terminal
> -        */
> -       opts.verbose_update = !o->quiet && (option_progress > 0 ||
> -                                          (option_progress < 0 && isatty(2)));
> +       opts.verbose_update = verbose_update(o);
>        opts.src_index = &the_index;
>        opts.dst_index = &the_index;
>        parse_tree(tree);
> @@ -515,7 +513,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 = verbose_update(opts);
>                topts.fn = twoway_merge;
>                if (opts->overwrite_ignore) {
>                        topts.dir = xcalloc(1, sizeof(*topts.dir));
> @@ -1170,7 +1168,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", &option_progress, N_("force
> progress reporting")),
> +               OPT_BOOL(0, "progress", &opts.show_progress, N_("force
> progress reporting")),
>                OPT_END(),
>        };
>
> @@ -1178,6 +1176,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);

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

[Junio C Hamano]

Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:

> +       /*
> +        * Final processing of show_progress
> +        * Any of these 3 conditions will make progress output be skipped:
> +        * - selected --quiet
> +        * - selected --no-progress
> +        * - didn't select --progress and not working on a terminal
> +        */
> +       opts.show_progress = !opts.quiet && opts.show_progress &&
> +                            (opts.show_progress >= 0 || isatty(2));
> +

I do find what Peff showed us a lot easier to follow.

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

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

[Edmundo Carmona Antoranz]

On Sat, Oct 31, 2015 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I do find what Peff showed us a lot easier to follow.
>
>         if (opts.show_progress < 0) {
>                 if (opts.quiet)
>                         opts.show_progress = 0;
>                 else
>                         opts.show_progress = isatty(2);
>         }
>

Ok.... let me rewrite it that way. Other than that, the other things are ok?

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

[Edmundo Carmona Antoranz]

I just noticed something interesting.

On Sat, Oct 31, 2015 at 11:42 AM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> On Sat, Oct 31, 2015 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I do find what Peff showed us a lot easier to follow.
>>
>>         if (opts.show_progress < 0) {
>>                 if (opts.quiet)
>>                         opts.show_progress = 0;
>>                 else
>>                         opts.show_progress = isatty(2);
>>         }
>>
>
> Ok.... let me rewrite it that way. Other than that, the other things are ok?

In Peff's implementation I think he uses -1 as --no-progress, 1 as
--progress and 0 as undefined, right?

In my implementation I'm using -1 as undefined and 0 as --no-progress.
What would be the standard approach? From what I can see on
parse_options's behavior, if you select --no-progress, the variable
ends up with a 0, which makes me think I'm using the right approach.

End result with my assumptions would be:

        if (opts.show_progress) {
                /* user selected --progress or didn't specify */
                if (opts.quiet) {
                        opts.show_progress = 0;
                } else if (opts.show_progress < 0) {
                        opts.show_progress = isatty(2);
                }
        }

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

[Jeff King]

On Sat, Oct 31, 2015 at 12:14:39PM -0600, Edmundo Carmona Antoranz wrote:

> On Sat, Oct 31, 2015 at 11:42 AM, Edmundo Carmona Antoranz
> <eantoranz@gmail.com> wrote:
> > On Sat, Oct 31, 2015 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> >> I do find what Peff showed us a lot easier to follow.
> >>
> >>         if (opts.show_progress < 0) {
> >>                 if (opts.quiet)
> >>                         opts.show_progress = 0;
> >>                 else
> >>                         opts.show_progress = isatty(2);
> >>         }
> >>
> >
> > Ok.... let me rewrite it that way. Other than that, the other things are ok?
> 
> In Peff's implementation I think he uses -1 as --no-progress, 1 as
> --progress and 0 as undefined, right?

I didn't mean to, though I don't promise I didn't send something buggy.
It looks right to me, though:

  if (opts.show_progress < 0) { /* if the user didn't say... */
          if (opts.quiet)
		opts.show_progress = 0; /* quiet means "no progress" */
	  else
		opts.show_progress = isatty(2); /* returns 0/1 bool */
  }

> In my implementation I'm using -1 as undefined and 0 as --no-progress.
> What would be the standard approach?

That's standard. And "1" is "--progress".

> From what I can see on
> parse_options's behavior, if you select --no-progress, the variable
> ends up with a 0, which makes me think I'm using the right approach.
> 
> End result with my assumptions would be:
> 
>         if (opts.show_progress) {
>                 /* user selected --progress or didn't specify */
>                 if (opts.quiet) {
>                         opts.show_progress = 0;
>                 } else if (opts.show_progress < 0) {
>                         opts.show_progress = isatty(2);
>                 }
>         }

The difference between mine and yours is that in mine, "--progress"
trumps "--quiet", whereas it is the other way around in yours. I don't
know if it is a huge deal, but mine makes more sense to me (because
"--progress" is more specific than "--quiet", which might silence other
messages, too).

-Peff

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

[Eric Sunshine]

On Fri, Oct 30, 2015 at 10:11 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> wrote:
> Ok.... this is like the previous patch (on top of my patch) but does
> post_processing right after parse_options and so there's no need for
> the function I had introduced before. Hope it can get your blessings
> so I can send-email the _second_ patch for this feature (following the
> one that has already being merged into pu, right?).

Patches in 'next' are pretty much set in stone, and those in 'master'
definitely are, but 'pu' is volatile, so just send the entire patch
revised, tagged v2 (or v3, etc.), rather than sending a patch to fix
what is in 'pu', and Junio will replace the previous version with the
new one.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index e28c36b..9e78835 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -27,8 +27,6 @@ static const char * const checkout_usage[] = {
>        NULL,
> };
>
> -static int option_progress = -1;
> -
> struct checkout_opts {
>        int patch_mode;
>        int quiet;
> @@ -39,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;
> @@ -419,19 +418,7 @@ static int reset_tree(struct tree *tree, const
> struct checkout_opts *o,
>        opts.reset = 1;
>        opts.merge = 1;
>        opts.fn = oneway_merge;
> -       /**
> -        * Rules to display progress:
> -        * -q is selected
> -        *      no verbiage
> -        * -q is _not_ selected and --no-progress _is_ selected,
> -        *      progress will be skipped
> -        * -q is _not_ selected and --progress _is_ selected,
> -        *      progress will be printed to stderr
> -        * -q is _not_ selected and --progress is 'undefined'
> -        *      progress will be printed to stderr _if_ working on a terminal
> -        */
> -       opts.verbose_update = !o->quiet && (option_progress > 0 ||
> -                                          (option_progress < 0 && isatty(2)));
> +       opts.verbose_update = o->show_progress;
>        opts.src_index = &the_index;
>        opts.dst_index = &the_index;
>        parse_tree(tree);
> @@ -515,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));
> @@ -1170,7 +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", &option_progress, N_("force
> progress reporting")),
> +               OPT_BOOL(0, "progress", &opts.show_progress, N_("force
> progress reporting")),
>                OPT_END(),
>        };
>
> @@ -1178,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);
> @@ -1187,6 +1175,16 @@ 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
> +        * Any of these 3 conditions will make progress output be skipped:
> +        * - selected --quiet
> +        * - selected --no-progress
> +        * - didn't select --progress and not working on a terminal
> +        */
> +       opts.show_progress = !opts.quiet && opts.show_progress &&
> +                            (opts.show_progress >= 0 || isatty(2));
> +
>        if (conflict_style) {
>                opts.merge = 1; /* implied */
>                git_xmerge_config("merge.conflictstyle", conflict_style, NULL);

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

[Edmundo Carmona Antoranz]

On Sat, Oct 31, 2015 at 6:45 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Patches in 'next' are pretty much set in stone, and those in 'master'
> definitely are, but 'pu' is volatile, so just send the entire patch
> revised, tagged v2 (or v3, etc.), rather than sending a patch to fix
> what is in 'pu', and Junio will replace the previous version with the
> new one.
>

Will do!