[PATCH] Support wrapping commit messages when you read them

[PATCH] Support wrapping commit messages when you read them

[Sidney San Martín]

Fairly simpleminded line wrapping that makes commit messages
readable if they weren’t wrapped by the committer.

- Use strbuf_add_wrapped_text() to do the dirty work
- Detect simple lists which begin with "+ ", "* ", or "- " and indent
  them appropriately (like this line)
- Print lines which begin with whitespace as-is (e.g. code samples)

Add --wrap[=<width>] and --no-wrap to commands that pretty-print commit
messages, and add log.wrap and log.wrap.width configuration options.

log.wrap defaults to never, and can be set to never/false, auto/true,
or always. If auto, hijack want_color() to decide whether it’s
appropriate to use line wrapping. (This is a little hacky, but as far
as I can tell the conditions for auto color and auto wrapping are the
same. Maybe it would make sense to rename want_color()?)

log.wrap.width defaults to 80.

Signed-off-by: Sidney San Martín <s@sidneysm.com>
---

I hope I’m doing this right!

Consider this a first draft of the new feature I brought up a few weeks ago.


 Documentation/config.txt         |    8 ++++
 Documentation/pretty-options.txt |   14 +++++++
 commit.h                         |    6 +++
 log-tree.c                       |    1 +
 pretty.c                         |   71 +++++++++++++++++++++++++++++++++++++-
 revision.c                       |   10 +++++
 revision.h                       |    3 ++
 7 files changed, 112 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e63b59..b8c1a81 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1403,6 +1403,14 @@ log.showroot::
 	Tools like linkgit:git-log[1] or linkgit:git-whatchanged[1], which
 	normally hide the root commit will now show it. True by default.
 
+log.wrap::
+	Commit message wrapping. May be set to always, false (or never) or
+	auto (or true), in which case commit messages are only wrapped when
+	the output is to a terminal. Defaults to false. See linkgit:git-log[1].
+
+log.wrap.width::
+	Line width for commit message wrapping. Defaults to 80 characters.
+
 mailmap.file::
 	The location of an augmenting mailmap file. The default
 	mailmap, located in the root of the repository, is loaded
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 2a3dc86..7601a43 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -10,6 +10,20 @@
 Note: you can specify the default pretty format in the repository
 configuration (see linkgit:git-config[1]).
 
+--wrap[=<width>]::
+	Word-wrap commit messages to the specified width, 80 characters
+	by default. Lines beginning with +, *, or - and a space, or with a
+	number followed by a period and a space, are interpreted as lists
+	and wrapped with a hanging indent. Lines beginning with whitespace
+	(e.g. code samples) are left as-is.
++
+Note: you can specify the default wrapping behavior in the repository
+configuration (see linkgit:git-config[1]).
+
+--no-wrap::
+	Turn off commit message wrapping. This is the default (unless
+	otherwise specified in the repository configuration).
+
 --abbrev-commit::
 	Instead of showing the full 40-byte hexadecimal commit object
 	name, show only a partial prefix.  Non default number of
diff --git a/commit.h b/commit.h
index 4df3978..1321666 100644
--- a/commit.h
+++ b/commit.h
@@ -86,6 +86,12 @@ struct pretty_print_context {
 	int show_notes;
 	struct reflog_walk_info *reflog_info;
 	const char *output_encoding;
+	struct wrap_options {
+		unsigned int
+			width,
+			wrap:1,
+			wrap_given:1;
+	} wrap;
 };
 
 struct userformat_want {
diff --git a/log-tree.c b/log-tree.c
index 319bd31..3dfa944 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -414,6 +414,7 @@ void show_log(struct rev_info *opt)
 
 	opt->loginfo = NULL;
 	ctx.show_notes = opt->show_notes;
+	ctx.wrap = opt->wrap;
 	if (!opt->verbose_header) {
 		graph_show_commit(opt->graph);
 
diff --git a/pretty.c b/pretty.c
index 1580299..133bc53 100644
--- a/pretty.c
+++ b/pretty.c
@@ -23,6 +23,10 @@ static size_t commit_formats_len;
 static size_t commit_formats_alloc;
 static struct cmt_fmt_map *find_commit_format(const char *sought);
 
+static unsigned int wrap_configured = 0;
+static unsigned int wrap = 0;
+static unsigned int wrap_width = 80;
+
 static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)
 {
 	free(user_format);
@@ -172,6 +176,63 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 	}
 }
 
