patchclang-format: adjust line break penalties

We really, really, really want to limit the columns to 80 per line: One of the few consistent style comments on the Git mailing list is that the lines should not have more than 80 columns/line (even if 79 columns/line would make more sense, given that the code is frequently viewed as diff, and diffs adding an extra character).

The penalty of 5 for excess characters is way too low to guarantee that, though, as pointed out by Brandon Williams.

From the existing clang-format examples and documentation, it appears that 100 is a penalty deemed appropriate for Stuff You Really Don't Want, so let's assign that as the penalty for "excess characters", i.e. overly long lines.

While at it, adjust the penalties further: we are actually not that keen on preventing new line breaks within comments or string literals, so the penalty of 100 seems awfully high.

Likewise, we are not all that adamant about keeping line breaks away from assignment operators (a lot of Git's code breaks immediately after the `=` character just to keep that 80 columns/line limit).

We do frown a little bit more about functions' return types being on their own line than the penalty 0 would suggest, so this was adjusted, too.

Finally, we do not particularly fancy breaking before the first parameter in a call, but if it keeps the line shorter than 80 columns/line, that's what we do, so lower the penalty for breaking before a call's first parameter, but not quite as much as introducing new line breaks to comments.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/clang-format-column-limit-v1
Fetch-It-Via: git fetch https://github.com/dscho/git clang-format-column-limit-v1
 .clang-format | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Hi Dscho,

Johannes Schindelin wrote:

Well executed and well explained. Thank you.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Going forward, is there an easy way to preview the effect of this kind of change (e.g., to run "make style" on the entire codebase so as to be able to compare the result with two different versions of .clang-format)?

Thanks, Jonathan

On 09/29, Jonathan Nieder wrote:

I don't think there's an easy way to do this yet (I'm sure we can make one) though the biggest barrier to that is that most of the code base probably isn't consistent with the current .clang-format.

I also took a look at the patch and agree with all your points. I'm sure we'll still have to do some tweaking of these parameters but I'll start using this locally and see if I find any problems.

Hi,

On 09/29/2017 08:40 PM, Jonathan Nieder wrote:

I just ran clang-format before and after the patch and pushed to github. The resulting diff is quite big:

https://github.com/sbeyer/git/commit/3d1186c4cf4dd7e40b97453af5fc1170f6868ccd

Cheers Stephan

PS: There should be a comment at the beginning of the .clang-format file
that says what version it is tested with (on my machine it worked with
5.0 but not with 4.0) and there should also probably a remark that the
clang-format-based style should only be understood as a hint or guidance
and that most of the Git codebase does not conform it.

Stephan Beyer wrote:

Thanks.  The first change I see there is

 -char *strbuf_realpath(struct strbuf *resolved, const char *path, int die_on_error)
 +char *
 +strbuf_realpath(struct strbuf *resolved, const char *path, int die_on_error)

I understand why the line is broken, but the choice of line break is wrong. Seems like the penalty for putting return type on its own line quite high enough.

My Reviewed-by still stands, though. It gets "make style" to signal long lines that should be broken, which is an improvement.

Sounds good to me.  Care to send it as a patch? :)

Thanks, Jonathan

Having a .clang-format file in a project can be understood in a way that code has to be in the style defined by the .clang-format file, i.e., you just have to run clang-format over all code and you are set. This is not the case in the Git project, which is now reflected by an comment in the beginning of the file.

Additionally, the working clang-format version is mentioned because the config directives change from time to time (in a compatibility-breaking way).

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

Notes:
    On 09/30/2017 12:45 AM, Jonathan Nieder wrote:
    > Sounds good to me.  Care to send it as a patch? :)
    
    Like this? :)

 .clang-format | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Stephan Beyer <s-beyer@gmx.net> writes:

I think this makes 50%-80% sense. As we have just seen in the patch that started this thread, the rules currently in this file is known not to be perfect (and I do not think the patch was meant to make, or claimed that it has made, the rules perfect---it was to fix the most problematic part that was observed and is a good incremental improvement), so we should treat it as such. "does not conform to" does not convey that--it makes as if a random patch to "make it conform" without thinking if the rules make sense were a welcome addition, which is absolutely the last signal we would want to send to the readers.

