Re: [PATCH] status: hint the user about -uno if read_directory takes too long

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index bbba728..e91d06f 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -178,6 +178,10 @@ advice.*::
>  		the template shown when writing commit messages in
>  		linkgit:git-commit[1], and in the help message shown
>  		by linkgit:git-checkout[1] when switching branch.
> +	statusUno::
> +		If collecting untracked files in linkgit:git-status[1]
> +		takes more than 2 seconds, hint the user that the option
> +		`-uno` could be used to stop collecting untracked files.

It looks to me that the way this paragraph conveys information is
vastly different from all the others in the section.  The section
begins with "by setting their corresponding variables to false
various advice messages can be squelched; here are the list of
variables and which advice message each of them controls", so the
description should be in "variable:: which advice message" form.

The noise this introduces to the test suite is a bit irritating and
makes us think twice if this really a good change.

> diff --git a/wt-status.c b/wt-status.c
> index ef405d0..6fde08b 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -540,7 +540,16 @@ void wt_status_collect(struct wt_status *s)
>  		wt_status_collect_changes_initial(s);
>  	else
>  		wt_status_collect_changes_index(s);
> -	wt_status_collect_untracked(s);
> +	if (s->show_untracked_files && advice_status_uno) {
> +		struct timeval tv1, tv2;
> +		gettimeofday(&tv1, NULL);
> +		wt_status_collect_untracked(s);
> +		gettimeofday(&tv2, NULL);
> +		s->untracked_in_ms =
> +			(uint64_t)tv2.tv_sec * 1000 + tv2.tv_usec / 1000 -
> +			((uint64_t)tv1.tv_sec * 1000 + tv1.tv_usec / 1000);
> +	} else
> +		wt_status_collect_untracked(s);
>  }

This is not wrong per-se but it took me two reads to spot that this
is not "if advise is active, do the timer but do not collect;
otherwise do just collect as before".  I wonder if we can structure
the code a bit better to make the timing bit less loud.

>  static void wt_status_print_unmerged(struct wt_status *s)
> @@ -1097,6 +1106,15 @@ void wt_status_print(struct wt_status *s)
>  		wt_status_print_other(s, &s->untracked, _("Untracked files"), "add");
>  		if (s->show_ignored_files)
>  			wt_status_print_other(s, &s->ignored, _("Ignored files"), "add -f");
> +		if (advice_status_uno && s->untracked_in_ms > 2000) {
> +			status_printf_ln(s, GIT_COLOR_NORMAL,
> +					 _("It took %.2f seconds to collect untracked files."),
> +					 (float)s->untracked_in_ms / 1000);
> +			status_printf_ln(s, GIT_COLOR_NORMAL,
> +					 _("If it happens often, you may want to use option -uno"));
> +			status_printf_ln(s, GIT_COLOR_NORMAL,
> +					 _("to speed up by stopping displaying untracked files"));
> +		}

"to speed up by stopping displaying untracked files" does not look
like giving a balanced suggestion.  It is increasing the risk of
forgetting about newly created files the user may want to add, but
the risk is not properly warned.

I tend to agree that the new advice would help users if phrased in a
right way.  Do we want them in COLOR_NORMAL, or do we want to make
them stand out a bit more (do we have COLOR_BLINK ;-)?