Re: [PATCH 2/4] help.c: make term_columns() cached and export it

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

Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:

> diff --git a/help.c b/help.c
> index bc15066..75b8d4b 100644
> --- a/help.c
> +++ b/help.c
> @@ -5,26 +5,41 @@
>  #include "help.h"
>  #include "common-cmds.h"
>  
> -/* most GUI terminals set COLUMNS (although some don't export it) */
> -static int term_columns(void)
> +/* cache for term_columns() value. Set on first use or when
> + * installing a pager and replacing stdout.
> + */

Just a style.

	/*
         * We format multi-line
         * comment block like
         * this.
         */

> +static int term_columns_cache;
> +
> +/* Return cached value (iff set) or $COLUMNS (iff set and positive) or
> + * ioctl(1, TIOCGWINSZ).ws_col (if positive) or 80.
> + *
> + * $COLUMNS even if set, is usually not exported, so
> + * the variable can be used to override autodection.
> + */
> +int term_columns(void)
>  {
> -	char *col_string = getenv("COLUMNS");
> -	int n_cols;
> +	if (term_columns_cache)
> +		return term_columns_cache;
>  
> -	if (col_string && (n_cols = atoi(col_string)) > 0)
> -		return n_cols;
> +	{
> +		char *col_string = getenv("COLUMNS");
> +		int n_cols;
> +
> +		if (col_string && (n_cols = atoi(col_string)) > 0)
> +			return (term_columns_cache = n_cols);
> +	}

I can see the justification for the existing one inside the #ifdef below,
but why do you need the extra scope above?

We tend to avoid assignment as a part of another expression.  The old code
already violates that convention, but please do not make it worse by
introducing three new ones.