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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2015, 19:37 UTC
Message-ID: <xmqq4mh8kv0e.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20151030193151.GB5336@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

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