Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gerrit: complains about commit bodies wrapped at 72 chars #17941

Closed
mvdan opened this issue Nov 16, 2016 · 13 comments
Closed

gerrit: complains about commit bodies wrapped at 72 chars #17941

mvdan opened this issue Nov 16, 2016 · 13 comments

Comments

@mvdan
Copy link
Member

mvdan commented Nov 16, 2016

 $ git-codereview mail
remote: Processing changes: new: 1, done
remote: (W) f3388a2: too many commit message lines longer than 70 characters; manually wrap lines
[...]

It is my understanding that this is a setting that can be changed in gerrit. Is there a reason why 70 was chosen?

As far as I can tell, 72 is the most commonly used wrapping length for commit bodies in git:

http://chris.beams.io/posts/git-commit/#wrap-72
https://bugs.chromium.org/p/gerrit/issues/detail?id=841
https://medium.com/@preslavrachev/what-s-with-the-50-72-rule-8a906f61f09c#.3154gadgk

At the same time, I can't find any reference to this in any official Git docs.

Is there a reason why our gerrit instance is configured like this?

@0xmohit
Copy link
Contributor

0xmohit commented Nov 16, 2016

The choice may be subjective, but the default fill-column for emacs is 70.

@mvdan
Copy link
Member Author

mvdan commented Nov 16, 2016

For comparison, vim seems to default to 72:

 $ grep textwidth </usr/share/vim/vim80/ftplugin/gitcommit.vim
setlocal nomodeline tabstop=8 formatoptions+=tl textwidth=72

I don't think editor defaults should play a major role here, though.

@robpike
Copy link
Contributor

robpike commented Nov 16, 2016

Just ignore the warning. I do.

No person should care how many characters are on a line, but for a computer to care is just ridiculous.

@mvdan
Copy link
Member Author

mvdan commented Nov 16, 2016

@robpike given that git doesn't play well with non-wrapped commit bodies, I do think a warning (not an error) like this is somewhat useful for people who aren't wrapping their non-quoted text.

What I'm proposing is that since 72 chars seems to be common practice and still plays well with git, the warning should be made less strict. Removing the warning altogether is also an option, but this change is a simple compromise.

@bradfitz
Copy link
Contributor

I'm not sure this is even a Gerrit setting. @spearce?

Quick Google search didn't find anything at least. I didn't try too hard.

@bradfitz bradfitz added this to the Unreleased milestone Nov 16, 2016
@mvdan
Copy link
Member Author

mvdan commented Nov 16, 2016

@bradfitz have you looked at https://bugs.chromium.org/p/gerrit/issues/detail?id=841?

It's something that gerrit remote is saying, so I don't know where else this could be set. It's definitely not a git thing.

@bradfitz
Copy link
Contributor

Ah, great. I missed that.

For reference,

Commit Message Length Configuration
===================================

The maximum lengths of the subject and message body can be
configured in the standard Gerrit config file `gerrit.config`.

commitmessage.maxSubjectLength
:       Maximum length of the commit message's subject line.  Defaults
        to 65 if not specified or less than 0.

commitmessage.maxLineLength
:       Maximum length of a line in the commit message's body.  Defaults
        to 70 if not specified or less than 0.

commitmessage.longLinesThreshold
:       Percentage of commit message lines allowed to exceed the
        maximum length before a warning or error is generated.  Defaults
        to 33 if not specified or less than 0.

commitmessage.rejectTooLong
:       If set to `true`, reject commits whose subject or line
        length exceeds the maximum allowed length.  If not
        specified, defaults to `false`.

@bradfitz
Copy link
Contributor

Okay, I sent https://go-review.googlesource.com/#/c/33323/ to @ianlancetaylor adding:

[commitmessage]
    maxSubjectLength = 76
    maxLineLength = 72

... to the Go All-Projects configuration.

@mvdan
Copy link
Member Author

mvdan commented Nov 16, 2016

Thanks!

Any rationale behind the subject length being so large? Usually it's kept lower, like 50 as mentioned in the links I provided or 65 according to gerrit.

@robpike
Copy link
Contributor

robpike commented Nov 16, 2016

Is there some reason we're picking 76? Is it some celebration of the signing of the US Declaration of Independence? Can't it be something useful, like 100 or 200 or 1e6?

I need to oil my 026 keypunch http://www.columbia.edu/cu/computinghistory/026.html

@bradfitz
Copy link
Contributor

I'm bored of this bug.

I'll let somebody else take this over.

@rsc
Copy link
Contributor

rsc commented Nov 15, 2017

A year later, I have taken this over, in part because the default limits seem to be decreasing over time. I set both limits to 1000 in https://go-review.googlesource.com/c/All-Projects/+/78090/3/project.config. I hope we can all agree that lines longer than 1000 characters do deserve a warning.

@rsc rsc closed this as completed Nov 15, 2017
@rsc
Copy link
Contributor

rsc commented Nov 16, 2017

OK, that CL did nothing. For future reference, the right magic appears to be:

gob-ctl <<<'update_host{host: "go" set_gerrit_config{name: "commitmessage.maxSubjectLength" value: "1000"}}'
gob-ctl <<<'update_host{host: "go" set_gerrit_config{name: "commitmessage.maxLineLength" value: "1000"}}'

or else to have edited the gerrit.config stored in Google's internal repos and waited for the next (day or two) config push.
I hope the gob-ctl commands do not get overwritten.

Update: Apologies for continuing to dump this here, but we might be able to find it later if I do. Apparently after running those commands the suggested next step is to update the internal devtools/gerritcodereview/hosted_sites/go/etc/gerrit.config, send that out as an internal CL, wait until it gets pushed, and then unset the gob-ctl commands. The gob-ctl commands are persisted but harder to see, so it's better have config in the file and use the gob-ctl commands as temporary band-aids over the main config file to reduce the latency of making changes.

@golang golang locked and limited conversation to collaborators Nov 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants