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

From: Jeff King <peff@peff.net>
Date: Oct 30, 2015, 19:31 UTC
Message-ID: <20151030193151.GB5336@sigill.intra.peff.net>
In-Reply-To: <xmqqh9l8kxa3.fsf@gitster.mtv.corp.google.com>

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

Show 12 quoted lines

> > 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.

Show 10 quoted lines

> 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.

Show 15 quoted lines

> @@ -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