+static int git_wrap_config(const char *name, const char *value, void *cb)
+{
+	wrap_configured = 1;
+
+	if (prefixcmp(name, "log.wrap"))
+		return 0;
+
+	if (name[8] == '\0') {
+		if (value && !strcmp(value, "always")) {
+			wrap = 1;
+		} else if (value && !strcmp(value, "never")) {
+			wrap = 0;
+		} else if (!value || !strcmp(value, "auto") || git_config_bool(name, value)) {
+			wrap = want_color(GIT_COLOR_AUTO);
+		}
+	} else if (name[8] == '.') {
+		name += 9;
+		if (!strcmp(name, "width")) {
+			wrap_width = git_config_int(name, value);
+		}
+	}
+	return 0;
+}
+
+static unsigned int want_wrap(struct wrap_options options)
+{
+	if(!wrap_configured)
+		git_config(git_wrap_config, NULL);
+	return (options.wrap_given ? options.wrap : wrap);
+}
+static unsigned int get_wrap_width(struct wrap_options options)
+{
+	if(!wrap_configured)
+		git_config(git_wrap_config, NULL);
+	return options.width ? options.width : wrap_width;
+}
+
+static int line_list_prefix(const char *line, int len)
+{
+	unsigned int numberLength = 0;
+	const char *pos = line;
+	if (len < 3) {
+		return 0;
+	} else if ((line[0] == '*' || line[0] == '+' || line[0] == '-') && line[1] == ' ') {
+		return 2;
+	} else {
+		while (pos - line < len && pos[0] >= '0' && pos[0] <= '9'){
+			numberLength++;
+			pos++;
+		}
+		if (numberLength && pos - line + 1 < len && pos[0] == '.' && pos[1] == ' ') {
+			return numberLength + 2;
+		}
+	}
+	return 0;
+}
+
 /*
  * Generic support for pretty-printing the header
  */
@@ -1246,6 +1307,8 @@ void pp_remainder(const struct pretty_print_context *pp,
 		  struct strbuf *sb,
 		  int indent)
 {
+	unsigned int wrap = want_wrap(pp->wrap);
+	unsigned int width = get_wrap_width(pp->wrap);
 	int first = 1;
 	for (;;) {
 		const char *line = *msg_p;
@@ -1268,7 +1331,13 @@ void pp_remainder(const struct pretty_print_context *pp,
 			memset(sb->buf + sb->len, ' ', indent);
 			strbuf_setlen(sb, sb->len + indent);
 		}
-		strbuf_add(sb, line, linelen);
+		if (wrap && linelen && line[0] != ' ' && line[0] != '\t') {
+			struct strbuf wrapped = STRBUF_INIT;
+			strbuf_add(&wrapped, line, linelen);
+			strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + line_list_prefix(line, linelen), width - indent);
+		} else {
+			strbuf_add(sb, line, linelen);
+		}
 		strbuf_addch(sb, '\n');
 	}
 }
diff --git a/revision.c b/revision.c
index 8764dde..ca4b386 100644
--- a/revision.c
+++ b/revision.c
@@ -1465,6 +1465,16 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
 		get_commit_format(arg+9, revs);
+	} else if (!strcmp(arg, "--wrap")) {
+		revs->wrap.wrap = 1;
+		revs->wrap.wrap_given = 1;
+	} else if (!prefixcmp(arg, "--wrap=")) {
+		revs->wrap.wrap = 1;
+		revs->wrap.wrap_given = 1;
+		revs->wrap.width = atoi(arg+7);
+	} else if (!prefixcmp(arg, "--no-wrap")) {
+		revs->wrap.wrap = 0;
+		revs->wrap.wrap_given = 1;
 	} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
