[PATCH] clang-format: adjust line break penalties

[PATCH] clang-format: adjust line break penalties

[Johannes Schindelin]

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(-)

diff --git a/.clang-format b/.clang-format
index 3ede2628d2d..56822c116b1 100644
--- a/.clang-format
+++ b/.clang-format
@@ -153,13 +153,13 @@ KeepEmptyLinesAtTheStartOfBlocks: false
 
 # Penalties
 # This decides what order things should be done if a line is too long
-PenaltyBreakAssignment: 100
-PenaltyBreakBeforeFirstCallParameter: 100
-PenaltyBreakComment: 100
+PenaltyBreakAssignment: 10
+PenaltyBreakBeforeFirstCallParameter: 30
+PenaltyBreakComment: 10
 PenaltyBreakFirstLessLess: 0
-PenaltyBreakString: 100
-PenaltyExcessCharacter: 5
-PenaltyReturnTypeOnItsOwnLine: 0
+PenaltyBreakString: 10
+PenaltyExcessCharacter: 100
+PenaltyReturnTypeOnItsOwnLine: 5
 
 # Don't sort #include's
 SortIncludes: false

base-commit: ea220ee40cbb03a63ebad2be902057bf742492fd
-- 
2.14.2.windows.1

Re: [PATCH] clang-format: adjust line break penalties

[Jonathan Nieder]

Hi Dscho,

Johannes Schindelin wrote:

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  .clang-format | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

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

Re: [PATCH] clang-format: adjust line break penalties

[Brandon Williams]

On 09/29, Jonathan Nieder wrote:
> Hi Dscho,
> 
> Johannes Schindelin wrote:
> 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  .clang-format | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> 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

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.

-- 
Brandon Williams

Re: [PATCH] clang-format: adjust line break penalties

[Stephan Beyer]

Hi,

On 09/29/2017 08:40 PM, Jonathan Nieder wrote:
> 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)?

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.

Re: [PATCH] clang-format: adjust line break penalties

[Jonathan Nieder]

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

>> 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)?
>
> 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

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.

> 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.

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

Thanks,
Jonathan

[PATCH] Add a comment to .clang-format about the meaning of the file

[Stephan Beyer]

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(-)

diff --git a/.clang-format b/.clang-format
index 3ede2628d..558fc7fd8 100644
--- a/.clang-format
+++ b/.clang-format
@@ -1,4 +1,8 @@
-# Defaults
+# This file is an example configuration for clang-format 5.0.
+#
+# Note that this style definition should only be understood as a hint
+# for writing new code. Most of Git's codebase does not conform to
+# this definition.
 
 # Use tabs whenever we need to fill whitespace that spans at least from one tab
 # stop to the next one.
-- 
2.14.2.677.g5a59ab275

Re: [PATCH] clang-format: adjust line break penalties

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi Dscho,
>
> Johannes Schindelin wrote:
>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>>  .clang-format | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> Well executed and well explained. Thank you.
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

Re: [PATCH] Add a comment to .clang-format about the meaning of the file

[Junio C Hamano]

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

> 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(-)
>
> diff --git a/.clang-format b/.clang-format
> index 3ede2628d..558fc7fd8 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -1,4 +1,8 @@
> -# Defaults
> +# This file is an example configuration for clang-format 5.0.
> +#
> +# Note that this style definition should only be understood as a hint
> +# for writing new code. Most of Git's codebase does not conform to
> +# this definition.

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.

[PATCH v2] Add a comment to .clang-format about the meaning of the file

[Stephan Beyer]

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(-)

diff --git a/.clang-format b/.clang-format
index 3ede2628d..041b7be03 100644
--- a/.clang-format
+++ b/.clang-format
@@ -1,4 +1,8 @@
-# Defaults
+# This file is an example configuration for clang-format 5.0.
+#
+# Note that this style definition should only be understood as a hint
+# for writing new code. In the end, human aesthetics judgement overrules
+# mechanical rules.
 
 # Use tabs whenever we need to fill whitespace that spans at least from one tab
 # stop to the next one.
-- 
2.14.2.677.g5a59ab275

Re: [PATCH v2] Add a comment to .clang-format about the meaning of the file

[Junio C Hamano]

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

> 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.

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.

Re: [PATCH v2] Add a comment to .clang-format about the meaning of the file

[Stephan Beyer]

On 10/01/2017 10:30 PM, Junio C Hamano wrote:
> 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.
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

[PATCH v3] clang-format: add a comment about the meaning/status of the

[Junio C Hamano]

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(-)

diff --git a/.clang-format b/.clang-format
index 56822c116b..7670eec8df 100644
--- a/.clang-format
+++ b/.clang-format
@@ -1,4 +1,8 @@
-# Defaults
+# This file is an example configuration for clang-format 5.0.
+#
+# Note that this style definition should only be understood as a hint
+# for writing new code. The rules are still work-in-progress and does
+# not yet exactly match the style we have in the existing code.
 
 # Use tabs whenever we need to fill whitespace that spans at least from one tab
 # stop to the next one.
-- 
2.14.2-820-gefeff4fbff

Re: [PATCH v3] clang-format: add a comment about the meaning/status of the

[Stephan Beyer]

Hi,

On 10/02/2017 01:37 AM, Junio C Hamano wrote:
> diff --git a/.clang-format b/.clang-format
> index 56822c116b..7670eec8df 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -1,4 +1,8 @@
> -# Defaults
> +# This file is an example configuration for clang-format 5.0.
> +#
> +# Note that this style definition should only be understood as a hint
> +# for writing new code. The rules are still work-in-progress and does
> +# not yet exactly match the style we have in the existing code.

I'm totally fine with this.

Stephan

Re: [PATCH v3] clang-format: add a comment about the meaning/status of the

[Brandon Williams]

On 10/02, Junio C Hamano wrote:
> 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(-)
> 
> diff --git a/.clang-format b/.clang-format
> index 56822c116b..7670eec8df 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -1,4 +1,8 @@
> -# Defaults
> +# This file is an example configuration for clang-format 5.0.
> +#
> +# Note that this style definition should only be understood as a hint
> +# for writing new code. The rules are still work-in-progress and does
> +# not yet exactly match the style we have in the existing code.

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!

>  
>  # Use tabs whenever we need to fill whitespace that spans at least from one tab
>  # stop to the next one.
> -- 
> 2.14.2-820-gefeff4fbff
> 

-- 
Brandon Williams

Re: [PATCH v3] clang-format: add a comment about the meaning/status of the

[Ramsay Jones]

On 02/10/17 18:21, Brandon Williams wrote:
> On 10/02, Junio C Hamano wrote:
>> 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(-)
>>
>> diff --git a/.clang-format b/.clang-format
>> index 56822c116b..7670eec8df 100644
>> --- a/.clang-format
>> +++ b/.clang-format
>> @@ -1,4 +1,8 @@
>> -# Defaults
>> +# This file is an example configuration for clang-format 5.0.
>> +#
>> +# Note that this style definition should only be understood as a hint
>> +# for writing new code. The rules are still work-in-progress and does
>> +# not yet exactly match the style we have in the existing code.
> 
> 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.

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