Ah, thank you! On Dec 9, 2011, at 11:49 AM, Frans Klaver wrote: > I'm adding git@vger... again, cause there didn't seem to be a reason not to. > > On Fri, Dec 9, 2011 at 3:10 PM, Sidney San Martín <s@xxxxxxxxxxxx> wrote: >> On Dec 9, 2011, at 2:05 AM, Frans Klaver wrote: >> >>> On Fri, 09 Dec 2011 02:59:06 +0100, Sidney San Martín <s@xxxxxxxxxxxx> wrote: >>> >>>> Hey, I want to ask about the practice of wrapping commit messages to 70-something charaters. >>>> >>>> The webpage most cited about it, which I otherwise really like, is >>>> >>>> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html >>>> >>>> *Nothing else* in my everyday life works this way anymore. Line wrapping gets done on the display end in my email client, my web browser, my ebook reader entirely automatically, and it adapts to the size of the window. >>> >>> Actually, opera-mail autowraps at 72 characters but sets the text format to flowed. It also wraps the quoted text when you reply. But there's a reasonable chance that you don't use opera in your daily life. On the other hand I would not be surprised if most decent e-mail clients worked that way. >>> >> >> Interesting… either way, the end result is that the receiving mail client can wrap the lines to whatever length it (or you, as its operator) desires, which I think we can agree is a good thing, right? >> > > Yes. > >>> Hm. Saying "that's how the tool works" is not a good reason in my opinion. There might be tons of other reasons for wrapping at 80 characters. Readability is one that comes to mind for me. >>> >> >> That's my basic point. I hope it didn't seem like I was arguing against reading commit messages wrapped to 80 columns, by default. I only wanted to discuss whether it makes more sense to handle it on the display end instead of asking committers to do it in advance. >> > > It somewhat looked like that. I think it might make sense for clients > to ignore the line wrapping when they can only show less than 80 > characters on a line, but that would probably break the code part of a > patch mail. > > >> - My phone shows text most comfortably at about 40 characters per line. I do look at terminals at 80 columns most of the time, but not always, and I sometimes browse projects in GUI tools that use a proportional font in a window may be narrower or wider than that. >> >> - Right now, when I *am* in an 80-col terminal I have to trust everyone else to wrap their commit messages. Not everyone does. I feel like it would be more effective to give git the ability to wrap them automatically when I read them. >> > > It could be a useful option to wrap when the lines extend the window > width, but I'd actually think it's better to leave that up to the > pager than to git. > >>>> >>>> Second: >>>> >>>>> git format-patch --stdout converts a series of commits to a series of emails, using the messages for the message body. Good email netiquette dictates we wrap our plain text emails such that there’s room for a few levels of nested reply indicators without overflow in an 80 column terminal. (The current rails.git workflow doesn’t include email, but who knows what the future will bring.) >>>> >>>> There's been a standard for flowed plain text emails (which don't have to wrap at 80 columns) for well over ten years, RFC-2646 and is widely supported. Besides, code in diffs is often longer than 7x characters, and wrapping, like `git log`, could be done inside git. FWIW, there are a bunch of merge commits with lines longer than 80 characters in the git repo itself. >>> >>> Yes, that standard allows e-mail clients to display the text more fluidly, even if the source text is word-wrapped. While git uses e-mail format, it isn't an e-mail client. I always interpreted this whole thing as git basically creating plain-text e-mails. You're actually writing the source of the e-mail in your commit message. If you care about actual use in e-mail (like we do here on the list) you might want to add the relevant header to the mails. That said, Apple Mail (the client you used to send your mail) doesn't even use the RFC you quote in the sent message. That mail is going to be a pain in the butt to read in mutt from work ;). >>> >> >> Sorry, I'm not sure what you mean by, “If you care about actual use in e-mail (like we do here on the list) you might want to add the relevant header to the mails”. > > I thought you might want to have wrapped text in the git commit > messages, but actually put a format flowed tag into the mail header. > I'm not sure what that would do to the code though. > > >> Interesting, I didn't realize that Mail didn't use it. It does, however, use quoted-printable which, as far as I can tell, has a similar effect on line wrapping. What happens when you view this email in mutt? >> > > I had no idea quoted printable had any effect on line wrapping. As far > as I know it's just a way to encode non-ascii characters in 7bit, no > more no less. Your current e-mail happens to end lines with =<nl>, > which probably handles the wrapping. Your original message didn't have > that. > > >>>> - - - >>>> >>>> From a93b390d1506652d4ad41d1cbd987ba98a8deca0 Mon Sep 17 00:00:00 2001 >>>> From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@xxxxxxxxxxxx> >>>> Date: Thu, 8 Dec 2011 20:26:23 -0500 >>>> Subject: [PATCH] Wrap commit messages on display >>>> >>>> - Wrap to 80 characters minus the indent >>>> - Use a hanging indent for lines which begin with "- " >>>> - Do not wrap lines which begin with whitespace >>>> --- >>>> pretty.c | 10 ++++++++-- >>>> 1 files changed, 8 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/pretty.c b/pretty.c >>>> index 230fe1c..15804ce 100644 >>>> --- a/pretty.c >>>> +++ b/pretty.c >>>> @@ -1243,8 +1243,14 @@ 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); >>>> - strbuf_addch(sb, '\n'); >>>> + if (line[0] == ' ' || line[0] == '\t') { >>>> + strbuf_add(sb, line, linelen); >>>> + } else { >>>> + struct strbuf wrapped = STRBUF_INIT; >>>> + strbuf_add(&wrapped, line, linelen); >>>> + strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + (line[0] == '-' && line[1] == ' ' ? 2 : 0), 80 - indent); >>> >>> While on the subject, In my mail view, the new line started with the [1] from line[1], in the quote the line looks entirely different. Now this is code we're talking about, so it makes slightly more sense to have a proper wrapping hard-coded. Compare the above with the following: >>> >>> + int hanging_indent = ((line[0] == '-' && line[1] == ' ') ? 2 : 0); >>> [...] >>> + strbuf_add_wrapped_text(sb, wrapped.buf, 0, >>> + indent + hanging_indent, >>> + 80 - indent); >>> >>> Much clearer, no? I personally usually have two or three terminals tucked next to each other, so I can look at two or three things at the same time. 80 characters limit is a nice feature then. >> >> Good point, that makes it clearer either way. I put an updated patch at the bottom of this email (also fixed forgetting the newline after lines with leading whitespace). I hope it's OK to include patches this way, I understand that they're supposed to represent whole emails but want to include them with this discussion. >> > > You can include them in the discussion. While it is probably OK to put > some code into your mail to propose something (I've seen it happen > more than once), the end result is supposed to be submitted with a > git-format-patch'd commit. You can read more about contributing in > Documentation/SubmittingPatches. > > >>> >>> >>>> + strbuf_addch(sb, '\n'); >>>> + } >>>> } >>>> } >>>> >>> >>> Cheers, >>> Frans >> >> >> From 53fd7deedaf5ac522c9d752e79cf71561cc57f07 Mon Sep 17 00:00:00 2001 >> From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@xxxxxxxxxxxx> >> Date: Thu, 8 Dec 2011 20:26:23 -0500 >> Subject: [PATCH] Wrap commit messages on display >> >> - Wrap to 80 characters, minus the indent >> - Use a hanging indent for lines which begin with "- " >> - Do not wrap lines which begin with whitespace >> --- >> pretty.c | 9 ++++++++- >> 1 files changed, 8 insertions(+), 1 deletions(-) >> >> diff --git a/pretty.c b/pretty.c >> index 230fe1c..841ccd1 100644 >> --- a/pretty.c >> +++ b/pretty.c >> @@ -1243,7 +1243,14 @@ 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 (line[0] == ' ' || line[0] == '\t') { >> + strbuf_add(sb, line, linelen); >> + } else { >> + struct strbuf wrapped = STRBUF_INIT; >> + strbuf_add(&wrapped, line, linelen); >> + int hanging_indent = ((line[0] == '-' && line[1] == ' ') ? 2 : 0); >> + strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + hanging_indent, 80 - indent); > > It's common in C (and in certain flavors even required) to have your > variable declaration at the beginning of the scope: > > + } else { > + int hanging_indent; > + struct strbuf wrapped = STRBUF_INIT; > + strbuf_add(&wrapped, line, linelen); > + hanging_indent = ((line[0] == '-' && line[1] > == ' ') ? 2 : 0); > + strbuf_add_wrapped_text(sb, wrapped.buf, 0, > indent + hanging_indent, 80 - indent); > > > Gmail webclient mucks up the whitespace. Don't copy & paste ;) > > > As I said earlier in the mail, I'm not sure if this is something that > should be done by git. Maybe someone else can shed some light on that. > > >> + } >> strbuf_addch(sb, '\n'); >> } >> } >> -- >> 1.7.8 >> >> -- 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