diff --git a/revision.h b/revision.h
index 6aa53d1..f812685 100644
--- a/revision.h
+++ b/revision.h
@@ -117,6 +117,9 @@ struct rev_info {
 			missing_newline:1,
 			date_mode_explicit:1,
 			preserve_subject:1;
+
+	struct wrap_options wrap;
+
 	unsigned int	disable_stdin:1;
 	unsigned int	leak_pending:1;
 
-- 
1.7.8.1

Re: [PATCH] Support wrapping commit messages when you read them

[Junio C Hamano]

Sidney San Martín <s@sidneysm.com> writes:

> Fairly simpleminded line wrapping that makes commit messages
> readable if they weren’t wrapped by the committer.

This does not say anything useful, other than "this is a naïve
implementation of message wrapper" and invites "So what?".

The most simple-minded solution is to reject such commits with crappy log
message.

After all, SCM is merely a method to help communication between
developers, and sticking to the common denominator is a proven good way to
make sure everybody involved in the project can use what is recorded in
the repository. This is not limited only to the log message, but equally
applies to filenames (e.g. don't create xt_tcpmss.c and xt_TCPMSS.c in the
same directory if you want your project extractable on case insensitive
filesystems) and even to the sources.

You need to justify the cause a bit better. Why is such a new logic
justified?

> - Use strbuf_add_wrapped_text() to do the dirty work
> - Detect simple lists which begin with "+ ", "* ", or "- " and indent
>   them appropriately (like this line)
> - Print lines which begin with whitespace as-is (e.g. code samples)

I suspect the above would make it more palatable than format=flowed
brought up in earlier discussions, which is unsuitable for nothing but
straight text.

> Add --wrap[=<width>] and --no-wrap to commands that pretty-print commit
> messages, and add log.wrap and log.wrap.width configuration options.

Why do you need two separate options and configurations that look as if
they are independent but in reality not?  If you say "no wrap", there is
no room for you to say "wrap width is 72".

I would expect something like:

 --log-message-wrap, --log-message-wrap=72, --log-message-wrap=no 

with --log-message-wrap=yes as a synonym for --log-message-wrap to give
consistency. The corresponding configuraiton would be log.messageWrap
whose values could be the usual bool-or-int.

> log.wrap defaults to never, and can be set to never/false, auto/true,
> or always. If auto, hijack want_color() to decide whether it’s
> appropriate to use line wrapping. (This is a little hacky, but as far
> as I can tell the conditions for auto color and auto wrapping are the
> same.

Why does coloring have _anything_ to do with line wrapping? Maybe your
personaly preference might be "wrap and color if interactive terminal" but
that is conflating two unrelated concepts. A user may not expect coloring
on a dumb interactive terminal, but wrapping may still be useful.

> log.wrap.width defaults to 80.

This does not deserve a comment as I already rejected the "two
configuration" approach, but do not use three-level names this way. We try
to reserve three-level names only for cases where the second level is used
for an unbound collection (e.g. "remote.$name.url", "branch.$name.merge").
that is user-specified.

Re: [PATCH] Support wrapping commit messages when you read them

[Sidney San Martín]

Hey Junio,

I apologize for the delay, I do want to keep working on this idea. I had to put it down but should have more time now if you're willing to keep talking about it.

Thanks for taking the time to look at my first patch.

On Dec 25, 2011, at 4:57 AM, Junio C Hamano wrote:

>> Fairly simpleminded line wrapping that makes commit messages
>> readable if they weren’t wrapped by the committer.
> 
> This does not say anything useful, other than "this is a naïve
> implementation of message wrapper" and invites "So what?".
> 
> The most simple-minded solution is to reject such commits with crappy log
> message.
> 
> After all, SCM is merely a method to help communication between
> developers, and sticking to the common denominator is a proven good way to
> make sure everybody involved in the project can use what is recorded in
> the repository. This is not limited only to the log message, but equally
> applies to filenames (e.g. don't create xt_tcpmss.c and xt_TCPMSS.c in the
> same directory if you want your project extractable on case insensitive
> filesystems) and even to the sources.
> 
> You need to justify the cause a bit better. Why is such a new logic
> justified?

You’re right, that sentence doesn't say anything.

I agree that projects need to have standards for their commit messages, but I also think that line wrapping should be taken care of by the computer so that the humans can think about the content of their commit messages. It's easier for everyone.

It also makes sense to not assume the user is using an 80-column terminal. Like I mentioned in another email, other tools work this way (e.g. manpages). It turns out that "git help" already has code to detect the width of the terminal, and it formats its output to fit it. I want to adapt that logic for this feature.

How about replacing that paragraph with this:

“Git didn’t previously support formatting commit messages for a user’s terminal, and the common practice has been to pre-wrap commit messages to under 80 columns. This is necessary for some projects, especially those which trade patches over email where mail clients might damage longer lines, but in many cases it’s only done so that the messages are readable in "git log" and the like. Supporting line wrapping in git lets users choose to leave their commit messages unwrapped and have them formatted for their terminal when displayed.”

>> - Use strbuf_add_wrapped_text() to do the dirty work
>> - Detect simple lists which begin with "+ ", "* ", or "- " and indent
>>  them appropriately (like this line)
>> - Print lines which begin with whitespace as-is (e.g. code samples)
> 
> I suspect the above would make it more palatable than format=flowed
> brought up in earlier discussions, which is unsuitable for nothing but
> straight text.
> 
>> Add --wrap[=<width>] and --no-wrap to commands that pretty-print commit
>> messages, and add log.wrap and log.wrap.width configuration options.
> 
> Why do you need two separate options and configurations that look as if
> they are independent but in reality not?  If you say "no wrap", there is
> no room for you to say "wrap width is 72".
> 
> I would expect something like:
> 
> --log-message-wrap, --log-message-wrap=72, --log-message-wrap=no 
> 
> with --log-message-wrap=yes as a synonym for --log-message-wrap to give
> consistency. The corresponding configuraiton would be log.messageWrap
> whose values could be the usual bool-or-int.

I stole this from other options: --progress/--no-progress, --color/--color=[<when>]/--no-color, --track/--no-track, etc.

The separate wrap/wrap.width config options were so that you could set it separately to auto or always and also specify a width. But, I don't know if that's needed anymore. See below.

>> log.wrap defaults to never, and can be set to never/false, auto/true,
>> or always. If auto, hijack want_color() to decide whether it’s
>> appropriate to use line wrapping. (This is a little hacky, but as far
>> as I can tell the conditions for auto color and auto wrapping are the
>> same.
> 
> Why does coloring have _anything_ to do with line wrapping? Maybe your
> personaly preference might be "wrap and color if interactive terminal" but
> that is conflating two unrelated concepts. A user may not expect coloring
> on a dumb interactive terminal, but wrapping may still be useful.

It doesn’t — I used want_color() so that I could get the patch out there without making other changes to the codebase or duplicating its code, so you could comment on the rest of it. I'll get it out of the next version of the patch, which I'll try to get to you later today or tomorrow.

>> log.wrap.width defaults to 80.
> 
> This does not deserve a comment as I already rejected the "two
> configuration" approach, but do not use three-level names this way. We try
> to reserve three-level names only for cases where the second level is used
> for an unbound collection (e.g. "remote.$name.url", "branch.$name.merge").
> that is user-specified.

OK, that was a misunderstanding on my part. Actually, I would be in favor of getting rid of that option completely, moving in support for detecting the terminal width from "git help" and making it just a boolean, auto-only. How does that sound?

Sidney

Re: [PATCH] Support wrapping commit messages when you read them

[Junio C Hamano]

Sidney San Martín <s@sidneysm.com> writes:

>> After all, SCM is merely a method to help communication between
>> developers, and sticking to the common denominator is a proven good way to
>> make sure everybody involved in the project can use what is recorded in
>> the repository. This is not limited only to the log message, but equally
>> applies to filenames (e.g. don't create xt_tcpmss.c and xt_TCPMSS.c in the
>> same directory if you want your project extractable on case insensitive
>> filesystems) and even to the sources.
>> 
>> You need to justify the cause a bit better. Why is such a new logic
>> justified?
>
> You’re right, that sentence doesn't say anything.
>
> I agree that projects need to have standards for their commit messages,
> but I also think that line wrapping should be taken care of by the
> computer so that the humans can think about the content of their commit
> messages. It's easier for everyone.

I just typed M-q to wrap the above paragraph from you to make it readable.

"Computers are good at automating" is true, and that is why real editors
give an easy way to auto-wrap long prose in a paragraph while composing.
But "computers are good at automating" is not a convincing justification
to let the composer leave unreasonably long lines in the commit log object
and force the reader side to line-wrap the mess only to fix it up.

Re: [PATCH] Support wrapping commit messages when you read them

[Holger Hellmuth]

On 13.02.2012 23:25, Junio C Hamano wrote:
> "Computers are good at automating" is true, and that is why real editors
> give an easy way to auto-wrap long prose in a paragraph while composing.
> But "computers are good at automating" is not a convincing justification
> to let the composer leave unreasonably long lines in the commit log object
> and force the reader side to line-wrap the mess only to fix it up.

Maybe this is more convincing: Only the reader side knows the 
line-length of the display or window.

Re: [PATCH] Support wrapping commit messages when you read them

[Sidney San Martín]

On Feb 13, 2012, at 5:25 PM, Junio C Hamano wrote:

> I just typed M-q to wrap the above paragraph from you to make it readable.

Out of curiosity, how do you read your mail? I don’t know anyone whose mail
is set up like that.

I’m happy to wrap my text if it’s tricky for you to read it otherwise — but
FWIW my mail client doesn’t support hard wrapping (I’m doing it in my editor).

> "Computers are good at automating" is true, and that is why real editors
> give an easy way to auto-wrap long prose in a paragraph while composing.
> But "computers are good at automating" is not a convincing justification
> to let the composer leave unreasonably long lines in the commit log object
> and force the reader side to line-wrap the mess only to fix it up.

I asked in #git how other people handle wrapping. Out of three people who
responded, only one said that they had configured their editor (the other two
do it by hand). One thought that Git already did dumb (character-level) line
wrapping, but it turns out he had set LESS= and GIT_PAGER='less -FRX'.

So, even if it is possible to set up your editor to wrap prose appropriately,
I don’t think it’s as common as one might hope.

I’m not suggesting that the reader side should take care of the wrapping
because it *can*, I’m suggesting that it shouldn’t take specially-configured
editors to get consistent and good results — which I assume is why virtually
all new prose-writing tools do wrapping on the viewing end.

What do you think about Git UIs which use proportional fonts for text where
hard wrapping doesn’t work at all? (I brought this up before but want your
take on it).

And, using man and, now, "git help -a" as examples: they both adapt their
output to the width of the user’s terminal. Isn’t that a good thing?

If those aren’t good justification… what would be?