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

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

[Joe Perches]

Currently the coverletter options for wrapping long lines
in the shortlog are: on, wrap as position72, with fixed indents.

I think these defaults can produce poor looking output.

This patch allows these to be optionally specified on the
command line with --cover-letter[=wrap[,pos[,in1[,in2]]]]

I'm not sure this is the right approach though.

Suggestions?

diff --git a/builtin-log.c b/builtin-log.c
index 5eaec5d..de26c04 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;
+static int coverletter_wraplen = 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_wraplen;
+	log.in1 = coverletter_indent1;
+	log.in2 = coverletter_indent2;
 	for (i = 0; i < nr; i++)
 		shortlog_add_commit(&log, list[i]);
 
@@ -866,8 +871,17 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			rev.subject_prefix = argv[i] + 17;
 		} else if (!prefixcmp(argv[i], "--suffix="))
 			fmt_patch_suffix = argv[i] + 9;
-		else if (!strcmp(argv[i], "--cover-letter"))
+		else if (!prefixcmp(argv[i], "--cover-letter")) {
 			cover_letter = 1;
+			if (*(argv[i] + 14) == '=') {
+				if (sscanf(argv[i] + 15, "%d,%d,%d,%d",
+					   &coverletter_wrap,
+					   &coverletter_wraplen,
+					   &coverletter_indent1,
+					   &coverletter_indent2) <= 0)
+					die("Need options for --cover-letter=");
+			}
+		}
 		else if (!strcmp(argv[i], "--no-binary"))
 			no_binary_diff = 1;
 		else if (!prefixcmp(argv[i], "--add-header="))

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

[Junio C Hamano]

Joe Perches <joe@perches.com> writes:

> Currently the coverletter options for wrapping long lines
> in the shortlog are: on, wrap as position72, with fixed indents.
>
> I think these defaults can produce poor looking output.
>
> This patch allows these to be optionally specified on the
> command line with --cover-letter[=wrap[,pos[,in1[,in2]]]]

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.

 - Currently we never generate a cover without being asked, but if we ever
   switch that default in the future, we would want to be able to decline
   it from the command line with --cover-letter=no (or --no-cover-letter).
   The "no" here is different from "please do not wrap but do produce
   cover letter".

 - Currently there is only one style of cover letter, but people may want
   to add a mechanism to let them use different styles that suit their
   project better in the future, and a natural syntax to ask for a
   different style is --cover-letter=style, where "style" may be
   "default", "shortlog", or some other token that the user invents.
   --cover-letter=no will still ask to suppress cover letters.

This is a tangent, but I do not think the current cover-letter that uses
shortlog matches everybody's needs.  The shortlog format lists commits
grouped by the author and does not number them, and it makes it hard to
match which message in the series corresponds to which entry in the cover
letter, especially when your series have a resend of somebody else's patch
in it.  I wouldn't be surprised if somebody comes up with a different
style that is based on "git log --reverse --oneline A..B" output (perhaps
without the shortened object name part) and name it the "oneline" style,
e.g.

    From: Jeff King

    *** BLURB HERE ***
    The following patches do ...

    1/2	parseopt: add OPT_NEGBIT (Réne Scharfe)
    2/2 ls-files: make --no-empty-directory negatable

     Documentation/technical/api-parse-options.txt |    4 +++
     ...
     test-parse-options.c                          |    1 +
     6 files changed, 45 insertions(+), 3 deletions(-)


Now, where does "line wrapping parameters" fit in the picture of this
possible future with multiple styles?  From the implementation convenience
viewpoint, you could make the line wrapping knobs specific to the
"shortlog" style.  It however is conceivable that "oneline" style (yet to
be written by somebody) may want to wrap its output, and the parameters it
would want to use may well be the same set.

I think it would make sense to introduce a separate parameter:

	--cover-letter-wrap=<pos>,<indent>,<wrap-offset>

at this point in your patch.  It does not add oneline or any other
different styles, but it would keep the door open for later additions
without breaking the UI.

By the way, don't people find the semantics of in1/in2 to shortlog
wrapping unnatural?  By the above three-tuple parameter, I meant:

	wrap at position [pos], indent the whole thing by this much
	[indent], and offset the second and subsequent lines by this much
	[wrap-offset].

and that reads much better than the -w option of shortlog that says

	wrap at position [pos], indent the whole thing by this much
	[in1], and offset the second and subsequent lines by this much
	[in2 minus in1].

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

[Joe Perches]

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?

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;
+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;
+			}
 		else if (!strcmp(argv[i], "--no-binary"))
 			no_binary_diff = 1;
 		else if (!prefixcmp(argv[i], "--add-header="))

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

[Junio C Hamano]

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.

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

[Joe Perches]

On Fri, 2009-05-15 at 14:51 -0700, Junio C Hamano wrote:
> 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;

This doesn't change the default behavior.
The default is still wrap enabled.

> 	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.

Prefixing "no-" to git arguments seems widely used.

Perhaps:
  --no-cover-letter-wrap
and
  --cover-letter-wrap=pos[,indent1[,indent2]]

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

[Junio C Hamano]

Joe Perches <joe@perches.com> writes:

>> > +static int coverletter_wrap = 1;
>> 
>> Do not change the default behaviour before people agree it is a good
>> feature;
>
> This doesn't change the default behavior.
> The default is still wrap enabled.

You are correct; my mistake.

> Prefixing "no-" to git arguments seems widely used.
>
> Perhaps:
>   --no-cover-letter-wrap

Again you are right; it looks much better.

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

