Re: [PATCH] Tweak the handling of $HISTSIZE edge cases [PR gdb/16999]

[Pedro Alves <palves@redhat.com>]

On 05/21/2015 11:50 PM, Patrick Palka wrote:

The test is OK.  Good use of with_test_prefix.

Minor nit: I found no other use of with_test_prefix with
spaces around the '='.

> diff --git a/gdb/top.c b/gdb/top.c
> index 74e1e07..38b4e5d 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -1684,17 +1684,21 @@ init_history (void)
>    if (tmpenv)
>      {
>        int var;
> +      char *endptr;
>  
> -      var = atoi (tmpenv);
> -      if (var < 0)
> -	{
> -	  /* Prefer ending up with no history rather than overflowing
> -	     readline's history interface, which uses signed 'int'
> -	     everywhere.  */
> -	  var = 0;
> -	}
> +      var = strtol (tmpenv, &endptr, 10);
>  
> -      history_size_setshow_var = var;
> +      /* If HISTSIZE is the empty string, negative, or non-numeric then set the
> +	 history size to unlimited.  This behavior is mostly consistent with
> +	 that of bash.  Whereas bash ignores a non-numeric HISTSIZE, we set the
> +	 history to unlimited in that case to avoid potentially truncating the
> +	 user's history.  */
> +      if (strlen (tmpenv) == 0
> +	  || var < 0
> +	  || *endptr != '\0')

If I'm reading correctly, this treats HISTSIZE=" " as "disable history".
Is that intended?

Also, a nit: I find it a bit odd to see strlen to check empty string
in one case, and != '\0' in another, instead of:

      if (*tmpenv == '\0'
	  || var < 0
	  || *endptr != '\0')

> +	history_size_setshow_var = -1;
> +      else
> +	history_size_setshow_var = var;
>      }
>    /* If the init file hasn't set a size yet, pick the default.  */
>    else if (history_size_setshow_var == -2)

Thanks,
Pedro Alves