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

[Junio C Hamano]

Jonathan Nieder  writes:

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

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

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

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

[Brandon Williams]

On 09/29, Jonathan Nieder wrote:
> Hi Dscho,
> 
> Johannes Schindelin wrote:
> 
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  .clang-format | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Well executed and well explained. Thank you.
> 
> Reviewed-by: Jonathan Nieder 
> 
> 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

[Jonathan Nieder]

Hi Dscho,

Johannes Schindelin wrote:

> Signed-off-by: Johannes Schindelin 
> ---
>  .clang-format | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)

Well executed and well explained. Thank you.

Reviewed-by: Jonathan Nieder 

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