[RFC] clang-format: outline the git project's coding style

[RFC] clang-format: outline the git project's coding style

[Brandon Williams]

Add a '.clang-format' file which outlines the git project's coding
style.  This can be used with clang-format to auto-format .c and .h
files to conform with git's style.

Signed-off-by: Brandon Williams <bmwill@google.com>
---

I'm sure this sort of thing comes up every so often on the list but back at
git-merge I mentioned how it would be nice to not have to worry about style
when reviewing patches as that is something mechanical and best left to a
machine (for the most part).  I saw that 'clang-format' was brought up on the
list once before a couple years ago
(https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing
really came of it.  I spent a little bit of time combing through the various
options and came up with this config based on the general style of our code
base.  The big issue though is that our code base isn't consistent so try as
you might you wont be able to come up with a config which matches everything we
do (mostly due to the inconsistencies in our code base).

Anyway, I thought I'd bring this topic back up and see how people feel about it.

 .clang-format | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 166 insertions(+)
 create mode 100644 .clang-format

diff --git a/.clang-format b/.clang-format
new file mode 100644
index 000000000..7f28dc259
--- /dev/null
+++ b/.clang-format
@@ -0,0 +1,166 @@
+# Defaults
+
+# Use tabs whenever we need to fill whitespace that spans at least from one tab
+# stop to the next one.
+UseTab: Always
+TabWidth: 8
+IndentWidth: 8
+ContinuationIndentWidth: 8
+ColumnLimit: 80
+
+# C Language specifics
+Language: Cpp
+
+# Align parameters on the open bracket
+# someLongFunction(argument1,
+#                  argument2);
+AlignAfterOpenBracket: Align
+
+# Don't align consecutive assignments
+# int aaaa = 12;
+# int b = 14;
+AlignConsecutiveAssignments: false
+
+# Don't align consecutive declarations
+# int aaaa = 12;
+# double b = 3.14;
+AlignConsecutiveDeclarations: false
+
+# Align escaped newlines as far left as possible
+# #define A   \
+#   int aaaa; \
+#   int b;    \
+#   int cccccccc;
+AlignEscapedNewlines: Left
+
+# Align operands of binary and ternary expressions
+# int aaa = bbbbbbbbbbb +
+#           cccccc;
+AlignOperands: true
+
+# Don't align trailing comments
+# int a; // Comment a
+# int b = 2; // Comment b
+AlignTrailingComments: false
+
+# By default don't allow putting parameters onto the next line
+# myFunction(foo, bar, baz);
+AllowAllParametersOfDeclarationOnNextLine: false
+
+# Don't allow short braced statements to be on a single line
+# if (a)           not       if (a) return;
+#   return;
+AllowShortBlocksOnASingleLine: false
+AllowShortCaseLabelsOnASingleLine: false
+AllowShortFunctionsOnASingleLine: false
+AllowShortIfStatementsOnASingleLine: false
+AllowShortLoopsOnASingleLine: false
+
+# Add a line break after the return type of top-level functions
+# int
+# foo();
+AlwaysBreakAfterReturnType: TopLevel
+
+# Pack as many parameters or arguments onto the same line as possible
+# int myFunction(int aaaaaaaaaaaa, int bbbbbbbb,
+#                int cccc);
+BinPackArguments: true
+BinPackParameters: true
+
+# Attach braces to surrounding context except break before braces on function
+# definitions.
+# void foo()
+# {
+#    if (true) {
+#    } else {
+#    }
+# };
+BreakBeforeBraces: Linux
+
+# Break after operators
+# int valuve = aaaaaaaaaaaaa +
+#              bbbbbb -
+#              ccccccccccc;
+BreakBeforeBinaryOperators: None
+BreakBeforeTernaryOperators: false
+
+# Don't break string literals
+BreakStringLiterals: false
+
+# Use the same indentation level as for the switch statement.
+# Switch statement body is always indented one level more than case labels.
+IndentCaseLabels: false
+
+# Don't indent a function definition or declaration if it is wrapped after the
+# type
+IndentWrappedFunctionNames: false
+
+# Align pointer to the right
+# int *a;
+PointerAlignment: Right
+
+# Insert a space after a cast
+# x = (int32) y;    not    x = (int32)y;
+SpaceAfterCStyleCast: true
+
+# Insert spaces before and after assignment operators
+# int a = 5;    not    int a=5;
+# a += 42;             a+=42;
+SpaceBeforeAssignmentOperators: true
+
+# Put a space before opening parentheses only after control statement keywords.
+# void f() {
+#   if (true) {
+#     f();
+#   }
+# }
+SpaceBeforeParens: ControlStatements
+
+# Don't insert spaces inside empty '()'
+SpaceInEmptyParentheses: false
+
+# The number of spaces before trailing line comments (// - comments).
+# This does not affect trailing block comments (/* - comments).
+SpacesBeforeTrailingComments: 1
+
+# Don't insert spaces in casts
+# x = (int32) y;    not    x = ( int32 ) y;
+SpacesInCStyleCastParentheses: false
+
+# Don't insert spaces inside container literals
+# var arr = [1, 2, 3];    not    var arr = [ 1, 2, 3 ];
+SpacesInContainerLiterals: false
+
+# Don't insert spaces after '(' or before ')'
+# f(arg);    not    f( arg );
+SpacesInParentheses: false
+
+# Don't insert spaces after '[' or before ']'
+# int a[5];    not    int a[ 5 ];
+SpacesInSquareBrackets: false
+
+# Insert a space after '{' and before '}' in struct initializers
+Cpp11BracedListStyle: false
+
+# A list of macros that should be interpreted as foreach loops instead of as
+# function calls.
+ForEachMacros: ['for_each_string_list_item']
+
+# The maximum number of consecutive empty lines to keep.
+MaxEmptyLinesToKeep: 1
+
+# No empty line at the start of a block.
+KeepEmptyLinesAtTheStartOfBlocks: false
+
+# Penalties
+# This decides what order things should be done if a line is too long
+PenaltyBreakAssignment: 100
+PenaltyBreakBeforeFirstCallParameter: 100
+PenaltyBreakComment: 100
+PenaltyBreakFirstLessLess: 0
+PenaltyBreakString: 100
+PenaltyExcessCharacter: 5
+PenaltyReturnTypeOnItsOwnLine: 0
+
+# Don't sort #include's
+SortIncludes: false
-- 
2.14.0.434.g98096fd7a8-goog

Re: [RFC] clang-format: outline the git project's coding style

[Johannes Schindelin]

Hi Brandon,

On Mon, 7 Aug 2017, Brandon Williams wrote:

> Add a '.clang-format' file which outlines the git project's coding
> style.  This can be used with clang-format to auto-format .c and .h
> files to conform with git's style.
> 
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> 
> I'm sure this sort of thing comes up every so often on the list but back at
> git-merge I mentioned how it would be nice to not have to worry about style
> when reviewing patches as that is something mechanical and best left to a
> machine (for the most part).

Amen.

If I never have to see a review mentioning an unwrapped line, I am quite
certain I will be quite content.

Ciao,
Dscho

Re: [RFC] clang-format: outline the git project's coding style

[Stefan Beller]

On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Brandon,
>
> On Mon, 7 Aug 2017, Brandon Williams wrote:
>
>> Add a '.clang-format' file which outlines the git project's coding
>> style.  This can be used with clang-format to auto-format .c and .h
>> files to conform with git's style.
>>
>> Signed-off-by: Brandon Williams <bmwill@google.com>
>> ---
>>
>> I'm sure this sort of thing comes up every so often on the list but back at
>> git-merge I mentioned how it would be nice to not have to worry about style
>> when reviewing patches as that is something mechanical and best left to a
>> machine (for the most part).
>
> Amen.
>
> If I never have to see a review mentioning an unwrapped line, I am quite
> certain I will be quite content.
>
> Ciao,
> Dscho

As a thought experiment I'd like to propose to take it one step further:

  If the code was formatted perfectly in one style such that a formatter for
  this style would not produce changes when rerun again on the code, then
  each individual could have a clean/smudge filter to work in their preferred
  style, and only the exchange and storage of code is in a mutual agreed
  style. If the mutually agreed style is close to what I prefer, I don't have to
  use clean/smudge filters.

Additionally to this patch, we'd want to either put a note into
SubmittingPatches or Documentation/gitworkflows.txt to hint at
how to use this formatting to just affect the patch that is currently
worked on or rather a pre-commit hook?

Re: [RFC] clang-format: outline the git project's coding style

[Brandon Williams]

On 08/08, Stefan Beller wrote:
> On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi Brandon,
> >
> > On Mon, 7 Aug 2017, Brandon Williams wrote:
> >
> >> Add a '.clang-format' file which outlines the git project's coding
> >> style.  This can be used with clang-format to auto-format .c and .h
> >> files to conform with git's style.
> >>
> >> Signed-off-by: Brandon Williams <bmwill@google.com>
> >> ---
> >>
> >> I'm sure this sort of thing comes up every so often on the list but back at
> >> git-merge I mentioned how it would be nice to not have to worry about style
> >> when reviewing patches as that is something mechanical and best left to a
> >> machine (for the most part).
> >
> > Amen.
> >
> > If I never have to see a review mentioning an unwrapped line, I am quite
> > certain I will be quite content.
> >
> > Ciao,
> > Dscho
> 
> As a thought experiment I'd like to propose to take it one step further:
> 
>   If the code was formatted perfectly in one style such that a formatter for
>   this style would not produce changes when rerun again on the code, then
>   each individual could have a clean/smudge filter to work in their preferred
>   style, and only the exchange and storage of code is in a mutual agreed
>   style. If the mutually agreed style is close to what I prefer, I don't have to
>   use clean/smudge filters.
> 
> Additionally to this patch, we'd want to either put a note into
> SubmittingPatches or Documentation/gitworkflows.txt to hint at
> how to use this formatting to just affect the patch that is currently
> worked on or rather a pre-commit hook?

I'm sure both of these things will probably happen if we decide to make
use of a code-formatter.  This RFC is really just trying to ask the
question: "Is this something we want to entertain doing?"
My response would be 'Yes' but there's many other opinions to consider
first :)

-- 
Brandon Williams

Re: [RFC] clang-format: outline the git project's coding style

[Johannes Schindelin]

Hi,

On Tue, 8 Aug 2017, Brandon Williams wrote:

> On 08/08, Stefan Beller wrote:
> > On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > > Hi Brandon,
> > >
> > > On Mon, 7 Aug 2017, Brandon Williams wrote:
> > >
> > >> Add a '.clang-format' file which outlines the git project's coding
> > >> style.  This can be used with clang-format to auto-format .c and .h
> > >> files to conform with git's style.
> > >>
> > >> Signed-off-by: Brandon Williams <bmwill@google.com>
> > >> ---
> > >>
> > >> I'm sure this sort of thing comes up every so often on the list but back at
> > >> git-merge I mentioned how it would be nice to not have to worry about style
> > >> when reviewing patches as that is something mechanical and best left to a
> > >> machine (for the most part).
> > >
> > > Amen.
> > >
> > > If I never have to see a review mentioning an unwrapped line, I am quite
> > > certain I will be quite content.
> > >
> > > Ciao,
> > > Dscho
> > 
> > As a thought experiment I'd like to propose to take it one step further:
> > 
> >   If the code was formatted perfectly in one style such that a formatter for
> >   this style would not produce changes when rerun again on the code, then
> >   each individual could have a clean/smudge filter to work in their preferred
> >   style, and only the exchange and storage of code is in a mutual agreed
> >   style. If the mutually agreed style is close to what I prefer, I don't have to
> >   use clean/smudge filters.
> > 
> > Additionally to this patch, we'd want to either put a note into
> > SubmittingPatches or Documentation/gitworkflows.txt to hint at
> > how to use this formatting to just affect the patch that is currently
> > worked on or rather a pre-commit hook?
> 
> I'm sure both of these things will probably happen if we decide to make
> use of a code-formatter.  This RFC is really just trying to ask the
> question: "Is this something we want to entertain doing?"
> My response would be 'Yes' but there's many other opinions to consider
> first :)

I am sure that something even better will be possible: a Continuous
"Integration" that fixes the coding style automatically by using
`filter-branch` (avoiding the merge conflicts that would arise if `rebase
-i` was used).

Ciao,
Dscho

Re: [RFC] clang-format: outline the git project's coding style

[Stefan Beller]

>>
>> I'm sure both of these things will probably happen if we decide to make
>> use of a code-formatter.  This RFC is really just trying to ask the
>> question: "Is this something we want to entertain doing?"
>> My response would be 'Yes' but there's many other opinions to consider
>> first :)

Mine would be 'Yes', too.

>
> I am sure that something even better will be possible: a Continuous
> "Integration" that fixes the coding style automatically by using
> `filter-branch` (avoiding the merge conflicts that would arise if `rebase
> -i` was used).

I do not quite follow. Is that to be used by Junio while integrating branches?

By the wording it doesn't sound like you think of a one-time shot to be
used in the transition phase from now to a clean future, but the "CI" would
run continually on all new patches?

Re: [RFC] clang-format: outline the git project's coding style

[Johannes Schindelin]

Hi Stefan,

On Wed, 9 Aug 2017, Stefan Beller wrote:

> > I am sure that something even better will be possible: a Continuous
> > "Integration" that fixes the coding style automatically by using
> > `filter-branch` (avoiding the merge conflicts that would arise if
> > `rebase -i` was used).
> 
> I do not quite follow. Is that to be used by Junio while integrating
> branches?

I was more thinking about a bot on GitHub. "Code cleanup as a service".

Ciao,
Dscho

Re: [RFC] clang-format: outline the git project's coding style

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Wed, 9 Aug 2017, Stefan Beller wrote:
>
>> > I am sure that something even better will be possible: a Continuous
>> > "Integration" that fixes the coding style automatically by using
>> > `filter-branch` (avoiding the merge conflicts that would arise if
>> > `rebase -i` was used).
>> 
>> I do not quite follow. Is that to be used by Junio while integrating
>> branches?
>
> I was more thinking about a bot on GitHub. "Code cleanup as a service".

I vaguely recall that there was a discussion to have SubmitGit wait
for success from Travis CI; if that is already in place, then I can
sort of see how it would help individual contributors to have the
style checker in that pipeline as well.  

I have a mixed feelings about "fixing" styles automatically, though.

Re: [RFC] clang-format: outline the git project's coding style

[Brandon Williams]

On 08/10, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 9 Aug 2017, Stefan Beller wrote:
> >
> >> > I am sure that something even better will be possible: a Continuous
> >> > "Integration" that fixes the coding style automatically by using
> >> > `filter-branch` (avoiding the merge conflicts that would arise if
> >> > `rebase -i` was used).
> >> 
> >> I do not quite follow. Is that to be used by Junio while integrating
> >> branches?
> >
> > I was more thinking about a bot on GitHub. "Code cleanup as a service".
> 
> I vaguely recall that there was a discussion to have SubmitGit wait
> for success from Travis CI; if that is already in place, then I can
> sort of see how it would help individual contributors to have the
> style checker in that pipeline as well.  
> 
> I have a mixed feelings about "fixing" styles automatically, though.
> 

I still think we are far away from a world where we can fix style
automatically.  If we do want to keep pursuing this there are a number
steps we'd want to take first.

1. Settle on a concrete style and document it using a formatter's rules
   (in say a .clang-format file).  This style would most likely need to
   be tuned a little bit, at least the 'Penalty' configuration would
   need to be tuned which (as far as I understand it) is used to
   determine which rule to break first to ensure a line isn't too long.

2. Start getting contributors to use the tool to format their patches.
   This would include having some script or hook that a contributor
   could run to only format the sections of code that they touched.

3. Slowly the code base would begin to have a uniform style.  At
   some point we may want to then reformat the remaining sections of the
   code base.  At this point we could have some automated bot that fixes
   style.

I'm sure I missed a step in there somewhere though.

-- 
Brandon Williams

Re: [RFC] clang-format: outline the git project's coding style

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> On 08/10, Junio C Hamano wrote:
>
>> I vaguely recall that there was a discussion to have SubmitGit wait
>> for success from Travis CI; if that is already in place, then I can
>> sort of see how it would help individual contributors to have the
>> style checker in that pipeline as well.  
>> 
>> I have a mixed feelings about "fixing" styles automatically, though.
>
> I still think we are far away from a world where we can fix style
> automatically.  If we do want to keep pursuing this there are a number
> steps we'd want to take first.
>
> 1. Settle on a concrete style and document it using a formatter's rules
>    (in say a .clang-format file).  This style would most likely need to
>    be tuned a little bit, at least the 'Penalty' configuration would
>    need to be tuned which (as far as I understand it) is used to
>    determine which rule to break first to ensure a line isn't too long.

Yes.  I think this is what you started to get the ball rolling.
Together with what checkpatch.pl already diagnoses, I think we can
get a guideline that is more or less reasonable.

> 2. Start getting contributors to use the tool to format their patches.
>    This would include having some script or hook that a contributor
>    could run to only format the sections of code that they touched.

This, too.  Running checkpatch.pl (possibly combined with a bit of
tweaking it to match our needs) already catches many of the issues,
so a tool with a similar interface would be easy to use, I would
imagine.

> 3. Slowly the code base would begin to have a uniform style.  At
>    some point we may want to then reformat the remaining sections of the
>    code base.  At this point we could have some automated bot that fixes
>    style.

I suspect I am discussing this based on a different assumption.

I think the primary goal of this effort is to make it easier to
cleanse the new patches that appear on the list of trivial style
issues, so that contributors and reviewers do not have to spend
bandwidth and brain cycles during the review.  And I have been
assuming that we can do so even without waiting for a "tree wide"
code churn on existing code to complete.