Having a .clang-format file in a project can be understood in a way that code has to be in the style defined by the .clang-format file, i.e., you just have to run clang-format over all code and you are set. This is not the case in the Git project, which is now reflected by a comment in the beginning of the file.

Additionally, the working clang-format version is mentioned because the config directives change from time to time (in a compatibility-breaking way).

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

Notes:
    On 10/01/2017 04:45 AM, Junio C Hamano wrote:
    > it makes as if a random patch to "make it
    > conform" without thinking if the rules make sense were a welcome
    > addition, which is absolutely the last signal we would want to send
    > to the readers.
    
    Right. I dropped that last sentence and replaced it by a sentence about human
    aesthetics judgement overruling mechanical rules -- I think that's somehow quoted
    from a comment of yours on the list.

 .clang-format | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Stephan Beyer <s-beyer@gmx.net> writes:

Sorry, but that is not what I meant.

I think we do want the endgame to be that .clang-format defines how the code should look like. It's that we are not there yet, and I think that is what we should say in this comment.

	Note that this style definition does not yet quite reflect
	how we want our code to look like, and adjusting the rules
	to match our style is still work in progress.  Do not
	blindly adjust the style of _existing_ code, without
	checking if the code is styled incorrectly, or the style
	definition in this file is still wrong.

is what I should have suggested when writing my response.

On 10/01/2017 10:30 PM, Junio C Hamano wrote:

Pretty long but okay. I tried to be shorter and more implicit (also because the CodingGuidelines are already pretty verbose on not changing existing code style) and you're heading in the direction that there will be some clang-format definition that matches the desired coding style (I doubt that at least for the current clang-format versions, but that's another topic).

Erm, so you're going to replace the comment? Or is it my task now to make a v3 patch with your text? (The latter doesn't look useful to me...)

Stephan

From: Stephan Beyer <s-beyer@gmx.net>

Having a .clang-format file in a project can be understood in a way that code has to be in the style defined by the .clang-format file, i.e., you just have to run clang-format over all code and you are set.

This unfortunately is not yet the case in the Git project, as the format file is still work in progress. Explain it with a comment in the beginning of the file.

Additionally, the working clang-format version is mentioned because the config directives change from time to time (in a compatibility-breaking way).

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * So here is a counter-proposal in a patch form.  I agree that my
   earlier suggestion was unnecessarily verbose; this one spends
   just as many lines and not more than the v2 round of Stephan's
   patch.

 .clang-format | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Hi,

On 10/02/2017 01:37 AM, Junio C Hamano wrote:

I'm totally fine with this.

Stephan

On 10/02, Junio C Hamano wrote:

Thanks for writing up this header comment to the .clang-format file, it's something I definitely should have included when I introduced it.

And I like the wording that you've both settled on, as it reflects our intentions (of having the code eventually conform to the format rules) and making note that this set of rules still needs to be tuned.

Thanks!

On 02/10/17 18:21, Brandon Williams wrote:

Just for the record, I have 'clang-format version 3.8.0-2ubuntu4
 (tags/RELEASE_380/final)' on Linux Mint 18.2, which requires me
to comment out:

    AlignEscapedNewlines: Left
    BreakStringLiterals: false
    PenaltyBreakAssignment: 100

And on cygwin, I have 'clang-format version 4.0.1
 (tags/RELEASE_401/final)', which requires me to
comment out:

    AlignEscapedNewlines: Left
    PenaltyBreakAssignment: 100

So, I don't think I can play along! :(

[When playing with 3.8 on Linux, I noted that clang-format seemed to ignore *all* settings in .clang-format, if it found *any* config that it didn't know about! Not very friendly. :-P ]

ATB, Ramsay Jones

Jonathan Nieder <jrnieder@gmail.com> writes:

Thanks, both.  I think the adjustment in the patch makes sense.