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