Re: [RFC] clang-format: outline the git project's coding style

[Brandon Williams]

On 08/10, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > On 08/10, Junio C Hamano wrote:
> >
> >> I vaguely recall that there was a discussion to have SubmitGit wait
> >> for success from Travis CI; if that is already in place, then I can
> >> sort of see how it would help individual contributors to have the
> >> style checker in that pipeline as well.  
> >> 
> >> I have a mixed feelings about "fixing" styles automatically, though.
> >
> > I still think we are far away from a world where we can fix style
> > automatically.  If we do want to keep pursuing this there are a number
> > steps we'd want to take first.
> >
> > 1. Settle on a concrete style and document it using a formatter's rules
> >    (in say a .clang-format file).  This style would most likely need to
> >    be tuned a little bit, at least the 'Penalty' configuration would
> >    need to be tuned which (as far as I understand it) is used to
> >    determine which rule to break first to ensure a line isn't too long.
> 
> Yes.  I think this is what you started to get the ball rolling.
> Together with what checkpatch.pl already diagnoses, I think we can
> get a guideline that is more or less reasonable.
> 
> > 2. Start getting contributors to use the tool to format their patches.
> >    This would include having some script or hook that a contributor
> >    could run to only format the sections of code that they touched.
> 
> This, too.  Running checkpatch.pl (possibly combined with a bit of
> tweaking it to match our needs) already catches many of the issues,
> so a tool with a similar interface would be easy to use, I would
> imagine.
> 
> > 3. Slowly the code base would begin to have a uniform style.  At
> >    some point we may want to then reformat the remaining sections of the
> >    code base.  At this point we could have some automated bot that fixes
> >    style.
> 
> I suspect I am discussing this based on a different assumption.
> 
> I think the primary goal of this effort is to make it easier to
> cleanse the new patches that appear on the list of trivial style
> issues, so that contributors and reviewers do not have to spend
> bandwidth and brain cycles during the review.  And I have been
> assuming that we can do so even without waiting for a "tree wide"
> code churn on existing code to complete.

Yes that's one of the steps I missed we can call it 2.5 ;)  (3) could be
a long term goal which is what I was trying to get at by saying:

> > 3. Slowly the code base would begin to have a uniform style.

-- 
Brandon Williams

Re: [RFC] clang-format: outline the git project's coding style

[Ben Peart]

On 8/10/2017 5:30 PM, Brandon Williams wrote:
> On 08/10, Junio C Hamano wrote:
>> Brandon Williams <bmwill@google.com> writes:
>>
>>> On 08/10, Junio C Hamano wrote:
>>>
>>>> I vaguely recall that there was a discussion to have SubmitGit wait
>>>> for success from Travis CI; if that is already in place, then I can
>>>> sort of see how it would help individual contributors to have the
>>>> style checker in that pipeline as well.
>>>>
>>>> I have a mixed feelings about "fixing" styles automatically, though.
>>>
>>> I still think we are far away from a world where we can fix style
>>> automatically.  If we do want to keep pursuing this there are a number
>>> steps we'd want to take first.
>>>
>>> 1. Settle on a concrete style and document it using a formatter's rules
>>>     (in say a .clang-format file).  This style would most likely need to
>>>     be tuned a little bit, at least the 'Penalty' configuration would
>>>     need to be tuned which (as far as I understand it) is used to
>>>     determine which rule to break first to ensure a line isn't too long.
>>
>> Yes.  I think this is what you started to get the ball rolling.
>> Together with what checkpatch.pl already diagnoses, I think we can
>> get a guideline that is more or less reasonable.
>>
>>> 2. Start getting contributors to use the tool to format their patches.
>>>     This would include having some script or hook that a contributor
>>>     could run to only format the sections of code that they touched.
>>
>> This, too.  Running checkpatch.pl (possibly combined with a bit of
>> tweaking it to match our needs) already catches many of the issues,
>> so a tool with a similar interface would be easy to use, I would
>> imagine.
>>
>>> 3. Slowly the code base would begin to have a uniform style.  At
>>>     some point we may want to then reformat the remaining sections of the
>>>     code base.  At this point we could have some automated bot that fixes
>>>     style.
>>
>> I suspect I am discussing this based on a different assumption.
>>
>> I think the primary goal of this effort is to make it easier to
>> cleanse the new patches that appear on the list of trivial style
>> issues, so that contributors and reviewers do not have to spend
>> bandwidth and brain cycles during the review.  And I have been
>> assuming that we can do so even without waiting for a "tree wide"
>> code churn on existing code to complete.
> 
> Yes that's one of the steps I missed we can call it 2.5 ;)  (3) could be
> a long term goal which is what I was trying to get at by saying:
> 
>>> 3. Slowly the code base would begin to have a uniform style.
> 

Just adding my "Yes!" vote.

Consistent formatting makes the code easier to read and maintain.  I've 
been in religious wars debating whether the opening brace should be on 
the same line or the next line and while I have my personal preferences, 
I can work with just about anything as long as it is consistent. When it 
comes to dealing with a tool, I am willing to live with some "I would 
have wrapped that differently" if I can stop spending so much time 
manually wrapping code.

I think the goals should be to 1) increase readability of the code 2) 
reduce the time spent by reviewers and 3) reduce the time spent by 
contributors.  A pre-commit hook that checked for errors (and gave the 
instructions on how to automatically have the tool correct them) would 
be very helpful.

Please continue to push this forward!

Re: [RFC] clang-format: outline the git project's coding style

[Johannes Schindelin]

Hi Junio,

On Thu, 10 Aug 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 9 Aug 2017, Stefan Beller wrote:
> >
> >> > I am sure that something even better will be possible: a Continuous
> >> > "Integration" that fixes the coding style automatically by using
> >> > `filter-branch` (avoiding the merge conflicts that would arise if
> >> > `rebase -i` was used).
> >> 
> >> I do not quite follow. Is that to be used by Junio while integrating
> >> branches?
> >
> > I was more thinking about a bot on GitHub. "Code cleanup as a service".
> 
> I vaguely recall that there was a discussion to have SubmitGit wait
> for success from Travis CI; if that is already in place, then I can
> sort of see how it would help individual contributors to have the
> style checker in that pipeline as well.  
> 
> I have a mixed feelings about "fixing" styles automatically, though.

That's too bad. I would much rather focus on quality of code than
conformance of style, even if the latter is a lot easier than the former.

Ciao,
Dscho

Re: [RFC] clang-format: outline the git project's coding style

[Johannes Schindelin]

Hi all,

On Thu, 10 Aug 2017, Johannes Schindelin wrote:

> On Tue, 8 Aug 2017, Brandon Williams wrote:
> 
> > On 08/08, Stefan Beller wrote:
> > > On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
> > > >
> > > > On Mon, 7 Aug 2017, Brandon Williams wrote:
> > > >
> > > >> Add a '.clang-format' file which outlines the git project's
> > > >> coding style.  This can be used with clang-format to auto-format
> > > >> .c and .h files to conform with git's style.
> > > >>
> > > >> Signed-off-by: Brandon Williams <bmwill@google.com>
> > > >> ---
> > > >>
> > > >> I'm sure this sort of thing comes up every so often on the list
> > > >> but back at git-merge I mentioned how it would be nice to not
> > > >> have to worry about style when reviewing patches as that is
> > > >> something mechanical and best left to a machine (for the most
> > > >> part).
> > > >
> > > > Amen.
> > > >
> > > > If I never have to see a review mentioning an unwrapped line, I am quite
> > > > certain I will be quite content.
> > > 
> > > As a thought experiment I'd like to propose to take it one step further:
> > > 
> > >   If the code was formatted perfectly in one style such that a
> > >   formatter for this style would not produce changes when rerun
> > >   again on the code, then each individual could have a clean/smudge
> > >   filter to work in their preferred style, and only the exchange and
> > >   storage of code is in a mutual agreed style. If the mutually
> > >   agreed style is close to what I prefer, I don't have to use
> > >   clean/smudge filters.
> > > 
> > > Additionally to this patch, we'd want to either put a note into
> > > SubmittingPatches or Documentation/gitworkflows.txt to hint at
> > > how to use this formatting to just affect the patch that is currently
> > > worked on or rather a pre-commit hook?
> > 
> > I'm sure both of these things will probably happen if we decide to make
> > use of a code-formatter.  This RFC is really just trying to ask the
> > question: "Is this something we want to entertain doing?"
> > My response would be 'Yes' but there's many other opinions to consider
> > first :)
> 
> I am sure that something even better will be possible: a Continuous
> "Integration" that fixes the coding style automatically by using
> `filter-branch` (avoiding the merge conflicts that would arise if `rebase
> -i` was used).

FWIW I just set this up in my VSTS account, with the following build step
(performed in one of the Hosted Linux agents, i.e. I do not even have to
have a VM dedicated to the task):

-- snip --
die () {
    echo "$*" >&2
    exit 1
}

head=$(git rev-parse HEAD) ||
die "Could not determine HEAD"

test -n "$BUILD_SOURCEBRANCHNAME" ||
die "Need a source branch name to work with"

base=$(git merge-base origin/core/master HEAD) &&
count=$(git rev-list --count $base..) &&
test 0 -lt $count ||
die "Could not determine commits to clean up (count: $count)"

test -f .clang-format ||
git show origin/core/pu/.clang-format >.clang-format ||
die "Need a .clang-format"

sudo add-apt-repository 'deb http://apt.llvm.org/xenial/
llvm-toolchain-xenial main' &&
sudo apt-get update &&
sudo apt-get --allow-unauthenticated -y install clang-format-6.0 ||
die "Could not install clang-format 6.0"

git filter-branch -f --tree-filter \
    'git diff HEAD^.. |
     clang-format-diff-6.0 -p 1 -i &&

     git update-index --refresh --ignore-submodules &&
     git diff-files --quiet --ignore-submodules ||
     git commit --amend -C HEAD' $base.. ||
die "Could not rewrite branch"

if test "$head" = "$(git rev-parse HEAD)"
then
    echo "No changes in $BUILD_SOURCEBRANCHNAME introduced by clang-format" >&2
else
    git push vsts +HEAD:refs/heads/clang-format/"$BUILD_SOURCEBRANCHNAME" ||
    die "Could not push clang-format/$BUILD_SOURCEBRANCHNAME"
    echo "Clean branch pushed as clang-format/$BUILD_SOURCEBRANCHNAME" >&2
    exit 123
fi
-- snap --

A couple of notes for the interested:

- you can easily set this up for yourself, as Visual Studio Team Services
  is free for small teams up to five people (including single developer
  "teams"): https://www.visualstudio.com/team-services/, and of course you
  can back it by your own git.git fork on GitHub, no need to host the code
  in VSTS.

  Disclaimer: I work closely with the developers behind Visual Studio Team
  Services, and I am a genuine fan, yet I understand if anybody thinks of
  this as advertising the service, so this will be the only time I mention
  this.

- the script assumes that there is a `core/master` tracking upstream Git's
  master branch, then reformats the commits in the current branch that are
  not also reachable from core/master.

- The push credentials to push the result at the end are of course not
  included in the script, they need to be provided separately.

- the exit code 123 when the branch needed to be rewritten indicates to
  any consumer that the build "failed". The reason is that I want to
  integrate this into a system where I open a PR in my own account, which
  triggers the build automatically contingent on the base branch being
  core/master, and if the build fails, the PR gets "blocked", providing a
  very easy way to see that there is still work to be done.

- for the moment, I do not push back to the original branch, even if I
  could. The reason is that already my first test produced a dubious
  result, see below.

I am reasonably happy with the way this build job works right now,
especially given that I do not have to mess up any other setup I have just
to get the bleeding edge version of Clang.

Now for the dubious result.

