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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Joe Perches <joe@xxxxxxxxxxx> 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].
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]