[Joe Perches]

Perhaps this?

Signed-off-by: Joe Perches <joe@perches.com>
---
 Documentation/git-format-patch.txt     |   13 +++++++++++++
 builtin-log.c                          |   22 ++++++++++++++++++----
 contrib/completion/git-completion.bash |    1 +
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6f1fc80..f6b34ff 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -20,6 +20,8 @@ SYNOPSIS
 		   [--subject-prefix=Subject-Prefix]
 		   [--cc=<email>]
 		   [--cover-letter]
+		   [--cover-letter-wrap=width[,indent1[,indent2]]]
+		   [--no-cover-letter-wrap]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
 
@@ -168,6 +170,17 @@ if that is not set.
 	containing the shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
 
+--cover-letter-wrap=<width>[,<indent1>[,<indent2>]]]::
+	Linewrap the cover-letter shortlog output by wrapping each line at
+	`width`.  The first line of each entry is indented by `indent1`
+	spaces, and the second and subsequent lines are indented by
+	`indent2` spaces.
+	`width`, `indent1`, and `indent2` default to 72, 2 and 4 respectively.
+
+--no-cover-letter-wrap::
+	Do not linewrap the cover-letter shortlog output.
+	indent is fixed at 6.
+
 --suffix=.<sfx>::
 	Instead of using `.patch` as the suffix for generated
 	filenames, use specified suffix.  A common alternative is
diff --git a/builtin-log.c b/builtin-log.c
index 5eaec5d..271cbc1 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 cover_letter_wrap = 1;
+static int cover_letter_wrappos = 72;
+static int cover_letter_indent1 = 2;
+static int cover_letter_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 = cover_letter_wrap;
+	log.wrap = cover_letter_wrappos;
+	log.in1 = cover_letter_indent1;
+	log.in2 = cover_letter_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 (!strcmp(argv[i], "--no-cover-letter-wrap"))
+			cover_letter_wrap = 0;
+		else if (!prefixcmp(argv[i], "--cover-letter-wrap=")) {
+			if (sscanf(argv[i] + 20, "%d,%d,%d",
+				   &cover_letter_wrappos,
+				   &cover_letter_indent1,
+				   &cover_letter_indent2) <= 0)
+				die("Need options for --cover-letter-wrap=");
+			}
 		else if (!strcmp(argv[i], "--no-binary"))
 			no_binary_diff = 1;
 		else if (!prefixcmp(argv[i], "--add-header="))
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ad26b7c..2f5c42b 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -969,6 +969,7 @@ _git_format_patch ()
 			--full-index --binary
 			--not --all
 			--cover-letter
+			--no-cover-letter-wrap --cover-letter-wrap=
 			--no-prefix --src-prefix= --dst-prefix=
 			--inline --suffix= --ignore-if-in-upstream
 			--subject-prefix=
-- 
1.6.3.1.9.g95405b.dirty

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

[Junio C Hamano]

Joe Perches <joe@perches.com> writes:

> Perhaps this?

Yup, but that comes after the --- we see below ;-).

> Signed-off-by: Joe Perches <joe@perches.com>
> ---

The patch looks good.

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

[Jeff King]

On Fri, May 15, 2009 at 11:11:13AM -0700, Junio C Hamano wrote:

> This is a tangent, but I do not think the current cover-letter that uses
> shortlog matches everybody's needs.  The shortlog format lists commits
> grouped by the author and does not number them, and it makes it hard to
> match which message in the series corresponds to which entry in the cover
> letter, especially when your series have a resend of somebody else's patch
> in it.  I wouldn't be surprised if somebody comes up with a different
> style that is based on "git log --reverse --oneline A..B" output (perhaps
> without the shortened object name part) and name it the "oneline" style,
> e.g.
> 
>     From: Jeff King
> 
>     *** BLURB HERE ***
>     The following patches do ...
> 
>     1/2	parseopt: add OPT_NEGBIT (Réne Scharfe)
>     2/2 ls-files: make --no-empty-directory negatable

At one point I was working on --pretty=format specifiers for "total
number of commits" and "incremental commit number". The eventual goal
being an option like coverletter.logformat that you could set to
"%xi/%xn %s".

Sadly, the code got a bit messy because the feature straddles the line
of pretty.c and actual rev traversal. I started some refactoring, but
dropped it halfway through, and now of course it is woefully out of date
(a lesson in "merge early, merge often"). So I just pipe "git log
--oneline" through nl manually. ;)

So yes, I think somebody would be interested in alternate styles. And
while I think most people would want to set their default style as a
config variable, it may make sense to override on the command-line
(e.g., for a series that is mostly from you versus one that is from
mixed authors).

-Peff

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

[James Cloos]

>>>>> "Joe" == Joe Perches <joe@perches.com> writes:

Joe> Currently the coverletter options for wrapping long lines
Joe> in the shortlog are: on, wrap as position72, with fixed indents.

Joe> I think these defaults can produce poor looking output.

Joe> This patch allows these to be optionally specified on the
Joe> command line with --cover-letter[=wrap[,pos[,in1[,in2]]]]

Joe> I'm not sure this is the right approach though.

If one wants to be really cool, one could borrow the relevant code from
GNU fmt(1).  (Be sure to grab from a release before the switch to GPL3.)

GNU fmt(1) uses an algorithm based on TeX's and produces *much* better
results than anything else I've seen.

It is currently distributed as part of coreutils.

-JimC
-- 
James Cloos <cloos@jhcloos.com>         OpenPGP: 1024D/ED7DAEA6