I took my most recent contribution, the lazyload one (which you can easily
get yourself by fetching the lazyload-v2 tag from
https://github.com/dscho/git), because it was pretty self-contained and
small, only one patch. With the current .clang-format as per git.git's
master (or for that matter, pu, as they are identical), the output `git
show | clang-format-diff-6.0 -p 1` ends in this hunk:

-- snip --
@@ -43,8 +43,7 @@
        if (!proc->initialized) {
                HANDLE hnd;
                proc->initialized = 1;
-               hnd = LoadLibraryExA(proc->dll, NULL,
-                                    LOAD_LIBRARY_SEARCH_SYSTEM32);
+               hnd = LoadLibraryExA(proc->dll, NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
                if (hnd)
                        proc->pfunction = GetProcAddress(hnd, proc->function);
        }
-- snap --

(tabs intentionally converted to spaces, to show the extent of the damage
clearly)

In other words, despite the column limit of 80 (with a tab width of 8
spaces), it takes a perfectly well formatted pair of lines and combines
them into a single line that now has 84 columns. Absolutely not what we
want.

Even worse: if I replace the column limit of 80 by 79, like so:

-- snip --
diff --git a/.clang-format b/.clang-format
index 3ede2628d2d..9f686c1ed5a 100644
--- a/.clang-format
+++ b/.clang-format
@@ -6,7 +6,7 @@ UseTab: Always
 TabWidth: 8
 IndentWidth: 8
 ContinuationIndentWidth: 8
-ColumnLimit: 80
+ColumnLimit: 79
 
 # C Language specifics
 Language: Cpp
-- snap --

then that hunk vanishes and clang-format leaves the LoadLibraryEx() lines
alone!

Even stranger, if I revert to 80 columns, copy the offending line above
the conditional block so that the indentation level is different (based on
a hunch that this may have something to do with clang's understanding of
tab widths) and extend the last parameter artificially, it still breaks at
exactly 84 columns, i.e. if I make the line 84 columns long, it keeps it
as one line, if I extend it to 85 columns, it breaks the line into two.

In fact, the same holds true even with no indentation at all: if I turn
this line into a static variable assignment outside of the function, it
again breaks the line as soon as I extend it to 85 columns.

Then I repeated the exercise with clang-format-6.0 instead of
clang-format-diff-6.0, and the finding still holds. 85 columns, despite
the explicit ColumnLimit: 80 in .clang-format.

I then tried to format a file containing only the line "int i123, j123;"
with various values for ColumnLimit, and could not get it to break at all.

Any insights?

Thanks,
Dscho

Re: [RFC] clang-format: outline the git project's coding style

[Brandon Williams]

On 09/28, Johannes Schindelin wrote:
> Hi all,
>
> On Thu, 10 Aug 2017, Johannes Schindelin wrote:
>
> > On Tue, 8 Aug 2017, Brandon Williams wrote:
> >
> > > On 08/08, Stefan Beller wrote:
> > > > On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin
> > > > <Johannes.Schindelin@gmx.de> wrote:
> > > > >
> > > > > On Mon, 7 Aug 2017, Brandon Williams wrote:
> > > > >
> > > > >> Add a '.clang-format' file which outlines the git project's
> > > > >> coding style.  This can be used with clang-format to auto-format
> > > > >> .c and .h files to conform with git's style.
> > > > >>
> > > > >> Signed-off-by: Brandon Williams <bmwill@google.com>
> > > > >> ---
> > > > >>
> > > > >> I'm sure this sort of thing comes up every so often on the list
> > > > >> but back at git-merge I mentioned how it would be nice to not
> > > > >> have to worry about style when reviewing patches as that is
> > > > >> something mechanical and best left to a machine (for the most
> > > > >> part).
> > > > >
> > > > > Amen.
> > > > >
> > > > > If I never have to see a review mentioning an unwrapped line, I am quite
> > > > > certain I will be quite content.
> > > >
> > > > As a thought experiment I'd like to propose to take it one step further:
> > > >
> > > >   If the code was formatted perfectly in one style such that a
> > > >   formatter for this style would not produce changes when rerun
> > > >   again on the code, then each individual could have a clean/smudge
> > > >   filter to work in their preferred style, and only the exchange and
> > > >   storage of code is in a mutual agreed style. If the mutually
> > > >   agreed style is close to what I prefer, I don't have to use
> > > >   clean/smudge filters.
> > > >
> > > > Additionally to this patch, we'd want to either put a note into
> > > > SubmittingPatches or Documentation/gitworkflows.txt to hint at
> > > > how to use this formatting to just affect the patch that is currently
> > > > worked on or rather a pre-commit hook?
> > >
> > > I'm sure both of these things will probably happen if we decide to make
> > > use of a code-formatter.  This RFC is really just trying to ask the
> > > question: "Is this something we want to entertain doing?"
> > > My response would be 'Yes' but there's many other opinions to consider
> > > first :)
> >
> > I am sure that something even better will be possible: a Continuous
> > "Integration" that fixes the coding style automatically by using
> > `filter-branch` (avoiding the merge conflicts that would arise if `rebase
> > -i` was used).
>
> FWIW I just set this up in my VSTS account, with the following build step
> (performed in one of the Hosted Linux agents, i.e. I do not even have to
> have a VM dedicated to the task):
>
> -- snip --
> die () {
>     echo "$*" >&2
>     exit 1
> }
>
> head=$(git rev-parse HEAD) ||
> die "Could not determine HEAD"
>
> test -n "$BUILD_SOURCEBRANCHNAME" ||
> die "Need a source branch name to work with"
>
> base=$(git merge-base origin/core/master HEAD) &&
> count=$(git rev-list --count $base..) &&
> test 0 -lt $count ||
> die "Could not determine commits to clean up (count: $count)"
>
> test -f .clang-format ||
> git show origin/core/pu/.clang-format >.clang-format ||
> die "Need a .clang-format"
>
> sudo add-apt-repository 'deb http://apt.llvm.org/xenial/
> llvm-toolchain-xenial main' &&
> sudo apt-get update &&
> sudo apt-get --allow-unauthenticated -y install clang-format-6.0 ||
> die "Could not install clang-format 6.0"
>
> git filter-branch -f --tree-filter \
>     'git diff HEAD^.. |
>      clang-format-diff-6.0 -p 1 -i &&
>
>      git update-index --refresh --ignore-submodules &&
>      git diff-files --quiet --ignore-submodules ||
>      git commit --amend -C HEAD' $base.. ||
> die "Could not rewrite branch"
>
> if test "$head" = "$(git rev-parse HEAD)"
> then
>     echo "No changes in $BUILD_SOURCEBRANCHNAME introduced by clang-format" >&2
> else
>     git push vsts +HEAD:refs/heads/clang-format/"$BUILD_SOURCEBRANCHNAME" ||
>     die "Could not push clang-format/$BUILD_SOURCEBRANCHNAME"
>     echo "Clean branch pushed as clang-format/$BUILD_SOURCEBRANCHNAME" >&2
>     exit 123
> fi
> -- snap --
>
> A couple of notes for the interested:
>
> - you can easily set this up for yourself, as Visual Studio Team Services
>   is free for small teams up to five people (including single developer
>   "teams"): https://www.visualstudio.com/team-services/, and of course you
>   can back it by your own git.git fork on GitHub, no need to host the code
>   in VSTS.
>
>   Disclaimer: I work closely with the developers behind Visual Studio Team
>   Services, and I am a genuine fan, yet I understand if anybody thinks of
>   this as advertising the service, so this will be the only time I mention
>   this.
>
> - the script assumes that there is a `core/master` tracking upstream Git's
>   master branch, then reformats the commits in the current branch that are
>   not also reachable from core/master.
>
> - The push credentials to push the result at the end are of course not
>   included in the script, they need to be provided separately.
>
> - the exit code 123 when the branch needed to be rewritten indicates to
>   any consumer that the build "failed". The reason is that I want to
>   integrate this into a system where I open a PR in my own account, which
>   triggers the build automatically contingent on the base branch being
>   core/master, and if the build fails, the PR gets "blocked", providing a
>   very easy way to see that there is still work to be done.
>
> - for the moment, I do not push back to the original branch, even if I
>   could. The reason is that already my first test produced a dubious
>   result, see below.
>
> I am reasonably happy with the way this build job works right now,
> especially given that I do not have to mess up any other setup I have just
> to get the bleeding edge version of Clang.
>
> Now for the dubious result.
>
> I took my most recent contribution, the lazyload one (which you can easily
> get yourself by fetching the lazyload-v2 tag from
> https://github.com/dscho/git), because it was pretty self-contained and
> small, only one patch. With the current .clang-format as per git.git's
> master (or for that matter, pu, as they are identical), the output `git
> show | clang-format-diff-6.0 -p 1` ends in this hunk:
>
> -- snip --
> @@ -43,8 +43,7 @@
>         if (!proc->initialized) {
>                 HANDLE hnd;
>                 proc->initialized = 1;
> -               hnd = LoadLibraryExA(proc->dll, NULL,
> -                                    LOAD_LIBRARY_SEARCH_SYSTEM32);
> +               hnd = LoadLibraryExA(proc->dll, NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
>                 if (hnd)
>                         proc->pfunction = GetProcAddress(hnd, proc->function);
>         }
> -- snap --
>
> (tabs intentionally converted to spaces, to show the extent of the damage
> clearly)
>
> In other words, despite the column limit of 80 (with a tab width of 8
> spaces), it takes a perfectly well formatted pair of lines and combines
> them into a single line that now has 84 columns. Absolutely not what we
> want.
>
> Even worse: if I replace the column limit of 80 by 79, like so:
>
> -- snip --
> diff --git a/.clang-format b/.clang-format
> index 3ede2628d2d..9f686c1ed5a 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -6,7 +6,7 @@ UseTab: Always
>  TabWidth: 8
>  IndentWidth: 8
>  ContinuationIndentWidth: 8
> -ColumnLimit: 80
> +ColumnLimit: 79
>
>  # C Language specifics
>  Language: Cpp
> -- snap --
>
> then that hunk vanishes and clang-format leaves the LoadLibraryEx() lines
> alone!
>
> Even stranger, if I revert to 80 columns, copy the offending line above
> the conditional block so that the indentation level is different (based on
> a hunch that this may have something to do with clang's understanding of
> tab widths) and extend the last parameter artificially, it still breaks at
> exactly 84 columns, i.e. if I make the line 84 columns long, it keeps it
> as one line, if I extend it to 85 columns, it breaks the line into two.
>
> In fact, the same holds true even with no indentation at all: if I turn
> this line into a static variable assignment outside of the function, it
> again breaks the line as soon as I extend it to 85 columns.
>
> Then I repeated the exercise with clang-format-6.0 instead of
> clang-format-diff-6.0, and the finding still holds. 85 columns, despite
> the explicit ColumnLimit: 80 in .clang-format.
>
> I then tried to format a file containing only the line "int i123, j123;"
> with various values for ColumnLimit, and could not get it to break at all.
>
> Any insights?

I think I know what is happening here, and this is something that we
will need to tweak based on more people beginning to use the formatting
tool.

In the .clang-format file there are a number of parameters at the end:

    # Penalties
    # This decides what order things should be done if a line is too long
    PenaltyBreakAssignment: 100
    PenaltyBreakBeforeFirstCallParameter: 100
    PenaltyBreakComment: 100
    PenaltyBreakFirstLessLess: 0
    PenaltyBreakString: 100
    PenaltyExcessCharacter: 5
    PenaltyReturnTypeOnItsOwnLine: 0

These penalties are used to determine which line breaking events should be
done based on some programmable weight (or penalty).  I thing that if we
bump up the penalty on excess characters to something higher than '5'
(which i think is quite low) then it may format more like you were
expecting.

--
Brandon Williams

Re: [RFC] clang-format: outline the git project's coding style

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Add a '.clang-format' file which outlines the git project's coding
> style.  This can be used with clang-format to auto-format .c and .h
> files to conform with git's style.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>
> I'm sure this sort of thing comes up every so often on the list but back at
> git-merge I mentioned how it would be nice to not have to worry about style
> when reviewing patches as that is something mechanical and best left to a
> machine (for the most part).  I saw that 'clang-format' was brought up on the
> list once before a couple years ago
> (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing
> really came of it.  I spent a little bit of time combing through the various
> options and came up with this config based on the general style of our code
> base.  The big issue though is that our code base isn't consistent so try as
> you might you wont be able to come up with a config which matches everything we
> do (mostly due to the inconsistencies in our code base).
>
> Anyway, I thought I'd bring this topic back up and see how people feel about it.

I gave just one pass over all the rules you have here.  I didn't
think too deeply about implications of some of them, but most of
them looked sensible.  Thanks for compiling this set of rules.

Having said that, it is easier to refine individual rules you have
below than to make sure that among the develoepers there is a shared
notion of how these rules are to be used.  If we get that part wrong,
we'd see unpleasant consequences, e.g.

 - We may see unwanted code churn on existing codebase, only for the
   sake of satisfying the formatting rules specified here.

 - We may see far more style-only critique to patches posted on the
   list simply because there are more readers than writers, and it
   is likely that running the tool to nitpick other people's patches
   is far easier than writing these patches in the first place (and
   forgetting to ask the formatter to nitpick before sending them
   out).

 - Human aesthetics judgement often is necessary to overrule
   mechanical rules (e.g. A human may have two pairs of <ptr, len>
   parameters on separate lines in a function declaration;
   BinPackParameters may try to break after ptrA, lenA, ptrB before
   lenB in such a case).

We need to set our expectation and a guideline to apply these rules
straight, before introducing something like this.


>  .clang-format | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 166 insertions(+)
>  create mode 100644 .clang-format
>
> diff --git a/.clang-format b/.clang-format
> new file mode 100644
> index 000000000..7f28dc259
> --- /dev/null
> +++ b/.clang-format
> @@ -0,0 +1,166 @@
> +# Defaults
> +
> +# Use tabs whenever we need to fill whitespace that spans at least from one tab
> +# stop to the next one.
> +UseTab: Always
> +TabWidth: 8
> +IndentWidth: 8
> +ContinuationIndentWidth: 8
> +ColumnLimit: 80

I often deliberately chomp my lines much shorter than this limit,
and also I deliberately write a line that is a tad longer than this
limit some other times, if doing so makes the result easier to read.
And I know other develoepers with good taste do the same.  It is
pointless to have a discussion that begins with "who uses a physical
terminal these days that can only show 80-columns?" to tweak this
value, I think.  It is more important to give a guideline on what to
do when lines in your code goes over this limit.

A mechanical "formatter" would just find an appropriate point in a
line with least "penalty" and chomp it into two.  But an appropriate
way to make such a code that is way too deeply indented readable may
instead be judicious use of goto's and one-time helper functions,
for example, which mechanical tools would not do.

That is an example of what I meant above, i.e. a guideline to apply
these rules.  We would not want to say "clang-format suggests this
rewrite, so we should accept the resulting code that is still too
deeply indented as-is"---using the tool to point out an issue is
good, though.

> +
> +# C Language specifics
> +Language: Cpp

Hmph ;-)

> +# Add a line break after the return type of top-level functions
> +# int
> +# foo();
> +AlwaysBreakAfterReturnType: TopLevel

We do that?

> +# Pack as many parameters or arguments onto the same line as possible
> +# int myFunction(int aaaaaaaaaaaa, int bbbbbbbb,
> +#                int cccc);
> +BinPackArguments: true
> +BinPackParameters: true

I am OK with this but with the caveats I already mentioned.

> +# Insert a space after a cast
> +# x = (int32) y;    not    x = (int32)y;
> +SpaceAfterCStyleCast: true

Hmph, I thought we did the latter, i.e. cast sticks to the casted
expression without SP.

Thanks.

Re: [RFC] clang-format: outline the git project's coding style

[Brandon Williams]

On 08/08, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > Add a '.clang-format' file which outlines the git project's coding
> > style.  This can be used with clang-format to auto-format .c and .h
> > files to conform with git's style.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >
> > I'm sure this sort of thing comes up every so often on the list but back at
> > git-merge I mentioned how it would be nice to not have to worry about style
> > when reviewing patches as that is something mechanical and best left to a
> > machine (for the most part).  I saw that 'clang-format' was brought up on the
> > list once before a couple years ago
> > (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing
> > really came of it.  I spent a little bit of time combing through the various
> > options and came up with this config based on the general style of our code
> > base.  The big issue though is that our code base isn't consistent so try as
> > you might you wont be able to come up with a config which matches everything we
> > do (mostly due to the inconsistencies in our code base).
> >
> > Anyway, I thought I'd bring this topic back up and see how people feel about it.
> 
> I gave just one pass over all the rules you have here.  I didn't
> think too deeply about implications of some of them, but most of
> them looked sensible.  Thanks for compiling this set of rules.
> 
> Having said that, it is easier to refine individual rules you have
> below than to make sure that among the develoepers there is a shared
> notion of how these rules are to be used.  If we get that part wrong,
> we'd see unpleasant consequences, e.g.
> 
>  - We may see unwanted code churn on existing codebase, only for the
>    sake of satisfying the formatting rules specified here.

This is an issue when you have an inconsistent code base such as ours as
the tool, even when used to format a diff, will format the surrounding
context.

> 
>  - We may see far more style-only critique to patches posted on the
>    list simply because there are more readers than writers, and it
>    is likely that running the tool to nitpick other people's patches
>    is far easier than writing these patches in the first place (and
>    forgetting to ask the formatter to nitpick before sending them
>    out).

I would hope that use of such a tool would eventually completely
eliminate style-only critiques.  

> 
>  - Human aesthetics judgement often is necessary to overrule
>    mechanical rules (e.g. A human may have two pairs of <ptr, len>
>    parameters on separate lines in a function declaration;
>    BinPackParameters may try to break after ptrA, lenA, ptrB before
>    lenB in such a case).

I know that when you introduce a formatter there are bound to be some
issues where a human would choose one way over the other.  If we start
using a formatter I would expect some of the penalties would need to be
tweaked overtime before we rely too heavily on it.

> 
> We need to set our expectation and a guideline to apply these rules
> straight, before introducing something like this.

> 
> 
> >  .clang-format | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 166 insertions(+)
> >  create mode 100644 .clang-format
> >
> > diff --git a/.clang-format b/.clang-format
> > new file mode 100644
> > index 000000000..7f28dc259
> > --- /dev/null
> > +++ b/.clang-format
> > @@ -0,0 +1,166 @@
> > +# Defaults
> > +
> > +# Use tabs whenever we need to fill whitespace that spans at least from one tab
> > +# stop to the next one.
> > +UseTab: Always
> > +TabWidth: 8
> > +IndentWidth: 8
> > +ContinuationIndentWidth: 8
> > +ColumnLimit: 80
> 
> I often deliberately chomp my lines much shorter than this limit,
> and also I deliberately write a line that is a tad longer than this
> limit some other times, if doing so makes the result easier to read.
> And I know other develoepers with good taste do the same.  It is
> pointless to have a discussion that begins with "who uses a physical
> terminal these days that can only show 80-columns?" to tweak this
> value, I think.  It is more important to give a guideline on what to
> do when lines in your code goes over this limit.
> 
> A mechanical "formatter" would just find an appropriate point in a
> line with least "penalty" and chomp it into two.  But an appropriate
> way to make such a code that is way too deeply indented readable may
> instead be judicious use of goto's and one-time helper functions,
> for example, which mechanical tools would not do.
> 
> That is an example of what I meant above, i.e. a guideline to apply
> these rules.  We would not want to say "clang-format suggests this
> rewrite, so we should accept the resulting code that is still too
> deeply indented as-is"---using the tool to point out an issue is
> good, though.
> 
> > +
> > +# C Language specifics
> > +Language: Cpp
> 
> Hmph ;-)

Well there's no 'C' Lang option so Cpp is the closest there is ;)

> 
> > +# Add a line break after the return type of top-level functions
> > +# int
> > +# foo();
> > +AlwaysBreakAfterReturnType: TopLevel
> 
> We do that?

Haha So generally no we don't do this.  Though there are definitely many
places in our code base where we do.  Personally this makes it a bit
easier to read when you end up having long function names.  I also
worked on a code base which did this and it made it incredible easy to
grep for the definition of a function.  If you grep for 'foo()' then
you'd get all the uses of the function including the definition but if
you grep for '^foo()' you'd get only the definition.

But that's my preference, if we end up using this tool it would probably
make sense to change this.

> 
> > +# Pack as many parameters or arguments onto the same line as possible
> > +# int myFunction(int aaaaaaaaaaaa, int bbbbbbbb,
> > +#                int cccc);
> > +BinPackArguments: true
> > +BinPackParameters: true
> 
> I am OK with this but with the caveats I already mentioned.

The alternative (with this config) is to have arguments and parameters
each on their own separate line.

> 
> > +# Insert a space after a cast
> > +# x = (int32) y;    not    x = (int32)y;
> > +SpaceAfterCStyleCast: true
> 
> Hmph, I thought we did the latter, i.e. cast sticks to the casted
> expression without SP.

I've seen both and I wasn't sure which was the correct form to use.

> 
> Thanks.

-- 
Brandon Williams

Re: [RFC] clang-format: outline the git project's coding style

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

>> > +# Add a line break after the return type of top-level functions
>> > +# int
>> > +# foo();
>> > +AlwaysBreakAfterReturnType: TopLevel
>> 
>> We do that?
>
> Haha So generally no we don't do this.  Though there are definitely many
> places in our code base where we do.  Personally this makes it a bit
> easier to read when you end up having long function names.  I also
> worked on a code base which did this and it made it incredible easy to
> grep for the definition of a function.  If you grep for 'foo()' then
> you'd get all the uses of the function including the definition but if
> you grep for '^foo()' you'd get only the definition.
>
> But that's my preference, if we end up using this tool it would probably
> make sense to change this.

Yeah, I even know people who did

		int
	foo(void)

for greppability of "^foo".  It took some effort to get used to that
style.

>> > +# Insert a space after a cast
>> > +# x = (int32) y;    not    x = (int32)y;
>> > +SpaceAfterCStyleCast: true
>> 
>> Hmph, I thought we did the latter, i.e. cast sticks to the casted
>> expression without SP.
>
> I've seen both and I wasn't sure which was the correct form to use.

We do the latter because checkpatch.pl from the kernel project tells
us to, I think.

Re: [RFC] clang-format: outline the git project's coding style

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>> > +# Insert a space after a cast
>>> > +# x = (int32) y;    not    x = (int32)y;
>>> > +SpaceAfterCStyleCast: true
>>> 
>>> Hmph, I thought we did the latter, i.e. cast sticks to the casted
>>> expression without SP.
>>
>> I've seen both and I wasn't sure which was the correct form to use.
>
> We do the latter because checkpatch.pl from the kernel project tells
> us to, I think.

Before I forget, there are some rules in checkpatch.pl that I
deliberately ignore while accepting patches from the list.  

I appreciate the tool for pointing out overlong lines, but I often
find them easier to read as-is than split into two lines, as the
ones the people send in real life rarely excessively exceed the
80-col limit.  We also use things like SHA_CTX that trigger "avoid
camelcase", which I also ignore.

One thing we probably should standardize is the way the width of
bitfields in struct is specified.  I think checkpatch.pl wants to do

	struct {
		unsigned int three_bits : 3;
	};

with SP around the colon, but our codebase does not always have the
spaces there, and I see no technical reason not to follow suit, as
long as we are following most of what checkpatch.pl wants us to do.

By the way, I do not recall seeing a rule about this in your clang
format rules.  Can we spell it out in there?

I also see this:

    WARNING: __printf(string-index, first-to-check) is preferred over
    __attribute__((format(printf, string-index, first-to-check)))

but I think it is specific to the kernel source (the macro is
defined in include/linux/compiler-gcc.h and expands to the latter),
so I also ignore it.

checkpatch.pl also warns a SP immediately before HT, which I do pay
attention to, as well as trailing whitespaces.  If clang-format can
be told to check that, I think we would want to have such a rule.

For a reference, here is a sample set of changes to cache.h to
squelch most of the warnings and errors checkpatch.pl points out
that I do not ignore.

 cache.h | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/cache.h b/cache.h
index 4e390e6af8..dec807b3b0 100644
--- a/cache.h
+++ b/cache.h
@@ -25,7 +25,7 @@
 #define platform_SHA_CTX	SHA_CTX
 #define platform_SHA1_Init	SHA1_Init
 #define platform_SHA1_Update	SHA1_Update
-#define platform_SHA1_Final    	SHA1_Final
+#define platform_SHA1_Final	SHA1_Final
 #endif
 
 #define git_SHA_CTX		platform_SHA_CTX
@@ -260,7 +260,7 @@ static inline void copy_cache_entry(struct cache_entry *dst,
 
 static inline unsigned create_ce_flags(unsigned stage)
 {
-	return (stage << CE_STAGESHIFT);
+	return stage << CE_STAGESHIFT;
 }
 
 #define ce_namelen(ce) ((ce)->ce_namelen)
@@ -317,7 +317,7 @@ static inline unsigned int canon_mode(unsigned int mode)
 	return S_IFGITLINK;
 }
 
-#define cache_entry_size(len) (offsetof(struct cache_entry,name) + (len) + 1)
+#define cache_entry_size(len) (offsetof(struct cache_entry, name) + (len) + 1)
 
 #define SOMETHING_CHANGED	(1 << 0) /* unclassified changes go here */
 #define CE_ENTRY_CHANGED	(1 << 1)
@@ -373,7 +373,7 @@ extern void free_name_hash(struct index_state *istate);
 #define read_cache_unmerged() read_index_unmerged(&the_index)
 #define discard_cache() discard_index(&the_index)
 #define unmerged_cache() unmerged_index(&the_index)
-#define cache_name_pos(name, namelen) index_name_pos(&the_index,(name),(namelen))
+#define cache_name_pos(name, namelen) index_name_pos(&the_index, (name), (namelen))
 #define add_cache_entry(ce, option) add_index_entry(&the_index, (ce), (option))
 #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))
 #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))
@@ -1483,10 +1483,10 @@ struct checkout {
 	const char *base_dir;
 	int base_dir_len;
 	struct delayed_checkout *delayed_checkout;
-	unsigned force:1,
-		 quiet:1,
-		 not_new:1,
-		 refresh_cache:1;
+	unsigned force : 1,
+		 quiet : 1,
+		 not_new : 1,
+		 refresh_cache : 1;
 };
 #define CHECKOUT_INIT { NULL, "" }
 
@@ -1534,7 +1534,7 @@ extern struct alternate_object_database {
 	char path[FLEX_ARRAY];
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
-extern void read_info_alternates(const char * relative_base, int depth);
+extern void read_info_alternates(const char *relative_base, int depth);
 extern char *compute_alternate_path(const char *path, struct strbuf *err);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern int foreach_alt_odb(alt_odb_fn, void*);
@@ -1587,10 +1587,10 @@ extern struct packed_git {
 	int index_version;
 	time_t mtime;
 	int pack_fd;
-	unsigned pack_local:1,
-		 pack_keep:1,
-		 freshened:1,
-		 do_not_close:1;
+	unsigned pack_local : 1,
+		 pack_keep : 1,
+		 freshened : 1,
+		 do_not_close : 1;
 	unsigned char sha1[20];
 	struct revindex_entry *revindex;
 	/* something like ".git/objects/pack/xxxxx.pack" */
@@ -1767,10 +1767,10 @@ struct object_info {
 	union {
 		/*
 		 * struct {
-		 * 	... Nothing to expose in this case
+		 *	... Nothing to expose in this case
 		 * } cached;
 		 * struct {
-		 * 	... Nothing to expose in this case
+		 *	... Nothing to expose in this case
 		 * } loose;
 		 */
 		struct {

Re: [RFC] clang-format: outline the git project's coding style

[Brandon Williams]

On 08/09, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >>> > +# Insert a space after a cast
> >>> > +# x = (int32) y;    not    x = (int32)y;
> >>> > +SpaceAfterCStyleCast: true
> >>> 
> >>> Hmph, I thought we did the latter, i.e. cast sticks to the casted
> >>> expression without SP.
> >>
> >> I've seen both and I wasn't sure which was the correct form to use.
> >
> > We do the latter because checkpatch.pl from the kernel project tells
> > us to, I think.
> 
> Before I forget, there are some rules in checkpatch.pl that I
> deliberately ignore while accepting patches from the list.  
> 
> I appreciate the tool for pointing out overlong lines, but I often
> find them easier to read as-is than split into two lines, as the
> ones the people send in real life rarely excessively exceed the
> 80-col limit.  We also use things like SHA_CTX that trigger "avoid
> camelcase", which I also ignore.
> 
> One thing we probably should standardize is the way the width of
> bitfields in struct is specified.  I think checkpatch.pl wants to do
> 
> 	struct {
> 		unsigned int three_bits : 3;
> 	};
> 

I couldn't find a specific setting for this, but it seems like the
.clang-format file I made wants bit fields to be formatted with spaces
surrounding the ':'.

> with SP around the colon, but our codebase does not always have the
> spaces there, and I see no technical reason not to follow suit, as
> long as we are following most of what checkpatch.pl wants us to do.
> 
> By the way, I do not recall seeing a rule about this in your clang
> format rules.  Can we spell it out in there?
> 
> I also see this:
> 
>     WARNING: __printf(string-index, first-to-check) is preferred over
>     __attribute__((format(printf, string-index, first-to-check)))
> 
> but I think it is specific to the kernel source (the macro is
> defined in include/linux/compiler-gcc.h and expands to the latter),
> so I also ignore it.
> 
> checkpatch.pl also warns a SP immediately before HT, which I do pay

I'm sorry what's 'HT'?

> attention to, as well as trailing whitespaces.  If clang-format can
> be told to check that, I think we would want to have such a rule.

Clang-format by default removes trailing whitespace.

> 
> For a reference, here is a sample set of changes to cache.h to
> squelch most of the warnings and errors checkpatch.pl points out
> that I do not ignore.
> 
>  cache.h | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index 4e390e6af8..dec807b3b0 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -25,7 +25,7 @@
>  #define platform_SHA_CTX	SHA_CTX
>  #define platform_SHA1_Init	SHA1_Init
>  #define platform_SHA1_Update	SHA1_Update
> -#define platform_SHA1_Final    	SHA1_Final
> +#define platform_SHA1_Final	SHA1_Final
>  #endif
>  
>  #define git_SHA_CTX		platform_SHA_CTX
> @@ -260,7 +260,7 @@ static inline void copy_cache_entry(struct cache_entry *dst,
>  
>  static inline unsigned create_ce_flags(unsigned stage)
>  {
> -	return (stage << CE_STAGESHIFT);
> +	return stage << CE_STAGESHIFT;
>  }
>  
>  #define ce_namelen(ce) ((ce)->ce_namelen)
> @@ -317,7 +317,7 @@ static inline unsigned int canon_mode(unsigned int mode)
>  	return S_IFGITLINK;
>  }
>  
> -#define cache_entry_size(len) (offsetof(struct cache_entry,name) + (len) + 1)
> +#define cache_entry_size(len) (offsetof(struct cache_entry, name) + (len) + 1)
>  
>  #define SOMETHING_CHANGED	(1 << 0) /* unclassified changes go here */
>  #define CE_ENTRY_CHANGED	(1 << 1)
> @@ -373,7 +373,7 @@ extern void free_name_hash(struct index_state *istate);
>  #define read_cache_unmerged() read_index_unmerged(&the_index)
>  #define discard_cache() discard_index(&the_index)
>  #define unmerged_cache() unmerged_index(&the_index)
> -#define cache_name_pos(name, namelen) index_name_pos(&the_index,(name),(namelen))
> +#define cache_name_pos(name, namelen) index_name_pos(&the_index, (name), (namelen))
>  #define add_cache_entry(ce, option) add_index_entry(&the_index, (ce), (option))
>  #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))
>  #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))
> @@ -1483,10 +1483,10 @@ struct checkout {
>  	const char *base_dir;
>  	int base_dir_len;
>  	struct delayed_checkout *delayed_checkout;
> -	unsigned force:1,
> -		 quiet:1,
> -		 not_new:1,
> -		 refresh_cache:1;
> +	unsigned force : 1,
> +		 quiet : 1,
> +		 not_new : 1,
> +		 refresh_cache : 1;
>  };
>  #define CHECKOUT_INIT { NULL, "" }
>  
> @@ -1534,7 +1534,7 @@ extern struct alternate_object_database {
>  	char path[FLEX_ARRAY];
>  } *alt_odb_list;
>  extern void prepare_alt_odb(void);
> -extern void read_info_alternates(const char * relative_base, int depth);
> +extern void read_info_alternates(const char *relative_base, int depth);
>  extern char *compute_alternate_path(const char *path, struct strbuf *err);
>  typedef int alt_odb_fn(struct alternate_object_database *, void *);
>  extern int foreach_alt_odb(alt_odb_fn, void*);
> @@ -1587,10 +1587,10 @@ extern struct packed_git {
>  	int index_version;
>  	time_t mtime;
>  	int pack_fd;
> -	unsigned pack_local:1,
> -		 pack_keep:1,
> -		 freshened:1,
> -		 do_not_close:1;
> +	unsigned pack_local : 1,
> +		 pack_keep : 1,
> +		 freshened : 1,
> +		 do_not_close : 1;
>  	unsigned char sha1[20];
>  	struct revindex_entry *revindex;
>  	/* something like ".git/objects/pack/xxxxx.pack" */
> @@ -1767,10 +1767,10 @@ struct object_info {
>  	union {
>  		/*
>  		 * struct {
> -		 * 	... Nothing to expose in this case
> +		 *	... Nothing to expose in this case
>  		 * } cached;
>  		 * struct {
> -		 * 	... Nothing to expose in this case
> +		 *	... Nothing to expose in this case
>  		 * } loose;
>  		 */
>  		struct {

-- 
Brandon Williams

Re: [RFC] clang-format: outline the git project's coding style

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

>> checkpatch.pl also warns a SP immediately before HT, which I do pay
>
> I'm sorry what's 'HT'?

Horizontal tab.

"man ascii" or http://man7.org/linux/man-pages/man7/ascii.7.html

Re: [RFC] clang-format: outline the git project's coding style

[Jeff King]

On Mon, Aug 07, 2017 at 06:25:54PM -0700, Brandon Williams wrote:

> I'm sure this sort of thing comes up every so often on the list but back at
> git-merge I mentioned how it would be nice to not have to worry about style
> when reviewing patches as that is something mechanical and best left to a
> machine (for the most part).  I saw that 'clang-format' was brought up on the
> list once before a couple years ago
> (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing
> really came of it.  I spent a little bit of time combing through the various
> options and came up with this config based on the general style of our code
> base.  The big issue though is that our code base isn't consistent so try as
> you might you wont be able to come up with a config which matches everything we
> do (mostly due to the inconsistencies in our code base).

Right, the reason I stopped pursuing it was that I couldn't find a way
to have it make suggestions for new code without nagging about existing
code. If we were to aggressively reformat to match the tool for existing
code, that would help. But I'm a bit worried that there would always be
suggestions from the tool that we don't agree with (i.e., where the
guiding principle is "do what is readable").

I dunno. I guess "go fmt" people decided to just treat the tool's output
as the One True Way. I haven't written enough Go to have an opinion
myself, but it seems to at least work for them.

What does the tooling look like these days for just adjusting lines
touched by a given patch?

-Peff

Re: [RFC] clang-format: outline the git project's coding style

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 09 2017, Jeff King jotted:

> On Mon, Aug 07, 2017 at 06:25:54PM -0700, Brandon Williams wrote:
>
>> I'm sure this sort of thing comes up every so often on the list but back at
>> git-merge I mentioned how it would be nice to not have to worry about style
>> when reviewing patches as that is something mechanical and best left to a
>> machine (for the most part).  I saw that 'clang-format' was brought up on the
>> list once before a couple years ago
>> (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing
>> really came of it.  I spent a little bit of time combing through the various
>> options and came up with this config based on the general style of our code
>> base.  The big issue though is that our code base isn't consistent so try as
>> you might you wont be able to come up with a config which matches everything we
>> do (mostly due to the inconsistencies in our code base).
>
> Right, the reason I stopped pursuing it was that I couldn't find a way
> to have it make suggestions for new code without nagging about existing
> code. If we were to aggressively reformat to match the tool for existing
> code, that would help. But I'm a bit worried that there would always be
> suggestions from the tool that we don't agree with (i.e., where the
> guiding principle is "do what is readable").
>
> I dunno. I guess "go fmt" people decided to just treat the tool's output
> as the One True Way. I haven't written enough Go to have an opinion
> myself, but it seems to at least work for them.

(I have no opinion either way on whether this clang formatting this is a
good idea or not)

> What does the tooling look like these days for just adjusting lines
> touched by a given patch?

Presumably even if it sucked we could easily write a "./git-fmt-check.sh
<file>" script to do it which would do the following:

 1. Check out the master branch
 2. Apply code formatting to entire project (or just the files you
    changed)
 3. Commit that on a throwaway branch
 4. Switch back to your WIP branch
 5. See if it would merge cleanly with the throwaway code formatting
    branch (I forget the actual 'not a real merge but check' command to
    do this, but it exists).

If there were any reported conflicts presumably the new code you're
adding is violating the coding standards laid out in this file. If not
you're good.

Re: [RFC] clang-format: outline the git project's coding style

[Stefan Beller]

On Wed, Aug 9, 2017 at 7:03 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Wed, Aug 09 2017, Jeff King jotted:
>
>> On Mon, Aug 07, 2017 at 06:25:54PM -0700, Brandon Williams wrote:
>>
>>> I'm sure this sort of thing comes up every so often on the list but back at
>>> git-merge I mentioned how it would be nice to not have to worry about style
>>> when reviewing patches as that is something mechanical and best left to a
>>> machine (for the most part).  I saw that 'clang-format' was brought up on the
>>> list once before a couple years ago
>>> (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing
>>> really came of it.  I spent a little bit of time combing through the various
>>> options and came up with this config based on the general style of our code
>>> base.  The big issue though is that our code base isn't consistent so try as
>>> you might you wont be able to come up with a config which matches everything we
>>> do (mostly due to the inconsistencies in our code base).
>>
>> Right, the reason I stopped pursuing it was that I couldn't find a way
>> to have it make suggestions for new code without nagging about existing
>> code. If we were to aggressively reformat to match the tool for existing
>> code, that would help. But I'm a bit worried that there would always be
>> suggestions from the tool that we don't agree with (i.e., where the
>> guiding principle is "do what is readable").

We may have different opinions on what is readable/beautiful code.
If we were to follow a mutual agreed style that is produced by a tool,
we could use clean/smudge filters with different settings each.

But I think we'd rather want to find the closest approximation to our
current style first.

>> I dunno. I guess "go fmt" people decided to just treat the tool's output
>> as the One True Way. I haven't written enough Go to have an opinion
>> myself, but it seems to at least work for them.
>
> (I have no opinion either way on whether this clang formatting this is a
> good idea or not)

I think it is actually beneficial as it is one less thing to worry about
as a contributor.  Maybe compare it to programming language that
has garbage collection built in, which is also a feature to allow the
contributor to focus on "what is important". (style is not, all it can do
is hold back progress by too much nitpicking IMHO)

>> What does the tooling look like these days for just adjusting lines
>> touched by a given patch?

$ clang-format --help

USAGE: clang-format [options] [<file> ...]
..
  -i                        - Inplace edit <file>s, if specified..
  -lines=<string>           - <start line>:<end line> - format a range of
                              lines (both 1-based).
                              Multiple ranges can be formatted by specifying
                              several -lines arguments.
                              Can't be used with -offset and -length.
                              Can only be used with one input file.
..

I would think based on these options, a pre commit hook can be
written that formats precisely the touched lines of code of each file.

>
> Presumably even if it sucked we could easily write a "./git-fmt-check.sh
> <file>" script to do it which would do the following:
>
>  1. Check out the master branch
>  2. Apply code formatting to entire project (or just the files you
>     changed)
>  3. Commit that on a throwaway branch
>  4. Switch back to your WIP branch
>  5. See if it would merge cleanly with the throwaway code formatting
>     branch (I forget the actual 'not a real merge but check' command to
>     do this, but it exists).
>
> If there were any reported conflicts presumably the new code you're
> adding is violating the coding standards laid out in this file. If not
> you're good.

This approach certainly works, but it *adds* one more step to what
a contributor may need to do before sending a patch. I think the intention
with a codified style is to *remove* a step (as a machine will do it for you).

Re: [RFC] clang-format: outline the git project's coding style

[Stefan Beller]

On Wed, Aug 9, 2017 at 3:53 PM, Stefan Beller <sbeller@google.com> wrote:

> I would think based on these options, a pre commit hook can be
> written that formats precisely the touched lines of code of each file.

I did not search enough, "clang-tidy-diff.py --fix" should be all that is needed

Re: [RFC] clang-format: outline the git project's coding style

[Brandon Williams]

On 08/09, Stefan Beller wrote:
> On Wed, Aug 9, 2017 at 3:53 PM, Stefan Beller <sbeller@google.com> wrote:
> 
> > I would think based on these options, a pre commit hook can be
> > written that formats precisely the touched lines of code of each file.
> 
> I did not search enough, "clang-tidy-diff.py --fix" should be all that is needed

I believe clang-tidy is different from clang-format am I mistaken?

-- 
Brandon Williams

Re: [RFC] clang-format: outline the git project's coding style

[Jeff King]

On Fri, Aug 11, 2017 at 10:52:37AM -0700, Brandon Williams wrote:

> On 08/09, Stefan Beller wrote:
> > On Wed, Aug 9, 2017 at 3:53 PM, Stefan Beller <sbeller@google.com> wrote:
> > 
> > > I would think based on these options, a pre commit hook can be
> > > written that formats precisely the touched lines of code of each file.
> > 
> > I did not search enough, "clang-tidy-diff.py --fix" should be all that is needed
> 
> I believe clang-tidy is different from clang-format am I mistaken?

Yeah, I just dug in the archive. The script I ran way back when was
actually clang-format-diff.

-Peff

Re: [RFC] clang-format: outline the git project's coding style

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Aug 11, 2017 at 10:52:37AM -0700, Brandon Williams wrote:
>
>> On 08/09, Stefan Beller wrote:
>> > On Wed, Aug 9, 2017 at 3:53 PM, Stefan Beller <sbeller@google.com> wrote:
>> > 
>> > > I would think based on these options, a pre commit hook can be
>> > > written that formats precisely the touched lines of code of each file.
>> > 
>> > I did not search enough, "clang-tidy-diff.py --fix" should be all that is needed
>> 
>> I believe clang-tidy is different from clang-format am I mistaken?
>
> Yeah, I just dug in the archive. The script I ran way back when was
> actually clang-format-diff.

I am confident with the competence of people around here that we can
come up with a reasonable checker for obvious style violations. In
the worst case, we could customize and/or tweak checkpatch.pl and
start from there.

Assuming that we can have such a checker, I am more interested in
the way how people envision such a checker fits in our workflow to
help people.  Earlier Dscho floated an idea to integrate with the
GitHub pull requests in a way similar to how Travis and SubmitGit
are triggered, and I can sort of see how it may help, but I haven't
seen ideas from others.

Re: [RFC] clang-format: outline the git project's coding style

[Jeff King]

On Fri, Aug 11, 2017 at 09:39:11PM -0700, Junio C Hamano wrote:

> > Yeah, I just dug in the archive. The script I ran way back when was
> > actually clang-format-diff.
> 
> I am confident with the competence of people around here that we can
> come up with a reasonable checker for obvious style violations. In
> the worst case, we could customize and/or tweak checkpatch.pl and
> start from there.

I am confident we _can_, too. My question is whether we will. :)

> Assuming that we can have such a checker, I am more interested in
> the way how people envision such a checker fits in our workflow to
> help people.  Earlier Dscho floated an idea to integrate with the
> GitHub pull requests in a way similar to how Travis and SubmitGit
> are triggered, and I can sort of see how it may help, but I haven't
> seen ideas from others.

Yeah, I agree. I assume most people already run "make test" locally. I'd
be happy enough if we started with a "make style" that offers style
suggestions for you to accept. From there we can grow into
"automatically apply suggestions" and integrating with things like
submitGit.

-Peff

Re: [RFC] clang-format: outline the git project's coding style

[Ramsay Jones]

On 13/08/17 05:41, Jeff King wrote:
> On Fri, Aug 11, 2017 at 09:39:11PM -0700, Junio C Hamano wrote:
> 
>>> Yeah, I just dug in the archive. The script I ran way back when was
>>> actually clang-format-diff.
>>
>> I am confident with the competence of people around here that we can
>> come up with a reasonable checker for obvious style violations. In
>> the worst case, we could customize and/or tweak checkpatch.pl and
>> start from there.
> 
> I am confident we _can_, too. My question is whether we will. :)
> 
>> Assuming that we can have such a checker, I am more interested in
>> the way how people envision such a checker fits in our workflow to
>> help people.  Earlier Dscho floated an idea to integrate with the
>> GitHub pull requests in a way similar to how Travis and SubmitGit
>> are triggered, and I can sort of see how it may help, but I haven't
>> seen ideas from others.
> 
> Yeah, I agree. I assume most people already run "make test" locally. I'd
> be happy enough if we started with a "make style" that offers style
> suggestions for you to accept. From there we can grow into
> "automatically apply suggestions" and integrating with things like
> submitGit.

As a start, how about something like this:

-- >8 --
$ git diff
diff --git a/Makefile b/Makefile
index 461c845d3..7555def45 100644
--- a/Makefile
+++ b/Makefile
@@ -2440,6 +2440,18 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
+ST_C = $(patsubst %.o,%.stc,$(C_OBJ))
+ST_H = $(patsubst %.h,%.sth,$(LIB_H))
+
+$(ST_C): %.stc: %.c FORCE
+       checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<
+
+$(ST_H): %.sth: %.h FORCE
+       checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<
+
+.PHONY: style $(ST_C) $(ST_H)
+style: $(ST_C) $(ST_H)
+
 check: common-cmds.h
        @if sparse; \
        then \
$ 
-- >8 --

To give it a try:

    $ make git.stc    # just run checkpatch over git.c
    $ make cache.sth  # just run checkpatch over cache.h
    $ make -k style >style.out 2>&1  # yep, large output!

A bit crude, but workable. ;-)

Just FYI, for me:

$ wc -l style.out
144076 style.out
$ 
$ grep '^WARNING' style.out | cut -d: -f1,2 | sort | uniq -c
      2 WARNING:AVOID_EXTERNS
    495 WARNING:BLOCK_COMMENT_STYLE
    127 WARNING:BRACES
    213 WARNING:CONSTANT_COMPARISON
  12584 WARNING:CONST_STRUCT
      5 WARNING:CVS_KEYWORD
     26 WARNING:DEEP_INDENTATION
      2 WARNING:DEFAULT_NO_BREAK
     77 WARNING:EMBEDDED_FUNCTION_NAME
      5 WARNING:ENOSYS
    773 WARNING:FUNCTION_ARGUMENTS
     39 WARNING:INDENTED_LABEL
   2610 WARNING:LEADING_SPACE
      1 WARNING:LINE_CONTINUATIONS
   2680 WARNING:LINE_SPACING
   3975 WARNING:LONG_LINE
    330 WARNING:LONG_LINE_COMMENT
    380 WARNING:LONG_LINE_STRING
      2 WARNING:MACRO_WITH_FLOW_CONTROL
      1 WARNING:MISORDERED_TYPE
     17 WARNING:MISSING_SPACE
      1 WARNING:ONE_SEMICOLON
     77 WARNING:PREFER_PRINTF
      7 WARNING:QUOTED_WHITESPACE_BEFORE_NEWLINE
     10 WARNING:RETURN_VOID
      5 WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO
      6 WARNING:SIZEOF_PARENTHESIS
     61 WARNING:SPACE_BEFORE_TAB
    347 WARNING:SPACING
    322 WARNING:SPLIT_STRING
     87 WARNING:STATIC_CONST_CHAR_ARRAY
      2 WARNING:STORAGE_CLASS
    538 WARNING:SUSPECT_CODE_INDENT
     29 WARNING:SYMBOLIC_PERMS
    279 WARNING:TABSTOP
      9 WARNING:TRAILING_SEMICOLON
      3 WARNING:TYPECAST_INT_CONSTANT
      2 WARNING:UNNECESSARY_BREAK
     56 WARNING:UNNECESSARY_ELSE
    568 WARNING:UNSPECIFIED_INT
      4 WARNING:USE_NEGATIVE_ERRNO
     26 WARNING:VOLATILE
$ 

Hmm, on reflection, it may be a bit too crude! :-D

ATB,
Ramsay Jones

Re: [RFC] clang-format: outline the git project's coding style

[Ramsay Jones]

On 13/08/17 17:14, Ramsay Jones wrote:
> 
> 
> On 13/08/17 05:41, Jeff King wrote:
>> On Fri, Aug 11, 2017 at 09:39:11PM -0700, Junio C Hamano wrote:
>>
>>>> Yeah, I just dug in the archive. The script I ran way back when was
>>>> actually clang-format-diff.
>>>
>>> I am confident with the competence of people around here that we can
>>> come up with a reasonable checker for obvious style violations. In
>>> the worst case, we could customize and/or tweak checkpatch.pl and
>>> start from there.
>>
>> I am confident we _can_, too. My question is whether we will. :)
>>
>>> Assuming that we can have such a checker, I am more interested in
>>> the way how people envision such a checker fits in our workflow to
>>> help people.  Earlier Dscho floated an idea to integrate with the
>>> GitHub pull requests in a way similar to how Travis and SubmitGit
>>> are triggered, and I can sort of see how it may help, but I haven't
>>> seen ideas from others.
>>
>> Yeah, I agree. I assume most people already run "make test" locally. I'd
>> be happy enough if we started with a "make style" that offers style
>> suggestions for you to accept. From there we can grow into
>> "automatically apply suggestions" and integrating with things like
>> submitGit.
> 
> As a start, how about something like this:
> 
> -- >8 --
> $ git diff
> diff --git a/Makefile b/Makefile
> index 461c845d3..7555def45 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2440,6 +2440,18 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)
>  
> +ST_C = $(patsubst %.o,%.stc,$(C_OBJ))
> +ST_H = $(patsubst %.h,%.sth,$(LIB_H))
> +
> +$(ST_C): %.stc: %.c FORCE
> +       checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<
> +
> +$(ST_H): %.sth: %.h FORCE
> +       checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<
> +
> +.PHONY: style $(ST_C) $(ST_H)
> +style: $(ST_C) $(ST_H)
> +
>  check: common-cmds.h
>         @if sparse; \
>         then \
> $ 
> -- >8 --
> 
> To give it a try:
> 
>     $ make git.stc    # just run checkpatch over git.c
>     $ make cache.sth  # just run checkpatch over cache.h
>     $ make -k style >style.out 2>&1  # yep, large output!
> 
> A bit crude, but workable. ;-)
> 
> Just FYI, for me:
> 
> $ wc -l style.out
> 144076 style.out
> $ 
> $ grep '^WARNING' style.out | cut -d: -f1,2 | sort | uniq -c
>       2 WARNING:AVOID_EXTERNS
>     495 WARNING:BLOCK_COMMENT_STYLE
>     127 WARNING:BRACES
>     213 WARNING:CONSTANT_COMPARISON
>   12584 WARNING:CONST_STRUCT
>       5 WARNING:CVS_KEYWORD
>      26 WARNING:DEEP_INDENTATION
>       2 WARNING:DEFAULT_NO_BREAK
>      77 WARNING:EMBEDDED_FUNCTION_NAME
>       5 WARNING:ENOSYS
>     773 WARNING:FUNCTION_ARGUMENTS
>      39 WARNING:INDENTED_LABEL
>    2610 WARNING:LEADING_SPACE
>       1 WARNING:LINE_CONTINUATIONS
>    2680 WARNING:LINE_SPACING
>    3975 WARNING:LONG_LINE
>     330 WARNING:LONG_LINE_COMMENT
>     380 WARNING:LONG_LINE_STRING
>       2 WARNING:MACRO_WITH_FLOW_CONTROL
>       1 WARNING:MISORDERED_TYPE
>      17 WARNING:MISSING_SPACE
>       1 WARNING:ONE_SEMICOLON
>      77 WARNING:PREFER_PRINTF
>       7 WARNING:QUOTED_WHITESPACE_BEFORE_NEWLINE
>      10 WARNING:RETURN_VOID
>       5 WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO
>       6 WARNING:SIZEOF_PARENTHESIS
>      61 WARNING:SPACE_BEFORE_TAB
>     347 WARNING:SPACING
>     322 WARNING:SPLIT_STRING
>      87 WARNING:STATIC_CONST_CHAR_ARRAY
>       2 WARNING:STORAGE_CLASS
>     538 WARNING:SUSPECT_CODE_INDENT
>      29 WARNING:SYMBOLIC_PERMS
>     279 WARNING:TABSTOP
>       9 WARNING:TRAILING_SEMICOLON
>       3 WARNING:TYPECAST_INT_CONSTANT
>       2 WARNING:UNNECESSARY_BREAK
>      56 WARNING:UNNECESSARY_ELSE
>     568 WARNING:UNSPECIFIED_INT
>       4 WARNING:USE_NEGATIVE_ERRNO
>      26 WARNING:VOLATILE
> $ 

oops, I forgot this:

$ grep '^ERROR' style.out | cut -d: -f1,2 | sort | uniq -c
    216 ERROR:ASSIGN_IN_IF
     46 ERROR:CODE_INDENT
     41 ERROR:COMPLEX_MACRO
    317 ERROR:ELSE_AFTER_BRACE
      3 ERROR:FUNCTION_WITHOUT_ARGS
      1 ERROR:GLOBAL_INITIALISERS
     46 ERROR:INITIALISED_STATIC
      2 ERROR:INLINE_LOCATION
      5 ERROR:MULTISTATEMENT_MACRO_USE_DO_WHILE
    305 ERROR:OPEN_BRACE
    314 ERROR:POINTER_LOCATION
      5 ERROR:RETURN_PARENTHESES
   3464 ERROR:SPACING
      7 ERROR:SWITCH_CASE_INDENT_LEVEL
    326 ERROR:TRAILING_STATEMENTS
     15 ERROR:TRAILING_WHITESPACE
$ 

> Hmm, on reflection, it may be a bit too crude! :-D
> 
> ATB,
> Ramsay Jones
> 
> 

Re: [RFC] clang-format: outline the git project's coding style

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> As a start, how about something like this:
>
> -- >8 --
> $ git diff
> diff --git a/Makefile b/Makefile
> index 461c845d3..7555def45 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2440,6 +2440,18 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)
>  
> +ST_C = $(patsubst %.o,%.stc,$(C_OBJ))
> +ST_H = $(patsubst %.h,%.sth,$(LIB_H))
> +
> +$(ST_C): %.stc: %.c FORCE
> +       checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<
> +
> +$(ST_H): %.sth: %.h FORCE
> +       checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<
> +
> +.PHONY: style $(ST_C) $(ST_H)
> +style: $(ST_C) $(ST_H)
> +
>  check: common-cmds.h
>         @if sparse; \
>         then \
> $ 
> -- >8 --
> ...
> Hmm, on reflection, it may be a bit too crude! :-D

As you already saw in the output from this, I think this is a good
illustration that shows why we want an incremental tool that works
on the changes, not on full file contents.  Contributors who want
their changes accepted and want to help the review process by
avoiding trivial coding style violations in their patches should not
have to find _their_ piece from an output about the whole file
contents, most of which is likely to have been inherited from the
original.  They are not working on Git to produce unnecessary code
churn whose only purpose is to make existing and otherwise dormant
code conform to the style tool's liking.  That's not their focus.

IOW I was expecting something that works on the output from "git
diff HEAD" or "git format-patch --stdout @{u}.."

Unless you were somehow envisioning that having a baseline line
this, and then take another full dump after their patch and
comparing the two, would make a good foundation for that incremental
checker, that is.  I am not sure if that would be a workable
approach myself, though.

Thanks.

Re: [RFC] clang-format: outline the git project's coding style

[Ramsay Jones]

On 13/08/17 18:33, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>> Hmm, on reflection, it may be a bit too crude! :-D
> 
> As you already saw in the output from this, I think this is a good
> illustration that shows why we want an incremental tool that works
> on the changes, not on full file contents.  Contributors who want
> their changes accepted and want to help the review process by
> avoiding trivial coding style violations in their patches should not
> have to find _their_ piece from an output about the whole file
> contents, most of which is likely to have been inherited from the
> original.  They are not working on Git to produce unnecessary code
> churn whose only purpose is to make existing and otherwise dormant
> code conform to the style tool's liking.  That's not their focus.
> 
> IOW I was expecting something that works on the output from "git
> diff HEAD" or "git format-patch --stdout @{u}.."

Yes, I had already tried the following, which maybe more workable,
but it is only lightly tested. (we may want to create our own version
of checkpatch.pl, which is written specifically for the kernel ...)

$ git diff
diff --git a/Makefile b/Makefile
index 461c845d3..a25028e68 100644
--- a/Makefile
+++ b/Makefile
@@ -2440,6 +2440,15 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
+STYLE_REVS = HEAD
+STYLE_IGNORES = NEW_TYPEDEFS,INLINE
+
+.PHONY: style
+style:
+       @git diff $(STYLE_REVS) -- '*.[ch]' | \
+               checkpatch.pl -q --no-tree --show-types \
+               --ignore=$(STYLE_IGNORES) --patch - 2>/dev/null || true
+
 check: common-cmds.h
        @if sparse; \
        then \
$ make style  # I don't have any changes to *.[ch] files!
$ make STYLE_REVS=HEAD~3 style
WARNING:LONG_LINE: line over 80 characters
#181: FILE: git.c:320:
+		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&

WARNING:LONG_LINE: line over 80 characters
#221: FILE: revision.c:2317:
+	if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {

total: 0 errors, 2 warnings, 206 lines checked
$ 

I suspect this closer to what you had in mind. ;-)
(although the --ignore list may need adding to).

ATB,
Ramsay Jones

Re: [RFC] clang-format: outline the git project's coding style

[Jeff King]

On Wed, Aug 09, 2017 at 03:53:17PM -0700, Stefan Beller wrote:

> >> Right, the reason I stopped pursuing it was that I couldn't find a way
> >> to have it make suggestions for new code without nagging about existing
> >> code. If we were to aggressively reformat to match the tool for existing
> >> code, that would help. But I'm a bit worried that there would always be
> >> suggestions from the tool that we don't agree with (i.e., where the
> >> guiding principle is "do what is readable").
> 
> We may have different opinions on what is readable/beautiful code.
> If we were to follow a mutual agreed style that is produced by a tool,
> we could use clean/smudge filters with different settings each.

I'm less worried about a difference of opinion between humans. My
concern is that there are cases that the tool's formatting makes _worse_
than what any human would write. And either we accept ugly code because
the tool sucks, or we spend a bunch of time fighting with the tool to
try to make its output look good.

> > I would think based on these options, a pre commit hook can be
> > written that formats precisely the touched lines of code of each file.
> 
> I did not search enough, "clang-tidy-diff.py --fix" should be all that is needed

I think I found that script when we discussed this a while back, but I
couldn't get it to stop bugging me about lines that I hadn't touched.  I
haven't looked at it recently, though.  That's specifically what I was
wondering about with "is the tooling ready for this".

-Peff

Re: [RFC] clang-format: outline the git project's coding style

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

On Wed, Aug 09, 2017 at 07:19:00PM -0400, Jeff King wrote:
> On Wed, Aug 09, 2017 at 03:53:17PM -0700, Stefan Beller wrote:
> 
> > >> Right, the reason I stopped pursuing it was that I couldn't find a way
> > >> to have it make suggestions for new code without nagging about existing
> > >> code. If we were to aggressively reformat to match the tool for existing
> > >> code, that would help. But I'm a bit worried that there would always be
> > >> suggestions from the tool that we don't agree with (i.e., where the
> > >> guiding principle is "do what is readable").
> > 
> > We may have different opinions on what is readable/beautiful code.
> > If we were to follow a mutual agreed style that is produced by a tool,
> > we could use clean/smudge filters with different settings each.
> 
> I'm less worried about a difference of opinion between humans. My
> concern is that there are cases that the tool's formatting makes _worse_
> than what any human would write. And either we accept ugly code because
> the tool sucks, or we spend a bunch of time fighting with the tool to
> try to make its output look good.

This has been my issue with clang-format in the past.  I have an SHA-256
implementation with an array of 64 32-bit hex integers.  These fit six
to a line, but for neatness and consistency reasons, I'd like them four
to a line (4 divides 64, but 6 does not).  Last I checked, clang-format
didn't allow me that option: it reordered them because it could fit six
on a line.  This is not the only issue I discovered, just the most
memorable.

Other tools, such as perltidy, have traditionally honored existing line
breaks better (although not perfectly), which lets humans optimize for
readability.

Of course, clang-format could have dramatically improved since I last
looked (which was around clang 3.4 or 3.6, I think).

Overall, I do like the idea of using tidy tools, because it does reduce
quibbling over style quite a bit.  I just like the tools to be more
responsive to the whitespace they're given on input.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [RFC] clang-format: outline the git project's coding style

[Jonathan Nieder]

Hi,

brian m. carlson wrote:
>> On Wed, Aug 09, 2017 at 03:53:17PM -0700, Stefan Beller wrote:

>>> We may have different opinions on what is readable/beautiful code.
>>> If we were to follow a mutual agreed style that is produced by a tool,
>>> we could use clean/smudge filters with different settings each.

I think this is a long way away --- long enough away that by the time
such a change could be a serious possibility, a lot may have changed
and the project is likely to know a lot more.  In other words, I don't
see speculating about that future as being likely to produce useful
results today.

It would be a different story if we were writing a new codebase from
scratch.  In that case, I would be all for the gofmt approach. :)

> On Wed, Aug 09, 2017 at 07:19:00PM -0400, Jeff King wrote:

>> I'm less worried about a difference of opinion between humans. My
>> concern is that there are cases that the tool's formatting makes _worse_
>> than what any human would write. And either we accept ugly code because
>> the tool sucks, or we spend a bunch of time fighting with the tool to
>> try to make its output look good.
>
> This has been my issue with clang-format in the past.  I have an SHA-256
> implementation with an array of 64 32-bit hex integers.  These fit six
> to a line, but for neatness and consistency reasons, I'd like them four
> to a line (4 divides 64, but 6 does not).  Last I checked, clang-format
> didn't allow me that option: it reordered them because it could fit six
> on a line.  This is not the only issue I discovered, just the most
> memorable.

In case it comes up again for you in a project that has adopted the
gofmt approach: you can signify that your line breaks are intentional
by putting line comments at the end of each line and clang-format will
respect them.

The clang-format documentation also mentions[1] that you can do

  /* clang-format off */
  const double kIdentityMatrix[] = {
    1, 0, 0,
    0, 1, 0,
    0, 0, 1,
  };
  /* clang-format on */

Thanks,
Jonathan

[1] http://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code

[PATCH v2 0/2] clang-format

[Brandon Williams]

Changes in v2:
 * Changed a couple rules to be more inline with our coding style.
 * Added a Makefile build rule to run git-clang-format on the diff of the
   working tree to suggest style changes.

I found that the llvm project also has the git-clang-format tool which will
allow for doing formating changes based on diffs so that the parts of the code
you didn't touch won't be formated.  It also has a nice '-p' option to only
apply hunks of suggested changes.  I also saw what Ramsay did and attempted to
add a Makefile rule to run clang-format.  This bit may need more tweaking to
get it right.

Brandon Williams (2):
  clang-format: outline the git project's coding style
  Makefile: add style build rule

 .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 Makefile      |   4 ++
 2 files changed, 169 insertions(+)
 create mode 100644 .clang-format

-- 
2.14.1.480.gb18f417b89-goog

[PATCH v2 1/2] clang-format: outline the git project's coding style

[Brandon Williams]

Add a '.clang-format' file which outlines the git project's coding
style.  This can be used with clang-format to auto-format .c and .h
files to conform with git's style.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 165 insertions(+)
 create mode 100644 .clang-format

diff --git a/.clang-format b/.clang-format
new file mode 100644
index 000000000..3ede2628d
--- /dev/null
+++ b/.clang-format
@@ -0,0 +1,165 @@
+# Defaults
+
+# Use tabs whenever we need to fill whitespace that spans at least from one tab
+# stop to the next one.
+UseTab: Always
+TabWidth: 8
+IndentWidth: 8
+ContinuationIndentWidth: 8
+ColumnLimit: 80
+
+# C Language specifics
+Language: Cpp
+
+# Align parameters on the open bracket
+# someLongFunction(argument1,
+#                  argument2);
+AlignAfterOpenBracket: Align
+
+# Don't align consecutive assignments
+# int aaaa = 12;
+# int b = 14;
+AlignConsecutiveAssignments: false
+
+# Don't align consecutive declarations
+# int aaaa = 12;
+# double b = 3.14;
+AlignConsecutiveDeclarations: false
+
+# Align escaped newlines as far left as possible
+# #define A   \
+#   int aaaa; \
+#   int b;    \
+#   int cccccccc;
+AlignEscapedNewlines: Left
+
+# Align operands of binary and ternary expressions
+# int aaa = bbbbbbbbbbb +
+#           cccccc;
+AlignOperands: true
+
+# Don't align trailing comments
+# int a; // Comment a
+# int b = 2; // Comment b
+AlignTrailingComments: false
+
+# By default don't allow putting parameters onto the next line
+# myFunction(foo, bar, baz);
+AllowAllParametersOfDeclarationOnNextLine: false
+
+# Don't allow short braced statements to be on a single line
+# if (a)           not       if (a) return;
+#   return;
+AllowShortBlocksOnASingleLine: false
+AllowShortCaseLabelsOnASingleLine: false
+AllowShortFunctionsOnASingleLine: false
+AllowShortIfStatementsOnASingleLine: false
+AllowShortLoopsOnASingleLine: false
+
+# By default don't add a line break after the return type of top-level functions
+# int foo();
+AlwaysBreakAfterReturnType: None
+
+# Pack as many parameters or arguments onto the same line as possible
+# int myFunction(int aaaaaaaaaaaa, int bbbbbbbb,
+#                int cccc);
+BinPackArguments: true
+BinPackParameters: true
+
+# Attach braces to surrounding context except break before braces on function
+# definitions.
+# void foo()
+# {
+#    if (true) {
+#    } else {
+#    }
+# };
+BreakBeforeBraces: Linux
+
+# Break after operators
+# int valuve = aaaaaaaaaaaaa +
+#              bbbbbb -
+#              ccccccccccc;
+BreakBeforeBinaryOperators: None
+BreakBeforeTernaryOperators: false
+
+# Don't break string literals
+BreakStringLiterals: false
+
+# Use the same indentation level as for the switch statement.
+# Switch statement body is always indented one level more than case labels.
+IndentCaseLabels: false
+
+# Don't indent a function definition or declaration if it is wrapped after the
+# type
+IndentWrappedFunctionNames: false
+
+# Align pointer to the right
+# int *a;
+PointerAlignment: Right
+
+# Don't insert a space after a cast
+# x = (int32)y;    not    x = (int32) y;
+SpaceAfterCStyleCast: false
+
+# Insert spaces before and after assignment operators
+# int a = 5;    not    int a=5;
+# a += 42;             a+=42;
+SpaceBeforeAssignmentOperators: true
+
+# Put a space before opening parentheses only after control statement keywords.
+# void f() {
+#   if (true) {
+#     f();
+#   }
+# }
+SpaceBeforeParens: ControlStatements
+
+# Don't insert spaces inside empty '()'
+SpaceInEmptyParentheses: false
+
+# The number of spaces before trailing line comments (// - comments).
+# This does not affect trailing block comments (/* - comments).
+SpacesBeforeTrailingComments: 1
+
+# Don't insert spaces in casts
+# x = (int32) y;    not    x = ( int32 ) y;
+SpacesInCStyleCastParentheses: false
+
+# Don't insert spaces inside container literals
+# var arr = [1, 2, 3];    not    var arr = [ 1, 2, 3 ];
+SpacesInContainerLiterals: false
+
+# Don't insert spaces after '(' or before ')'
+# f(arg);    not    f( arg );
+SpacesInParentheses: false
+
+# Don't insert spaces after '[' or before ']'
+# int a[5];    not    int a[ 5 ];
+SpacesInSquareBrackets: false
+
+# Insert a space after '{' and before '}' in struct initializers
+Cpp11BracedListStyle: false
+
+# A list of macros that should be interpreted as foreach loops instead of as
+# function calls.
+ForEachMacros: ['for_each_string_list_item']
+
+# The maximum number of consecutive empty lines to keep.
+MaxEmptyLinesToKeep: 1
+
+# No empty line at the start of a block.
+KeepEmptyLinesAtTheStartOfBlocks: false
+
+# Penalties
+# This decides what order things should be done if a line is too long
+PenaltyBreakAssignment: 100
+PenaltyBreakBeforeFirstCallParameter: 100
+PenaltyBreakComment: 100
+PenaltyBreakFirstLessLess: 0
+PenaltyBreakString: 100
+PenaltyExcessCharacter: 5
+PenaltyReturnTypeOnItsOwnLine: 0
+
+# Don't sort #include's
+SortIncludes: false
-- 
2.14.1.480.gb18f417b89-goog

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Stefan Beller]

On Mon, Aug 14, 2017 at 2:30 PM, Brandon Williams <bmwill@google.com> wrote:
> Add a '.clang-format' file which outlines the git project's coding
> style.  This can be used with clang-format to auto-format .c and .h
> files to conform with git's style.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

Applying this patch and running
    clang-format -i -style file *.c *.h builtin/*.c
produces a diff, that I'd mostly agree with.
This style guide is close to our current style.

As noted in patch 2/2 we'd now need an easy way to
expose this for use in various situations, such as
* contributor wanting to format their patch
* reformatting code for readability

Thanks,
Stefan

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Ben Peart]

On 8/14/2017 6:02 PM, Stefan Beller wrote:
> On Mon, Aug 14, 2017 at 2:30 PM, Brandon Williams <bmwill@google.com> wrote:
>> Add a '.clang-format' file which outlines the git project's coding
>> style.  This can be used with clang-format to auto-format .c and .h
>> files to conform with git's style.
>>
>> Signed-off-by: Brandon Williams <bmwill@google.com>
> 
> Applying this patch and running
>      clang-format -i -style file *.c *.h builtin/*.c
> produces a diff, that I'd mostly agree with.
> This style guide is close to our current style.
> 

I'm happy to see progress being made in helping reduce the time spent 
manually reviewing and fixing style formatting errors.  In an effort to 
help, I installed this in Windows and tried it as well.  The tools all 
appear to be working fine and are supported on Windows.

For the most part, the formatting rules look pretty consistent with the 
existing style.  I ran the same test and looked at the diffs and saw a 
couple of things that looked odd. For example, how it wrapped the 
"static int" on the function header below was different.  Not sure why 
as it didn't wrap all the other function headers the same even later in 
the file it didn't do that with "static void mute_routine"

diff --git a/apply.c b/apply.c
index f2d599141d..bb77242e3d 100644
--- a/apply.c
+++ b/apply.c
@@ -58,12 +59,11 @@ static int parse_whitespace_option(struct 
apply_state *state, const char *option
         return error(_("unrecognized whitespace option '%s'"), option);
  }

-static int parse_ignorewhitespace_option(struct apply_state *state,
-                                                const char *option)
+static int
+parse_ignorewhitespace_option(struct apply_state *state, const char 
*option)
  {
-       if (!option || !strcmp(option, "no") ||
-           !strcmp(option, "false") || !strcmp(option, "never") ||
-           !strcmp(option, "none")) {
+       if (!option || !strcmp(option, "no") || !strcmp(option, "false") ||
+           !strcmp(option, "never") || !strcmp(option, "none")) {
                 state->ws_ignore_action = ignore_ws_none;
                 return 0;
         }


Later in the file it wraps some of them again: (add_line_info, 
prepare_image, find_name_common, etc).  Again, it appears to be 
inconsistent but there must be some rule that is causing this behavior.



Here is an example of how it wrapped bit fields differently.  Again, it 
didn't seem to be consistent with itself as just below this, it left 
them on separate lines.


@@ -182,8 +185,7 @@ struct fragment {
          * but some codepaths store an allocated buffer.
          */
         const char *patch;
-       unsigned free_patch:1,
-               rejected:1;
+       unsigned free_patch : 1, rejected : 1;
         int size;
         int linenr;
         struct fragment *next;


Big thanks to those working on this!

> As noted in patch 2/2 we'd now need an easy way to
> expose this for use in various situations, such as
> * contributor wanting to format their patch
> * reformatting code for readability
> 
> Thanks,
> Stefan
> 

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Brandon Williams]

