Re: [PATCH 2/2] format-patch: Add --cover-letter-wrap

[Stephen Boyd <bebarino@gmail.com>]

Joe Perches wrote:
> @@ -792,6 +806,27 @@ static int output_directory_callback(const struct option *opt, const char *arg,
>  	return 0;
>  }
>  
> +static int cls_callback(const struct option *opt, const char *arg, int unset)
> +{
> +	if (unset)
> +		cls.cover_letter_wrap = 0;
> +	else {
> +		int i1, i2, i3;
> +		if (!arg)
> +			return 1;
> +		int arg_count = sscanf(arg, "%d,%d,%d", &i1, &i2, &i3);
> +		if (arg_count <= 0)
> +			return 1;
> +		if (arg_count >= 1)
> +			cls.cover_letter_wrappos = i1;
> +		if (arg_count >= 2)
> +			cls.cover_letter_indent1 = i2;
> +		if (arg_count >= 3)
> +			cls.cover_letter_indent2 = i3;
> +		}

This bracket is one indent off.

I'm not sure, but can this be simplified to just setting the struct
members directly through sscanf? You won't need to have these if's in
that case. I think something like --cover-letter-wrap="" would be
equivalent to just using the defaults and not an error. Does that sound
right?

> +	return 0;
> +}
> +
>  static int thread_callback(const struct option *opt, const char *arg, int unset)
>  {
>  	int *thread = (int *)opt->value;
> @@ -875,6 +910,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			    "print patches to standard out"),
>  		OPT_BOOLEAN(0, "cover-letter", &cover_letter,
>  			    "generate a cover letter"),
> +		{ OPTION_CALLBACK, 0, "cover-letter-wrap", &cls, NULL,
> +			    "control the cover letter format",
> +			    PARSE_OPT_OPTARG, cls_callback },

Why is this PARSE_OPT_OPTARG? I only see the choice of having arguments
or prefixed with a --no. Also, please use PARSE_OPT_LITERAL_ARGHELP and
give it the help string you use in the docs
(<width>[,<indent1>[,<indent2>]]).