threads / rfc / 19358

RFC patchbuiltin-log: Add options to --coverletter

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

## tl;dr

10 messages between May 15, 2009 and May 16, 2009. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Joe Perches· May 15, 2009, 00:57 UTC · lore

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?
Show changes to builtin-log.c +19 −5
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="))
Junio C Hamano· May 15, 2009, 18:11 UTC · re: Joe Perches · lore

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

Joe Perches <joe@perches.com> writes:
Show 7 quoted lines
> 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].
Joe Perches· May 15, 2009, 20:19 UTC · re: Junio C Hamano · lore

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

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?
Show changes to builtin-log.c +18 −4
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="))
Junio C Hamano· May 15, 2009, 21:51 UTC · re: Joe Perches · lore

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

Joe Perches <joe@perches.com> writes:
Show 6 quoted lines
> 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...
Show 9 quoted lines
> 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;
Show 34 quoted lines
> +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.

Joe Perches· May 15, 2009, 22:07 UTC · re: Junio C Hamano · lore

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

On Fri, 2009-05-15 at 14:51 -0700, Junio C Hamano wrote:
Show 23 quoted lines
> 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.

Show 39 quoted lines
> 	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.
[]
Show 6 quoted lines
> 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]]
Junio C Hamano· May 16, 2009, 02:30 UTC · re: Joe Perches · lore

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

Joe Perches <joe@perches.com> writes:
Show 7 quoted lines
>> > +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.
Joe Perches· May 16, 2009, 00:46 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 3 files +32 −4

Documentation/git-format-patch.txt, builtin-log.c, contrib/completion/git-completion.bash

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
Junio C Hamano· May 16, 2009, 02:32 UTC · re: Joe Perches · lore

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

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.
Jeff King· May 16, 2009, 05:07 UTC · re: Junio C Hamano · lore

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

On Fri, May 15, 2009 at 11:11:13AM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> 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
James Cloos· May 16, 2009, 17:35 UTC · re: Joe Perches · lore

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

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

← back to recent threads