On 08/15, Ben Peart wrote:
> 
> 
> On 8/14/2017 6:02 PM, Stefan Beller wrote:
> >On Mon, Aug 14, 2017 at 2:30 PM, Brandon Williams <bmwill@google.com> wrote:
> >>Add a '.clang-format' file which outlines the git project's coding
> >>style.  This can be used with clang-format to auto-format .c and .h
> >>files to conform with git's style.
> >>
> >>Signed-off-by: Brandon Williams <bmwill@google.com>
> >
> >Applying this patch and running
> >     clang-format -i -style file *.c *.h builtin/*.c
> >produces a diff, that I'd mostly agree with.
> >This style guide is close to our current style.
> >
> 
> I'm happy to see progress being made in helping reduce the time
> spent manually reviewing and fixing style formatting errors.  In an
> effort to help, I installed this in Windows and tried it as well.
> The tools all appear to be working fine and are supported on
> Windows.
> 
> For the most part, the formatting rules look pretty consistent with
> the existing style.  I ran the same test and looked at the diffs and
> saw a couple of things that looked odd. For example, how it wrapped
> the "static int" on the function header below was different.  Not
> sure why as it didn't wrap all the other function headers the same
> even later in the file it didn't do that with "static void
> mute_routine"
> 
> diff --git a/apply.c b/apply.c
> index f2d599141d..bb77242e3d 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -58,12 +59,11 @@ static int parse_whitespace_option(struct
> apply_state *state, const char *option
>         return error(_("unrecognized whitespace option '%s'"), option);
>  }
> 
> -static int parse_ignorewhitespace_option(struct apply_state *state,
> -                                                const char *option)
> +static int
> +parse_ignorewhitespace_option(struct apply_state *state, const char
> *option)
>  {
> -       if (!option || !strcmp(option, "no") ||
> -           !strcmp(option, "false") || !strcmp(option, "never") ||
> -           !strcmp(option, "none")) {
> +       if (!option || !strcmp(option, "no") || !strcmp(option, "false") ||
> +           !strcmp(option, "never") || !strcmp(option, "none")) {
>                 state->ws_ignore_action = ignore_ws_none;
>                 return 0;
>         }
> 
> 
> Later in the file it wraps some of them again: (add_line_info,
> prepare_image, find_name_common, etc).  Again, it appears to be
> inconsistent but there must be some rule that is causing this
> behavior.

These have to deal with setting the penalties.  When a line gets to be
too long the tool needs to find a place to break the line based on a
penalty system.  The current .clang-format file I sent out has values
for the penalties which would most likely need to be tweaked through
trial and error.

> 
> 
> 
> Here is an example of how it wrapped bit fields differently.  Again,
> it didn't seem to be consistent with itself as just below this, it
> left them on separate lines.
> 
> 
> @@ -182,8 +185,7 @@ struct fragment {
>          * but some codepaths store an allocated buffer.
>          */
>         const char *patch;
> -       unsigned free_patch:1,
> -               rejected:1;
> +       unsigned free_patch : 1, rejected : 1;
>         int size;
>         int linenr;
>         struct fragment *next;

