Re: [RFC PATCH] builtin-log: Add options to --coverletter

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

Joe Perches <joe@perches.com> writes:

> On Fri, 2009-05-15 at 11:11 -0700, Junio C Hamano wrote:
>> I think it makes sense to let users affect how the short-log in the cover
>> letter is generated.  I do not think overloading the --cover-letter option
>> for doing it is the ideal approach, though.
>
> OK.  How about this patch?

I'd suggest...

> diff --git a/builtin-log.c b/builtin-log.c
> index 5eaec5d..49fd42a 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -460,6 +460,11 @@ static void add_header(const char *value)
>  static int thread = 0;
>  static int do_signoff = 0;
>  
> +static int coverletter_wrap = 1;

Do not change the default behaviour before people agree it is a good
feature;

	static int coverletter_wrap;

> +static int coverletter_wrappos = 72;
> +static int coverletter_indent1 = 2;
> +static int coverletter_indent2 = 4;
> +
>  static int git_format_config(const char *var, const char *value, void *cb)
>  {
>  	if (!strcmp(var, "format.headers")) {
> @@ -668,10 +673,10 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	strbuf_release(&sb);
>  
>  	shortlog_init(&log);
> -	log.wrap_lines = 1;
> -	log.wrap = 72;
> -	log.in1 = 2;
> -	log.in2 = 4;
> +	log.wrap_lines = coverletter_wrap;
> +	log.wrap = coverletter_wrappos;
> +	log.in1 = coverletter_indent1;
> +	log.in2 = coverletter_indent2;
>  	for (i = 0; i < nr; i++)
>  		shortlog_add_commit(&log, list[i]);
>  
> @@ -868,6 +873,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			fmt_patch_suffix = argv[i] + 9;
>  		else if (!strcmp(argv[i], "--cover-letter"))
>  			cover_letter = 1;
> +		else if (!prefixcmp(argv[i], "--cover-letter-wrap=")) {
> +			if (sscanf(argv[i] + 20, "%d,%d,%d",
> +				   &coverletter_wrappos,
> +				   &coverletter_indent1,
> +				   &coverletter_indent2) <= 0)
> +				die("Need options for --cover-letter-wrap=");
> +			if (coverletter_wrappos == 0)
> +				coverletter_wrap = 0;

... lose this "if ()"; if you are asking for --cover-letter-wrap from the
command line explicitly, you do want the result to be wrapped.

In order to prepare yourself for change of default in the future (or
adding configurable defaults), the command line parser (the sscanf()
above) needs to understand something like "--cover-letter-linewrap=no", in
addition to the up-to-three integers it currently takes via sscanf().
Treating "the resulting line should be wrapped at 0 column" as "please do
not wrap" may work in practice but I do not think it is a good style.