[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gameoftrees
Subject:    Re: improve invalid number error reporting
From:       Stefan Sperling <stsp () stsp ! name>
Date:       2022-02-09 14:30:11
Message-ID: YgPP8zJ9jaHhqNlc () benson ! stsp ! name
[Download RAW message or body]

On Wed, Feb 09, 2022 at 02:09:46PM +0100, Omar Polo wrote:
> Hello,
> 
> just another aesthetic diff :)
> 
> This tweaks the strtonum error reporting to be more in line with the
> example in the manpage, which also reads better IMHO.
> 
> before:
> 
> 	% got log -C 66
> 	got: -C option too large: Result too large
> 	% got log -l abc
> 	got: -l option invalid: Invalid argument
> 
> after:
> 
> 	% ./got/got log -C 66
> 	got: number of context lines is too large: 66
> 	% ./got/got log -l abc
> 	got: number of commits is invalid: abc
> 
> ok?

Yes, thanks!

In case you have not yet done it, please make sure tests are still passing.
I doubt they would break as nothing seems to be checking these error cases.
But it won't hurt to run regress anyway.

> (there is another use of strtonum in got-read-gotconfig/parse.c that
> doesn't follow the manpage patter but produces a similar error message
> anyway so I haven't modified it.)
> 
> diff d75b4088b08f12aea8079aad55996a65b7b312c8 /tmp/got
> blob - 03281c064021bf591b75ae61f102c19de7a4658f
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -4110,12 +4110,14 @@ cmd_log(int argc, char *argv[])
>  			diff_context = strtonum(optarg, 0, GOT_DIFF_MAX_CONTEXT,
>  			    &errstr);
>  			if (errstr != NULL)
> -				err(1, "-C option %s", errstr);
> +				errx(1, "number of context lines is %s: %s",
> +				    errstr, optarg);
>  			break;
>  		case 'l':
>  			limit = strtonum(optarg, 0, INT_MAX, &errstr);
>  			if (errstr != NULL)
> -				err(1, "-l option %s", errstr);
> +				errx(1, "number of commits is %s: %s",
> +				    errstr, optarg);
>  			break;
>  		case 'b':
>  			log_branches = 1;
> @@ -4530,7 +4532,8 @@ cmd_diff(int argc, char *argv[])
>  			diff_context = strtonum(optarg, 0, GOT_DIFF_MAX_CONTEXT,
>  			    &errstr);
>  			if (errstr != NULL)
> -				err(1, "-C option %s", errstr);
> +				errx(1, "number of context lines is %s: %s",
> +				    errstr, optarg);
>  			break;
>  		case 'r':
>  			repo_path = realpath(optarg, NULL);
> blob - 982154f8af09a3beb7bda5bd21732225bfbeeade
> file + tog/tog.c
> --- tog/tog.c
> +++ tog/tog.c
> @@ -3884,7 +3884,8 @@ cmd_diff(int argc, char *argv[])
>  			diff_context = strtonum(optarg, 0, GOT_DIFF_MAX_CONTEXT,
>  			    &errstr);
>  			if (errstr != NULL)
> -				err(1, "-C option %s", errstr);
> +				errx(1, "number of context lines is %s: %s",
> +				    errstr, errstr);
>  			break;
>  		case 'r':
>  			repo_path = realpath(optarg, NULL);
> 
> 



[prev in list] [next in list] [prev in thread] [next in thread]