If the return type was replicated then it would probably format the
different struct members on their own line.

> 
> 
> Big thanks to those working on this!
> 
> >As noted in patch 2/2 we'd now need an easy way to
> >expose this for use in various situations, such as
> >* contributor wanting to format their patch
> >* reformatting code for readability
> >
> >Thanks,
> >Stefan
> >

-- 
Brandon Williams

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Jeff King]

On Mon, Aug 14, 2017 at 02:30:45PM -0700, Brandon Williams wrote:

> +# Align escaped newlines as far left as possible
> +# #define A   \
> +#   int aaaa; \
> +#   int b;    \
> +#   int cccccccc;
> +AlignEscapedNewlines: Left

I get:

  $ git clang-format-5.0 --style file -p --extensions c,h
  YAML:34:23: error: unknown key 'AlignEscapedNewlines'
  AlignEscapedNewlines: Left
                        ^~~~
  Error reading /home/peff/compile/git/.clang-format: Invalid argument

Same with clang-format-3.8.

-Peff

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Jeff King]

On Mon, Aug 14, 2017 at 06:48:31PM -0400, Jeff King wrote:

> On Mon, Aug 14, 2017 at 02:30:45PM -0700, Brandon Williams wrote:
> 
> > +# Align escaped newlines as far left as possible
> > +# #define A   \
> > +#   int aaaa; \
> > +#   int b;    \
> > +#   int cccccccc;
> > +AlignEscapedNewlines: Left
> 
> I get:
> 
>   $ git clang-format-5.0 --style file -p --extensions c,h
>   YAML:34:23: error: unknown key 'AlignEscapedNewlines'
>   AlignEscapedNewlines: Left
>                         ^~~~
>   Error reading /home/peff/compile/git/.clang-format: Invalid argument
> 
> Same with clang-format-3.8.

