{"thread":{"id":"46536","subject":"[RFC] clang-format: outline the git project's coding style","startedAt":"2017-08-08T01:26:15Z","lastAt":"2017-09-28T17:17:06Z","messageCount":64,"participants":["Brandon Williams","Johannes Schindelin","Stefan Beller","Junio C Hamano","Jeff King","Ævar Arnfjörð Bjarmason","Ben Peart","Ramsay Jones","brian m. carlson","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"325777","messageId":"20170808012554.186051-1-bmwill@google.com","threadId":"46536","inReplyTo":null,"subject":"[RFC] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-08T01:25:54Z","receivedAt":"2017-08-08T01:26:15Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Add a '.clang-format' file which outlines the git project's coding\nstyle. This can be used with clang-format to auto-format .c and .h\nfiles to conform with git's style.\n\nSigned-off-by: Brandon Williams \n---\n\nI'm sure this sort of thing comes up every so often on the list but back at\ngit-merge I mentioned how it would be nice to not have to worry about style\nwhen reviewing patches as that is something mechanical and best left to a\nmachine (for the most part). I saw that 'clang-format' was brought up on the\nlist once before a couple years ago\n(https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing\nreally came of it. I spent a little bit of time combing through the various\noptions and came up with this config based on the general style of our code\nbase. The big issue though is that our code base isn't consistent so try as\nyou might you wont be able to come up with a config which matches everything we\ndo (mostly due to the inconsistencies in our code base).\n\nAnyway, I thought I'd bring this topic back up and see how people feel about it.\n\n .clang-format | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 166 insertions(+)\n create mode 100644 .clang-format\n\ndiff --git a/.clang-format b/.clang-format\nnew file mode 100644\nindex 000000000..7f28dc259\n--- /dev/null\n+++ b/.clang-format\n@@ -0,0 +1,166 @@\n+# Defaults\n+\n+# Use tabs whenever we need to fill whitespace that spans at least from one tab\n+# stop to the next one.\n+UseTab: Always\n+TabWidth: 8\n+IndentWidth: 8\n+ContinuationIndentWidth: 8\n+ColumnLimit: 80\n+\n+# C Language specifics\n+Language: Cpp\n+\n+# Align parameters on the open bracket\n+# someLongFunction(argument1,\n+# argument2);\n+AlignAfterOpenBracket: Align\n+\n+# Don't align consecutive assignments\n+# int aaaa = 12;\n+# int b = 14;\n+AlignConsecutiveAssignments: false\n+\n+# Don't align consecutive declarations\n+# int aaaa = 12;\n+# double b = 3.14;\n+AlignConsecutiveDeclarations: false\n+\n+# Align escaped newlines as far left as possible\n+# #define A \\\n+# int aaaa; \\\n+# int b; \\\n+# int cccccccc;\n+AlignEscapedNewlines: Left\n+\n+# Align operands of binary and ternary expressions\n+# int aaa = bbbbbbbbbbb +\n+# cccccc;\n+AlignOperands: true\n+\n+# Don't align trailing comments\n+# int a; // Comment a\n+# int b = 2; // Comment b\n+AlignTrailingComments: false\n+\n+# By default don't allow putting parameters onto the next line\n+# myFunction(foo, bar, baz);\n+AllowAllParametersOfDeclarationOnNextLine: false\n+\n+# Don't allow short braced statements to be on a single line\n+# if (a) not if (a) return;\n+# return;\n+AllowShortBlocksOnASingleLine: false\n+AllowShortCaseLabelsOnASingleLine: false\n+AllowShortFunctionsOnASingleLine: false\n+AllowShortIfStatementsOnASingleLine: false\n+AllowShortLoopsOnASingleLine: false\n+\n+# Add a line break after the return type of top-level functions\n+# int\n+# foo();\n+AlwaysBreakAfterReturnType: TopLevel\n+\n+# Pack as many parameters or arguments onto the same line as possible\n+# int myFunction(int aaaaaaaaaaaa, int bbbbbbbb,\n+# int cccc);\n+BinPackArguments: true\n+BinPackParameters: true\n+\n+# Attach braces to surrounding context except break before braces on function\n+# definitions.\n+# void foo()\n+# {\n+# if (true) {\n+# } else {\n+# }\n+# };\n+BreakBeforeBraces: Linux\n+\n+# Break after operators\n+# int valuve = aaaaaaaaaaaaa +\n+# bbbbbb -\n+# ccccccccccc;\n+BreakBeforeBinaryOperators: None\n+BreakBeforeTernaryOperators: false\n+\n+# Don't break string literals\n+BreakStringLiterals: false\n+\n+# Use the same indentation level as for the switch statement.\n+# Switch statement body is always indented one level more than case labels.\n+IndentCaseLabels: false\n+\n+# Don't indent a function definition or declaration if it is wrapped after the\n+# type\n+IndentWrappedFunctionNames: false\n+\n+# Align pointer to the right\n+# int *a;\n+PointerAlignment: Right\n+\n+# Insert a space after a cast\n+# x = (int32) y; not x = (int32)y;\n+SpaceAfterCStyleCast: true\n+\n+# Insert spaces before and after assignment operators\n+# int a = 5; not int a=5;\n+# a += 42; a+=42;\n+SpaceBeforeAssignmentOperators: true\n+\n+# Put a space before opening parentheses only after control statement keywords.\n+# void f() {\n+# if (true) {\n+# f();\n+# }\n+# }\n+SpaceBeforeParens: ControlStatements\n+\n+# Don't insert spaces inside empty '()'\n+SpaceInEmptyParentheses: false\n+\n+# The number of spaces before trailing line comments (// - comments).\n+# This does not affect trailing block comments (/* - comments).\n+SpacesBeforeTrailingComments: 1\n+\n+# Don't insert spaces in casts\n+# x = (int32) y; not x = ( int32 ) y;\n+SpacesInCStyleCastParentheses: false\n+\n+# Don't insert spaces inside container literals\n+# var arr = [1, 2, 3]; not var arr = [ 1, 2, 3 ];\n+SpacesInContainerLiterals: false\n+\n+# Don't insert spaces after '(' or before ')'\n+# f(arg); not f( arg );\n+SpacesInParentheses: false\n+\n+# Don't insert spaces after '[' or before ']'\n+# int a[5]; not int a[ 5 ];\n+SpacesInSquareBrackets: false\n+\n+# Insert a space after '{' and before '}' in struct initializers\n+Cpp11BracedListStyle: false\n+\n+# A list of macros that should be interpreted as foreach loops instead of as\n+# function calls.\n+ForEachMacros: ['for_each_string_list_item']\n+\n+# The maximum number of consecutive empty lines to keep.\n+MaxEmptyLinesToKeep: 1\n+\n+# No empty line at the start of a block.\n+KeepEmptyLinesAtTheStartOfBlocks: false\n+\n+# Penalties\n+# This decides what order things should be done if a line is too long\n+PenaltyBreakAssignment: 100\n+PenaltyBreakBeforeFirstCallParameter: 100\n+PenaltyBreakComment: 100\n+PenaltyBreakFirstLessLess: 0\n+PenaltyBreakString: 100\n+PenaltyExcessCharacter: 5\n+PenaltyReturnTypeOnItsOwnLine: 0\n+\n+# Don't sort #include's\n+SortIncludes: false\n-- \n2.14.0.434.g98096fd7a8-goog\n\n"},{"id":"325803","messageId":"alpine.DEB.2.21.1.1708081404200.4271@virtualbox","threadId":"46536","inReplyTo":"20170808012554.186051-1-bmwill@google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-08-08T12:05:13Z","receivedAt":"2017-08-08T12:11:39Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Mon, 7 Aug 2017, Brandon Williams wrote:\n\n> Add a '.clang-format' file which outlines the git project's coding\n> style. This can be used with clang-format to auto-format .c and .h\n> files to conform with git's style.\n> \n> Signed-off-by: Brandon Williams \n> ---\n> \n> I'm sure this sort of thing comes up every so often on the list but back at\n> git-merge I mentioned how it would be nice to not have to worry about style\n> when reviewing patches as that is something mechanical and best left to a\n> machine (for the most part).\n\nAmen.\n\nIf I never have to see a review mentioning an unwrapped line, I am quite\ncertain I will be quite content.\n\nCiao,\nDscho\n"},{"id":"325822","messageId":"CAGZ79kb2860nUs46bP=x-gAx9Ao6DxnvXQv3x387Wxw+kEyxJw@mail.gmail.com","threadId":"46536","inReplyTo":"alpine.DEB.2.21.1.1708081404200.4271@virtualbox","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-08T17:40:24Z","receivedAt":"2017-08-08T17:40:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin\n wrote:\n> Hi Brandon,\n>\n> On Mon, 7 Aug 2017, Brandon Williams wrote:\n>\n>> Add a '.clang-format' file which outlines the git project's coding\n>> style. This can be used with clang-format to auto-format .c and .h\n>> files to conform with git's style.\n>>\n>> Signed-off-by: Brandon Williams \n>> ---\n>>\n>> I'm sure this sort of thing comes up every so often on the list but back at\n>> git-merge I mentioned how it would be nice to not have to worry about style\n>> when reviewing patches as that is something mechanical and best left to a\n>> machine (for the most part).\n>\n> Amen.\n>\n> If I never have to see a review mentioning an unwrapped line, I am quite\n> certain I will be quite content.\n>\n> Ciao,\n> Dscho\n\nAs a thought experiment I'd like to propose to take it one step further:\n\n If the code was formatted perfectly in one style such that a formatter for\n this style would not produce changes when rerun again on the code, then\n each individual could have a clean/smudge filter to work in their preferred\n style, and only the exchange and storage of code is in a mutual agreed\n style. If the mutually agreed style is close to what I prefer, I don't have to\n use clean/smudge filters.\n\nAdditionally to this patch, we'd want to either put a note into\nSubmittingPatches or Documentation/gitworkflows.txt to hint at\nhow to use this formatting to just affect the patch that is currently\nworked on or rather a pre-commit hook?\n"},{"id":"325823","messageId":"xmqq3792c5sb.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"20170808012554.186051-1-bmwill@google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-08T17:45:24Z","receivedAt":"2017-08-08T17:45:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Add a '.clang-format' file which outlines the git project's coding\n> style. This can be used with clang-format to auto-format .c and .h\n> files to conform with git's style.\n>\n> Signed-off-by: Brandon Williams \n> ---\n>\n> I'm sure this sort of thing comes up every so often on the list but back at\n> git-merge I mentioned how it would be nice to not have to worry about style\n> when reviewing patches as that is something mechanical and best left to a\n> machine (for the most part). I saw that 'clang-format' was brought up on the\n> list once before a couple years ago\n> (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing\n> really came of it. I spent a little bit of time combing through the various\n> options and came up with this config based on the general style of our code\n> base. The big issue though is that our code base isn't consistent so try as\n> you might you wont be able to come up with a config which matches everything we\n> do (mostly due to the inconsistencies in our code base).\n>\n> Anyway, I thought I'd bring this topic back up and see how people feel about it.\n\nI gave just one pass over all the rules you have here. I didn't\nthink too deeply about implications of some of them, but most of\nthem looked sensible. Thanks for compiling this set of rules.\n\nHaving said that, it is easier to refine individual rules you have\nbelow than to make sure that among the develoepers there is a shared\nnotion of how these rules are to be used. If we get that part wrong,\nwe'd see unpleasant consequences, e.g.\n\n - We may see unwanted code churn on existing codebase, only for the\n sake of satisfying the formatting rules specified here.\n\n - We may see far more style-only critique to patches posted on the\n list simply because there are more readers than writers, and it\n is likely that running the tool to nitpick other people's patches\n is far easier than writing these patches in the first place (and\n forgetting to ask the formatter to nitpick before sending them\n out).\n\n - Human aesthetics judgement often is necessary to overrule\n mechanical rules (e.g. A human may have two pairs of \n parameters on separate lines in a function declaration;\n BinPackParameters may try to break after ptrA, lenA, ptrB before\n lenB in such a case).\n\nWe need to set our expectation and a guideline to apply these rules\nstraight, before introducing something like this.\n\n\n> .clang-format | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 166 insertions(+)\n> create mode 100644 .clang-format\n>\n> diff --git a/.clang-format b/.clang-format\n> new file mode 100644\n> index 000000000..7f28dc259\n> --- /dev/null\n> +++ b/.clang-format\n> @@ -0,0 +1,166 @@\n> +# Defaults\n> +\n> +# Use tabs whenever we need to fill whitespace that spans at least from one tab\n> +# stop to the next one.\n> +UseTab: Always\n> +TabWidth: 8\n> +IndentWidth: 8\n> +ContinuationIndentWidth: 8\n> +ColumnLimit: 80\n\nI often deliberately chomp my lines much shorter than this limit,\nand also I deliberately write a line that is a tad longer than this\nlimit some other times, if doing so makes the result easier to read.\nAnd I know other develoepers with good taste do the same. It is\npointless to have a discussion that begins with \"who uses a physical\nterminal these days that can only show 80-columns?\" to tweak this\nvalue, I think. It is more important to give a guideline on what to\ndo when lines in your code goes over this limit.\n\nA mechanical \"formatter\" would just find an appropriate point in a\nline with least \"penalty\" and chomp it into two. But an appropriate\nway to make such a code that is way too deeply indented readable may\ninstead be judicious use of goto's and one-time helper functions,\nfor example, which mechanical tools would not do.\n\nThat is an example of what I meant above, i.e. a guideline to apply\nthese rules. We would not want to say \"clang-format suggests this\nrewrite, so we should accept the resulting code that is still too\ndeeply indented as-is\"---using the tool to point out an issue is\ngood, though.\n\n> +\n> +# C Language specifics\n> +Language: Cpp\n\nHmph ;-)\n\n> +# Add a line break after the return type of top-level functions\n> +# int\n> +# foo();\n> +AlwaysBreakAfterReturnType: TopLevel\n\nWe do that?\n\n> +# Pack as many parameters or arguments onto the same line as possible\n> +# int myFunction(int aaaaaaaaaaaa, int bbbbbbbb,\n> +# int cccc);\n> +BinPackArguments: true\n> +BinPackParameters: true\n\nI am OK with this but with the caveats I already mentioned.\n\n> +# Insert a space after a cast\n> +# x = (int32) y; not x = (int32)y;\n> +SpaceAfterCStyleCast: true\n\nHmph, I thought we did the latter, i.e. cast sticks to the casted\nexpression without SP.\n\nThanks.\n"},{"id":"325824","messageId":"20170808180317.GA73298@google.com","threadId":"46536","inReplyTo":"xmqq3792c5sb.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-08T18:03:17Z","receivedAt":"2017-08-08T18:03:28Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/08, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > Add a '.clang-format' file which outlines the git project's coding\n> > style. This can be used with clang-format to auto-format .c and .h\n> > files to conform with git's style.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> >\n> > I'm sure this sort of thing comes up every so often on the list but back at\n> > git-merge I mentioned how it would be nice to not have to worry about style\n> > when reviewing patches as that is something mechanical and best left to a\n> > machine (for the most part). I saw that 'clang-format' was brought up on the\n> > list once before a couple years ago\n> > (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing\n> > really came of it. I spent a little bit of time combing through the various\n> > options and came up with this config based on the general style of our code\n> > base. The big issue though is that our code base isn't consistent so try as\n> > you might you wont be able to come up with a config which matches everything we\n> > do (mostly due to the inconsistencies in our code base).\n> >\n> > Anyway, I thought I'd bring this topic back up and see how people feel about it.\n> \n> I gave just one pass over all the rules you have here. I didn't\n> think too deeply about implications of some of them, but most of\n> them looked sensible. Thanks for compiling this set of rules.\n> \n> Having said that, it is easier to refine individual rules you have\n> below than to make sure that among the develoepers there is a shared\n> notion of how these rules are to be used. If we get that part wrong,\n> we'd see unpleasant consequences, e.g.\n> \n> - We may see unwanted code churn on existing codebase, only for the\n> sake of satisfying the formatting rules specified here.\n\nThis is an issue when you have an inconsistent code base such as ours as\nthe tool, even when used to format a diff, will format the surrounding\ncontext.\n\n> \n> - We may see far more style-only critique to patches posted on the\n> list simply because there are more readers than writers, and it\n> is likely that running the tool to nitpick other people's patches\n> is far easier than writing these patches in the first place (and\n> forgetting to ask the formatter to nitpick before sending them\n> out).\n\nI would hope that use of such a tool would eventually completely\neliminate style-only critiques. \n\n> \n> - Human aesthetics judgement often is necessary to overrule\n> mechanical rules (e.g. A human may have two pairs of \n> parameters on separate lines in a function declaration;\n> BinPackParameters may try to break after ptrA, lenA, ptrB before\n> lenB in such a case).\n\nI know that when you introduce a formatter there are bound to be some\nissues where a human would choose one way over the other. If we start\nusing a formatter I would expect some of the penalties would need to be\ntweaked overtime before we rely too heavily on it.\n\n> \n> We need to set our expectation and a guideline to apply these rules\n> straight, before introducing something like this.\n\n> \n> \n> > .clang-format | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 166 insertions(+)\n> > create mode 100644 .clang-format\n> >\n> > diff --git a/.clang-format b/.clang-format\n> > new file mode 100644\n> > index 000000000..7f28dc259\n> > --- /dev/null\n> > +++ b/.clang-format\n> > @@ -0,0 +1,166 @@\n> > +# Defaults\n> > +\n> > +# Use tabs whenever we need to fill whitespace that spans at least from one tab\n> > +# stop to the next one.\n> > +UseTab: Always\n> > +TabWidth: 8\n> > +IndentWidth: 8\n> > +ContinuationIndentWidth: 8\n> > +ColumnLimit: 80\n> \n> I often deliberately chomp my lines much shorter than this limit,\n> and also I deliberately write a line that is a tad longer than this\n> limit some other times, if doing so makes the result easier to read.\n> And I know other develoepers with good taste do the same. It is\n> pointless to have a discussion that begins with \"who uses a physical\n> terminal these days that can only show 80-columns?\" to tweak this\n> value, I think. It is more important to give a guideline on what to\n> do when lines in your code goes over this limit.\n> \n> A mechanical \"formatter\" would just find an appropriate point in a\n> line with least \"penalty\" and chomp it into two. But an appropriate\n> way to make such a code that is way too deeply indented readable may\n> instead be judicious use of goto's and one-time helper functions,\n> for example, which mechanical tools would not do.\n> \n> That is an example of what I meant above, i.e. a guideline to apply\n> these rules. We would not want to say \"clang-format suggests this\n> rewrite, so we should accept the resulting code that is still too\n> deeply indented as-is\"---using the tool to point out an issue is\n> good, though.\n> \n> > +\n> > +# C Language specifics\n> > +Language: Cpp\n> \n> Hmph ;-)\n\nWell there's no 'C' Lang option so Cpp is the closest there is ;)\n\n> \n> > +# Add a line break after the return type of top-level functions\n> > +# int\n> > +# foo();\n> > +AlwaysBreakAfterReturnType: TopLevel\n> \n> We do that?\n\nHaha So generally no we don't do this. Though there are definitely many\nplaces in our code base where we do. Personally this makes it a bit\neasier to read when you end up having long function names. I also\nworked on a code base which did this and it made it incredible easy to\ngrep for the definition of a function. If you grep for 'foo()' then\nyou'd get all the uses of the function including the definition but if\nyou grep for '^foo()' you'd get only the definition.\n\nBut that's my preference, if we end up using this tool it would probably\nmake sense to change this.\n\n> \n> > +# Pack as many parameters or arguments onto the same line as possible\n> > +# int myFunction(int aaaaaaaaaaaa, int bbbbbbbb,\n> > +# int cccc);\n> > +BinPackArguments: true\n> > +BinPackParameters: true\n> \n> I am OK with this but with the caveats I already mentioned.\n\nThe alternative (with this config) is to have arguments and parameters\neach on their own separate line.\n\n> \n> > +# Insert a space after a cast\n> > +# x = (int32) y; not x = (int32)y;\n> > +SpaceAfterCStyleCast: true\n> \n> Hmph, I thought we did the latter, i.e. cast sticks to the casted\n> expression without SP.\n\nI've seen both and I wasn't sure which was the correct form to use.\n\n> \n> Thanks.\n\n-- \nBrandon Williams\n"},{"id":"325825","messageId":"20170808182324.GB73298@google.com","threadId":"46536","inReplyTo":"CAGZ79kb2860nUs46bP=x-gAx9Ao6DxnvXQv3x387Wxw+kEyxJw@mail.gmail.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-08T18:23:24Z","receivedAt":"2017-08-08T18:23:32Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/08, Stefan Beller wrote:\n> On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin\n> wrote:\n> > Hi Brandon,\n> >\n> > On Mon, 7 Aug 2017, Brandon Williams wrote:\n> >\n> >> Add a '.clang-format' file which outlines the git project's coding\n> >> style. This can be used with clang-format to auto-format .c and .h\n> >> files to conform with git's style.\n> >>\n> >> Signed-off-by: Brandon Williams \n> >> ---\n> >>\n> >> I'm sure this sort of thing comes up every so often on the list but back at\n> >> git-merge I mentioned how it would be nice to not have to worry about style\n> >> when reviewing patches as that is something mechanical and best left to a\n> >> machine (for the most part).\n> >\n> > Amen.\n> >\n> > If I never have to see a review mentioning an unwrapped line, I am quite\n> > certain I will be quite content.\n> >\n> > Ciao,\n> > Dscho\n> \n> As a thought experiment I'd like to propose to take it one step further:\n> \n> If the code was formatted perfectly in one style such that a formatter for\n> this style would not produce changes when rerun again on the code, then\n> each individual could have a clean/smudge filter to work in their preferred\n> style, and only the exchange and storage of code is in a mutual agreed\n> style. If the mutually agreed style is close to what I prefer, I don't have to\n> use clean/smudge filters.\n> \n> Additionally to this patch, we'd want to either put a note into\n> SubmittingPatches or Documentation/gitworkflows.txt to hint at\n> how to use this formatting to just affect the patch that is currently\n> worked on or rather a pre-commit hook?\n\nI'm sure both of these things will probably happen if we decide to make\nuse of a code-formatter. This RFC is really just trying to ask the\nquestion: \"Is this something we want to entertain doing?\"\nMy response would be 'Yes' but there's many other opinions to consider\nfirst :)\n\n-- \nBrandon Williams\n"},{"id":"325826","messageId":"xmqqy3qtc3x4.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"20170808180317.GA73298@google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-08T18:25:43Z","receivedAt":"2017-08-08T18:25:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n>> > +# Add a line break after the return type of top-level functions\n>> > +# int\n>> > +# foo();\n>> > +AlwaysBreakAfterReturnType: TopLevel\n>> \n>> We do that?\n>\n> Haha So generally no we don't do this. Though there are definitely many\n> places in our code base where we do. Personally this makes it a bit\n> easier to read when you end up having long function names. I also\n> worked on a code base which did this and it made it incredible easy to\n> grep for the definition of a function. If you grep for 'foo()' then\n> you'd get all the uses of the function including the definition but if\n> you grep for '^foo()' you'd get only the definition.\n>\n> But that's my preference, if we end up using this tool it would probably\n> make sense to change this.\n\nYeah, I even know people who did\n\n\t\tint\n\tfoo(void)\n\nfor greppability of \"^foo\". It took some effort to get used to that\nstyle.\n\n>> > +# Insert a space after a cast\n>> > +# x = (int32) y; not x = (int32)y;\n>> > +SpaceAfterCStyleCast: true\n>> \n>> Hmph, I thought we did the latter, i.e. cast sticks to the casted\n>> expression without SP.\n>\n> I've seen both and I wasn't sure which was the correct form to use.\n\nWe do the latter because checkpatch.pl from the kernel project tells\nus to, I think.\n"},{"id":"325923","messageId":"xmqqpoc56x27.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"xmqqy3qtc3x4.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-09T07:05:04Z","receivedAt":"2017-08-09T07:05:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>>> > +# Insert a space after a cast\n>>> > +# x = (int32) y; not x = (int32)y;\n>>> > +SpaceAfterCStyleCast: true\n>>> \n>>> Hmph, I thought we did the latter, i.e. cast sticks to the casted\n>>> expression without SP.\n>>\n>> I've seen both and I wasn't sure which was the correct form to use.\n>\n> We do the latter because checkpatch.pl from the kernel project tells\n> us to, I think.\n\nBefore I forget, there are some rules in checkpatch.pl that I\ndeliberately ignore while accepting patches from the list. \n\nI appreciate the tool for pointing out overlong lines, but I often\nfind them easier to read as-is than split into two lines, as the\nones the people send in real life rarely excessively exceed the\n80-col limit. We also use things like SHA_CTX that trigger \"avoid\ncamelcase\", which I also ignore.\n\nOne thing we probably should standardize is the way the width of\nbitfields in struct is specified. I think checkpatch.pl wants to do\n\n\tstruct {\n\t\tunsigned int three_bits : 3;\n\t};\n\nwith SP around the colon, but our codebase does not always have the\nspaces there, and I see no technical reason not to follow suit, as\nlong as we are following most of what checkpatch.pl wants us to do.\n\nBy the way, I do not recall seeing a rule about this in your clang\nformat rules. Can we spell it out in there?\n\nI also see this:\n\n WARNING: __printf(string-index, first-to-check) is preferred over\n __attribute__((format(printf, string-index, first-to-check)))\n\nbut I think it is specific to the kernel source (the macro is\ndefined in include/linux/compiler-gcc.h and expands to the latter),\nso I also ignore it.\n\ncheckpatch.pl also warns a SP immediately before HT, which I do pay\nattention to, as well as trailing whitespaces. If clang-format can\nbe told to check that, I think we would want to have such a rule.\n\nFor a reference, here is a sample set of changes to cache.h to\nsquelch most of the warnings and errors checkpatch.pl points out\nthat I do not ignore.\n\n cache.h | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 4e390e6af8..dec807b3b0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -25,7 +25,7 @@\n #define platform_SHA_CTX\tSHA_CTX\n #define platform_SHA1_Init\tSHA1_Init\n #define platform_SHA1_Update\tSHA1_Update\n-#define platform_SHA1_Final \tSHA1_Final\n+#define platform_SHA1_Final\tSHA1_Final\n #endif\n \n #define git_SHA_CTX\t\tplatform_SHA_CTX\n@@ -260,7 +260,7 @@ static inline void copy_cache_entry(struct cache_entry *dst,\n \n static inline unsigned create_ce_flags(unsigned stage)\n {\n-\treturn (stage << CE_STAGESHIFT);\n+\treturn stage << CE_STAGESHIFT;\n }\n \n #define ce_namelen(ce) ((ce)->ce_namelen)\n@@ -317,7 +317,7 @@ static inline unsigned int canon_mode(unsigned int mode)\n \treturn S_IFGITLINK;\n }\n \n-#define cache_entry_size(len) (offsetof(struct cache_entry,name) + (len) + 1)\n+#define cache_entry_size(len) (offsetof(struct cache_entry, name) + (len) + 1)\n \n #define SOMETHING_CHANGED\t(1 << 0) /* unclassified changes go here */\n #define CE_ENTRY_CHANGED\t(1 << 1)\n@@ -373,7 +373,7 @@ extern void free_name_hash(struct index_state *istate);\n #define read_cache_unmerged() read_index_unmerged(&the_index)\n #define discard_cache() discard_index(&the_index)\n #define unmerged_cache() unmerged_index(&the_index)\n-#define cache_name_pos(name, namelen) index_name_pos(&the_index,(name),(namelen))\n+#define cache_name_pos(name, namelen) index_name_pos(&the_index, (name), (namelen))\n #define add_cache_entry(ce, option) add_index_entry(&the_index, (ce), (option))\n #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))\n #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n@@ -1483,10 +1483,10 @@ struct checkout {\n \tconst char *base_dir;\n \tint base_dir_len;\n \tstruct delayed_checkout *delayed_checkout;\n-\tunsigned force:1,\n-\t\t quiet:1,\n-\t\t not_new:1,\n-\t\t refresh_cache:1;\n+\tunsigned force : 1,\n+\t\t quiet : 1,\n+\t\t not_new : 1,\n+\t\t refresh_cache : 1;\n };\n #define CHECKOUT_INIT { NULL, \"\" }\n \n@@ -1534,7 +1534,7 @@ extern struct alternate_object_database {\n \tchar path[FLEX_ARRAY];\n } *alt_odb_list;\n extern void prepare_alt_odb(void);\n-extern void read_info_alternates(const char * relative_base, int depth);\n+extern void read_info_alternates(const char *relative_base, int depth);\n extern char *compute_alternate_path(const char *path, struct strbuf *err);\n typedef int alt_odb_fn(struct alternate_object_database *, void *);\n extern int foreach_alt_odb(alt_odb_fn, void*);\n@@ -1587,10 +1587,10 @@ extern struct packed_git {\n \tint index_version;\n \ttime_t mtime;\n \tint pack_fd;\n-\tunsigned pack_local:1,\n-\t\t pack_keep:1,\n-\t\t freshened:1,\n-\t\t do_not_close:1;\n+\tunsigned pack_local : 1,\n+\t\t pack_keep : 1,\n+\t\t freshened : 1,\n+\t\t do_not_close : 1;\n \tunsigned char sha1[20];\n \tstruct revindex_entry *revindex;\n \t/* something like \".git/objects/pack/xxxxx.pack\" */\n@@ -1767,10 +1767,10 @@ struct object_info {\n \tunion {\n \t\t/*\n \t\t * struct {\n-\t\t * \t... Nothing to expose in this case\n+\t\t *\t... Nothing to expose in this case\n \t\t * } cached;\n \t\t * struct {\n-\t\t * \t... Nothing to expose in this case\n+\t\t *\t... Nothing to expose in this case\n \t\t * } loose;\n \t\t */\n \t\tstruct {\n"},{"id":"325948","messageId":"20170809130127.ekd4tvyp2rrb7ftk@sigill.intra.peff.net","threadId":"46536","inReplyTo":"20170808012554.186051-1-bmwill@google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-09T13:01:27Z","receivedAt":"2017-08-09T13:01:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 07, 2017 at 06:25:54PM -0700, Brandon Williams wrote:\n\n> I'm sure this sort of thing comes up every so often on the list but back at\n> git-merge I mentioned how it would be nice to not have to worry about style\n> when reviewing patches as that is something mechanical and best left to a\n> machine (for the most part). I saw that 'clang-format' was brought up on the\n> list once before a couple years ago\n> (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing\n> really came of it. I spent a little bit of time combing through the various\n> options and came up with this config based on the general style of our code\n> base. The big issue though is that our code base isn't consistent so try as\n> you might you wont be able to come up with a config which matches everything we\n> do (mostly due to the inconsistencies in our code base).\n\nRight, the reason I stopped pursuing it was that I couldn't find a way\nto have it make suggestions for new code without nagging about existing\ncode. If we were to aggressively reformat to match the tool for existing\ncode, that would help. But I'm a bit worried that there would always be\nsuggestions from the tool that we don't agree with (i.e., where the\nguiding principle is \"do what is readable\").\n\nI dunno. I guess \"go fmt\" people decided to just treat the tool's output\nas the One True Way. I haven't written enough Go to have an opinion\nmyself, but it seems to at least work for them.\n\nWhat does the tooling look like these days for just adjusting lines\ntouched by a given patch?\n\n-Peff\n"},{"id":"325952","messageId":"87y3qsg7ni.fsf@gmail.com","threadId":"46536","inReplyTo":"20170809130127.ekd4tvyp2rrb7ftk@sigill.intra.peff.net","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-08-09T14:03:45Z","receivedAt":"2017-08-09T14:03:53Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 09 2017, Jeff King jotted:\n\n> On Mon, Aug 07, 2017 at 06:25:54PM -0700, Brandon Williams wrote:\n>\n>> I'm sure this sort of thing comes up every so often on the list but back at\n>> git-merge I mentioned how it would be nice to not have to worry about style\n>> when reviewing patches as that is something mechanical and best left to a\n>> machine (for the most part). I saw that 'clang-format' was brought up on the\n>> list once before a couple years ago\n>> (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing\n>> really came of it. I spent a little bit of time combing through the various\n>> options and came up with this config based on the general style of our code\n>> base. The big issue though is that our code base isn't consistent so try as\n>> you might you wont be able to come up with a config which matches everything we\n>> do (mostly due to the inconsistencies in our code base).\n>\n> Right, the reason I stopped pursuing it was that I couldn't find a way\n> to have it make suggestions for new code without nagging about existing\n> code. If we were to aggressively reformat to match the tool for existing\n> code, that would help. But I'm a bit worried that there would always be\n> suggestions from the tool that we don't agree with (i.e., where the\n> guiding principle is \"do what is readable\").\n>\n> I dunno. I guess \"go fmt\" people decided to just treat the tool's output\n> as the One True Way. I haven't written enough Go to have an opinion\n> myself, but it seems to at least work for them.\n\n(I have no opinion either way on whether this clang formatting this is a\ngood idea or not)\n\n> What does the tooling look like these days for just adjusting lines\n> touched by a given patch?\n\nPresumably even if it sucked we could easily write a \"./git-fmt-check.sh\n\" script to do it which would do the following:\n\n 1. Check out the master branch\n 2. Apply code formatting to entire project (or just the files you\n changed)\n 3. Commit that on a throwaway branch\n 4. Switch back to your WIP branch\n 5. See if it would merge cleanly with the throwaway code formatting\n branch (I forget the actual 'not a real merge but check' command to\n do this, but it exists).\n\nIf there were any reported conflicts presumably the new code you're\nadding is violating the coding standards laid out in this file. If not\nyou're good.\n"},{"id":"325999","messageId":"alpine.DEB.2.21.1.1708100032050.11175@virtualbox","threadId":"46536","inReplyTo":"20170808182324.GB73298@google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-08-09T22:33:28Z","receivedAt":"2017-08-09T22:33:37Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 8 Aug 2017, Brandon Williams wrote:\n\n> On 08/08, Stefan Beller wrote:\n> > On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin\n> > wrote:\n> > > Hi Brandon,\n> > >\n> > > On Mon, 7 Aug 2017, Brandon Williams wrote:\n> > >\n> > >> Add a '.clang-format' file which outlines the git project's coding\n> > >> style. This can be used with clang-format to auto-format .c and .h\n> > >> files to conform with git's style.\n> > >>\n> > >> Signed-off-by: Brandon Williams \n> > >> ---\n> > >>\n> > >> I'm sure this sort of thing comes up every so often on the list but back at\n> > >> git-merge I mentioned how it would be nice to not have to worry about style\n> > >> when reviewing patches as that is something mechanical and best left to a\n> > >> machine (for the most part).\n> > >\n> > > Amen.\n> > >\n> > > If I never have to see a review mentioning an unwrapped line, I am quite\n> > > certain I will be quite content.\n> > >\n> > > Ciao,\n> > > Dscho\n> > \n> > As a thought experiment I'd like to propose to take it one step further:\n> > \n> > If the code was formatted perfectly in one style such that a formatter for\n> > this style would not produce changes when rerun again on the code, then\n> > each individual could have a clean/smudge filter to work in their preferred\n> > style, and only the exchange and storage of code is in a mutual agreed\n> > style. If the mutually agreed style is close to what I prefer, I don't have to\n> > use clean/smudge filters.\n> > \n> > Additionally to this patch, we'd want to either put a note into\n> > SubmittingPatches or Documentation/gitworkflows.txt to hint at\n> > how to use this formatting to just affect the patch that is currently\n> > worked on or rather a pre-commit hook?\n> \n> I'm sure both of these things will probably happen if we decide to make\n> use of a code-formatter. This RFC is really just trying to ask the\n> question: \"Is this something we want to entertain doing?\"\n> My response would be 'Yes' but there's many other opinions to consider\n> first :)\n\nI am sure that something even better will be possible: a Continuous\n\"Integration\" that fixes the coding style automatically by using\n`filter-branch` (avoiding the merge conflicts that would arise if `rebase\n-i` was used).\n\nCiao,\nDscho\n"},{"id":"326000","messageId":"CAGZ79kb6Ljk8brLN1bbOnBLfm=Q=aCnkZ=ZBtDPzf7MZionmSw@mail.gmail.com","threadId":"46536","inReplyTo":"alpine.DEB.2.21.1.1708100032050.11175@virtualbox","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-09T22:42:11Z","receivedAt":"2017-08-09T22:42:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">>\n>> I'm sure both of these things will probably happen if we decide to make\n>> use of a code-formatter. This RFC is really just trying to ask the\n>> question: \"Is this something we want to entertain doing?\"\n>> My response would be 'Yes' but there's many other opinions to consider\n>> first :)\n\nMine would be 'Yes', too.\n\n>\n> I am sure that something even better will be possible: a Continuous\n> \"Integration\" that fixes the coding style automatically by using\n> `filter-branch` (avoiding the merge conflicts that would arise if `rebase\n> -i` was used).\n\nI do not quite follow. Is that to be used by Junio while integrating branches?\n\nBy the wording it doesn't sound like you think of a one-time shot to be\nused in the transition phase from now to a clean future, but the \"CI\" would\nrun continually on all new patches?\n"},{"id":"326001","messageId":"CAGZ79kYEyebHxFO++u5RkPBj16xx5nMcahBPxra4xWUfMrXydA@mail.gmail.com","threadId":"46536","inReplyTo":"87y3qsg7ni.fsf@gmail.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-09T22:53:17Z","receivedAt":"2017-08-09T22:53:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 9, 2017 at 7:03 AM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Wed, Aug 09 2017, Jeff King jotted:\n>\n>> On Mon, Aug 07, 2017 at 06:25:54PM -0700, Brandon Williams wrote:\n>>\n>>> I'm sure this sort of thing comes up every so often on the list but back at\n>>> git-merge I mentioned how it would be nice to not have to worry about style\n>>> when reviewing patches as that is something mechanical and best left to a\n>>> machine (for the most part). I saw that 'clang-format' was brought up on the\n>>> list once before a couple years ago\n>>> (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing\n>>> really came of it. I spent a little bit of time combing through the various\n>>> options and came up with this config based on the general style of our code\n>>> base. The big issue though is that our code base isn't consistent so try as\n>>> you might you wont be able to come up with a config which matches everything we\n>>> do (mostly due to the inconsistencies in our code base).\n>>\n>> Right, the reason I stopped pursuing it was that I couldn't find a way\n>> to have it make suggestions for new code without nagging about existing\n>> code. If we were to aggressively reformat to match the tool for existing\n>> code, that would help. But I'm a bit worried that there would always be\n>> suggestions from the tool that we don't agree with (i.e., where the\n>> guiding principle is \"do what is readable\").\n\nWe may have different opinions on what is readable/beautiful code.\nIf we were to follow a mutual agreed style that is produced by a tool,\nwe could use clean/smudge filters with different settings each.\n\nBut I think we'd rather want to find the closest approximation to our\ncurrent style first.\n\n>> I dunno. I guess \"go fmt\" people decided to just treat the tool's output\n>> as the One True Way. I haven't written enough Go to have an opinion\n>> myself, but it seems to at least work for them.\n>\n> (I have no opinion either way on whether this clang formatting this is a\n> good idea or not)\n\nI think it is actually beneficial as it is one less thing to worry about\nas a contributor. Maybe compare it to programming language that\nhas garbage collection built in, which is also a feature to allow the\ncontributor to focus on \"what is important\". (style is not, all it can do\nis hold back progress by too much nitpicking IMHO)\n\n>> What does the tooling look like these days for just adjusting lines\n>> touched by a given patch?\n\n$ clang-format --help\n\nUSAGE: clang-format [options] [ ...]\n..\n -i - Inplace edit s, if specified..\n -lines= - : - format a range of\n lines (both 1-based).\n Multiple ranges can be formatted by specifying\n several -lines arguments.\n Can't be used with -offset and -length.\n Can only be used with one input file.\n..\n\nI would think based on these options, a pre commit hook can be\nwritten that formats precisely the touched lines of code of each file.\n\n>\n> Presumably even if it sucked we could easily write a \"./git-fmt-check.sh\n> \" script to do it which would do the following:\n>\n> 1. Check out the master branch\n> 2. Apply code formatting to entire project (or just the files you\n> changed)\n> 3. Commit that on a throwaway branch\n> 4. Switch back to your WIP branch\n> 5. See if it would merge cleanly with the throwaway code formatting\n> branch (I forget the actual 'not a real merge but check' command to\n> do this, but it exists).\n>\n> If there were any reported conflicts presumably the new code you're\n> adding is violating the coding standards laid out in this file. If not\n> you're good.\n\nThis approach certainly works, but it *adds* one more step to what\na contributor may need to do before sending a patch. I think the intention\nwith a codified style is to *remove* a step (as a machine will do it for you).\n"},{"id":"326002","messageId":"CAGZ79kZRhTNez1jJq+DcCyERufd_YfWK7L+ujPjRCivzHz7LBw@mail.gmail.com","threadId":"46536","inReplyTo":"CAGZ79kYEyebHxFO++u5RkPBj16xx5nMcahBPxra4xWUfMrXydA@mail.gmail.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-09T23:11:56Z","receivedAt":"2017-08-09T23:12:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 9, 2017 at 3:53 PM, Stefan Beller wrote:\n\n> I would think based on these options, a pre commit hook can be\n> written that formats precisely the touched lines of code of each file.\n\nI did not search enough, \"clang-tidy-diff.py --fix\" should be all that is needed\n"},{"id":"326003","messageId":"20170809231900.3535ja3zqdvyerv4@sigill.intra.peff.net","threadId":"46536","inReplyTo":"CAGZ79kZRhTNez1jJq+DcCyERufd_YfWK7L+ujPjRCivzHz7LBw@mail.gmail.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-09T23:19:00Z","receivedAt":"2017-08-09T23:19:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 09, 2017 at 03:53:17PM -0700, Stefan Beller wrote:\n\n> >> Right, the reason I stopped pursuing it was that I couldn't find a way\n> >> to have it make suggestions for new code without nagging about existing\n> >> code. If we were to aggressively reformat to match the tool for existing\n> >> code, that would help. But I'm a bit worried that there would always be\n> >> suggestions from the tool that we don't agree with (i.e., where the\n> >> guiding principle is \"do what is readable\").\n> \n> We may have different opinions on what is readable/beautiful code.\n> If we were to follow a mutual agreed style that is produced by a tool,\n> we could use clean/smudge filters with different settings each.\n\nI'm less worried about a difference of opinion between humans. My\nconcern is that there are cases that the tool's formatting makes _worse_\nthan what any human would write. And either we accept ugly code because\nthe tool sucks, or we spend a bunch of time fighting with the tool to\ntry to make its output look good.\n\n> > I would think based on these options, a pre commit hook can be\n> > written that formats precisely the touched lines of code of each file.\n> \n> I did not search enough, \"clang-tidy-diff.py --fix\" should be all that is needed\n\nI think I found that script when we discussed this a while back, but I\ncouldn't get it to stop bugging me about lines that I hadn't touched. I\nhaven't looked at it recently, though. That's specifically what I was\nwondering about with \"is the tooling ready for this\".\n\n-Peff\n"},{"id":"326021","messageId":"alpine.DEB.2.21.1.1708101137190.11175@virtualbox","threadId":"46536","inReplyTo":"CAGZ79kb6Ljk8brLN1bbOnBLfm=Q=aCnkZ=ZBtDPzf7MZionmSw@mail.gmail.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-08-10T09:38:09Z","receivedAt":"2017-08-10T09:38:20Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Wed, 9 Aug 2017, Stefan Beller wrote:\n\n> > I am sure that something even better will be possible: a Continuous\n> > \"Integration\" that fixes the coding style automatically by using\n> > `filter-branch` (avoiding the merge conflicts that would arise if\n> > `rebase -i` was used).\n> \n> I do not quite follow. Is that to be used by Junio while integrating\n> branches?\n\nI was more thinking about a bot on GitHub. \"Code cleanup as a service\".\n\nCiao,\nDscho\n"},{"id":"326031","messageId":"xmqqk22b5q9m.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"alpine.DEB.2.21.1.1708101137190.11175@virtualbox","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-10T16:41:41Z","receivedAt":"2017-08-10T16:41:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Wed, 9 Aug 2017, Stefan Beller wrote:\n>\n>> > I am sure that something even better will be possible: a Continuous\n>> > \"Integration\" that fixes the coding style automatically by using\n>> > `filter-branch` (avoiding the merge conflicts that would arise if\n>> > `rebase -i` was used).\n>> \n>> I do not quite follow. Is that to be used by Junio while integrating\n>> branches?\n>\n> I was more thinking about a bot on GitHub. \"Code cleanup as a service\".\n\nI vaguely recall that there was a discussion to have SubmitGit wait\nfor success from Travis CI; if that is already in place, then I can\nsort of see how it would help individual contributors to have the\nstyle checker in that pipeline as well. \n\nI have a mixed feelings about \"fixing\" styles automatically, though.\n\n"},{"id":"326034","messageId":"20170810171545.GC73298@google.com","threadId":"46536","inReplyTo":"xmqqk22b5q9m.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-10T17:15:45Z","receivedAt":"2017-08-10T17:16:05Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/10, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> > On Wed, 9 Aug 2017, Stefan Beller wrote:\n> >\n> >> > I am sure that something even better will be possible: a Continuous\n> >> > \"Integration\" that fixes the coding style automatically by using\n> >> > `filter-branch` (avoiding the merge conflicts that would arise if\n> >> > `rebase -i` was used).\n> >> \n> >> I do not quite follow. Is that to be used by Junio while integrating\n> >> branches?\n> >\n> > I was more thinking about a bot on GitHub. \"Code cleanup as a service\".\n> \n> I vaguely recall that there was a discussion to have SubmitGit wait\n> for success from Travis CI; if that is already in place, then I can\n> sort of see how it would help individual contributors to have the\n> style checker in that pipeline as well. \n> \n> I have a mixed feelings about \"fixing\" styles automatically, though.\n> \n\nI still think we are far away from a world where we can fix style\nautomatically. If we do want to keep pursuing this there are a number\nsteps we'd want to take first.\n\n1. Settle on a concrete style and document it using a formatter's rules\n (in say a .clang-format file). This style would most likely need to\n be tuned a little bit, at least the 'Penalty' configuration would\n need to be tuned which (as far as I understand it) is used to\n determine which rule to break first to ensure a line isn't too long.\n\n2. Start getting contributors to use the tool to format their patches.\n This would include having some script or hook that a contributor\n could run to only format the sections of code that they touched.\n\n3. Slowly the code base would begin to have a uniform style. At\n some point we may want to then reformat the remaining sections of the\n code base. At this point we could have some automated bot that fixes\n style.\n\nI'm sure I missed a step in there somewhere though.\n\n-- \nBrandon Williams\n"},{"id":"326037","messageId":"xmqqzib749ix.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"20170810171545.GC73298@google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-10T17:28:38Z","receivedAt":"2017-08-10T17:28:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> On 08/10, Junio C Hamano wrote:\n>\n>> I vaguely recall that there was a discussion to have SubmitGit wait\n>> for success from Travis CI; if that is already in place, then I can\n>> sort of see how it would help individual contributors to have the\n>> style checker in that pipeline as well. \n>> \n>> I have a mixed feelings about \"fixing\" styles automatically, though.\n>\n> I still think we are far away from a world where we can fix style\n> automatically. If we do want to keep pursuing this there are a number\n> steps we'd want to take first.\n>\n> 1. Settle on a concrete style and document it using a formatter's rules\n> (in say a .clang-format file). This style would most likely need to\n> be tuned a little bit, at least the 'Penalty' configuration would\n> need to be tuned which (as far as I understand it) is used to\n> determine which rule to break first to ensure a line isn't too long.\n\nYes. I think this is what you started to get the ball rolling.\nTogether with what checkpatch.pl already diagnoses, I think we can\nget a guideline that is more or less reasonable.\n\n> 2. Start getting contributors to use the tool to format their patches.\n> This would include having some script or hook that a contributor\n> could run to only format the sections of code that they touched.\n\nThis, too. Running checkpatch.pl (possibly combined with a bit of\ntweaking it to match our needs) already catches many of the issues,\nso a tool with a similar interface would be easy to use, I would\nimagine.\n\n> 3. Slowly the code base would begin to have a uniform style. At\n> some point we may want to then reformat the remaining sections of the\n> code base. At this point we could have some automated bot that fixes\n> style.\n\nI suspect I am discussing this based on a different assumption.\n\nI think the primary goal of this effort is to make it easier to\ncleanse the new patches that appear on the list of trivial style\nissues, so that contributors and reviewers do not have to spend\nbandwidth and brain cycles during the review. And I have been\nassuming that we can do so even without waiting for a \"tree wide\"\ncode churn on existing code to complete.\n\n"},{"id":"326101","messageId":"20170810213013.GD73298@google.com","threadId":"46536","inReplyTo":"xmqqzib749ix.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-10T21:30:13Z","receivedAt":"2017-08-10T21:30:22Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/10, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > On 08/10, Junio C Hamano wrote:\n> >\n> >> I vaguely recall that there was a discussion to have SubmitGit wait\n> >> for success from Travis CI; if that is already in place, then I can\n> >> sort of see how it would help individual contributors to have the\n> >> style checker in that pipeline as well. \n> >> \n> >> I have a mixed feelings about \"fixing\" styles automatically, though.\n> >\n> > I still think we are far away from a world where we can fix style\n> > automatically. If we do want to keep pursuing this there are a number\n> > steps we'd want to take first.\n> >\n> > 1. Settle on a concrete style and document it using a formatter's rules\n> > (in say a .clang-format file). This style would most likely need to\n> > be tuned a little bit, at least the 'Penalty' configuration would\n> > need to be tuned which (as far as I understand it) is used to\n> > determine which rule to break first to ensure a line isn't too long.\n> \n> Yes. I think this is what you started to get the ball rolling.\n> Together with what checkpatch.pl already diagnoses, I think we can\n> get a guideline that is more or less reasonable.\n> \n> > 2. Start getting contributors to use the tool to format their patches.\n> > This would include having some script or hook that a contributor\n> > could run to only format the sections of code that they touched.\n> \n> This, too. Running checkpatch.pl (possibly combined with a bit of\n> tweaking it to match our needs) already catches many of the issues,\n> so a tool with a similar interface would be easy to use, I would\n> imagine.\n> \n> > 3. Slowly the code base would begin to have a uniform style. At\n> > some point we may want to then reformat the remaining sections of the\n> > code base. At this point we could have some automated bot that fixes\n> > style.\n> \n> I suspect I am discussing this based on a different assumption.\n> \n> I think the primary goal of this effort is to make it easier to\n> cleanse the new patches that appear on the list of trivial style\n> issues, so that contributors and reviewers do not have to spend\n> bandwidth and brain cycles during the review. And I have been\n> assuming that we can do so even without waiting for a \"tree wide\"\n> code churn on existing code to complete.\n\nYes that's one of the steps I missed we can call it 2.5 ;) (3) could be\na long term goal which is what I was trying to get at by saying:\n\n> > 3. Slowly the code base would begin to have a uniform style.\n\n-- \nBrandon Williams\n"},{"id":"326153","messageId":"20170811174958.GB59325@google.com","threadId":"46536","inReplyTo":"xmqqpoc56x27.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-11T17:49:58Z","receivedAt":"2017-08-11T17:50:07Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/09, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> >>> > +# Insert a space after a cast\n> >>> > +# x = (int32) y; not x = (int32)y;\n> >>> > +SpaceAfterCStyleCast: true\n> >>> \n> >>> Hmph, I thought we did the latter, i.e. cast sticks to the casted\n> >>> expression without SP.\n> >>\n> >> I've seen both and I wasn't sure which was the correct form to use.\n> >\n> > We do the latter because checkpatch.pl from the kernel project tells\n> > us to, I think.\n> \n> Before I forget, there are some rules in checkpatch.pl that I\n> deliberately ignore while accepting patches from the list. \n> \n> I appreciate the tool for pointing out overlong lines, but I often\n> find them easier to read as-is than split into two lines, as the\n> ones the people send in real life rarely excessively exceed the\n> 80-col limit. We also use things like SHA_CTX that trigger \"avoid\n> camelcase\", which I also ignore.\n> \n> One thing we probably should standardize is the way the width of\n> bitfields in struct is specified. I think checkpatch.pl wants to do\n> \n> \tstruct {\n> \t\tunsigned int three_bits : 3;\n> \t};\n> \n\nI couldn't find a specific setting for this, but it seems like the\n.clang-format file I made wants bit fields to be formatted with spaces\nsurrounding the ':'.\n\n> with SP around the colon, but our codebase does not always have the\n> spaces there, and I see no technical reason not to follow suit, as\n> long as we are following most of what checkpatch.pl wants us to do.\n> \n> By the way, I do not recall seeing a rule about this in your clang\n> format rules. Can we spell it out in there?\n> \n> I also see this:\n> \n> WARNING: __printf(string-index, first-to-check) is preferred over\n> __attribute__((format(printf, string-index, first-to-check)))\n> \n> but I think it is specific to the kernel source (the macro is\n> defined in include/linux/compiler-gcc.h and expands to the latter),\n> so I also ignore it.\n> \n> checkpatch.pl also warns a SP immediately before HT, which I do pay\n\nI'm sorry what's 'HT'?\n\n> attention to, as well as trailing whitespaces. If clang-format can\n> be told to check that, I think we would want to have such a rule.\n\nClang-format by default removes trailing whitespace.\n\n> \n> For a reference, here is a sample set of changes to cache.h to\n> squelch most of the warnings and errors checkpatch.pl points out\n> that I do not ignore.\n> \n> cache.h | 30 +++++++++++++++---------------\n> 1 file changed, 15 insertions(+), 15 deletions(-)\n> \n> diff --git a/cache.h b/cache.h\n> index 4e390e6af8..dec807b3b0 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -25,7 +25,7 @@\n> #define platform_SHA_CTX\tSHA_CTX\n> #define platform_SHA1_Init\tSHA1_Init\n> #define platform_SHA1_Update\tSHA1_Update\n> -#define platform_SHA1_Final \tSHA1_Final\n> +#define platform_SHA1_Final\tSHA1_Final\n> #endif\n> \n> #define git_SHA_CTX\t\tplatform_SHA_CTX\n> @@ -260,7 +260,7 @@ static inline void copy_cache_entry(struct cache_entry *dst,\n> \n> static inline unsigned create_ce_flags(unsigned stage)\n> {\n> -\treturn (stage << CE_STAGESHIFT);\n> +\treturn stage << CE_STAGESHIFT;\n> }\n> \n> #define ce_namelen(ce) ((ce)->ce_namelen)\n> @@ -317,7 +317,7 @@ static inline unsigned int canon_mode(unsigned int mode)\n> \treturn S_IFGITLINK;\n> }\n> \n> -#define cache_entry_size(len) (offsetof(struct cache_entry,name) + (len) + 1)\n> +#define cache_entry_size(len) (offsetof(struct cache_entry, name) + (len) + 1)\n> \n> #define SOMETHING_CHANGED\t(1 << 0) /* unclassified changes go here */\n> #define CE_ENTRY_CHANGED\t(1 << 1)\n> @@ -373,7 +373,7 @@ extern void free_name_hash(struct index_state *istate);\n> #define read_cache_unmerged() read_index_unmerged(&the_index)\n> #define discard_cache() discard_index(&the_index)\n> #define unmerged_cache() unmerged_index(&the_index)\n> -#define cache_name_pos(name, namelen) index_name_pos(&the_index,(name),(namelen))\n> +#define cache_name_pos(name, namelen) index_name_pos(&the_index, (name), (namelen))\n> #define add_cache_entry(ce, option) add_index_entry(&the_index, (ce), (option))\n> #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))\n> #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n> @@ -1483,10 +1483,10 @@ struct checkout {\n> \tconst char *base_dir;\n> \tint base_dir_len;\n> \tstruct delayed_checkout *delayed_checkout;\n> -\tunsigned force:1,\n> -\t\t quiet:1,\n> -\t\t not_new:1,\n> -\t\t refresh_cache:1;\n> +\tunsigned force : 1,\n> +\t\t quiet : 1,\n> +\t\t not_new : 1,\n> +\t\t refresh_cache : 1;\n> };\n> #define CHECKOUT_INIT { NULL, \"\" }\n> \n> @@ -1534,7 +1534,7 @@ extern struct alternate_object_database {\n> \tchar path[FLEX_ARRAY];\n> } *alt_odb_list;\n> extern void prepare_alt_odb(void);\n> -extern void read_info_alternates(const char * relative_base, int depth);\n> +extern void read_info_alternates(const char *relative_base, int depth);\n> extern char *compute_alternate_path(const char *path, struct strbuf *err);\n> typedef int alt_odb_fn(struct alternate_object_database *, void *);\n> extern int foreach_alt_odb(alt_odb_fn, void*);\n> @@ -1587,10 +1587,10 @@ extern struct packed_git {\n> \tint index_version;\n> \ttime_t mtime;\n> \tint pack_fd;\n> -\tunsigned pack_local:1,\n> -\t\t pack_keep:1,\n> -\t\t freshened:1,\n> -\t\t do_not_close:1;\n> +\tunsigned pack_local : 1,\n> +\t\t pack_keep : 1,\n> +\t\t freshened : 1,\n> +\t\t do_not_close : 1;\n> \tunsigned char sha1[20];\n> \tstruct revindex_entry *revindex;\n> \t/* something like \".git/objects/pack/xxxxx.pack\" */\n> @@ -1767,10 +1767,10 @@ struct object_info {\n> \tunion {\n> \t\t/*\n> \t\t * struct {\n> -\t\t * \t... Nothing to expose in this case\n> +\t\t *\t... Nothing to expose in this case\n> \t\t * } cached;\n> \t\t * struct {\n> -\t\t * \t... Nothing to expose in this case\n> +\t\t *\t... Nothing to expose in this case\n> \t\t * } loose;\n> \t\t */\n> \t\tstruct {\n\n-- \nBrandon Williams\n"},{"id":"326154","messageId":"20170811175237.GC59325@google.com","threadId":"46536","inReplyTo":"CAGZ79kZRhTNez1jJq+DcCyERufd_YfWK7L+ujPjRCivzHz7LBw@mail.gmail.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-11T17:52:37Z","receivedAt":"2017-08-11T17:52:45Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/09, Stefan Beller wrote:\n> On Wed, Aug 9, 2017 at 3:53 PM, Stefan Beller wrote:\n> \n> > I would think based on these options, a pre commit hook can be\n> > written that formats precisely the touched lines of code of each file.\n> \n> I did not search enough, \"clang-tidy-diff.py --fix\" should be all that is needed\n\nI believe clang-tidy is different from clang-format am I mistaken?\n\n-- \nBrandon Williams\n"},{"id":"326160","messageId":"xmqqpoc2ylou.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"20170811174958.GB59325@google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-11T19:00:01Z","receivedAt":"2017-08-11T19:00:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n>> checkpatch.pl also warns a SP immediately before HT, which I do pay\n>\n> I'm sorry what's 'HT'?\n\nHorizontal tab.\n\n\"man ascii\" or http://man7.org/linux/man-pages/man7/ascii.7.html\n"},{"id":"326164","messageId":"0378b54f-7332-7a39-23b1-40886c97c596@gmail.com","threadId":"46536","inReplyTo":"20170810213013.GD73298@google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-08-11T20:06:54Z","receivedAt":"2017-08-11T20:07:02Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 8/10/2017 5:30 PM, Brandon Williams wrote:\n> On 08/10, Junio C Hamano wrote:\n>> Brandon Williams writes:\n>>\n>>> On 08/10, Junio C Hamano wrote:\n>>>\n>>>> I vaguely recall that there was a discussion to have SubmitGit wait\n>>>> for success from Travis CI; if that is already in place, then I can\n>>>> sort of see how it would help individual contributors to have the\n>>>> style checker in that pipeline as well.\n>>>>\n>>>> I have a mixed feelings about \"fixing\" styles automatically, though.\n>>>\n>>> I still think we are far away from a world where we can fix style\n>>> automatically. If we do want to keep pursuing this there are a number\n>>> steps we'd want to take first.\n>>>\n>>> 1. Settle on a concrete style and document it using a formatter's rules\n>>> (in say a .clang-format file). This style would most likely need to\n>>> be tuned a little bit, at least the 'Penalty' configuration would\n>>> need to be tuned which (as far as I understand it) is used to\n>>> determine which rule to break first to ensure a line isn't too long.\n>>\n>> Yes. I think this is what you started to get the ball rolling.\n>> Together with what checkpatch.pl already diagnoses, I think we can\n>> get a guideline that is more or less reasonable.\n>>\n>>> 2. Start getting contributors to use the tool to format their patches.\n>>> This would include having some script or hook that a contributor\n>>> could run to only format the sections of code that they touched.\n>>\n>> This, too. Running checkpatch.pl (possibly combined with a bit of\n>> tweaking it to match our needs) already catches many of the issues,\n>> so a tool with a similar interface would be easy to use, I would\n>> imagine.\n>>\n>>> 3. Slowly the code base would begin to have a uniform style. At\n>>> some point we may want to then reformat the remaining sections of the\n>>> code base. At this point we could have some automated bot that fixes\n>>> style.\n>>\n>> I suspect I am discussing this based on a different assumption.\n>>\n>> I think the primary goal of this effort is to make it easier to\n>> cleanse the new patches that appear on the list of trivial style\n>> issues, so that contributors and reviewers do not have to spend\n>> bandwidth and brain cycles during the review. And I have been\n>> assuming that we can do so even without waiting for a \"tree wide\"\n>> code churn on existing code to complete.\n> \n> Yes that's one of the steps I missed we can call it 2.5 ;) (3) could be\n> a long term goal which is what I was trying to get at by saying:\n> \n>>> 3. Slowly the code base would begin to have a uniform style.\n> \n\nJust adding my \"Yes!\" vote.\n\nConsistent formatting makes the code easier to read and maintain. I've \nbeen in religious wars debating whether the opening brace should be on \nthe same line or the next line and while I have my personal preferences, \nI can work with just about anything as long as it is consistent. When it \ncomes to dealing with a tool, I am willing to live with some \"I would \nhave wrapped that differently\" if I can stop spending so much time \nmanually wrapping code.\n\nI think the goals should be to 1) increase readability of the code 2) \nreduce the time spent by reviewers and 3) reduce the time spent by \ncontributors. A pre-commit hook that checked for errors (and gave the \ninstructions on how to automatically have the tool correct them) would \nbe very helpful.\n\nPlease continue to push this forward!\n"},{"id":"326169","messageId":"20170811211845.tpgmafenhahus77o@sigill.intra.peff.net","threadId":"46536","inReplyTo":"20170811175237.GC59325@google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-11T21:18:45Z","receivedAt":"2017-08-11T21:18:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 11, 2017 at 10:52:37AM -0700, Brandon Williams wrote:\n\n> On 08/09, Stefan Beller wrote:\n> > On Wed, Aug 9, 2017 at 3:53 PM, Stefan Beller wrote:\n> > \n> > > I would think based on these options, a pre commit hook can be\n> > > written that formats precisely the touched lines of code of each file.\n> > \n> > I did not search enough, \"clang-tidy-diff.py --fix\" should be all that is needed\n> \n> I believe clang-tidy is different from clang-format am I mistaken?\n\nYeah, I just dug in the archive. The script I ran way back when was\nactually clang-format-diff.\n\n-Peff\n"},{"id":"326187","messageId":"xmqq378xxuvk.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"20170811211845.tpgmafenhahus77o@sigill.intra.peff.net","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-12T04:39:11Z","receivedAt":"2017-08-12T04:39:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Aug 11, 2017 at 10:52:37AM -0700, Brandon Williams wrote:\n>\n>> On 08/09, Stefan Beller wrote:\n>> > On Wed, Aug 9, 2017 at 3:53 PM, Stefan Beller wrote:\n>> > \n>> > > I would think based on these options, a pre commit hook can be\n>> > > written that formats precisely the touched lines of code of each file.\n>> > \n>> > I did not search enough, \"clang-tidy-diff.py --fix\" should be all that is needed\n>> \n>> I believe clang-tidy is different from clang-format am I mistaken?\n>\n> Yeah, I just dug in the archive. The script I ran way back when was\n> actually clang-format-diff.\n\nI am confident with the competence of people around here that we can\ncome up with a reasonable checker for obvious style violations. In\nthe worst case, we could customize and/or tweak checkpatch.pl and\nstart from there.\n\nAssuming that we can have such a checker, I am more interested in\nthe way how people envision such a checker fits in our workflow to\nhelp people. Earlier Dscho floated an idea to integrate with the\nGitHub pull requests in a way similar to how Travis and SubmitGit\nare triggered, and I can sort of see how it may help, but I haven't\nseen ideas from others.\n"},{"id":"326231","messageId":"20170813044145.xz4o47oog3z5eycg@sigill.intra.peff.net","threadId":"46536","inReplyTo":"xmqq378xxuvk.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-13T04:41:45Z","receivedAt":"2017-08-13T04:41:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 11, 2017 at 09:39:11PM -0700, Junio C Hamano wrote:\n\n> > Yeah, I just dug in the archive. The script I ran way back when was\n> > actually clang-format-diff.\n> \n> I am confident with the competence of people around here that we can\n> come up with a reasonable checker for obvious style violations. In\n> the worst case, we could customize and/or tweak checkpatch.pl and\n> start from there.\n\nI am confident we _can_, too. My question is whether we will. :)\n\n> Assuming that we can have such a checker, I am more interested in\n> the way how people envision such a checker fits in our workflow to\n> help people. Earlier Dscho floated an idea to integrate with the\n> GitHub pull requests in a way similar to how Travis and SubmitGit\n> are triggered, and I can sort of see how it may help, but I haven't\n> seen ideas from others.\n\nYeah, I agree. I assume most people already run \"make test\" locally. I'd\nbe happy enough if we started with a \"make style\" that offers style\nsuggestions for you to accept. From there we can grow into\n\"automatically apply suggestions\" and integrating with things like\nsubmitGit.\n\n-Peff\n"},{"id":"326240","messageId":"26bebb5b-857f-1501-5139-a5513d9875ec@ramsayjones.plus.com","threadId":"46536","inReplyTo":"20170813044145.xz4o47oog3z5eycg@sigill.intra.peff.net","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-08-13T16:14:55Z","receivedAt":"2017-08-13T16:15:06Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 13/08/17 05:41, Jeff King wrote:\n> On Fri, Aug 11, 2017 at 09:39:11PM -0700, Junio C Hamano wrote:\n> \n>>> Yeah, I just dug in the archive. The script I ran way back when was\n>>> actually clang-format-diff.\n>>\n>> I am confident with the competence of people around here that we can\n>> come up with a reasonable checker for obvious style violations. In\n>> the worst case, we could customize and/or tweak checkpatch.pl and\n>> start from there.\n> \n> I am confident we _can_, too. My question is whether we will. :)\n> \n>> Assuming that we can have such a checker, I am more interested in\n>> the way how people envision such a checker fits in our workflow to\n>> help people. Earlier Dscho floated an idea to integrate with the\n>> GitHub pull requests in a way similar to how Travis and SubmitGit\n>> are triggered, and I can sort of see how it may help, but I haven't\n>> seen ideas from others.\n> \n> Yeah, I agree. I assume most people already run \"make test\" locally. I'd\n> be happy enough if we started with a \"make style\" that offers style\n> suggestions for you to accept. From there we can grow into\n> \"automatically apply suggestions\" and integrating with things like\n> submitGit.\n\nAs a start, how about something like this:\n\n-- >8 --\n$ git diff\ndiff --git a/Makefile b/Makefile\nindex 461c845d3..7555def45 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2440,6 +2440,18 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n .PHONY: sparse $(SP_OBJ)\n sparse: $(SP_OBJ)\n \n+ST_C = $(patsubst %.o,%.stc,$(C_OBJ))\n+ST_H = $(patsubst %.h,%.sth,$(LIB_H))\n+\n+$(ST_C): %.stc: %.c FORCE\n+ checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<\n+\n+$(ST_H): %.sth: %.h FORCE\n+ checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<\n+\n+.PHONY: style $(ST_C) $(ST_H)\n+style: $(ST_C) $(ST_H)\n+\n check: common-cmds.h\n @if sparse; \\\n then \\\n$ \n-- >8 --\n\nTo give it a try:\n\n $ make git.stc # just run checkpatch over git.c\n $ make cache.sth # just run checkpatch over cache.h\n $ make -k style >style.out 2>&1 # yep, large output!\n\nA bit crude, but workable. ;-)\n\nJust FYI, for me:\n\n$ wc -l style.out\n144076 style.out\n$ \n$ grep '^WARNING' style.out | cut -d: -f1,2 | sort | uniq -c\n 2 WARNING:AVOID_EXTERNS\n 495 WARNING:BLOCK_COMMENT_STYLE\n 127 WARNING:BRACES\n 213 WARNING:CONSTANT_COMPARISON\n 12584 WARNING:CONST_STRUCT\n 5 WARNING:CVS_KEYWORD\n 26 WARNING:DEEP_INDENTATION\n 2 WARNING:DEFAULT_NO_BREAK\n 77 WARNING:EMBEDDED_FUNCTION_NAME\n 5 WARNING:ENOSYS\n 773 WARNING:FUNCTION_ARGUMENTS\n 39 WARNING:INDENTED_LABEL\n 2610 WARNING:LEADING_SPACE\n 1 WARNING:LINE_CONTINUATIONS\n 2680 WARNING:LINE_SPACING\n 3975 WARNING:LONG_LINE\n 330 WARNING:LONG_LINE_COMMENT\n 380 WARNING:LONG_LINE_STRING\n 2 WARNING:MACRO_WITH_FLOW_CONTROL\n 1 WARNING:MISORDERED_TYPE\n 17 WARNING:MISSING_SPACE\n 1 WARNING:ONE_SEMICOLON\n 77 WARNING:PREFER_PRINTF\n 7 WARNING:QUOTED_WHITESPACE_BEFORE_NEWLINE\n 10 WARNING:RETURN_VOID\n 5 WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO\n 6 WARNING:SIZEOF_PARENTHESIS\n 61 WARNING:SPACE_BEFORE_TAB\n 347 WARNING:SPACING\n 322 WARNING:SPLIT_STRING\n 87 WARNING:STATIC_CONST_CHAR_ARRAY\n 2 WARNING:STORAGE_CLASS\n 538 WARNING:SUSPECT_CODE_INDENT\n 29 WARNING:SYMBOLIC_PERMS\n 279 WARNING:TABSTOP\n 9 WARNING:TRAILING_SEMICOLON\n 3 WARNING:TYPECAST_INT_CONSTANT\n 2 WARNING:UNNECESSARY_BREAK\n 56 WARNING:UNNECESSARY_ELSE\n 568 WARNING:UNSPECIFIED_INT\n 4 WARNING:USE_NEGATIVE_ERRNO\n 26 WARNING:VOLATILE\n$ \n\nHmm, on reflection, it may be a bit too crude! :-D\n\nATB,\nRamsay Jones\n\n"},{"id":"326241","messageId":"4ded81fc-a495-023f-02e7-2dbd2ec1be68@ramsayjones.plus.com","threadId":"46536","inReplyTo":"26bebb5b-857f-1501-5139-a5513d9875ec@ramsayjones.plus.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-08-13T16:36:36Z","receivedAt":"2017-08-13T16:36:51Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 13/08/17 17:14, Ramsay Jones wrote:\n> \n> \n> On 13/08/17 05:41, Jeff King wrote:\n>> On Fri, Aug 11, 2017 at 09:39:11PM -0700, Junio C Hamano wrote:\n>>\n>>>> Yeah, I just dug in the archive. The script I ran way back when was\n>>>> actually clang-format-diff.\n>>>\n>>> I am confident with the competence of people around here that we can\n>>> come up with a reasonable checker for obvious style violations. In\n>>> the worst case, we could customize and/or tweak checkpatch.pl and\n>>> start from there.\n>>\n>> I am confident we _can_, too. My question is whether we will. :)\n>>\n>>> Assuming that we can have such a checker, I am more interested in\n>>> the way how people envision such a checker fits in our workflow to\n>>> help people. Earlier Dscho floated an idea to integrate with the\n>>> GitHub pull requests in a way similar to how Travis and SubmitGit\n>>> are triggered, and I can sort of see how it may help, but I haven't\n>>> seen ideas from others.\n>>\n>> Yeah, I agree. I assume most people already run \"make test\" locally. I'd\n>> be happy enough if we started with a \"make style\" that offers style\n>> suggestions for you to accept. From there we can grow into\n>> \"automatically apply suggestions\" and integrating with things like\n>> submitGit.\n> \n> As a start, how about something like this:\n> \n> -- >8 --\n> $ git diff\n> diff --git a/Makefile b/Makefile\n> index 461c845d3..7555def45 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2440,6 +2440,18 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n> .PHONY: sparse $(SP_OBJ)\n> sparse: $(SP_OBJ)\n> \n> +ST_C = $(patsubst %.o,%.stc,$(C_OBJ))\n> +ST_H = $(patsubst %.h,%.sth,$(LIB_H))\n> +\n> +$(ST_C): %.stc: %.c FORCE\n> + checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<\n> +\n> +$(ST_H): %.sth: %.h FORCE\n> + checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<\n> +\n> +.PHONY: style $(ST_C) $(ST_H)\n> +style: $(ST_C) $(ST_H)\n> +\n> check: common-cmds.h\n> @if sparse; \\\n> then \\\n> $ \n> -- >8 --\n> \n> To give it a try:\n> \n> $ make git.stc # just run checkpatch over git.c\n> $ make cache.sth # just run checkpatch over cache.h\n> $ make -k style >style.out 2>&1 # yep, large output!\n> \n> A bit crude, but workable. ;-)\n> \n> Just FYI, for me:\n> \n> $ wc -l style.out\n> 144076 style.out\n> $ \n> $ grep '^WARNING' style.out | cut -d: -f1,2 | sort | uniq -c\n> 2 WARNING:AVOID_EXTERNS\n> 495 WARNING:BLOCK_COMMENT_STYLE\n> 127 WARNING:BRACES\n> 213 WARNING:CONSTANT_COMPARISON\n> 12584 WARNING:CONST_STRUCT\n> 5 WARNING:CVS_KEYWORD\n> 26 WARNING:DEEP_INDENTATION\n> 2 WARNING:DEFAULT_NO_BREAK\n> 77 WARNING:EMBEDDED_FUNCTION_NAME\n> 5 WARNING:ENOSYS\n> 773 WARNING:FUNCTION_ARGUMENTS\n> 39 WARNING:INDENTED_LABEL\n> 2610 WARNING:LEADING_SPACE\n> 1 WARNING:LINE_CONTINUATIONS\n> 2680 WARNING:LINE_SPACING\n> 3975 WARNING:LONG_LINE\n> 330 WARNING:LONG_LINE_COMMENT\n> 380 WARNING:LONG_LINE_STRING\n> 2 WARNING:MACRO_WITH_FLOW_CONTROL\n> 1 WARNING:MISORDERED_TYPE\n> 17 WARNING:MISSING_SPACE\n> 1 WARNING:ONE_SEMICOLON\n> 77 WARNING:PREFER_PRINTF\n> 7 WARNING:QUOTED_WHITESPACE_BEFORE_NEWLINE\n> 10 WARNING:RETURN_VOID\n> 5 WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO\n> 6 WARNING:SIZEOF_PARENTHESIS\n> 61 WARNING:SPACE_BEFORE_TAB\n> 347 WARNING:SPACING\n> 322 WARNING:SPLIT_STRING\n> 87 WARNING:STATIC_CONST_CHAR_ARRAY\n> 2 WARNING:STORAGE_CLASS\n> 538 WARNING:SUSPECT_CODE_INDENT\n> 29 WARNING:SYMBOLIC_PERMS\n> 279 WARNING:TABSTOP\n> 9 WARNING:TRAILING_SEMICOLON\n> 3 WARNING:TYPECAST_INT_CONSTANT\n> 2 WARNING:UNNECESSARY_BREAK\n> 56 WARNING:UNNECESSARY_ELSE\n> 568 WARNING:UNSPECIFIED_INT\n> 4 WARNING:USE_NEGATIVE_ERRNO\n> 26 WARNING:VOLATILE\n> $ \n\noops, I forgot this:\n\n$ grep '^ERROR' style.out | cut -d: -f1,2 | sort | uniq -c\n 216 ERROR:ASSIGN_IN_IF\n 46 ERROR:CODE_INDENT\n 41 ERROR:COMPLEX_MACRO\n 317 ERROR:ELSE_AFTER_BRACE\n 3 ERROR:FUNCTION_WITHOUT_ARGS\n 1 ERROR:GLOBAL_INITIALISERS\n 46 ERROR:INITIALISED_STATIC\n 2 ERROR:INLINE_LOCATION\n 5 ERROR:MULTISTATEMENT_MACRO_USE_DO_WHILE\n 305 ERROR:OPEN_BRACE\n 314 ERROR:POINTER_LOCATION\n 5 ERROR:RETURN_PARENTHESES\n 3464 ERROR:SPACING\n 7 ERROR:SWITCH_CASE_INDENT_LEVEL\n 326 ERROR:TRAILING_STATEMENTS\n 15 ERROR:TRAILING_WHITESPACE\n$ \n\n> Hmm, on reflection, it may be a bit too crude! :-D\n> \n> ATB,\n> Ramsay Jones\n> \n> \n"},{"id":"326242","messageId":"xmqqy3qnwexy.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"26bebb5b-857f-1501-5139-a5513d9875ec@ramsayjones.plus.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-13T17:33:13Z","receivedAt":"2017-08-13T17:33:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> As a start, how about something like this:\n>\n> -- >8 --\n> $ git diff\n> diff --git a/Makefile b/Makefile\n> index 461c845d3..7555def45 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2440,6 +2440,18 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n> .PHONY: sparse $(SP_OBJ)\n> sparse: $(SP_OBJ)\n> \n> +ST_C = $(patsubst %.o,%.stc,$(C_OBJ))\n> +ST_H = $(patsubst %.h,%.sth,$(LIB_H))\n> +\n> +$(ST_C): %.stc: %.c FORCE\n> + checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<\n> +\n> +$(ST_H): %.sth: %.h FORCE\n> + checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS,INLINE -f $<\n> +\n> +.PHONY: style $(ST_C) $(ST_H)\n> +style: $(ST_C) $(ST_H)\n> +\n> check: common-cmds.h\n> @if sparse; \\\n> then \\\n> $ \n> -- >8 --\n> ...\n> Hmm, on reflection, it may be a bit too crude! :-D\n\nAs you already saw in the output from this, I think this is a good\nillustration that shows why we want an incremental tool that works\non the changes, not on full file contents. Contributors who want\ntheir changes accepted and want to help the review process by\navoiding trivial coding style violations in their patches should not\nhave to find _their_ piece from an output about the whole file\ncontents, most of which is likely to have been inherited from the\noriginal. They are not working on Git to produce unnecessary code\nchurn whose only purpose is to make existing and otherwise dormant\ncode conform to the style tool's liking. That's not their focus.\n\nIOW I was expecting something that works on the output from \"git\ndiff HEAD\" or \"git format-patch --stdout @{u}..\"\n\nUnless you were somehow envisioning that having a baseline line\nthis, and then take another full dump after their patch and\ncomparing the two, would make a good foundation for that incremental\nchecker, that is. I am not sure if that would be a workable\napproach myself, though.\n\nThanks.\n"},{"id":"326248","messageId":"d58118f7-e9d2-e74f-30be-7d66477b128b@ramsayjones.plus.com","threadId":"46536","inReplyTo":"xmqqy3qnwexy.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-08-13T19:17:04Z","receivedAt":"2017-08-13T19:17:13Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 13/08/17 18:33, Junio C Hamano wrote:\n> Ramsay Jones writes:\n>> Hmm, on reflection, it may be a bit too crude! :-D\n> \n> As you already saw in the output from this, I think this is a good\n> illustration that shows why we want an incremental tool that works\n> on the changes, not on full file contents. Contributors who want\n> their changes accepted and want to help the review process by\n> avoiding trivial coding style violations in their patches should not\n> have to find _their_ piece from an output about the whole file\n> contents, most of which is likely to have been inherited from the\n> original. They are not working on Git to produce unnecessary code\n> churn whose only purpose is to make existing and otherwise dormant\n> code conform to the style tool's liking. That's not their focus.\n> \n> IOW I was expecting something that works on the output from \"git\n> diff HEAD\" or \"git format-patch --stdout @{u}..\"\n\nYes, I had already tried the following, which maybe more workable,\nbut it is only lightly tested. (we may want to create our own version\nof checkpatch.pl, which is written specifically for the kernel ...)\n\n$ git diff\ndiff --git a/Makefile b/Makefile\nindex 461c845d3..a25028e68 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2440,6 +2440,15 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n .PHONY: sparse $(SP_OBJ)\n sparse: $(SP_OBJ)\n \n+STYLE_REVS = HEAD\n+STYLE_IGNORES = NEW_TYPEDEFS,INLINE\n+\n+.PHONY: style\n+style:\n+ @git diff $(STYLE_REVS) -- '*.[ch]' | \\\n+ checkpatch.pl -q --no-tree --show-types \\\n+ --ignore=$(STYLE_IGNORES) --patch - 2>/dev/null || true\n+\n check: common-cmds.h\n @if sparse; \\\n then \\\n$ make style # I don't have any changes to *.[ch] files!\n$ make STYLE_REVS=HEAD~3 style\nWARNING:LONG_LINE: line over 80 characters\n#181: FILE: git.c:320:\n+\t\tif (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&\n\nWARNING:LONG_LINE: line over 80 characters\n#221: FILE: revision.c:2317:\n+\tif (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {\n\ntotal: 0 errors, 2 warnings, 206 lines checked\n$ \n\nI suspect this closer to what you had in mind. ;-)\n(although the --ignore list may need adding to).\n\nATB,\nRamsay Jones\n\n\n"},{"id":"326271","messageId":"alpine.DEB.2.21.1.1708141751590.19382@virtualbox","threadId":"46536","inReplyTo":"xmqqk22b5q9m.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-08-14T15:52:39Z","receivedAt":"2017-08-14T15:52:59Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 10 Aug 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Wed, 9 Aug 2017, Stefan Beller wrote:\n> >\n> >> > I am sure that something even better will be possible: a Continuous\n> >> > \"Integration\" that fixes the coding style automatically by using\n> >> > `filter-branch` (avoiding the merge conflicts that would arise if\n> >> > `rebase -i` was used).\n> >> \n> >> I do not quite follow. Is that to be used by Junio while integrating\n> >> branches?\n> >\n> > I was more thinking about a bot on GitHub. \"Code cleanup as a service\".\n> \n> I vaguely recall that there was a discussion to have SubmitGit wait\n> for success from Travis CI; if that is already in place, then I can\n> sort of see how it would help individual contributors to have the\n> style checker in that pipeline as well. \n> \n> I have a mixed feelings about \"fixing\" styles automatically, though.\n\nThat's too bad. I would much rather focus on quality of code than\nconformance of style, even if the latter is a lot easier than the former.\n\nCiao,\nDscho\n"},{"id":"326312","messageId":"20170814213046.107576-1-bmwill@google.com","threadId":"46536","inReplyTo":"20170808012554.186051-1-bmwill@google.com","subject":"[PATCH v2 0/2] clang-format","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-14T21:30:44Z","receivedAt":"2017-08-14T21:30:59Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Changes in v2:\n * Changed a couple rules to be more inline with our coding style.\n * Added a Makefile build rule to run git-clang-format on the diff of the\n working tree to suggest style changes.\n\nI found that the llvm project also has the git-clang-format tool which will\nallow for doing formating changes based on diffs so that the parts of the code\nyou didn't touch won't be formated. It also has a nice '-p' option to only\napply hunks of suggested changes. I also saw what Ramsay did and attempted to\nadd a Makefile rule to run clang-format. This bit may need more tweaking to\nget it right.\n\nBrandon Williams (2):\n clang-format: outline the git project's coding style\n Makefile: add style build rule\n\n .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n Makefile | 4 ++\n 2 files changed, 169 insertions(+)\n create mode 100644 .clang-format\n\n-- \n2.14.1.480.gb18f417b89-goog\n\n"},{"id":"326313","messageId":"20170814213046.107576-3-bmwill@google.com","threadId":"46536","inReplyTo":"20170814213046.107576-1-bmwill@google.com","subject":"[PATCH v2 2/2] Makefile: add style build rule","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-14T21:30:46Z","receivedAt":"2017-08-14T21:31:02Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Add the 'style' build rule which will run git-clang-format on the diff\nbetween HEAD and the current worktree. The result is a diff of\nsuggested changes.\n\nSigned-off-by: Brandon Williams \n---\n Makefile | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex ba4359ef8..acfd096b7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2414,6 +2414,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n .PHONY: sparse $(SP_OBJ)\n sparse: $(SP_OBJ)\n \n+.PHONY: style\n+style:\n+\tgit clang-format --style file --diff --extensions c,h\n+\n check: common-cmds.h\n \t@if sparse; \\\n \tthen \\\n-- \n2.14.1.480.gb18f417b89-goog\n\n"},{"id":"326314","messageId":"20170814213046.107576-2-bmwill@google.com","threadId":"46536","inReplyTo":"20170814213046.107576-1-bmwill@google.com","subject":"[PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-14T21:30:45Z","receivedAt":"2017-08-14T21:31:05Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Add a '.clang-format' file which outlines the git project's coding\nstyle. This can be used with clang-format to auto-format .c and .h\nfiles to conform with git's style.\n\nSigned-off-by: Brandon Williams \n---\n .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 165 insertions(+)\n create mode 100644 .clang-format\n\ndiff --git a/.clang-format b/.clang-format\nnew file mode 100644\nindex 000000000..3ede2628d\n--- /dev/null\n+++ b/.clang-format\n@@ -0,0 +1,165 @@\n+# Defaults\n+\n+# Use tabs whenever we need to fill whitespace that spans at least from one tab\n+# stop to the next one.\n+UseTab: Always\n+TabWidth: 8\n+IndentWidth: 8\n+ContinuationIndentWidth: 8\n+ColumnLimit: 80\n+\n+# C Language specifics\n+Language: Cpp\n+\n+# Align parameters on the open bracket\n+# someLongFunction(argument1,\n+# argument2);\n+AlignAfterOpenBracket: Align\n+\n+# Don't align consecutive assignments\n+# int aaaa = 12;\n+# int b = 14;\n+AlignConsecutiveAssignments: false\n+\n+# Don't align consecutive declarations\n+# int aaaa = 12;\n+# double b = 3.14;\n+AlignConsecutiveDeclarations: false\n+\n+# Align escaped newlines as far left as possible\n+# #define A \\\n+# int aaaa; \\\n+# int b; \\\n+# int cccccccc;\n+AlignEscapedNewlines: Left\n+\n+# Align operands of binary and ternary expressions\n+# int aaa = bbbbbbbbbbb +\n+# cccccc;\n+AlignOperands: true\n+\n+# Don't align trailing comments\n+# int a; // Comment a\n+# int b = 2; // Comment b\n+AlignTrailingComments: false\n+\n+# By default don't allow putting parameters onto the next line\n+# myFunction(foo, bar, baz);\n+AllowAllParametersOfDeclarationOnNextLine: false\n+\n+# Don't allow short braced statements to be on a single line\n+# if (a) not if (a) return;\n+# return;\n+AllowShortBlocksOnASingleLine: false\n+AllowShortCaseLabelsOnASingleLine: false\n+AllowShortFunctionsOnASingleLine: false\n+AllowShortIfStatementsOnASingleLine: false\n+AllowShortLoopsOnASingleLine: false\n+\n+# By default don't add a line break after the return type of top-level functions\n+# int foo();\n+AlwaysBreakAfterReturnType: None\n+\n+# Pack as many parameters or arguments onto the same line as possible\n+# int myFunction(int aaaaaaaaaaaa, int bbbbbbbb,\n+# int cccc);\n+BinPackArguments: true\n+BinPackParameters: true\n+\n+# Attach braces to surrounding context except break before braces on function\n+# definitions.\n+# void foo()\n+# {\n+# if (true) {\n+# } else {\n+# }\n+# };\n+BreakBeforeBraces: Linux\n+\n+# Break after operators\n+# int valuve = aaaaaaaaaaaaa +\n+# bbbbbb -\n+# ccccccccccc;\n+BreakBeforeBinaryOperators: None\n+BreakBeforeTernaryOperators: false\n+\n+# Don't break string literals\n+BreakStringLiterals: false\n+\n+# Use the same indentation level as for the switch statement.\n+# Switch statement body is always indented one level more than case labels.\n+IndentCaseLabels: false\n+\n+# Don't indent a function definition or declaration if it is wrapped after the\n+# type\n+IndentWrappedFunctionNames: false\n+\n+# Align pointer to the right\n+# int *a;\n+PointerAlignment: Right\n+\n+# Don't insert a space after a cast\n+# x = (int32)y; not x = (int32) y;\n+SpaceAfterCStyleCast: false\n+\n+# Insert spaces before and after assignment operators\n+# int a = 5; not int a=5;\n+# a += 42; a+=42;\n+SpaceBeforeAssignmentOperators: true\n+\n+# Put a space before opening parentheses only after control statement keywords.\n+# void f() {\n+# if (true) {\n+# f();\n+# }\n+# }\n+SpaceBeforeParens: ControlStatements\n+\n+# Don't insert spaces inside empty '()'\n+SpaceInEmptyParentheses: false\n+\n+# The number of spaces before trailing line comments (// - comments).\n+# This does not affect trailing block comments (/* - comments).\n+SpacesBeforeTrailingComments: 1\n+\n+# Don't insert spaces in casts\n+# x = (int32) y; not x = ( int32 ) y;\n+SpacesInCStyleCastParentheses: false\n+\n+# Don't insert spaces inside container literals\n+# var arr = [1, 2, 3]; not var arr = [ 1, 2, 3 ];\n+SpacesInContainerLiterals: false\n+\n+# Don't insert spaces after '(' or before ')'\n+# f(arg); not f( arg );\n+SpacesInParentheses: false\n+\n+# Don't insert spaces after '[' or before ']'\n+# int a[5]; not int a[ 5 ];\n+SpacesInSquareBrackets: false\n+\n+# Insert a space after '{' and before '}' in struct initializers\n+Cpp11BracedListStyle: false\n+\n+# A list of macros that should be interpreted as foreach loops instead of as\n+# function calls.\n+ForEachMacros: ['for_each_string_list_item']\n+\n+# The maximum number of consecutive empty lines to keep.\n+MaxEmptyLinesToKeep: 1\n+\n+# No empty line at the start of a block.\n+KeepEmptyLinesAtTheStartOfBlocks: false\n+\n+# Penalties\n+# This decides what order things should be done if a line is too long\n+PenaltyBreakAssignment: 100\n+PenaltyBreakBeforeFirstCallParameter: 100\n+PenaltyBreakComment: 100\n+PenaltyBreakFirstLessLess: 0\n+PenaltyBreakString: 100\n+PenaltyExcessCharacter: 5\n+PenaltyReturnTypeOnItsOwnLine: 0\n+\n+# Don't sort #include's\n+SortIncludes: false\n-- \n2.14.1.480.gb18f417b89-goog\n\n"},{"id":"326319","messageId":"20170814213251.GA95777@google.com","threadId":"46536","inReplyTo":"20170814213046.107576-1-bmwill@google.com","subject":"Re: [PATCH v2 0/2] clang-format","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-14T21:32:51Z","receivedAt":"2017-08-14T21:32:58Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/14, Brandon Williams wrote:\n> Changes in v2:\n> * Changed a couple rules to be more inline with our coding style.\n> * Added a Makefile build rule to run git-clang-format on the diff of the\n> working tree to suggest style changes.\n> \n> I found that the llvm project also has the git-clang-format tool which will\n> allow for doing formating changes based on diffs so that the parts of the code\n> you didn't touch won't be formated. It also has a nice '-p' option to only\n> apply hunks of suggested changes. I also saw what Ramsay did and attempted to\n> add a Makefile rule to run clang-format. This bit may need more tweaking to\n> get it right.\n\nForgot to include the [RFC] bit because from the discussion it seems\nlike we still have a lot to talk about before we decide that this is the\npath we are taking.\n\n> \n> Brandon Williams (2):\n> clang-format: outline the git project's coding style\n> Makefile: add style build rule\n> \n> .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> Makefile | 4 ++\n> 2 files changed, 169 insertions(+)\n> create mode 100644 .clang-format\n> \n> -- \n> 2.14.1.480.gb18f417b89-goog\n> \n\n-- \nBrandon Williams\n"},{"id":"326322","messageId":"CAGZ79kbv-zdCiShVOdpJsX1OEqUayMVq3zUYcfxJVA+JoFw3oA@mail.gmail.com","threadId":"46536","inReplyTo":"20170814213046.107576-3-bmwill@google.com","subject":"Re: [PATCH v2 2/2] Makefile: add style build rule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-14T21:53:05Z","receivedAt":"2017-08-14T21:53:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 14, 2017 at 2:30 PM, Brandon Williams wrote:\n> Add the 'style' build rule which will run git-clang-format on the diff\n> between HEAD and the current worktree. The result is a diff of\n> suggested changes.\n\nNotes from in-office discussion:\n\n* 'git clang-format --style file -f --extensions c,h'\n to apply suggested changes. (Useful for contributors,\n maybe even as a precommit hook)\n* you can also give a range of commits to git clang-format, as\n git clang-format --diff origin/master..HEAD to see if other\n local commits need tweaking.\n\n\n>\n> Signed-off-by: Brandon Williams \n> ---\n> Makefile | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/Makefile b/Makefile\n> index ba4359ef8..acfd096b7 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2414,6 +2414,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n> .PHONY: sparse $(SP_OBJ)\n> sparse: $(SP_OBJ)\n>\n> +.PHONY: style\n> +style:\n> + git clang-format --style file --diff --extensions c,h\n> +\n> check: common-cmds.h\n> @if sparse; \\\n> then \\\n> --\n> 2.14.1.480.gb18f417b89-goog\n>\n"},{"id":"326324","messageId":"CAGZ79kau6_XeEQqYDhFC2FmyJiqWY2+SuRzvGFrfmLdhAaQS+Q@mail.gmail.com","threadId":"46536","inReplyTo":"20170814213046.107576-2-bmwill@google.com","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-14T22:02:09Z","receivedAt":"2017-08-14T22:02:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 14, 2017 at 2:30 PM, Brandon Williams wrote:\n> Add a '.clang-format' file which outlines the git project's coding\n> style. This can be used with clang-format to auto-format .c and .h\n> files to conform with git's style.\n>\n> Signed-off-by: Brandon Williams \n\nApplying this patch and running\n clang-format -i -style file *.c *.h builtin/*.c\nproduces a diff, that I'd mostly agree with.\nThis style guide is close to our current style.\n\nAs noted in patch 2/2 we'd now need an easy way to\nexpose this for use in various situations, such as\n* contributor wanting to format their patch\n* reformatting code for readability\n\nThanks,\nStefan\n"},{"id":"326330","messageId":"xmqqd17xu6qg.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"20170814213046.107576-3-bmwill@google.com","subject":"Re: [PATCH v2 2/2] Makefile: add style build rule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-14T22:25:43Z","receivedAt":"2017-08-14T22:25:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Add the 'style' build rule which will run git-clang-format on the diff\n> between HEAD and the current worktree. The result is a diff of\n> suggested changes.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> Makefile | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/Makefile b/Makefile\n> index ba4359ef8..acfd096b7 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2414,6 +2414,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n> .PHONY: sparse $(SP_OBJ)\n> sparse: $(SP_OBJ)\n> \n> +.PHONY: style\n> +style:\n> +\tgit clang-format --style file --diff --extensions c,h\n\nDid we get \"git clang-format\" subcommand, or is \"s/git //\" implied\nsomewhere?\n\n> +\n> check: common-cmds.h\n> \t@if sparse; \\\n> \tthen \\\n"},{"id":"326331","messageId":"CAGZ79kbAp0p8-y-UpamU0J65A2P0B9r7KEWM0ZCBv=x4E77sGw@mail.gmail.com","threadId":"46536","inReplyTo":"xmqqd17xu6qg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] Makefile: add style build rule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-14T22:28:50Z","receivedAt":"2017-08-14T22:29:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 14, 2017 at 3:25 PM, Junio C Hamano wrote:\n> Brandon Williams writes:\n>\n>> Add the 'style' build rule which will run git-clang-format on the diff\n>> between HEAD and the current worktree. The result is a diff of\n>> suggested changes.\n>>\n>> Signed-off-by: Brandon Williams \n>> ---\n>> Makefile | 4 ++++\n>> 1 file changed, 4 insertions(+)\n>>\n>> diff --git a/Makefile b/Makefile\n>> index ba4359ef8..acfd096b7 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -2414,6 +2414,10 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n>> .PHONY: sparse $(SP_OBJ)\n>> sparse: $(SP_OBJ)\n>>\n>> +.PHONY: style\n>> +style:\n>> + git clang-format --style file --diff --extensions c,h\n>\n> Did we get \"git clang-format\" subcommand, or is \"s/git //\" implied\n> somewhere?\n\nYou need to have clang-format installed (debian/Ubuntu package, or\nhowever it is named in your distribution), which provides this command\nfor us.\n"},{"id":"326335","messageId":"20170814224831.qhs7b44bg7v7tryx@sigill.intra.peff.net","threadId":"46536","inReplyTo":"20170814213046.107576-2-bmwill@google.com","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-14T22:48:31Z","receivedAt":"2017-08-14T22:48:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 02:30:45PM -0700, Brandon Williams wrote:\n\n> +# Align escaped newlines as far left as possible\n> +# #define A \\\n> +# int aaaa; \\\n> +# int b; \\\n> +# int cccccccc;\n> +AlignEscapedNewlines: Left\n\nI get:\n\n $ git clang-format-5.0 --style file -p --extensions c,h\n YAML:34:23: error: unknown key 'AlignEscapedNewlines'\n AlignEscapedNewlines: Left\n ^~~~\n Error reading /home/peff/compile/git/.clang-format: Invalid argument\n\nSame with clang-format-3.8.\n\n-Peff\n"},{"id":"326336","messageId":"20170814225108.qoq6noojqznp4be6@sigill.intra.peff.net","threadId":"46536","inReplyTo":"20170814224831.qhs7b44bg7v7tryx@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-14T22:51:08Z","receivedAt":"2017-08-14T22:51:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 06:48:31PM -0400, Jeff King wrote:\n\n> On Mon, Aug 14, 2017 at 02:30:45PM -0700, Brandon Williams wrote:\n> \n> > +# Align escaped newlines as far left as possible\n> > +# #define A \\\n> > +# int aaaa; \\\n> > +# int b; \\\n> > +# int cccccccc;\n> > +AlignEscapedNewlines: Left\n> \n> I get:\n> \n> $ git clang-format-5.0 --style file -p --extensions c,h\n> YAML:34:23: error: unknown key 'AlignEscapedNewlines'\n> AlignEscapedNewlines: Left\n> ^~~~\n> Error reading /home/peff/compile/git/.clang-format: Invalid argument\n> \n> Same with clang-format-3.8.\n\nAnd if I remove that line, I get:\n\n YAML:155:25: error: unknown key 'PenaltyBreakAssignment'\n PenaltyBreakAssignment: 100\n\nRemoving that gives:\n\n YAML:86:22: error: unknown key 'BreakStringLiterals'\n BreakStringLiterals: false\n\nAnd removing that gives me a clean output. I have no idea why my clang\ndoesn't like these (but presumably yours does). It's clang-format-5.0 in\nDebian unstable (and clang-format-3.8, etc).\n\n-Peff\n"},{"id":"326338","messageId":"20170814225430.GB95777@google.com","threadId":"46536","inReplyTo":"20170814225108.qoq6noojqznp4be6@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-14T22:54:30Z","receivedAt":"2017-08-14T22:54:42Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/14, Jeff King wrote:\n> On Mon, Aug 14, 2017 at 06:48:31PM -0400, Jeff King wrote:\n> \n> > On Mon, Aug 14, 2017 at 02:30:45PM -0700, Brandon Williams wrote:\n> > \n> > > +# Align escaped newlines as far left as possible\n> > > +# #define A \\\n> > > +# int aaaa; \\\n> > > +# int b; \\\n> > > +# int cccccccc;\n> > > +AlignEscapedNewlines: Left\n> > \n> > I get:\n> > \n> > $ git clang-format-5.0 --style file -p --extensions c,h\n> > YAML:34:23: error: unknown key 'AlignEscapedNewlines'\n> > AlignEscapedNewlines: Left\n> > ^~~~\n> > Error reading /home/peff/compile/git/.clang-format: Invalid argument\n> > \n> > Same with clang-format-3.8.\n> \n> And if I remove that line, I get:\n> \n> YAML:155:25: error: unknown key 'PenaltyBreakAssignment'\n> PenaltyBreakAssignment: 100\n> \n> Removing that gives:\n> \n> YAML:86:22: error: unknown key 'BreakStringLiterals'\n> BreakStringLiterals: false\n> \n> And removing that gives me a clean output. I have no idea why my clang\n> doesn't like these (but presumably yours does). It's clang-format-5.0 in\n> Debian unstable (and clang-format-3.8, etc).\n> \n> -Peff\n\nThose must be features in version 6 (which is what I seem to have\ninstalled on my machine).\n\n-- \nBrandon Williams\n"},{"id":"326339","messageId":"20170814225716.p5airw6j5o6dszbn@sigill.intra.peff.net","threadId":"46536","inReplyTo":"CAGZ79kbAp0p8-y-UpamU0J65A2P0B9r7KEWM0ZCBv=x4E77sGw@mail.gmail.com","subject":"Re: [PATCH v2 2/2] Makefile: add style build rule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-14T22:57:16Z","receivedAt":"2017-08-14T22:57:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 03:28:50PM -0700, Stefan Beller wrote:\n\n> >> +.PHONY: style\n> >> +style:\n> >> + git clang-format --style file --diff --extensions c,h\n> >\n> > Did we get \"git clang-format\" subcommand, or is \"s/git //\" implied\n> > somewhere?\n> \n> You need to have clang-format installed (debian/Ubuntu package, or\n> however it is named in your distribution), which provides this command\n> for us.\n\nSadly it is called git-clang-format-3.8, git-clang-format-5.0, etc, in\nthe Debian packages. I think we need a CLANG_FORMAT variable that can be\noverridden.\n\nI am surprised that there's no base \"git-clang-format\" symlink for the\ndefault version. There is for \"clang-format\" and \"clang\" themselves. So\narguably this is a bug in the Debian packaging.\n\nI suspect the \"-p\" version is going to be the one people invoke the most\noften. Should it take the coveted \"make style\" slot, and the diff get\npushed off to another target?\n\nI was also confused at first that the \"-p\" version requires you to stage\nthe changes first. I don't know if we can make that less confusing via a\n\"make style\". Or if it's just something people would get used to. But\nsadly it makes the command not-quite orthogonal to \"make test\" in the\nworkflow. You can't \"make style && make test && git add -p\". You have\nto add first, then check style, then you'd want to test that result to\nmake sure it didn't change the meaning of the code.\n\n-Peff\n"},{"id":"326340","messageId":"20170814230108.wucznh66vfblrsys@sigill.intra.peff.net","threadId":"46536","inReplyTo":"20170814225430.GB95777@google.com","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-14T23:01:08Z","receivedAt":"2017-08-14T23:01:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 03:54:30PM -0700, Brandon Williams wrote:\n\n> > And removing that gives me a clean output. I have no idea why my clang\n> > doesn't like these (but presumably yours does). It's clang-format-5.0 in\n> > Debian unstable (and clang-format-3.8, etc).\n> \n> Those must be features in version 6 (which is what I seem to have\n> installed on my machine).\n\nOK, that makes sense. The most recent one package for Debian unstable is\n5.0. AFAICT 5.0 is actually in release freeze for another week or two,\nand 6 is just bleeding-edge that moved on after the release freeze a few\nweeks ago.\n\nI'm not sure which version it makes sense to target as a minimum, but\nprobably not 6 yet. :)\n\n-Peff\n"},{"id":"326341","messageId":"20170814230657.bmn7exoqmqw3tlyh@sigill.intra.peff.net","threadId":"46536","inReplyTo":"20170814213046.107576-1-bmwill@google.com","subject":"Re: [PATCH v2 0/2] clang-format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-14T23:06:57Z","receivedAt":"2017-08-14T23:07:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 02:30:44PM -0700, Brandon Williams wrote:\n\n> Changes in v2:\n> * Changed a couple rules to be more inline with our coding style.\n> * Added a Makefile build rule to run git-clang-format on the diff of the\n> working tree to suggest style changes.\n> \n> I found that the llvm project also has the git-clang-format tool which will\n> allow for doing formating changes based on diffs so that the parts of the code\n> you didn't touch won't be formated. It also has a nice '-p' option to only\n> apply hunks of suggested changes. I also saw what Ramsay did and attempted to\n> add a Makefile rule to run clang-format. This bit may need more tweaking to\n> get it right.\n\nOne more oddity I found while playing with this that Git folks might run\ninto:\n\n $ git init tmp && cd tmp\n $ git commit --allow-empty -m foo\n $ echo \"[mysection]mykey\" >>.git/config\n $ git clang-format-5.0\n Traceback (most recent call last):\n File \"/usr/bin/git-clang-format-5.0\", line 579, in \n main()\n File \"/usr/bin/git-clang-format-5.0\", line 62, in main\n config = load_git_config()\n File \"/usr/bin/git-clang-format-5.0\", line 194, in load_git_config\n name, value = entry.split('\\n', 1)\n ValueError: need more than 1 value to unpack\n\n $ sed -i 's/mykey/&=true/' .git/config\n $ git clang-format-5.0\n no modified files to format\n\nSo it looks like they do their own config parsing and it's not quite\ncompatible. :(\n\nThat's not the end of the world, and something we can try to fix\nupstream. I just wanted to mention it here so other people don't waste\ntime trying to track down the problem.\n\n-Peff\n"},{"id":"326343","messageId":"CAGZ79kZgRTzWiufZUE02YYOJpH66x_hShBfcv=4RWDn-qR2LUg@mail.gmail.com","threadId":"46536","inReplyTo":"20170814230657.bmn7exoqmqw3tlyh@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/2] clang-format","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-14T23:15:40Z","receivedAt":"2017-08-14T23:15:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+ llvm-dev@lists.llvm.org\n\nThe Git community is currently discussing adopting a coding style\ndefined by clang-format, here is a bug report:\n\nOn Mon, Aug 14, 2017 at 4:06 PM, Jeff King wrote:\n>\n> One more oddity I found while playing with this that Git folks might run\n> into:\n>\n> $ git init tmp && cd tmp\n> $ git commit --allow-empty -m foo\n> $ echo \"[mysection]mykey\" >>.git/config\n> $ git clang-format-5.0\n> Traceback (most recent call last):\n> File \"/usr/bin/git-clang-format-5.0\", line 579, in \n> main()\n> File \"/usr/bin/git-clang-format-5.0\", line 62, in main\n> config = load_git_config()\n> File \"/usr/bin/git-clang-format-5.0\", line 194, in load_git_config\n> name, value = entry.split('\\n', 1)\n> ValueError: need more than 1 value to unpack\n>\n> $ sed -i 's/mykey/&=true/' .git/config\n> $ git clang-format-5.0\n> no modified files to format\n>\n> So it looks like they do their own config parsing and it's not quite\n> compatible. :(\n>\n> That's not the end of the world, and something we can try to fix\n> upstream. I just wanted to mention it here so other people don't waste\n> time trying to track down the problem.\n>\n> -Peff\n"},{"id":"326345","messageId":"xmqqzib1sp6z.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"20170814225716.p5airw6j5o6dszbn@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] Makefile: add style build rule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-14T23:29:56Z","receivedAt":"2017-08-14T23:30:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I suspect the \"-p\" version is going to be the one people invoke the most\n> often. Should it take the coveted \"make style\" slot, and the diff get\n> pushed off to another target?\n>\n> I was also confused at first that the \"-p\" version requires you to stage\n> the changes first. I don't know if we can make that less confusing via a\n> \"make style\". Or if it's just something people would get used to. But\n> sadly it makes the command not-quite orthogonal to \"make test\" in the\n> workflow. You can't \"make style && make test && git add -p\". You have\n> to add first, then check style, then you'd want to test that result to\n> make sure it didn't change the meaning of the code.\n\nPerhaps.\n\nBy the way, I do not know which vintage of /usr/bin/git-clang-format\nI happen to have on my box, but I needed a crude workaround patch\n(attached at the end) to get it even run. The first thing it does\nis to call load_git_config() and it barfs because I have boolean\nconfiguration variables set to true in the correct way, which it\ndoes not seem to recognise.\n\nAs to what it does, the first example I tried may not have been a\ngreat one. I got this:\n\n git clang-format --style file --diff --extensions c,h\n diff --git a/cache.h b/cache.h\n index 73e0085186..6462fe25bc 100644\n --- a/cache.h\n +++ b/cache.h\n @@ -1498,11 +1498,8 @@ struct checkout {\n const char *base_dir;\n int base_dir_len;\n struct delayed_checkout *delayed_checkout;\n -\tunsigned force:1,\n -\t\t quiet:1,\n -\t\t not_new:1,\n -\t\ta_new_field:1,\n -\t\t refresh_cache:1;\n +\tunsigned force : 1, quiet : 1, not_new : 1, a_new_field : 1,\n +\t\trefresh_cache : 1;\n };\n #define CHECKOUT_INIT { NULL, \"\" }\n \nwhich is not wrong per-se, but I have a mixed feelings. I do not\nwant it to complain if the original tried to fit many items on a\nsingle line, but if the original wanted to have one item per line,\nI'd rather see it kept as-is.\n\nAnyway, we cannot have perfect checker from the day one, and\nconsidering this is an initial attempt, I'd say it is a good start.\n\nThanks.\n\ndiff --git a/git-clang-format b/git-clang-format\nindex 60cd4fb25b..e8429b2750 100755\n--- a/usr/bin/git-clang-format\n+++ b/usr/local/google/home/jch/g/Ubuntu-14.04-x86_64/gitstuff/bin/git-clang-format\n@@ -191,10 +191,13 @@ def load_git_config(non_string_options=None):\n out = {}\n for entry in run('git', 'config', '--list', '--null').split('\\0'):\n if entry:\n- name, value = entry.split('\\n', 1)\n- if name in non_string_options:\n- value = run('git', 'config', non_string_options[name], name)\n- out[name] = value\n+ if '\\n' in entry:\n+ name, value = entry.split('\\n', 1)\n+ if name in non_string_options:\n+ value = run('git', 'config', non_string_options[name], name)\n+ out[name] = value\n+ else:\n+\tout[entry] = \"true\";\n return out\n \n \n"},{"id":"326346","messageId":"xmqqvalpsoda.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"xmqqzib1sp6z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] Makefile: add style build rule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-14T23:47:45Z","receivedAt":"2017-08-14T23:47:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> By the way, I do not know which vintage of /usr/bin/git-clang-format\n> I happen to have on my box, but I needed a crude workaround patch\n> (attached at the end) ...\n\nI guess you hit the same thing while our messages crossing ;-)\n\n\n> As to what it does, the first example I tried may not have been a\n> great one. I got this:\n>\n> git clang-format --style file --diff --extensions c,h\n> diff --git a/cache.h b/cache.h\n> index 73e0085186..6462fe25bc 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1498,11 +1498,8 @@ struct checkout {\n> const char *base_dir;\n> int base_dir_len;\n> struct delayed_checkout *delayed_checkout;\n> -\tunsigned force:1,\n> -\t\t quiet:1,\n> -\t\t not_new:1,\n> -\t\ta_new_field:1,\n> -\t\t refresh_cache:1;\n> +\tunsigned force : 1, quiet : 1, not_new : 1, a_new_field : 1,\n> +\t\trefresh_cache : 1;\n> };\n> #define CHECKOUT_INIT { NULL, \"\" }\n> \n> which is not wrong per-se, but I have a mixed feelings. I do not\n> want it to complain if the original tried to fit many items on a\n> single line, but if the original wanted to have one item per line,\n> I'd rather see it kept as-is.\n\nTo clarify, the above is after I added a_new_field that is one-bit\nwide without doing anything else. I do not mind the checker\ncomplaining the existing force, quiet, etc. whose widths are all\nspelled without SP around ':', because they appear near-by, as a\ncollateral damage. My only gripe is that the result got squished\ninto a single line.\n\n> Anyway, we cannot have perfect checker from the day one, and\n> considering this is an initial attempt, I'd say it is a good start.\n"},{"id":"326352","messageId":"20170815000533.GC95777@google.com","threadId":"46536","inReplyTo":"xmqqvalpsoda.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] Makefile: add style build rule","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-15T00:05:33Z","receivedAt":"2017-08-15T00:05:41Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/14, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > By the way, I do not know which vintage of /usr/bin/git-clang-format\n> > I happen to have on my box, but I needed a crude workaround patch\n> > (attached at the end) ...\n> \n> I guess you hit the same thing while our messages crossing ;-)\n> \n> \n> > As to what it does, the first example I tried may not have been a\n> > great one. I got this:\n> >\n> > git clang-format --style file --diff --extensions c,h\n> > diff --git a/cache.h b/cache.h\n> > index 73e0085186..6462fe25bc 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -1498,11 +1498,8 @@ struct checkout {\n> > const char *base_dir;\n> > int base_dir_len;\n> > struct delayed_checkout *delayed_checkout;\n> > -\tunsigned force:1,\n> > -\t\t quiet:1,\n> > -\t\t not_new:1,\n> > -\t\ta_new_field:1,\n> > -\t\t refresh_cache:1;\n> > +\tunsigned force : 1, quiet : 1, not_new : 1, a_new_field : 1,\n> > +\t\trefresh_cache : 1;\n> > };\n> > #define CHECKOUT_INIT { NULL, \"\" }\n> > \n> > which is not wrong per-se, but I have a mixed feelings. I do not\n> > want it to complain if the original tried to fit many items on a\n> > single line, but if the original wanted to have one item per line,\n> > I'd rather see it kept as-is.\n> \n> To clarify, the above is after I added a_new_field that is one-bit\n> wide without doing anything else. I do not mind the checker\n> complaining the existing force, quiet, etc. whose widths are all\n> spelled without SP around ':', because they appear near-by, as a\n> collateral damage. My only gripe is that the result got squished\n> into a single line.\n\nYeah the result doesn't look too good and I'm not sure which option to\ntweak for that. I'm sure that the problem would fix itself if all the\nbit fields where defined on their own lines:\n\n unsigned force : 1;\n unsigned not_new : 1; \n ... etc ...\n\nI'm sure there are a bunch of other things that we'd need to tweak\nbefore this is ready to be used by all contributors. Specifically the\npenalties to help determine when to break a line.\n\n> \n> > Anyway, we cannot have perfect checker from the day one, and\n> > considering this is an initial attempt, I'd say it is a good start.\n\n-- \nBrandon Williams\n"},{"id":"326354","messageId":"20170815004036.m5ejio6gdyvu6e6a@genre.crustytoothpaste.net","threadId":"46536","inReplyTo":"20170809231900.3535ja3zqdvyerv4@sigill.intra.peff.net","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-08-15T00:40:36Z","receivedAt":"2017-08-15T00:41:07Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Aug 09, 2017 at 07:19:00PM -0400, Jeff King wrote:\n> On Wed, Aug 09, 2017 at 03:53:17PM -0700, Stefan Beller wrote:\n> \n> > >> Right, the reason I stopped pursuing it was that I couldn't find a way\n> > >> to have it make suggestions for new code without nagging about existing\n> > >> code. If we were to aggressively reformat to match the tool for existing\n> > >> code, that would help. But I'm a bit worried that there would always be\n> > >> suggestions from the tool that we don't agree with (i.e., where the\n> > >> guiding principle is \"do what is readable\").\n> > \n> > We may have different opinions on what is readable/beautiful code.\n> > If we were to follow a mutual agreed style that is produced by a tool,\n> > we could use clean/smudge filters with different settings each.\n> \n> I'm less worried about a difference of opinion between humans. My\n> concern is that there are cases that the tool's formatting makes _worse_\n> than what any human would write. And either we accept ugly code because\n> the tool sucks, or we spend a bunch of time fighting with the tool to\n> try to make its output look good.\n\nThis has been my issue with clang-format in the past. I have an SHA-256\nimplementation with an array of 64 32-bit hex integers. These fit six\nto a line, but for neatness and consistency reasons, I'd like them four\nto a line (4 divides 64, but 6 does not). Last I checked, clang-format\ndidn't allow me that option: it reordered them because it could fit six\non a line. This is not the only issue I discovered, just the most\nmemorable.\n\nOther tools, such as perltidy, have traditionally honored existing line\nbreaks better (although not perfectly), which lets humans optimize for\nreadability.\n\nOf course, clang-format could have dramatically improved since I last\nlooked (which was around clang 3.4 or 3.6, I think).\n\nOverall, I do like the idea of using tidy tools, because it does reduce\nquibbling over style quite a bit. I just like the tools to be more\nresponsive to the whitespace they're given on input.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"326356","messageId":"20170815010354.GB78174@aiede.mtv.corp.google.com","threadId":"46536","inReplyTo":"20170815004036.m5ejio6gdyvu6e6a@genre.crustytoothpaste.net","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-08-15T01:03:54Z","receivedAt":"2017-08-15T01:04:02Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nbrian m. carlson wrote:\n>> On Wed, Aug 09, 2017 at 03:53:17PM -0700, Stefan Beller wrote:\n\n>>> We may have different opinions on what is readable/beautiful code.\n>>> If we were to follow a mutual agreed style that is produced by a tool,\n>>> we could use clean/smudge filters with different settings each.\n\nI think this is a long way away --- long enough away that by the time\nsuch a change could be a serious possibility, a lot may have changed\nand the project is likely to know a lot more. In other words, I don't\nsee speculating about that future as being likely to produce useful\nresults today.\n\nIt would be a different story if we were writing a new codebase from\nscratch. In that case, I would be all for the gofmt approach. :)\n\n> On Wed, Aug 09, 2017 at 07:19:00PM -0400, Jeff King wrote:\n\n>> I'm less worried about a difference of opinion between humans. My\n>> concern is that there are cases that the tool's formatting makes _worse_\n>> than what any human would write. And either we accept ugly code because\n>> the tool sucks, or we spend a bunch of time fighting with the tool to\n>> try to make its output look good.\n>\n> This has been my issue with clang-format in the past. I have an SHA-256\n> implementation with an array of 64 32-bit hex integers. These fit six\n> to a line, but for neatness and consistency reasons, I'd like them four\n> to a line (4 divides 64, but 6 does not). Last I checked, clang-format\n> didn't allow me that option: it reordered them because it could fit six\n> on a line. This is not the only issue I discovered, just the most\n> memorable.\n\nIn case it comes up again for you in a project that has adopted the\ngofmt approach: you can signify that your line breaks are intentional\nby putting line comments at the end of each line and clang-format will\nrespect them.\n\nThe clang-format documentation also mentions[1] that you can do\n\n /* clang-format off */\n const double kIdentityMatrix[] = {\n 1, 0, 0,\n 0, 1, 0,\n 0, 0, 1,\n };\n /* clang-format on */\n\nThanks,\nJonathan\n\n[1] http://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code\n"},{"id":"326357","messageId":"20170815014704.thmpoz5xteuhqh3d@sigill.intra.peff.net","threadId":"46536","inReplyTo":"CAGZ79kZgRTzWiufZUE02YYOJpH66x_hShBfcv=4RWDn-qR2LUg@mail.gmail.com","subject":"Re: [PATCH v2 0/2] clang-format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-15T01:47:05Z","receivedAt":"2017-08-15T01:47:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 04:15:40PM -0700, Stefan Beller wrote:\n\n> + llvm-dev@lists.llvm.org\n> \n> The Git community is currently discussing adopting a coding style\n> defined by clang-format, here is a bug report:\n\nSince we've added a cc, let me try to give a little more context.\n\n> > One more oddity I found while playing with this that Git folks might run\n> > into:\n> >\n> > $ git init tmp && cd tmp\n> > $ git commit --allow-empty -m foo\n> > $ echo \"[mysection]mykey\" >>.git/config\n> > $ git clang-format-5.0\n> > Traceback (most recent call last):\n> > File \"/usr/bin/git-clang-format-5.0\", line 579, in \n> > main()\n> > File \"/usr/bin/git-clang-format-5.0\", line 62, in main\n> > config = load_git_config()\n> > File \"/usr/bin/git-clang-format-5.0\", line 194, in load_git_config\n> > name, value = entry.split('\\n', 1)\n> > ValueError: need more than 1 value to unpack\n> >\n> > $ sed -i 's/mykey/&=true/' .git/config\n> > $ git clang-format-5.0\n> > no modified files to format\n> >\n> > So it looks like they do their own config parsing and it's not quite\n> > compatible. :(\n\nIn Git's config files, doing this:\n\n [mysection]\n mykey\n\nis a shorthand for setting mysection.mkykey to \"true\". And the output\nfrom \"git config --list\" will show just the keyname without a value,\nlike:\n\n mysection.mykey\n\ninstead of:\n\n some.key=this one has a value\n\nThere's a possible patch elsewhere in the thread:\n\n https://public-inbox.org/git/xmqqzib1sp6z.fsf@gitster.mtv.corp.google.com/\n\nI'm happy to see it is running \"git config --list\", which means it's\nresponding to syntactic funny-ness in the output of that command, not in\nthe original config (and other features like includes should Just Work\nwithout the script caring).\n\nI'm tempted to say that \"config --list\" should normalize this case into:\n\n mysection.mykey=true\n\nNormally we avoid coercing values without knowing the context in which\nthey'll be used. But the syntax in the original file means the user is\ntelling us it's a boolean and they expect it to be treated that way.\n\nThe only downside is if the user is wrong, it might be coerced into\nthe string \"true\" instead of throwing an error. That seems like a minor\ndrawback for eliminating a potentially confusing corner case from the\nplumbing output.\n\n-Peff\n"},{"id":"326358","messageId":"20170815015256.c423wjwrq7crghu4@sigill.intra.peff.net","threadId":"46536","inReplyTo":"xmqqvalpsoda.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] Makefile: add style build rule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-15T01:52:57Z","receivedAt":"2017-08-15T01:53:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 04:47:45PM -0700, Junio C Hamano wrote:\n\n> > By the way, I do not know which vintage of /usr/bin/git-clang-format\n> > I happen to have on my box, but I needed a crude workaround patch\n> > (attached at the end) ...\n> \n> I guess you hit the same thing while our messages crossing ;-)\n\nYep. Our solutions were at opposite ends of the spectrum, though. :)\n\n> > As to what it does, the first example I tried may not have been a\n> > great one. I got this:\n> >\n> > git clang-format --style file --diff --extensions c,h\n> > diff --git a/cache.h b/cache.h\n> > index 73e0085186..6462fe25bc 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -1498,11 +1498,8 @@ struct checkout {\n> > const char *base_dir;\n> > int base_dir_len;\n> > struct delayed_checkout *delayed_checkout;\n> > -\tunsigned force:1,\n> > -\t\t quiet:1,\n> > -\t\t not_new:1,\n> > -\t\ta_new_field:1,\n> > -\t\t refresh_cache:1;\n> > +\tunsigned force : 1, quiet : 1, not_new : 1, a_new_field : 1,\n> > +\t\trefresh_cache : 1;\n> > };\n> > #define CHECKOUT_INIT { NULL, \"\" }\n> > \n> > which is not wrong per-se, but I have a mixed feelings. I do not\n> > want it to complain if the original tried to fit many items on a\n> > single line, but if the original wanted to have one item per line,\n> > I'd rather see it kept as-is.\n> \n> To clarify, the above is after I added a_new_field that is one-bit\n> wide without doing anything else. I do not mind the checker\n> complaining the existing force, quiet, etc. whose widths are all\n> spelled without SP around ':', because they appear near-by, as a\n> collateral damage. My only gripe is that the result got squished\n> into a single line.\n\nYes, agreed. My personal rule with a list like this is often \"once you\nhave to start breaking across multiple lines, you should put one per\nline\". I don't know if there's a way to codify that in clang-format,\nthough.\n\nThe case I fed it (which is just nonsense I made up that does not fit\nour style) also left me a bit confused at first, but I think it was\nbecause the .clang-format parser was bailing as soon as it found an\nunrecognized entry, but then formatting according to bogus rules. With\nthe original file from Brandon I got:\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 7e8371670b..8994450e0c 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -21,10 +21,7 @@ static int label_cb(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n-static\n-int foo(void* bar,int baz) {\n-\t/* nothing */\n-}\n+static int foo(void *bar, int baz) { /* nothing */ }\n \n int cmd_merge_file(int argc, const char **argv, const char *prefix)\n {\n\nwhich is clearly not our style. And then after removing the entries I\nmentioned elsewhere, I get:\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 7e8371670b..574ba6d86f 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -21,8 +21,8 @@ static int label_cb(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n-static\n-int foo(void* bar,int baz) {\n+static int foo(void *bar, int baz)\n+{\n \t/* nothing */\n }\n\nwhich looks right. So you might want to double-check that it was\nrespecting our settings, and there were no warnings to stderr.\n\n-Peff\n"},{"id":"326361","messageId":"xmqqmv71sfbc.fsf@gitster.mtv.corp.google.com","threadId":"46536","inReplyTo":"20170815014704.thmpoz5xteuhqh3d@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/2] clang-format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T03:03:19Z","receivedAt":"2017-08-15T03:03:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 14, 2017 at 04:15:40PM -0700, Stefan Beller wrote:\n>\n>> + llvm-dev@lists.llvm.org\n>> \n>> The Git community is currently discussing adopting a coding style\n>> defined by clang-format, here is a bug report:\n>\n> Since we've added a cc, let me try to give a little more context.\n\nThanks for relaying; I've dropped them from CC: for this message, as\nthe remainder is not of interest to them.\n\n> I'm tempted to say that \"config --list\" should normalize this case into:\n>\n> mysection.mykey=true\n>\n> Normally we avoid coercing values without knowing the context in which\n> they'll be used. But the syntax in the original file means the user is\n> telling us it's a boolean and they expect it to be treated that way.\n>\n> The only downside is if the user is wrong, it might be coerced into\n> the string \"true\" instead of throwing an error. That seems like a minor\n> drawback for eliminating a potentially confusing corner case from the\n> plumbing output.\n\nBecause we cannot sensibly always normalize a variable set to 'yes',\n'on', etc. to all \"true\", the degree it can help the reading scripts\nis quite limited, as they need to be prepared to see other\nrepresentation of the truth values anyway. Even though I too found\nthe approach somewhat tempting, because there is no ambiguity in\n\"[section] var\" that it means a boolean \"true\", I doubt it would\nhelp very much.\n\nThe way they pass \"non_string_options\" dict to the loader is quite\nsensible for that purpose, as it allows the reader to say \"I care\nabout this and that variables, and I know I want them interpreted as\nint (e.g. 1M) and bool (e.g. 'on') and returned in a normalized\nform\".\n\nI do not mind adding \"git config --list --autotype\" option, though,\nwith which the reading script tells us that it accepts the chance of\nfalse conversion, so that\n\n\t[my]\n\t\tintVal = 2k\n\t\tboolVal\n someVal = on\n\t\totherVal = 1\n\t\tmoreVal = 2\n\t\tanotherVal = 0\n\t\tno = no\n\nmight be listed as\n\n\tmy.intval=2048\n\tmy.boolval=true\n\tmy.someval=true\n\tmy.otherval=1\n\tmy.moreval=2\n\tmy.anotherval=0\n\tmy.no=false\n\nDisambiguation rules are up to debate; the above illustrates an\nextreme position that coerses anything that could be taken as an int\nand a string that can be taken as a bool as such, but it may not\nhelp very much if the reader wants to see boolean values.\n"},{"id":"326362","messageId":"20170815033832.m6fdfvnyf5kvwzkw@sigill.intra.peff.net","threadId":"46536","inReplyTo":"xmqqmv71sfbc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/2] clang-format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-15T03:38:32Z","receivedAt":"2017-08-15T03:38:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 14, 2017 at 08:03:19PM -0700, Junio C Hamano wrote:\n\n> > I'm tempted to say that \"config --list\" should normalize this case into:\n> >\n> > mysection.mykey=true\n> >\n> > Normally we avoid coercing values without knowing the context in which\n> > they'll be used. But the syntax in the original file means the user is\n> > telling us it's a boolean and they expect it to be treated that way.\n> >\n> > The only downside is if the user is wrong, it might be coerced into\n> > the string \"true\" instead of throwing an error. That seems like a minor\n> > drawback for eliminating a potentially confusing corner case from the\n> > plumbing output.\n> \n> Because we cannot sensibly always normalize a variable set to 'yes',\n> 'on', etc. to all \"true\", the degree it can help the reading scripts\n> is quite limited, as they need to be prepared to see other\n> representation of the truth values anyway. Even though I too found\n> the approach somewhat tempting, because there is no ambiguity in\n> \"[section] var\" that it means a boolean \"true\", I doubt it would\n> help very much.\n\nGood point. This is the only case that is _syntactically_ a problem at\nthe key/value level, which is why we noticed it (the reader barfed on\nthe unknown input). But that same reader could be interpreting values\nincorrectly and we'd have no idea. And that applies to types beyond\nbooleans (you showed numbers like \"2k\" below, but there are others, like\n--path or --get-color).\n\nThe one nice thing about fixing this syntactic issue is that the current\nbehavior affected this reader even though it didn't care about\nparticular config key in question. I.e., in this output:\n\n my.boolVal\n my.intVal=2k\n my.valueWeCareAbout=some string\n\nif we don't care about the meaning of boolVal or intVal, we could still\nparse this output fine if not for the syntactic irregularity of\nmy.boolVal.\n\nThat's what might tempt me to fix this independent of the deeper\nproblem. But I really think we should try to address that deeper\nproblem.\n\n> The way they pass \"non_string_options\" dict to the loader is quite\n> sensible for that purpose, as it allows the reader to say \"I care\n> about this and that variables, and I know I want them interpreted as\n> int (e.g. 1M) and bool (e.g. 'on') and returned in a normalized\n> form\".\n> \n> I do not mind adding \"git config --list --autotype\" option, though,\n> with which the reading script tells us that it accepts the chance of\n> false conversion, so that\n> [...]\n\nI think an \"--autotype\" is always going to have false positives.\nIntegers and booleans we can make guesses at. But \"--path\" or \"--color\"\nare much tougher.\n\nThe right answer is to make it easier for that non_string_options\ninformation to make it to git-config so it can do the interpretation for\nthe caller. The way that happens now is:\n\n git config --int my.intVal\n git config --bool my.boolVal\n git config --path my.pathVal\n\nand so on. But I think one reason people turn to --list is that it\nrequires only a single process invocation, rather than repeatedly\ncalling git-config for each variable which might be of interest. I know\nthat diff-highlight's startup is measurably slower due to the six \"git\nconfig --get-color\" calls it must make, and I've been looking for a way\nto do it with a single invocation.\n\nI suspect we need a \"--get-stdin\" which can accept (key,type) tuples and\nreturn the result over stdout. And in some cases it's more than just a\npair; for example, colors need an extra \"parse this default\" argument).\n\n-Peff\n"},{"id":"326399","messageId":"41c8c119-af98-f16f-1585-81b29429afe8@gmail.com","threadId":"46536","inReplyTo":"CAGZ79kau6_XeEQqYDhFC2FmyJiqWY2+SuRzvGFrfmLdhAaQS+Q@mail.gmail.com","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-08-15T13:56:07Z","receivedAt":"2017-08-15T13:56:17Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 8/14/2017 6:02 PM, Stefan Beller wrote:\n> On Mon, Aug 14, 2017 at 2:30 PM, Brandon Williams wrote:\n>> Add a '.clang-format' file which outlines the git project's coding\n>> style. This can be used with clang-format to auto-format .c and .h\n>> files to conform with git's style.\n>>\n>> Signed-off-by: Brandon Williams \n> \n> Applying this patch and running\n> clang-format -i -style file *.c *.h builtin/*.c\n> produces a diff, that I'd mostly agree with.\n> This style guide is close to our current style.\n> \n\nI'm happy to see progress being made in helping reduce the time spent \nmanually reviewing and fixing style formatting errors. In an effort to \nhelp, I installed this in Windows and tried it as well. The tools all \nappear to be working fine and are supported on Windows.\n\nFor the most part, the formatting rules look pretty consistent with the \nexisting style. I ran the same test and looked at the diffs and saw a \ncouple of things that looked odd. For example, how it wrapped the \n\"static int\" on the function header below was different. Not sure why \nas it didn't wrap all the other function headers the same even later in \nthe file it didn't do that with \"static void mute_routine\"\n\ndiff --git a/apply.c b/apply.c\nindex f2d599141d..bb77242e3d 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -58,12 +59,11 @@ static int parse_whitespace_option(struct \napply_state *state, const char *option\n return error(_(\"unrecognized whitespace option '%s'\"), option);\n }\n\n-static int parse_ignorewhitespace_option(struct apply_state *state,\n- const char *option)\n+static int\n+parse_ignorewhitespace_option(struct apply_state *state, const char \n*option)\n {\n- if (!option || !strcmp(option, \"no\") ||\n- !strcmp(option, \"false\") || !strcmp(option, \"never\") ||\n- !strcmp(option, \"none\")) {\n+ if (!option || !strcmp(option, \"no\") || !strcmp(option, \"false\") ||\n+ !strcmp(option, \"never\") || !strcmp(option, \"none\")) {\n state->ws_ignore_action = ignore_ws_none;\n return 0;\n }\n\n\nLater in the file it wraps some of them again: (add_line_info, \nprepare_image, find_name_common, etc). Again, it appears to be \ninconsistent but there must be some rule that is causing this behavior.\n\n\n\nHere is an example of how it wrapped bit fields differently. Again, it \ndidn't seem to be consistent with itself as just below this, it left \nthem on separate lines.\n\n\n@@ -182,8 +185,7 @@ struct fragment {\n * but some codepaths store an allocated buffer.\n */\n const char *patch;\n- unsigned free_patch:1,\n- rejected:1;\n+ unsigned free_patch : 1, rejected : 1;\n int size;\n int linenr;\n struct fragment *next;\n\n\nBig thanks to those working on this!\n\n> As noted in patch 2/2 we'd now need an easy way to\n> expose this for use in various situations, such as\n> * contributor wanting to format their patch\n> * reformatting code for readability\n> \n> Thanks,\n> Stefan\n> \n"},{"id":"326402","messageId":"9c9a9b98-9308-95d6-a912-85a49bd3e21d@gmail.com","threadId":"46536","inReplyTo":"20170814213046.107576-2-bmwill@google.com","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-08-15T14:28:40Z","receivedAt":"2017-08-15T14:28:48Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 8/14/2017 5:30 PM, Brandon Williams wrote:\n> Add a '.clang-format' file which outlines the git project's coding\n> style. This can be used with clang-format to auto-format .c and .h\n> files to conform with git's style.\n> \n> Signed-off-by: Brandon Williams \n> ---\n> .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 165 insertions(+)\n> create mode 100644 .clang-format\n> \n\nI applied this and then ran:\n clang-format -i -style file *.c *.h builtin/*.c\n\nThe modified files generate lots of line ending warnings. Apparently \nclang-format isn't respecting the git settings for line endings as it \nconverted CRLF to LF in all the files it edited.\n\n$ git add .\nwarning: LF will be replaced by CRLF in alloc.c.\nThe file will have its original line endings in your working directory.\nwarning: LF will be replaced by CRLF in base85.c.\nThe file will have its original line endings in your working directory.\nwarning: LF will be replaced by CRLF in bisect.h.\n[...]\n\n"},{"id":"326416","messageId":"20170815173445.GA21874@google.com","threadId":"46536","inReplyTo":"9c9a9b98-9308-95d6-a912-85a49bd3e21d@gmail.com","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-15T17:34:45Z","receivedAt":"2017-08-15T17:34:55Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/15, Ben Peart wrote:\n> \n> \n> On 8/14/2017 5:30 PM, Brandon Williams wrote:\n> >Add a '.clang-format' file which outlines the git project's coding\n> >style. This can be used with clang-format to auto-format .c and .h\n> >files to conform with git's style.\n> >\n> >Signed-off-by: Brandon Williams \n> >---\n> > .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 165 insertions(+)\n> > create mode 100644 .clang-format\n> >\n> \n> I applied this and then ran:\n> clang-format -i -style file *.c *.h builtin/*.c\n> \n> The modified files generate lots of line ending warnings.\n> Apparently clang-format isn't respecting the git settings for line\n> endings as it converted CRLF to LF in all the files it edited.\n> \n> $ git add .\n> warning: LF will be replaced by CRLF in alloc.c.\n> The file will have its original line endings in your working directory.\n> warning: LF will be replaced by CRLF in base85.c.\n> The file will have its original line endings in your working directory.\n> warning: LF will be replaced by CRLF in bisect.h.\n> [...]\n> \n\nI would think that there must be some setting to permit CRLF on windows\nmachines. As I work on a unix machine I don't see this sort of\nbehavior.\n\n-- \nBrandon Williams\n"},{"id":"326419","messageId":"20170815173735.GB21874@google.com","threadId":"46536","inReplyTo":"41c8c119-af98-f16f-1585-81b29429afe8@gmail.com","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-15T17:37:35Z","receivedAt":"2017-08-15T17:37:43Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/15, Ben Peart wrote:\n> \n> \n> On 8/14/2017 6:02 PM, Stefan Beller wrote:\n> >On Mon, Aug 14, 2017 at 2:30 PM, Brandon Williams wrote:\n> >>Add a '.clang-format' file which outlines the git project's coding\n> >>style. This can be used with clang-format to auto-format .c and .h\n> >>files to conform with git's style.\n> >>\n> >>Signed-off-by: Brandon Williams \n> >\n> >Applying this patch and running\n> > clang-format -i -style file *.c *.h builtin/*.c\n> >produces a diff, that I'd mostly agree with.\n> >This style guide is close to our current style.\n> >\n> \n> I'm happy to see progress being made in helping reduce the time\n> spent manually reviewing and fixing style formatting errors. In an\n> effort to help, I installed this in Windows and tried it as well.\n> The tools all appear to be working fine and are supported on\n> Windows.\n> \n> For the most part, the formatting rules look pretty consistent with\n> the existing style. I ran the same test and looked at the diffs and\n> saw a couple of things that looked odd. For example, how it wrapped\n> the \"static int\" on the function header below was different. Not\n> sure why as it didn't wrap all the other function headers the same\n> even later in the file it didn't do that with \"static void\n> mute_routine\"\n> \n> diff --git a/apply.c b/apply.c\n> index f2d599141d..bb77242e3d 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -58,12 +59,11 @@ static int parse_whitespace_option(struct\n> apply_state *state, const char *option\n> return error(_(\"unrecognized whitespace option '%s'\"), option);\n> }\n> \n> -static int parse_ignorewhitespace_option(struct apply_state *state,\n> - const char *option)\n> +static int\n> +parse_ignorewhitespace_option(struct apply_state *state, const char\n> *option)\n> {\n> - if (!option || !strcmp(option, \"no\") ||\n> - !strcmp(option, \"false\") || !strcmp(option, \"never\") ||\n> - !strcmp(option, \"none\")) {\n> + if (!option || !strcmp(option, \"no\") || !strcmp(option, \"false\") ||\n> + !strcmp(option, \"never\") || !strcmp(option, \"none\")) {\n> state->ws_ignore_action = ignore_ws_none;\n> return 0;\n> }\n> \n> \n> Later in the file it wraps some of them again: (add_line_info,\n> prepare_image, find_name_common, etc). Again, it appears to be\n> inconsistent but there must be some rule that is causing this\n> behavior.\n\nThese have to deal with setting the penalties. When a line gets to be\ntoo long the tool needs to find a place to break the line based on a\npenalty system. The current .clang-format file I sent out has values\nfor the penalties which would most likely need to be tweaked through\ntrial and error.\n\n> \n> \n> \n> Here is an example of how it wrapped bit fields differently. Again,\n> it didn't seem to be consistent with itself as just below this, it\n> left them on separate lines.\n> \n> \n> @@ -182,8 +185,7 @@ struct fragment {\n> * but some codepaths store an allocated buffer.\n> */\n> const char *patch;\n> - unsigned free_patch:1,\n> - rejected:1;\n> + unsigned free_patch : 1, rejected : 1;\n> int size;\n> int linenr;\n> struct fragment *next;\n\nIf the return type was replicated then it would probably format the\ndifferent struct members on their own line.\n\n> \n> \n> Big thanks to those working on this!\n> \n> >As noted in patch 2/2 we'd now need an easy way to\n> >expose this for use in various situations, such as\n> >* contributor wanting to format their patch\n> >* reformatting code for readability\n> >\n> >Thanks,\n> >Stefan\n> >\n\n-- \nBrandon Williams\n"},{"id":"326420","messageId":"CAGZ79kZc_Mh8i7kMmP_BNGWBAp8rGiiVdCYgTHtfHhsJ6YfGSA@mail.gmail.com","threadId":"46536","inReplyTo":"20170815173445.GA21874@google.com","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T17:41:57Z","receivedAt":"2017-08-15T17:42:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 10:34 AM, Brandon Williams wrote:\n> On 08/15, Ben Peart wrote:\n>>\n>>\n>> On 8/14/2017 5:30 PM, Brandon Williams wrote:\n>> >Add a '.clang-format' file which outlines the git project's coding\n>> >style. This can be used with clang-format to auto-format .c and .h\n>> >files to conform with git's style.\n>> >\n>> >Signed-off-by: Brandon Williams \n>> >---\n>> > .clang-format | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>> > 1 file changed, 165 insertions(+)\n>> > create mode 100644 .clang-format\n>> >\n>>\n>> I applied this and then ran:\n>> clang-format -i -style file *.c *.h builtin/*.c\n>>\n>> The modified files generate lots of line ending warnings.\n>> Apparently clang-format isn't respecting the git settings for line\n>> endings as it converted CRLF to LF in all the files it edited.\n>>\n>> $ git add .\n>> warning: LF will be replaced by CRLF in alloc.c.\n>> The file will have its original line endings in your working directory.\n>> warning: LF will be replaced by CRLF in base85.c.\n>> The file will have its original line endings in your working directory.\n>> warning: LF will be replaced by CRLF in bisect.h.\n>> [...]\n>>\n\nThis sounds as if core.safecrlf is set to \"warn\" (by default?),\nyou could set it to false to not have these warnings. However\nthat is side stepping the problem of clang-format producing\nnon-crlf line endings.\n\nMaybe the workflow can be setup such that this side stepping\nis ok, and no harm is done by the LF line endings by the formatting\n(If the formatting was a pre-commit hook, then Git would convert\nLF/CRLF and there would be no impact to your workflow I'd imagine).\n"},{"id":"326498","messageId":"alpine.DEB.2.21.1.1708161414120.19382@virtualbox","threadId":"46536","inReplyTo":"20170814230108.wucznh66vfblrsys@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] clang-format: outline the git project's coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-08-16T12:18:46Z","receivedAt":"2017-08-16T12:19:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 14 Aug 2017, Jeff King wrote:\n\n> On Mon, Aug 14, 2017 at 03:54:30PM -0700, Brandon Williams wrote:\n> \n> > > And removing that gives me a clean output. I have no idea why my clang\n> > > doesn't like these (but presumably yours does). It's clang-format-5.0 in\n> > > Debian unstable (and clang-format-3.8, etc).\n> > \n> > Those must be features in version 6 (which is what I seem to have\n> > installed on my machine).\n> \n> OK, that makes sense. The most recent one package for Debian unstable is\n> 5.0. AFAICT 5.0 is actually in release freeze for another week or two,\n> and 6 is just bleeding-edge that moved on after the release freeze a few\n> weeks ago.\n> \n> I'm not sure which version it makes sense to target as a minimum, but\n> probably not 6 yet. :)\n\nI agree.\n\nThere is most likely a middle path between too old and too new, and with\nthe current pace of the review 5.0 will probably be good enough by the\ntime this patch series can possibly hit `master`. So I'd guess 5.0 would\nbe a good version to aim for.\n\nBesides, it may not matter *all* that much which version we target: As I\nmentioned elsewhere, the contributor experience would most likely be\nvastly improved if this was a bot, say, monitoring GitHub Pull Requests.\nIt could use filter-branch to apply clang-format to each commit's diff and\nreport back to the Pull Request either by saying that the style is okay,\nor by linking to another repository where the fixed commits live (combined\nwith instructions how to update the local branch).\n\nCiao,\nDscho\n"},{"id":"329104","messageId":"alpine.DEB.2.21.1.1709281249520.40514@virtualbox","threadId":"46536","inReplyTo":"alpine.DEB.2.21.1.1708100032050.11175@virtualbox","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-28T11:41:52Z","receivedAt":"2017-09-28T11:42:01Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi all,\n\nOn Thu, 10 Aug 2017, Johannes Schindelin wrote:\n\n> On Tue, 8 Aug 2017, Brandon Williams wrote:\n> \n> > On 08/08, Stefan Beller wrote:\n> > > On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin\n> > > wrote:\n> > > >\n> > > > On Mon, 7 Aug 2017, Brandon Williams wrote:\n> > > >\n> > > >> Add a '.clang-format' file which outlines the git project's\n> > > >> coding style. This can be used with clang-format to auto-format\n> > > >> .c and .h files to conform with git's style.\n> > > >>\n> > > >> Signed-off-by: Brandon Williams \n> > > >> ---\n> > > >>\n> > > >> I'm sure this sort of thing comes up every so often on the list\n> > > >> but back at git-merge I mentioned how it would be nice to not\n> > > >> have to worry about style when reviewing patches as that is\n> > > >> something mechanical and best left to a machine (for the most\n> > > >> part).\n> > > >\n> > > > Amen.\n> > > >\n> > > > If I never have to see a review mentioning an unwrapped line, I am quite\n> > > > certain I will be quite content.\n> > > \n> > > As a thought experiment I'd like to propose to take it one step further:\n> > > \n> > > If the code was formatted perfectly in one style such that a\n> > > formatter for this style would not produce changes when rerun\n> > > again on the code, then each individual could have a clean/smudge\n> > > filter to work in their preferred style, and only the exchange and\n> > > storage of code is in a mutual agreed style. If the mutually\n> > > agreed style is close to what I prefer, I don't have to use\n> > > clean/smudge filters.\n> > > \n> > > Additionally to this patch, we'd want to either put a note into\n> > > SubmittingPatches or Documentation/gitworkflows.txt to hint at\n> > > how to use this formatting to just affect the patch that is currently\n> > > worked on or rather a pre-commit hook?\n> > \n> > I'm sure both of these things will probably happen if we decide to make\n> > use of a code-formatter. This RFC is really just trying to ask the\n> > question: \"Is this something we want to entertain doing?\"\n> > My response would be 'Yes' but there's many other opinions to consider\n> > first :)\n> \n> I am sure that something even better will be possible: a Continuous\n> \"Integration\" that fixes the coding style automatically by using\n> `filter-branch` (avoiding the merge conflicts that would arise if `rebase\n> -i` was used).\n\nFWIW I just set this up in my VSTS account, with the following build step\n(performed in one of the Hosted Linux agents, i.e. I do not even have to\nhave a VM dedicated to the task):\n\n-- snip --\ndie () {\n echo \"$*\" >&2\n exit 1\n}\n\nhead=$(git rev-parse HEAD) ||\ndie \"Could not determine HEAD\"\n\ntest -n \"$BUILD_SOURCEBRANCHNAME\" ||\ndie \"Need a source branch name to work with\"\n\nbase=$(git merge-base origin/core/master HEAD) &&\ncount=$(git rev-list --count $base..) &&\ntest 0 -lt $count ||\ndie \"Could not determine commits to clean up (count: $count)\"\n\ntest -f .clang-format ||\ngit show origin/core/pu/.clang-format >.clang-format ||\ndie \"Need a .clang-format\"\n\nsudo add-apt-repository 'deb http://apt.llvm.org/xenial/\nllvm-toolchain-xenial main' &&\nsudo apt-get update &&\nsudo apt-get --allow-unauthenticated -y install clang-format-6.0 ||\ndie \"Could not install clang-format 6.0\"\n\ngit filter-branch -f --tree-filter \\\n 'git diff HEAD^.. |\n clang-format-diff-6.0 -p 1 -i &&\n\n git update-index --refresh --ignore-submodules &&\n git diff-files --quiet --ignore-submodules ||\n git commit --amend -C HEAD' $base.. ||\ndie \"Could not rewrite branch\"\n\nif test \"$head\" = \"$(git rev-parse HEAD)\"\nthen\n echo \"No changes in $BUILD_SOURCEBRANCHNAME introduced by clang-format\" >&2\nelse\n git push vsts +HEAD:refs/heads/clang-format/\"$BUILD_SOURCEBRANCHNAME\" ||\n die \"Could not push clang-format/$BUILD_SOURCEBRANCHNAME\"\n echo \"Clean branch pushed as clang-format/$BUILD_SOURCEBRANCHNAME\" >&2\n exit 123\nfi\n-- snap --\n\nA couple of notes for the interested:\n\n- you can easily set this up for yourself, as Visual Studio Team Services\n is free for small teams up to five people (including single developer\n \"teams\"): https://www.visualstudio.com/team-services/, and of course you\n can back it by your own git.git fork on GitHub, no need to host the code\n in VSTS.\n\n Disclaimer: I work closely with the developers behind Visual Studio Team\n Services, and I am a genuine fan, yet I understand if anybody thinks of\n this as advertising the service, so this will be the only time I mention\n this.\n\n- the script assumes that there is a `core/master` tracking upstream Git's\n master branch, then reformats the commits in the current branch that are\n not also reachable from core/master.\n\n- The push credentials to push the result at the end are of course not\n included in the script, they need to be provided separately.\n\n- the exit code 123 when the branch needed to be rewritten indicates to\n any consumer that the build \"failed\". The reason is that I want to\n integrate this into a system where I open a PR in my own account, which\n triggers the build automatically contingent on the base branch being\n core/master, and if the build fails, the PR gets \"blocked\", providing a\n very easy way to see that there is still work to be done.\n\n- for the moment, I do not push back to the original branch, even if I\n could. The reason is that already my first test produced a dubious\n result, see below.\n\nI am reasonably happy with the way this build job works right now,\nespecially given that I do not have to mess up any other setup I have just\nto get the bleeding edge version of Clang.\n\nNow for the dubious result.\n\nI took my most recent contribution, the lazyload one (which you can easily\nget yourself by fetching the lazyload-v2 tag from\nhttps://github.com/dscho/git), because it was pretty self-contained and\nsmall, only one patch. With the current .clang-format as per git.git's\nmaster (or for that matter, pu, as they are identical), the output `git\nshow | clang-format-diff-6.0 -p 1` ends in this hunk:\n\n-- snip --\n@@ -43,8 +43,7 @@\n if (!proc->initialized) {\n HANDLE hnd;\n proc->initialized = 1;\n- hnd = LoadLibraryExA(proc->dll, NULL,\n- LOAD_LIBRARY_SEARCH_SYSTEM32);\n+ hnd = LoadLibraryExA(proc->dll, NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);\n if (hnd)\n proc->pfunction = GetProcAddress(hnd, proc->function);\n }\n-- snap --\n\n(tabs intentionally converted to spaces, to show the extent of the damage\nclearly)\n\nIn other words, despite the column limit of 80 (with a tab width of 8\nspaces), it takes a perfectly well formatted pair of lines and combines\nthem into a single line that now has 84 columns. Absolutely not what we\nwant.\n\nEven worse: if I replace the column limit of 80 by 79, like so:\n\n-- snip --\ndiff --git a/.clang-format b/.clang-format\nindex 3ede2628d2d..9f686c1ed5a 100644\n--- a/.clang-format\n+++ b/.clang-format\n@@ -6,7 +6,7 @@ UseTab: Always\n TabWidth: 8\n IndentWidth: 8\n ContinuationIndentWidth: 8\n-ColumnLimit: 80\n+ColumnLimit: 79\n \n # C Language specifics\n Language: Cpp\n-- snap --\n\nthen that hunk vanishes and clang-format leaves the LoadLibraryEx() lines\nalone!\n\nEven stranger, if I revert to 80 columns, copy the offending line above\nthe conditional block so that the indentation level is different (based on\na hunch that this may have something to do with clang's understanding of\ntab widths) and extend the last parameter artificially, it still breaks at\nexactly 84 columns, i.e. if I make the line 84 columns long, it keeps it\nas one line, if I extend it to 85 columns, it breaks the line into two.\n\nIn fact, the same holds true even with no indentation at all: if I turn\nthis line into a static variable assignment outside of the function, it\nagain breaks the line as soon as I extend it to 85 columns.\n\nThen I repeated the exercise with clang-format-6.0 instead of\nclang-format-diff-6.0, and the finding still holds. 85 columns, despite\nthe explicit ColumnLimit: 80 in .clang-format.\n\nI then tried to format a file containing only the line \"int i123, j123;\"\nwith various values for ColumnLimit, and could not get it to break at all.\n\nAny insights?\n\nThanks,\nDscho\n"},{"id":"329111","messageId":"20170928171657.GB153914@google.com","threadId":"46536","inReplyTo":"alpine.DEB.2.21.1.1709281249520.40514@virtualbox","subject":"Re: [RFC] clang-format: outline the git project's coding style","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-09-28T17:16:57Z","receivedAt":"2017-09-28T17:17:06Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 09/28, Johannes Schindelin wrote:\n> Hi all,\n>\n> On Thu, 10 Aug 2017, Johannes Schindelin wrote:\n>\n> > On Tue, 8 Aug 2017, Brandon Williams wrote:\n> >\n> > > On 08/08, Stefan Beller wrote:\n> > > > On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin\n> > > > wrote:\n> > > > >\n> > > > > On Mon, 7 Aug 2017, Brandon Williams wrote:\n> > > > >\n> > > > >> Add a '.clang-format' file which outlines the git project's\n> > > > >> coding style. This can be used with clang-format to auto-format\n> > > > >> .c and .h files to conform with git's style.\n> > > > >>\n> > > > >> Signed-off-by: Brandon Williams \n> > > > >> ---\n> > > > >>\n> > > > >> I'm sure this sort of thing comes up every so often on the list\n> > > > >> but back at git-merge I mentioned how it would be nice to not\n> > > > >> have to worry about style when reviewing patches as that is\n> > > > >> something mechanical and best left to a machine (for the most\n> > > > >> part).\n> > > > >\n> > > > > Amen.\n> > > > >\n> > > > > If I never have to see a review mentioning an unwrapped line, I am quite\n> > > > > certain I will be quite content.\n> > > >\n> > > > As a thought experiment I'd like to propose to take it one step further:\n> > > >\n> > > > If the code was formatted perfectly in one style such that a\n> > > > formatter for this style would not produce changes when rerun\n> > > > again on the code, then each individual could have a clean/smudge\n> > > > filter to work in their preferred style, and only the exchange and\n> > > > storage of code is in a mutual agreed style. If the mutually\n> > > > agreed style is close to what I prefer, I don't have to use\n> > > > clean/smudge filters.\n> > > >\n> > > > Additionally to this patch, we'd want to either put a note into\n> > > > SubmittingPatches or Documentation/gitworkflows.txt to hint at\n> > > > how to use this formatting to just affect the patch that is currently\n> > > > worked on or rather a pre-commit hook?\n> > >\n> > > I'm sure both of these things will probably happen if we decide to make\n> > > use of a code-formatter. This RFC is really just trying to ask the\n> > > question: \"Is this something we want to entertain doing?\"\n> > > My response would be 'Yes' but there's many other opinions to consider\n> > > first :)\n> >\n> > I am sure that something even better will be possible: a Continuous\n> > \"Integration\" that fixes the coding style automatically by using\n> > `filter-branch` (avoiding the merge conflicts that would arise if `rebase\n> > -i` was used).\n>\n> FWIW I just set this up in my VSTS account, with the following build step\n> (performed in one of the Hosted Linux agents, i.e. I do not even have to\n> have a VM dedicated to the task):\n>\n> -- snip --\n> die () {\n> echo \"$*\" >&2\n> exit 1\n> }\n>\n> head=$(git rev-parse HEAD) ||\n> die \"Could not determine HEAD\"\n>\n> test -n \"$BUILD_SOURCEBRANCHNAME\" ||\n> die \"Need a source branch name to work with\"\n>\n> base=$(git merge-base origin/core/master HEAD) &&\n> count=$(git rev-list --count $base..) &&\n> test 0 -lt $count ||\n> die \"Could not determine commits to clean up (count: $count)\"\n>\n> test -f .clang-format ||\n> git show origin/core/pu/.clang-format >.clang-format ||\n> die \"Need a .clang-format\"\n>\n> sudo add-apt-repository 'deb http://apt.llvm.org/xenial/\n> llvm-toolchain-xenial main' &&\n> sudo apt-get update &&\n> sudo apt-get --allow-unauthenticated -y install clang-format-6.0 ||\n> die \"Could not install clang-format 6.0\"\n>\n> git filter-branch -f --tree-filter \\\n> 'git diff HEAD^.. |\n> clang-format-diff-6.0 -p 1 -i &&\n>\n> git update-index --refresh --ignore-submodules &&\n> git diff-files --quiet --ignore-submodules ||\n> git commit --amend -C HEAD' $base.. ||\n> die \"Could not rewrite branch\"\n>\n> if test \"$head\" = \"$(git rev-parse HEAD)\"\n> then\n> echo \"No changes in $BUILD_SOURCEBRANCHNAME introduced by clang-format\" >&2\n> else\n> git push vsts +HEAD:refs/heads/clang-format/\"$BUILD_SOURCEBRANCHNAME\" ||\n> die \"Could not push clang-format/$BUILD_SOURCEBRANCHNAME\"\n> echo \"Clean branch pushed as clang-format/$BUILD_SOURCEBRANCHNAME\" >&2\n> exit 123\n> fi\n> -- snap --\n>\n> A couple of notes for the interested:\n>\n> - you can easily set this up for yourself, as Visual Studio Team Services\n> is free for small teams up to five people (including single developer\n> \"teams\"): https://www.visualstudio.com/team-services/, and of course you\n> can back it by your own git.git fork on GitHub, no need to host the code\n> in VSTS.\n>\n> Disclaimer: I work closely with the developers behind Visual Studio Team\n> Services, and I am a genuine fan, yet I understand if anybody thinks of\n> this as advertising the service, so this will be the only time I mention\n> this.\n>\n> - the script assumes that there is a `core/master` tracking upstream Git's\n> master branch, then reformats the commits in the current branch that are\n> not also reachable from core/master.\n>\n> - The push credentials to push the result at the end are of course not\n> included in the script, they need to be provided separately.\n>\n> - the exit code 123 when the branch needed to be rewritten indicates to\n> any consumer that the build \"failed\". The reason is that I want to\n> integrate this into a system where I open a PR in my own account, which\n> triggers the build automatically contingent on the base branch being\n> core/master, and if the build fails, the PR gets \"blocked\", providing a\n> very easy way to see that there is still work to be done.\n>\n> - for the moment, I do not push back to the original branch, even if I\n> could. The reason is that already my first test produced a dubious\n> result, see below.\n>\n> I am reasonably happy with the way this build job works right now,\n> especially given that I do not have to mess up any other setup I have just\n> to get the bleeding edge version of Clang.\n>\n> Now for the dubious result.\n>\n> I took my most recent contribution, the lazyload one (which you can easily\n> get yourself by fetching the lazyload-v2 tag from\n> https://github.com/dscho/git), because it was pretty self-contained and\n> small, only one patch. With the current .clang-format as per git.git's\n> master (or for that matter, pu, as they are identical), the output `git\n> show | clang-format-diff-6.0 -p 1` ends in this hunk:\n>\n> -- snip --\n> @@ -43,8 +43,7 @@\n> if (!proc->initialized) {\n> HANDLE hnd;\n> proc->initialized = 1;\n> - hnd = LoadLibraryExA(proc->dll, NULL,\n> - LOAD_LIBRARY_SEARCH_SYSTEM32);\n> + hnd = LoadLibraryExA(proc->dll, NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);\n> if (hnd)\n> proc->pfunction = GetProcAddress(hnd, proc->function);\n> }\n> -- snap --\n>\n> (tabs intentionally converted to spaces, to show the extent of the damage\n> clearly)\n>\n> In other words, despite the column limit of 80 (with a tab width of 8\n> spaces), it takes a perfectly well formatted pair of lines and combines\n> them into a single line that now has 84 columns. Absolutely not what we\n> want.\n>\n> Even worse: if I replace the column limit of 80 by 79, like so:\n>\n> -- snip --\n> diff --git a/.clang-format b/.clang-format\n> index 3ede2628d2d..9f686c1ed5a 100644\n> --- a/.clang-format\n> +++ b/.clang-format\n> @@ -6,7 +6,7 @@ UseTab: Always\n> TabWidth: 8\n> IndentWidth: 8\n> ContinuationIndentWidth: 8\n> -ColumnLimit: 80\n> +ColumnLimit: 79\n>\n> # C Language specifics\n> Language: Cpp\n> -- snap --\n>\n> then that hunk vanishes and clang-format leaves the LoadLibraryEx() lines\n> alone!\n>\n> Even stranger, if I revert to 80 columns, copy the offending line above\n> the conditional block so that the indentation level is different (based on\n> a hunch that this may have something to do with clang's understanding of\n> tab widths) and extend the last parameter artificially, it still breaks at\n> exactly 84 columns, i.e. if I make the line 84 columns long, it keeps it\n> as one line, if I extend it to 85 columns, it breaks the line into two.\n>\n> In fact, the same holds true even with no indentation at all: if I turn\n> this line into a static variable assignment outside of the function, it\n> again breaks the line as soon as I extend it to 85 columns.\n>\n> Then I repeated the exercise with clang-format-6.0 instead of\n> clang-format-diff-6.0, and the finding still holds. 85 columns, despite\n> the explicit ColumnLimit: 80 in .clang-format.\n>\n> I then tried to format a file containing only the line \"int i123, j123;\"\n> with various values for ColumnLimit, and could not get it to break at all.\n>\n> Any insights?\n\nI think I know what is happening here, and this is something that we\nwill need to tweak based on more people beginning to use the formatting\ntool.\n\nIn the .clang-format file there are a number of parameters at the end:\n\n # Penalties\n # This decides what order things should be done if a line is too long\n PenaltyBreakAssignment: 100\n PenaltyBreakBeforeFirstCallParameter: 100\n PenaltyBreakComment: 100\n PenaltyBreakFirstLessLess: 0\n PenaltyBreakString: 100\n PenaltyExcessCharacter: 5\n PenaltyReturnTypeOnItsOwnLine: 0\n\nThese penalties are used to determine which line breaking events should be\ndone based on some programmable weight (or penalty). I thing that if we\nbump up the penalty on excess characters to something higher than '5'\n(which i think is quite low) then it may format more like you were\nexpecting.\n\n--\nBrandon Williams\n"}]}