And if I remove that line, I get:

  YAML:155:25: error: unknown key 'PenaltyBreakAssignment'
  PenaltyBreakAssignment: 100

Removing that gives:

  YAML:86:22: error: unknown key 'BreakStringLiterals'
  BreakStringLiterals: false

And removing that gives me a clean output. I have no idea why my clang
doesn't like these (but presumably yours does). It's clang-format-5.0 in
Debian unstable (and clang-format-3.8, etc).

-Peff

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Brandon Williams]

On 08/14, Jeff King wrote:
> On Mon, Aug 14, 2017 at 06:48:31PM -0400, Jeff King wrote:
> 
> > On Mon, Aug 14, 2017 at 02:30:45PM -0700, Brandon Williams wrote:
> > 
> > > +# Align escaped newlines as far left as possible
> > > +# #define A   \
> > > +#   int aaaa; \
> > > +#   int b;    \
> > > +#   int cccccccc;
> > > +AlignEscapedNewlines: Left
> > 
> > I get:
> > 
> >   $ git clang-format-5.0 --style file -p --extensions c,h
> >   YAML:34:23: error: unknown key 'AlignEscapedNewlines'
> >   AlignEscapedNewlines: Left
> >                         ^~~~
> >   Error reading /home/peff/compile/git/.clang-format: Invalid argument
> > 
> > Same with clang-format-3.8.
> 
> And if I remove that line, I get:
> 
>   YAML:155:25: error: unknown key 'PenaltyBreakAssignment'
>   PenaltyBreakAssignment: 100
> 
> Removing that gives:
> 
>   YAML:86:22: error: unknown key 'BreakStringLiterals'
>   BreakStringLiterals: false
> 
> And removing that gives me a clean output. I have no idea why my clang
> doesn't like these (but presumably yours does). It's clang-format-5.0 in
> Debian unstable (and clang-format-3.8, etc).
> 
> -Peff

Those must be features in version 6 (which is what I seem to have
installed on my machine).

-- 
Brandon Williams

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Jeff King]

On Mon, Aug 14, 2017 at 03:54:30PM -0700, Brandon Williams wrote:

> > And removing that gives me a clean output. I have no idea why my clang
> > doesn't like these (but presumably yours does). It's clang-format-5.0 in
> > Debian unstable (and clang-format-3.8, etc).
> 
> Those must be features in version 6 (which is what I seem to have
> installed on my machine).

OK, that makes sense. The most recent one package for Debian unstable is
5.0. AFAICT 5.0 is actually in release freeze for another week or two,
and 6 is just bleeding-edge that moved on after the release freeze a few
weeks ago.

I'm not sure which version it makes sense to target as a minimum, but
probably not 6 yet. :)

-Peff

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Johannes Schindelin]

Hi Peff,

On Mon, 14 Aug 2017, Jeff King wrote:

> On Mon, Aug 14, 2017 at 03:54:30PM -0700, Brandon Williams wrote:
> 
> > > And removing that gives me a clean output. I have no idea why my clang
> > > doesn't like these (but presumably yours does). It's clang-format-5.0 in
> > > Debian unstable (and clang-format-3.8, etc).
> > 
> > Those must be features in version 6 (which is what I seem to have
> > installed on my machine).
> 
> OK, that makes sense. The most recent one package for Debian unstable is
> 5.0. AFAICT 5.0 is actually in release freeze for another week or two,
> and 6 is just bleeding-edge that moved on after the release freeze a few
> weeks ago.
> 
> I'm not sure which version it makes sense to target as a minimum, but
> probably not 6 yet. :)

I agree.

There is most likely a middle path between too old and too new, and with
the current pace of the review 5.0 will probably be good enough by the
time this patch series can possibly hit `master`. So I'd guess 5.0 would
be a good version to aim for.

Besides, it may not matter *all* that much which version we target: As I
mentioned elsewhere, the contributor experience would most likely be
vastly improved if this was a bot, say, monitoring GitHub Pull Requests.
It could use filter-branch to apply clang-format to each commit's diff and
report back to the Pull Request either by saying that the style is okay,
or by linking to another repository where the fixed commits live (combined
with instructions how to update the local branch).

Ciao,
Dscho

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Ben Peart]

On 8/14/2017 5:30 PM, Brandon Williams wrote:
> Add a '.clang-format' file which outlines the git project's coding
> style.  This can be used with clang-format to auto-format .c and .h
> files to conform with git's style.
> 
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>   .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 165 insertions(+)
>   create mode 100644 .clang-format
> 

I applied this and then ran:
     clang-format -i -style file *.c *.h builtin/*.c

The modified files generate lots of line ending warnings.  Apparently 
clang-format isn't respecting the git settings for line endings as it 
converted CRLF to LF in all the files it edited.

$ git add .
warning: LF will be replaced by CRLF in alloc.c.
The file will have its original line endings in your working directory.
warning: LF will be replaced by CRLF in base85.c.
The file will have its original line endings in your working directory.
warning: LF will be replaced by CRLF in bisect.h.
[...]

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Brandon Williams]

On 08/15, Ben Peart wrote:
> 
> 
> On 8/14/2017 5:30 PM, Brandon Williams wrote:
> >Add a '.clang-format' file which outlines the git project's coding
> >style.  This can be used with clang-format to auto-format .c and .h
> >files to conform with git's style.
> >
> >Signed-off-by: Brandon Williams <bmwill@google.com>
> >---
> >  .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 165 insertions(+)
> >  create mode 100644 .clang-format
> >
> 
> I applied this and then ran:
>     clang-format -i -style file *.c *.h builtin/*.c
> 
> The modified files generate lots of line ending warnings.
> Apparently clang-format isn't respecting the git settings for line
> endings as it converted CRLF to LF in all the files it edited.
> 
> $ git add .
> warning: LF will be replaced by CRLF in alloc.c.
> The file will have its original line endings in your working directory.
> warning: LF will be replaced by CRLF in base85.c.
> The file will have its original line endings in your working directory.
> warning: LF will be replaced by CRLF in bisect.h.
> [...]
> 

I would think that there  must be some setting to permit CRLF on windows
machines.  As I work on a unix machine I don't see this sort of
behavior.

-- 
Brandon Williams

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

[Stefan Beller]

On Tue, Aug 15, 2017 at 10:34 AM, Brandon Williams <bmwill@google.com> wrote:
> On 08/15, Ben Peart wrote:
>>
>>
>> On 8/14/2017 5:30 PM, Brandon Williams wrote:
>> >Add a '.clang-format' file which outlines the git project's coding
>> >style.  This can be used with clang-format to auto-format .c and .h
>> >files to conform with git's style.
>> >
>> >Signed-off-by: Brandon Williams <bmwill@google.com>
>> >---
>> >  .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 165 insertions(+)
>> >  create mode 100644 .clang-format
>> >
>>
>> I applied this and then ran:
>>     clang-format -i -style file *.c *.h builtin/*.c
>>
>> The modified files generate lots of line ending warnings.
>> Apparently clang-format isn't respecting the git settings for line
>> endings as it converted CRLF to LF in all the files it edited.
>>
>> $ git add .
>> warning: LF will be replaced by CRLF in alloc.c.
>> The file will have its original line endings in your working directory.
>> warning: LF will be replaced by CRLF in base85.c.
>> The file will have its original line endings in your working directory.
>> warning: LF will be replaced by CRLF in bisect.h.
>> [...]
>>

This sounds as if core.safecrlf is set to "warn" (by default?),
you could set it to false to not have these warnings. However
that is side stepping the problem of clang-format producing
non-crlf line endings.

Maybe the workflow can be setup such that this side stepping
is ok, and no harm is done by the LF line endings by the formatting
(If the formatting was a pre-commit hook, then Git would convert
LF/CRLF and there would be no impact to your workflow I'd imagine).

[PATCH v2 2/2] Makefile: add style build rule

[Brandon Williams]

Add the 'style' build rule which will run git-clang-format on the diff
between HEAD and the current worktree.  The result is a diff of
suggested changes.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Makefile b/Makefile
index ba4359ef8..acfd096b7 100644
--- a/Makefile
+++ b/Makefile
@@ -2414,6 +2414,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
+.PHONY: style
+style:
+	git clang-format --style file --diff --extensions c,h
+
 check: common-cmds.h
 	@if sparse; \
 	then \
-- 
2.14.1.480.gb18f417b89-goog

Re: [PATCH v2 2/2] Makefile: add style build rule

[Stefan Beller]

On Mon, Aug 14, 2017 at 2:30 PM, Brandon Williams <bmwill@google.com> wrote:
> Add the 'style' build rule which will run git-clang-format on the diff
> between HEAD and the current worktree.  The result is a diff of
> suggested changes.

Notes from in-office discussion:

* 'git clang-format --style file -f --extensions c,h'
   to apply suggested changes. (Useful for contributors,
   maybe even as a precommit hook)
* you can also give a range of commits to git clang-format, as
   git clang-format --diff origin/master..HEAD to see if other
   local commits need tweaking.


>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  Makefile | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index ba4359ef8..acfd096b7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2414,6 +2414,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)
>
> +.PHONY: style
> +style:
> +       git clang-format --style file --diff --extensions c,h
> +
>  check: common-cmds.h
>         @if sparse; \
>         then \
> --
> 2.14.1.480.gb18f417b89-goog
>

Re: [PATCH v2 2/2] Makefile: add style build rule

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Add the 'style' build rule which will run git-clang-format on the diff
> between HEAD and the current worktree.  The result is a diff of
> suggested changes.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  Makefile | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index ba4359ef8..acfd096b7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2414,6 +2414,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)
>  
> +.PHONY: style
> +style:
> +	git clang-format --style file --diff --extensions c,h

Did we get "git clang-format" subcommand, or is "s/git //" implied
somewhere?

> +
>  check: common-cmds.h
>  	@if sparse; \
>  	then \

Re: [PATCH v2 2/2] Makefile: add style build rule

[Stefan Beller]

On Mon, Aug 14, 2017 at 3:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> Add the 'style' build rule which will run git-clang-format on the diff
>> between HEAD and the current worktree.  The result is a diff of
>> suggested changes.
>>
>> Signed-off-by: Brandon Williams <bmwill@google.com>
>> ---
>>  Makefile | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index ba4359ef8..acfd096b7 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2414,6 +2414,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>>  .PHONY: sparse $(SP_OBJ)
>>  sparse: $(SP_OBJ)
>>
>> +.PHONY: style
>> +style:
>> +     git clang-format --style file --diff --extensions c,h
>
> Did we get "git clang-format" subcommand, or is "s/git //" implied
> somewhere?

You need to have clang-format installed (debian/Ubuntu package, or
however it is named in your distribution), which provides this command
for us.

Re: [PATCH v2 2/2] Makefile: add style build rule

[Jeff King]

On Mon, Aug 14, 2017 at 03:28:50PM -0700, Stefan Beller wrote:

> >> +.PHONY: style
> >> +style:
> >> +     git clang-format --style file --diff --extensions c,h
> >
> > Did we get "git clang-format" subcommand, or is "s/git //" implied
> > somewhere?
> 
> You need to have clang-format installed (debian/Ubuntu package, or
> however it is named in your distribution), which provides this command
> for us.

Sadly it is called git-clang-format-3.8, git-clang-format-5.0, etc, in
the Debian packages. I think we need a CLANG_FORMAT variable that can be
overridden.

I am surprised that there's no base "git-clang-format" symlink for the
default version. There is for "clang-format" and "clang" themselves. So
arguably this is a bug in the Debian packaging.

I suspect the "-p" version is going to be the one people invoke the most
often. Should it take the coveted "make style" slot, and the diff get
pushed off to another target?

I was also confused at first that the "-p" version requires you to stage
the changes first. I don't know if we can make that less confusing via a
"make style". Or if it's just something people would get used to. But
sadly it makes the command not-quite orthogonal to "make test" in the
workflow. You can't "make style && make test && git add -p".  You have
to add first, then check style, then you'd want to test that result to
make sure it didn't change the meaning of the code.

-Peff

Re: [PATCH v2 2/2] Makefile: add style build rule

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I suspect the "-p" version is going to be the one people invoke the most
> often. Should it take the coveted "make style" slot, and the diff get
> pushed off to another target?
>
> I was also confused at first that the "-p" version requires you to stage
> the changes first. I don't know if we can make that less confusing via a
> "make style". Or if it's just something people would get used to. But
> sadly it makes the command not-quite orthogonal to "make test" in the
> workflow. You can't "make style && make test && git add -p".  You have
> to add first, then check style, then you'd want to test that result to
> make sure it didn't change the meaning of the code.

Perhaps.

By the way, I do not know which vintage of /usr/bin/git-clang-format
I happen to have on my box, but I needed a crude workaround patch
(attached at the end) to get it even run.  The first thing it does
is to call load_git_config() and it barfs because I have boolean
configuration variables set to true in the correct way, which it
does not seem to recognise.

As to what it does, the first example I tried may not have been a
great one.  I got this:

        git clang-format --style file --diff --extensions c,h
        diff --git a/cache.h b/cache.h
        index 73e0085186..6462fe25bc 100644
        --- a/cache.h
        +++ b/cache.h
        @@ -1498,11 +1498,8 @@ struct checkout {
                const char *base_dir;
                int base_dir_len;
                struct delayed_checkout *delayed_checkout;
        -	unsigned force:1,
        -		 quiet:1,
        -		 not_new:1,
        -		a_new_field:1,
        -		 refresh_cache:1;
        +	unsigned force : 1, quiet : 1, not_new : 1, a_new_field : 1,
        +		refresh_cache : 1;
         };
         #define CHECKOUT_INIT { NULL, "" }
 
which is not wrong per-se, but I have a mixed feelings.  I do not
want it to complain if the original tried to fit many items on a
single line, but if the original wanted to have one item per line,
I'd rather see it kept as-is.

Anyway, we cannot have perfect checker from the day one, and
considering this is an initial attempt, I'd say it is a good start.

Thanks.

diff --git a/git-clang-format b/git-clang-format
index 60cd4fb25b..e8429b2750 100755
--- a/usr/bin/git-clang-format
+++ b/usr/local/google/home/jch/g/Ubuntu-14.04-x86_64/gitstuff/bin/git-clang-format
@@ -191,10 +191,13 @@ def load_git_config(non_string_options=None):
   out = {}
   for entry in run('git', 'config', '--list', '--null').split('\0'):
     if entry:
-      name, value = entry.split('\n', 1)
-      if name in non_string_options:
-        value = run('git', 'config', non_string_options[name], name)
-      out[name] = value
+      if '\n' in entry:
+        name, value = entry.split('\n', 1)
+        if name in non_string_options:
+          value = run('git', 'config', non_string_options[name], name)
+        out[name] = value
+      else:
+	out[entry] = "true";
   return out
 
 

Re: [PATCH v2 2/2] Makefile: add style build rule

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> By the way, I do not know which vintage of /usr/bin/git-clang-format
> I happen to have on my box, but I needed a crude workaround patch
> (attached at the end) ...

I guess you hit the same thing while our messages crossing ;-)


> As to what it does, the first example I tried may not have been a
> great one.  I got this:
>
>         git clang-format --style file --diff --extensions c,h
>         diff --git a/cache.h b/cache.h
>         index 73e0085186..6462fe25bc 100644
>         --- a/cache.h
>         +++ b/cache.h
>         @@ -1498,11 +1498,8 @@ struct checkout {
>                 const char *base_dir;
>                 int base_dir_len;
>                 struct delayed_checkout *delayed_checkout;
>         -	unsigned force:1,
>         -		 quiet:1,
>         -		 not_new:1,
>         -		a_new_field:1,
>         -		 refresh_cache:1;
>         +	unsigned force : 1, quiet : 1, not_new : 1, a_new_field : 1,
>         +		refresh_cache : 1;
>          };
>          #define CHECKOUT_INIT { NULL, "" }
>  
> which is not wrong per-se, but I have a mixed feelings.  I do not
> want it to complain if the original tried to fit many items on a
> single line, but if the original wanted to have one item per line,
> I'd rather see it kept as-is.

To clarify, the above is after I added a_new_field that is one-bit
wide without doing anything else.  I do not mind the checker
complaining the existing force, quiet, etc. whose widths are all
spelled without SP around ':', because they appear near-by, as a
collateral damage.  My only gripe is that the result got squished
into a single line.

> Anyway, we cannot have perfect checker from the day one, and
> considering this is an initial attempt, I'd say it is a good start.

Re: [PATCH v2 2/2] Makefile: add style build rule

[Brandon Williams]

On 08/14, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > By the way, I do not know which vintage of /usr/bin/git-clang-format
> > I happen to have on my box, but I needed a crude workaround patch
> > (attached at the end) ...
> 
> I guess you hit the same thing while our messages crossing ;-)
> 
> 
> > As to what it does, the first example I tried may not have been a
> > great one.  I got this:
> >
> >         git clang-format --style file --diff --extensions c,h
> >         diff --git a/cache.h b/cache.h
> >         index 73e0085186..6462fe25bc 100644
> >         --- a/cache.h
> >         +++ b/cache.h
> >         @@ -1498,11 +1498,8 @@ struct checkout {
> >                 const char *base_dir;
> >                 int base_dir_len;
> >                 struct delayed_checkout *delayed_checkout;
> >         -	unsigned force:1,
> >         -		 quiet:1,
> >         -		 not_new:1,
> >         -		a_new_field:1,
> >         -		 refresh_cache:1;
> >         +	unsigned force : 1, quiet : 1, not_new : 1, a_new_field : 1,
> >         +		refresh_cache : 1;
> >          };
> >          #define CHECKOUT_INIT { NULL, "" }
> >  
> > which is not wrong per-se, but I have a mixed feelings.  I do not
> > want it to complain if the original tried to fit many items on a
> > single line, but if the original wanted to have one item per line,
> > I'd rather see it kept as-is.
> 
> To clarify, the above is after I added a_new_field that is one-bit
> wide without doing anything else.  I do not mind the checker
> complaining the existing force, quiet, etc. whose widths are all
> spelled without SP around ':', because they appear near-by, as a
> collateral damage.  My only gripe is that the result got squished
> into a single line.

Yeah the result doesn't look too good and I'm not sure which option to
tweak for that.  I'm sure that the problem would fix itself if all the
bit fields where defined on their own lines:

  unsigned force : 1;
  unsigned not_new : 1; 
  ... etc ...

I'm sure there are a bunch of other things that we'd need to tweak
before this is ready to be used by all contributors.  Specifically the
penalties to help determine when to break a line.

> 
> > Anyway, we cannot have perfect checker from the day one, and
> > considering this is an initial attempt, I'd say it is a good start.

-- 
Brandon Williams

Re: [PATCH v2 2/2] Makefile: add style build rule

[Jeff King]

On Mon, Aug 14, 2017 at 04:47:45PM -0700, Junio C Hamano wrote:

> > By the way, I do not know which vintage of /usr/bin/git-clang-format
> > I happen to have on my box, but I needed a crude workaround patch
> > (attached at the end) ...
> 
> I guess you hit the same thing while our messages crossing ;-)

Yep. Our solutions were at opposite ends of the spectrum, though. :)

> > As to what it does, the first example I tried may not have been a
> > great one.  I got this:
> >
> >         git clang-format --style file --diff --extensions c,h
> >         diff --git a/cache.h b/cache.h
> >         index 73e0085186..6462fe25bc 100644
> >         --- a/cache.h
> >         +++ b/cache.h
> >         @@ -1498,11 +1498,8 @@ struct checkout {
> >                 const char *base_dir;
> >                 int base_dir_len;
> >                 struct delayed_checkout *delayed_checkout;
> >         -	unsigned force:1,
> >         -		 quiet:1,
> >         -		 not_new:1,
> >         -		a_new_field:1,
> >         -		 refresh_cache:1;
> >         +	unsigned force : 1, quiet : 1, not_new : 1, a_new_field : 1,
> >         +		refresh_cache : 1;
> >          };
> >          #define CHECKOUT_INIT { NULL, "" }
> >  
> > which is not wrong per-se, but I have a mixed feelings.  I do not
> > want it to complain if the original tried to fit many items on a
> > single line, but if the original wanted to have one item per line,
> > I'd rather see it kept as-is.
> 
> To clarify, the above is after I added a_new_field that is one-bit
> wide without doing anything else.  I do not mind the checker
> complaining the existing force, quiet, etc. whose widths are all
> spelled without SP around ':', because they appear near-by, as a
> collateral damage.  My only gripe is that the result got squished
> into a single line.

Yes, agreed. My personal rule with a list like this is often "once you
have to start breaking across multiple lines, you should put one per
line". I don't know if there's a way to codify that in clang-format,
though.

The case I fed it (which is just nonsense I made up that does not fit
our style) also left me a bit confused at first, but I think it was
because the .clang-format parser was bailing as soon as it found an
unrecognized entry, but then formatting according to bogus rules. With
the original file from Brandon I got:

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 7e8371670b..8994450e0c 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -21,10 +21,7 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static
-int foo(void* bar,int baz) {
-	/* nothing */
-}
+static int foo(void *bar, int baz) { /* nothing */ }
 
 int cmd_merge_file(int argc, const char **argv, const char *prefix)
 {

which is clearly not our style. And then after removing the entries I
mentioned elsewhere, I get:

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 7e8371670b..574ba6d86f 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -21,8 +21,8 @@ static int label_cb(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static
-int foo(void* bar,int baz) {
+static int foo(void *bar, int baz)
+{
 	/* nothing */
 }

which looks right. So you might want to double-check that it was
respecting our settings, and there were no warnings to stderr.

-Peff

Re: [PATCH v2 0/2] clang-format

[Brandon Williams]

On 08/14, Brandon Williams wrote:
> Changes in v2:
>  * Changed a couple rules to be more inline with our coding style.
>  * Added a Makefile build rule to run git-clang-format on the diff of the
>    working tree to suggest style changes.
> 
> I found that the llvm project also has the git-clang-format tool which will
> allow for doing formating changes based on diffs so that the parts of the code
> you didn't touch won't be formated.  It also has a nice '-p' option to only
> apply hunks of suggested changes.  I also saw what Ramsay did and attempted to
> add a Makefile rule to run clang-format.  This bit may need more tweaking to
> get it right.

Forgot to include the [RFC] bit because from the discussion it seems
like we still have a lot to talk about before we decide that this is the
path we are taking.

> 
> Brandon Williams (2):
>   clang-format: outline the git project's coding style
>   Makefile: add style build rule
> 
>  .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  Makefile      |   4 ++
>  2 files changed, 169 insertions(+)
>  create mode 100644 .clang-format
> 
> -- 
> 2.14.1.480.gb18f417b89-goog
> 

-- 
Brandon Williams

Re: [PATCH v2 0/2] clang-format

[Jeff King]

On Mon, Aug 14, 2017 at 02:30:44PM -0700, Brandon Williams wrote:

> Changes in v2:
>  * Changed a couple rules to be more inline with our coding style.
>  * Added a Makefile build rule to run git-clang-format on the diff of the
>    working tree to suggest style changes.
> 
> I found that the llvm project also has the git-clang-format tool which will
> allow for doing formating changes based on diffs so that the parts of the code
> you didn't touch won't be formated.  It also has a nice '-p' option to only
> apply hunks of suggested changes.  I also saw what Ramsay did and attempted to
> add a Makefile rule to run clang-format.  This bit may need more tweaking to
> get it right.

One more oddity I found while playing with this that Git folks might run
into:

  $ git init tmp && cd tmp
  $ git commit --allow-empty -m foo
  $ echo "[mysection]mykey" >>.git/config
  $ git clang-format-5.0
  Traceback (most recent call last):
    File "/usr/bin/git-clang-format-5.0", line 579, in <module>
      main()
    File "/usr/bin/git-clang-format-5.0", line 62, in main
      config = load_git_config()
    File "/usr/bin/git-clang-format-5.0", line 194, in load_git_config
      name, value = entry.split('\n', 1)
  ValueError: need more than 1 value to unpack

  $ sed -i 's/mykey/&=true/' .git/config
  $ git clang-format-5.0
  no modified files to format

So it looks like they do their own config parsing and it's not quite
compatible. :(

That's not the end of the world, and something we can try to fix
upstream. I just wanted to mention it here so other people don't waste
time trying to track down the problem.

-Peff

Re: [PATCH v2 0/2] clang-format

[Stefan Beller]

+ llvm-dev@lists.llvm.org

The Git community is currently discussing adopting a coding style
defined by clang-format, here is a bug report:

On Mon, Aug 14, 2017 at 4:06 PM, Jeff King <peff@peff.net> wrote:
>
> One more oddity I found while playing with this that Git folks might run
> into:
>
>   $ git init tmp && cd tmp
>   $ git commit --allow-empty -m foo
>   $ echo "[mysection]mykey" >>.git/config
>   $ git clang-format-5.0
>   Traceback (most recent call last):
>     File "/usr/bin/git-clang-format-5.0", line 579, in <module>
>       main()
>     File "/usr/bin/git-clang-format-5.0", line 62, in main
>       config = load_git_config()
>     File "/usr/bin/git-clang-format-5.0", line 194, in load_git_config
>       name, value = entry.split('\n', 1)
>   ValueError: need more than 1 value to unpack
>
>   $ sed -i 's/mykey/&=true/' .git/config
>   $ git clang-format-5.0
>   no modified files to format
>
> So it looks like they do their own config parsing and it's not quite
> compatible. :(
>
> That's not the end of the world, and something we can try to fix
> upstream. I just wanted to mention it here so other people don't waste
> time trying to track down the problem.
>
> -Peff

Re: [PATCH v2 0/2] clang-format

[Jeff King]

On Mon, Aug 14, 2017 at 04:15:40PM -0700, Stefan Beller wrote:

> + llvm-dev@lists.llvm.org
> 
> The Git community is currently discussing adopting a coding style
> defined by clang-format, here is a bug report:

Since we've added a cc, let me try to give a little more context.

> > One more oddity I found while playing with this that Git folks might run
> > into:
> >
> >   $ git init tmp && cd tmp
> >   $ git commit --allow-empty -m foo
> >   $ echo "[mysection]mykey" >>.git/config
> >   $ git clang-format-5.0
> >   Traceback (most recent call last):
> >     File "/usr/bin/git-clang-format-5.0", line 579, in <module>
> >       main()
> >     File "/usr/bin/git-clang-format-5.0", line 62, in main
> >       config = load_git_config()
> >     File "/usr/bin/git-clang-format-5.0", line 194, in load_git_config
> >       name, value = entry.split('\n', 1)
> >   ValueError: need more than 1 value to unpack
> >
> >   $ sed -i 's/mykey/&=true/' .git/config
> >   $ git clang-format-5.0
> >   no modified files to format
> >
> > So it looks like they do their own config parsing and it's not quite
> > compatible. :(

In Git's config files, doing this:

  [mysection]
  mykey

is a shorthand for setting mysection.mkykey to "true". And the output
from "git config --list" will show just the keyname without a value,
like:

  mysection.mykey

instead of:

  some.key=this one has a value

There's a possible patch elsewhere in the thread:

  https://public-inbox.org/git/xmqqzib1sp6z.fsf@gitster.mtv.corp.google.com/

I'm happy to see it is running "git config --list", which means it's
responding to syntactic funny-ness in the output of that command, not in
the original config (and other features like includes should Just Work
without the script caring).

I'm tempted to say that "config --list" should normalize this case into:

  mysection.mykey=true

Normally we avoid coercing values without knowing the context in which
they'll be used. But the syntax in the original file means the user is
telling us it's a boolean and they expect it to be treated that way.

The only downside is if the user is wrong, it might be coerced into
the string "true" instead of throwing an error. That seems like a minor
drawback for eliminating a potentially confusing corner case from the
plumbing output.

-Peff

Re: [PATCH v2 0/2] clang-format

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 14, 2017 at 04:15:40PM -0700, Stefan Beller wrote:
>
>> + llvm-dev@lists.llvm.org
>> 
>> The Git community is currently discussing adopting a coding style
>> defined by clang-format, here is a bug report:
>
> Since we've added a cc, let me try to give a little more context.

Thanks for relaying; I've dropped them from CC: for this message, as
the remainder is not of interest to them.

> I'm tempted to say that "config --list" should normalize this case into:
>
>   mysection.mykey=true
>
> Normally we avoid coercing values without knowing the context in which
> they'll be used. But the syntax in the original file means the user is
> telling us it's a boolean and they expect it to be treated that way.
>
> The only downside is if the user is wrong, it might be coerced into
> the string "true" instead of throwing an error. That seems like a minor
> drawback for eliminating a potentially confusing corner case from the
> plumbing output.

Because we cannot sensibly always normalize a variable set to 'yes',
'on', etc. to all "true", the degree it can help the reading scripts
is quite limited, as they need to be prepared to see other
representation of the truth values anyway.  Even though I too found
the approach somewhat tempting, because there is no ambiguity in
"[section] var" that it means a boolean "true", I doubt it would
help very much.

The way they pass "non_string_options" dict to the loader is quite
sensible for that purpose, as it allows the reader to say "I care
about this and that variables, and I know I want them interpreted as
int (e.g. 1M) and bool (e.g. 'on') and returned in a normalized
form".

I do not mind adding "git config --list --autotype" option, though,
with which the reading script tells us that it accepts the chance of
false conversion, so that

	[my]
		intVal = 2k
		boolVal
                someVal = on
		otherVal = 1
		moreVal = 2
		anotherVal = 0
		no = no

might be listed as

	my.intval=2048
	my.boolval=true
	my.someval=true
	my.otherval=1
	my.moreval=2
	my.anotherval=0
	my.no=false

Disambiguation rules are up to debate; the above illustrates an
extreme position that coerses anything that could be taken as an int
and a string that can be taken as a bool as such, but it may not
help very much if the reader wants to see boolean values.

Re: [PATCH v2 0/2] clang-format

[Jeff King]

On Mon, Aug 14, 2017 at 08:03:19PM -0700, Junio C Hamano wrote:

> > I'm tempted to say that "config --list" should normalize this case into:
> >
> >   mysection.mykey=true
> >
> > Normally we avoid coercing values without knowing the context in which
> > they'll be used. But the syntax in the original file means the user is
> > telling us it's a boolean and they expect it to be treated that way.
> >
> > The only downside is if the user is wrong, it might be coerced into
> > the string "true" instead of throwing an error. That seems like a minor
> > drawback for eliminating a potentially confusing corner case from the
> > plumbing output.
> 
> Because we cannot sensibly always normalize a variable set to 'yes',
> 'on', etc. to all "true", the degree it can help the reading scripts
> is quite limited, as they need to be prepared to see other
> representation of the truth values anyway.  Even though I too found
> the approach somewhat tempting, because there is no ambiguity in
> "[section] var" that it means a boolean "true", I doubt it would
> help very much.

Good point. This is the only case that is _syntactically_ a problem at
the key/value level, which is why we noticed it (the reader barfed on
the unknown input). But that same reader could be interpreting values
incorrectly and we'd have no idea. And that applies to types beyond
booleans (you showed numbers like "2k" below, but there are others, like
--path or --get-color).

The one nice thing about fixing this syntactic issue is that the current
behavior affected this reader even though it didn't care about
particular config key in question. I.e., in this output:

  my.boolVal
  my.intVal=2k
  my.valueWeCareAbout=some string

if we don't care about the meaning of boolVal or intVal, we could still
parse this output fine if not for the syntactic irregularity of
my.boolVal.

That's what might tempt me to fix this independent of the deeper
problem. But I really think we should try to address that deeper
problem.

> The way they pass "non_string_options" dict to the loader is quite
> sensible for that purpose, as it allows the reader to say "I care
> about this and that variables, and I know I want them interpreted as
> int (e.g. 1M) and bool (e.g. 'on') and returned in a normalized
> form".
> 
> I do not mind adding "git config --list --autotype" option, though,
> with which the reading script tells us that it accepts the chance of
> false conversion, so that
> [...]

I think an "--autotype" is always going to have false positives.
Integers and booleans we can make guesses at. But "--path" or "--color"
are much tougher.

The right answer is to make it easier for that non_string_options
information to make it to git-config so it can do the interpretation for
the caller. The way that happens now is:

  git config --int my.intVal
  git config --bool my.boolVal
  git config --path my.pathVal

and so on. But I think one reason people turn to --list is that it
requires only a single process invocation, rather than repeatedly
calling git-config for each variable which might be of interest.  I know
that diff-highlight's startup is measurably slower due to the six "git
config --get-color" calls it must make, and I've been looking for a way
to do it with a single invocation.

I suspect we need a "--get-stdin" which can accept (key,type) tuples and
return the result over stdout. And in some cases it's more than just a
pair; for example, colors need an extra "parse this default" argument).

-Peff