From 94ea90aae4a022978d34cd0192070a7c0f628e7e Mon Sep 17 00:00:00 2001 From: Roland Fischer Date: Tue, 7 Jul 2020 17:54:36 -0400 Subject: [PATCH 1/4] Add clang-format file clang-format allows to auto-format C code. The settings here are set up to follow the code style, see https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style. Some bike shedding should still happen. Includes sample formatted code. One can set up .clang-format as "differential setting based on an existing project" or specify all settings. Includes both for people to compare. DO NOT MERGE AS IS. --- .clang-format | 43 ++ clang-format.full.do_not_merge | 135 +++++++ sample_formatted_code.do_not_merge.c | 562 +++++++++++++++++++++++++++ 3 files changed, 740 insertions(+) create mode 100644 .clang-format create mode 100644 clang-format.full.do_not_merge create mode 100644 sample_formatted_code.do_not_merge.c diff --git a/.clang-format b/.clang-format new file mode 100644 index 000000000000..f53be901167d --- /dev/null +++ b/.clang-format @@ -0,0 +1,43 @@ +# Suricata settings as per +# https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style +# +# Some discussions to be had. See companion sample_formatted_code.do_not_merge.c +# for what this looks like. +--- +BasedOnStyle: LLVM +AlignAfterOpenBracket: DontAlign +AlignConsecutiveMacros: true +AlignEscapedNewlines: Left +# clang 10: AllowShortBlocksOnASingleLine: Never +# clang 11: AllowShortEnumsOnASingleLine: false +AllowShortFunctionsOnASingleLine: None +# BreakBeforeBraces: Mozilla is closest, but does not split empty functions/structs +BraceWrapping: + AfterClass: true + AfterControlStatement: false + AfterEnum: false + AfterFunction: true + AfterStruct: false + AfterUnion: false + AfterExternBlock: true + BeforeElse: false + IndentBraces: false + SplitEmptyFunction: true + SplitEmptyRecord: true +BreakBeforeBraces: Custom +Cpp11BracedListStyle: true +ConstructorInitializerIndentWidth: 8 +ContinuationIndentWidth: 8 +ForEachMacros: ['json_array_foreach', 'json_object_foreach'] +IndentCaseLabels: true +IndentWidth: 4 +ReflowComments: true +SortIncludes: false + +# implicit by LLVM style +#BreakBeforeTernaryOperators: true +#ColumnLimit: 80 +#UseTab: Never +#TabWidth: 8 + +... diff --git a/clang-format.full.do_not_merge b/clang-format.full.do_not_merge new file mode 100644 index 000000000000..eb5582231141 --- /dev/null +++ b/clang-format.full.do_not_merge @@ -0,0 +1,135 @@ +# To use minimalistic .clang-format with BasedOnStyle: or the full shebang like +# this? +# +# This probably needs some more pruning if we wanted the full thing, such as +# cleaning up IncludeCategories (that is not used as SortIncludes:false). +# +# Reproduce from minimalistic .clang-format with: +# clang-format --dump-config --style=file >clang-format.full.do_not_merge +# +--- +Language: Cpp +AccessModifierOffset: -2 +AlignAfterOpenBracket: DontAlign +AlignConsecutiveMacros: true +AlignConsecutiveAssignments: false +AlignConsecutiveDeclarations: false +AlignEscapedNewlines: Left +AlignOperands: true +AlignTrailingComments: true +AllowAllArgumentsOnNextLine: true +AllowAllConstructorInitializersOnNextLine: true +AllowAllParametersOfDeclarationOnNextLine: true +AllowShortBlocksOnASingleLine: false +AllowShortCaseLabelsOnASingleLine: false +AllowShortFunctionsOnASingleLine: None +AllowShortLambdasOnASingleLine: All +AllowShortIfStatementsOnASingleLine: Never +AllowShortLoopsOnASingleLine: false +AlwaysBreakAfterDefinitionReturnType: None +AlwaysBreakAfterReturnType: None +AlwaysBreakBeforeMultilineStrings: false +AlwaysBreakTemplateDeclarations: MultiLine +BinPackArguments: true +BinPackParameters: true +BraceWrapping: + AfterCaseLabel: false + AfterClass: true + AfterControlStatement: false + AfterEnum: false + AfterFunction: true + AfterNamespace: false + AfterObjCDeclaration: false + AfterStruct: false + AfterUnion: false + AfterExternBlock: true + BeforeCatch: false + BeforeElse: false + IndentBraces: false + SplitEmptyFunction: true + SplitEmptyRecord: true + SplitEmptyNamespace: true +BreakBeforeBinaryOperators: None +BreakBeforeBraces: Custom +BreakBeforeInheritanceComma: false +BreakInheritanceList: BeforeColon +BreakBeforeTernaryOperators: true +BreakConstructorInitializersBeforeComma: false +BreakConstructorInitializers: BeforeColon +BreakAfterJavaFieldAnnotations: false +BreakStringLiterals: true +ColumnLimit: 80 +CommentPragmas: '^ IWYU pragma:' +CompactNamespaces: false +ConstructorInitializerAllOnOneLineOrOnePerLine: false +ConstructorInitializerIndentWidth: 8 +ContinuationIndentWidth: 8 +Cpp11BracedListStyle: true +DerivePointerAlignment: false +DisableFormat: false +ExperimentalAutoDetectBinPacking: false +FixNamespaceComments: true +ForEachMacros: + - json_array_foreach + - json_object_foreach + - BOOST_FOREACH +IncludeBlocks: Preserve +IncludeCategories: + - Regex: '^"(llvm|llvm-c|clang|clang-c)/' + Priority: 2 + - Regex: '^(<|"(gtest|gmock|isl|json)/)' + Priority: 3 + - Regex: '.*' + Priority: 1 +IncludeIsMainRegex: '(Test)?$' +IndentCaseLabels: true +IndentPPDirectives: None +IndentWidth: 4 +IndentWrappedFunctionNames: false +JavaScriptQuotes: Leave +JavaScriptWrapImports: true +KeepEmptyLinesAtTheStartOfBlocks: true +MacroBlockBegin: '' +MacroBlockEnd: '' +MaxEmptyLinesToKeep: 1 +NamespaceIndentation: None +ObjCBinPackProtocolList: Auto +ObjCBlockIndentWidth: 2 +ObjCSpaceAfterProperty: false +ObjCSpaceBeforeProtocolList: true +PenaltyBreakAssignment: 2 +PenaltyBreakBeforeFirstCallParameter: 19 +PenaltyBreakComment: 300 +PenaltyBreakFirstLessLess: 120 +PenaltyBreakString: 1000 +PenaltyBreakTemplateDeclaration: 10 +PenaltyExcessCharacter: 1000000 +PenaltyReturnTypeOnItsOwnLine: 60 +PointerAlignment: Right +ReflowComments: true +SortIncludes: false +SortUsingDeclarations: true +SpaceAfterCStyleCast: false +SpaceAfterLogicalNot: false +SpaceAfterTemplateKeyword: true +SpaceBeforeAssignmentOperators: true +SpaceBeforeCpp11BracedList: false +SpaceBeforeCtorInitializerColon: true +SpaceBeforeInheritanceColon: true +SpaceBeforeParens: ControlStatements +SpaceBeforeRangeBasedForLoopColon: true +SpaceInEmptyParentheses: false +SpacesBeforeTrailingComments: 1 +SpacesInAngles: false +SpacesInContainerLiterals: true +SpacesInCStyleCastParentheses: false +SpacesInParentheses: false +SpacesInSquareBrackets: false +Standard: Cpp11 +StatementMacros: + - Q_UNUSED + - QT_REQUIRE_VERSION +TabWidth: 8 +UseTab: Never +... + diff --git a/sample_formatted_code.do_not_merge.c b/sample_formatted_code.do_not_merge.c new file mode 100644 index 000000000000..e0840fcde1fd --- /dev/null +++ b/sample_formatted_code.do_not_merge.c @@ -0,0 +1,562 @@ +// Formatted using +// clang-format -i sample_formatted_code.do_not_merge.c + +// SortIncludes +#include +#include +#include +#include "sample.llvm.h" + +//--- Line Length --- +// https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style#Line-length +// ColumnLimit: 80 +// ContinuationIndentWidth: 8 +static int some_really_long_variable_definition_that_is_80_chars_long = 1234567; +static int some_long_variable_definition_that_wraps_and_continues_at_next_line = + 1234567890123; + +//--- Indent --- +// https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style#Indent +// IndentWidth: 4 +// AlignAfterOpenBracket +int DecodeEthernet(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p, + uint8_t *pkt, uint16_t len, PacketQueue *pq) +{ + SCPerfCounterIncr(dtv->counter_eth, tv->sc_perf_pca); + + if (unlikely(len < ETHERNET_HEADER_LEN)) { + ENGINE_SET_INVALID_EVENT(p, ETHERNET_PKT_TOO_SMALL); + return TM_ECODE_FAILED; + } +} + +//--- Braces --- +// https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style#Braces +int SomeFunction(void) +{ + DoSomething(); +} + +void brace_style() +{ + if (unlikely(len < ETHERNET_HEADER_LEN)) { + ENGINE_SET_INVALID_EVENT(p, ETHERNET_PKT_TOO_SMALL); + return TM_ECODE_FAILED; + } + + if (this) { + DoThis(); + } else { + DoThat(); + } +} + +//--- Flow --- +// https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style#Flow +void if_style() +{ + // AllowShortIfStatementsOnASingleLine + if (a) + b = a; // <- right + + if (a) + b(); + else + c(); + + if (a) + return; + else { + return; + } + + if (a) + return; +} + +void for_loop_style() +{ + // for (no parens, would fit on one line if wanted) + for (int i = 0; i < 32; ++i) + i += 1; + + // for (parens, would fit on one line if wanted) + for (int i = 0; i < 32; ++i) { + i += 1; + } + + // for + for (int i : 0; i < some_max_number; ++i) { + int b = someFunctionCall(int16_t)*LongNameForParameter2, + (float *)LongNameForParameter2); + s.second++; + } +} + +void while_style() +{ + // AllowShortBlocksOnASingleLine + while (some) { + } + while (some) { + continue; + } + + // AllowShortLoopsOnASingleLine + while (true) + continue; +} + +void do_while_style() +{ + do { + a++; + } while (a == 0); + + do { + if (a) + a--; + else + a++; + } while (false); +} + +// functions - Functions should have the opening bracket on a newline: +// https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style#curly-braces-brackets +int some_function() +{ + int a = 13; + return a * a; +} + +// BraceWrapping:SplitEmptyFunction if BreakBeforeBraces: Custom +void empty_function(void) +{ +} + +// AllowShortFunctionsOnASingleLine +int short_function(void) +{ + return 1; +} + +// all params fit on continuation line +// AlignAfterOpenBracket +static void function_with_params_split( + const char *key, json_t *value, idmef_alert_t *alert) +{ + bla(); +} + +// params too long to fit on one continuation line, broken apart over multiple +// lines +// AlignAfterOpenBracket +int some_function_with_parms_split(uint32_t *LongNameForParameter1, + double *LongNameForParameter2, const float *LongNameForParameter3, + const struct SomeStructWithALongName LongNameForParameter4) +{ + int a = 3; + return a * a; +} + +//--- switch --- +// https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style#switch-statements +void switch_style() +{ + // IndentCaseBlocks + // IndentCaseLabels + + // Switch statements are indented like in the following example, so the + // 'case' is indented from the switch + switch (ntohs(p->ethh->eth_type)) { + case ETHERNET_TYPE_IP: + DecodeIPV4(tv, dtv, p, pkt + ETHERNET_HEADER_LEN, + len - ETHERNET_HEADER_LEN, pq); + break; + } + + // Fall through cases will be commented with /* fall through */. E.g.: + switch (suri->run_mode) { + case RUNMODE_PCAP_DEV: + case RUNMODE_AFP_DEV: + case RUNMODE_PFRING: + /* find payload for interface and use it */ + default_packet_size = GetIfaceMaxPacketSize(suri->pcap_dev); + if (default_packet_size) + break; + /* fall through */ + default: + default_packet_size = DEFAULT_PACKET_SIZE; + } + + // BraceWrapping:AfterCaseLabel if BreakBeforeBraces: Custom + // AllowShortCaseLabelsOnASingleLine + switch (a) { + case 13: { + int a = bla(); + break; + } + case 15: + blu(); + break; + default: + gugus(); + } +} + +//--- goto --- +// https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style#goto +void goto_style() +{ + DetectFileextData *fileext = NULL; + + fileext = SCMalloc(sizeof(DetectFileextData)); + if (unlikely(fileext == NULL)) + goto error; + + memset(fileext, 0x00, sizeof(DetectFileextData)); + + if (DetectContentDataParse("fileext", str, &fileext->ext, &fileext->len, + &fileext->flags) == -1) { + goto error; + } + + return fileext; + +error: + if (fileext != NULL) + DetectFileextFree(fileext); + return NULL; +} + +int goto_style_nested() +{ + // IndentGotoLabels + if (foo()) { + label1: + bar(); + } + +label2: + return 1; +} + +//--- ternary style --- +// BreakBeforeTernaryOperators - whether ? and : are on next line +void ternary_op_style() +{ + // fits on one line + float droppy = a > 0 ? a * 100 : 0; + + // split across lines - Continuation indent based on '=' fits line + float drop_percent = likely(ptv->last_stats64.ps_recv > 0) + ? (((float)ptv->last_stats64.ps_drop) / + (float)ptv->last_stats64.ps_recv) * + 100 + : 0; + + // split across lines - Continuation indent based on '=' would be too long, + // uses normal continuation indent based on start of line + float drop_percent_a_bit_longer = + likely(ptv->last_stats64.ps_recv > 0) + ? (((float)ptv->last_stats64.ps_drop) / + (float)ptv->last_stats64.ps_recv) * + 100 + : 0; +} + +//--- enum style --- +// clang 11: AllowShortEnumsOnASingleLine +// clang < 11: +// - merges short enums on one line if BraceWrapping: AfterEnum: false +// - one-value-by-line if BraceWrapping: AfterEnum: true +enum Gugus { bla, bli, blu }; + +enum ThisIsTooLongForOneLine { + blablablablablablablabla, + blibliblibliblibliblibli, + blublublublublublublublu +}; + +enum { A, B } myEnum; + +// trailing comma forces one-value-by-line +enum { + NFS_DECODER_EVENT_EMPTY_MESSAGE, +}; + +//--- union style --- +typedef union { + int gugus; +} Bla; + +union bla { + int gugus; +}; + +// --- struct style --- +struct bla { + int gugus; +}; + +struct bla_ { + int gugus; +} Bla; + +typedef struct bla_ { + int gugus; +} Bla; + +//--- Alignment --- +struct bla { + // AlignConsecutiveDeclarations + // AlignTrailingComments + int a; /* comment */ + unsigned bb; /* comment */ + int *ccc; /* comment */ +}; + +// pointers +// DerivePointerAlignment +// PointerAlignment +void *ptr; +void f(int *a, const char *b); +void (*foo)(int *); + +void alignment() +{ + // multiple consecutive vars (comments and vars can be aligned) + // AlignConsecutiveAssignments + // AlignConsecutiveDeclarations + // AlignTrailingComments + int a = 13; /* comment */ + int32_t abc = 1312; /* comment */ + int abcdefghikl = 13; /* comment */ + + // AlwaysBreakBeforeMultilineStrings + aaaa = "bbbb" + "ccc"; + + //--- variable init continuation behaviour + // Cpp11BracedListStyle impacts if space at beginning and end of brace + // e.g. false: Bla bla[] = { 1, 2, 3 }; + // true: Bla bla[] = {1, 2, 3}; + // Note, the different indentation of continuation line: + // - Cpp11BracedListStyle:false does NOT use ContinuationIndentWidth (8) + // but rather the regular IndentWidth (4)! Bug? Feature? + // - Cpp11BracedListStyle:true uses ContinuationIndentWidth (8) + int list[] = {1, 2, 3, 4, 5, 6, 7, 8, 9}; + int list[] = { + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18}; + int list[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, + 19, 20, 21, 22}; + PcapStats64 last = {0, 1234, 8765}; + // trailing comma forces one-value-by-line + PcapStats64 trailing_comma = { + 0, + 1234, + 8765, + }; + struct pcap_stat current = {12, 2345, 9876, 1345}; + struct pcap_stat current = { + 12, 2345, 9876, 1345, 333, 444, 5555, 66666, 7777777}; + current = {12, 2345, 9876, 1345, 333, 444, 5555, 66666, 7777777, 888888, + 9999999}; + + //--- designated initializer continuation behaviour + // Current clang-format disables BinPacking for designated intializers when + // continuing on more than one line. + PcapStats64 last = {.ps_recv = 0, .ps_drop = 1234, .ps_ifdrop = 8765}; + // Cpp11BracedListStyle:false puts end brace onto separate line iff + // continuation line can hold all intializers. Bug? Feature? + struct pcap_stat current = { + .ps_recv = 12, .ps_drop = 2345, .ps_ifdrop = 9876, .ps_what = 134}; + pcap_stat current = { + .ps_recv = 12, .ps_drop = 2345, .ps_ifdrop = 9876, .ps_what = 134}; + // One designated initializer per line if it does not fit into one + // continuation line as BinPacking is disabled for designated intializer. + struct pcap_stat current = {.ps_recv = 12, + .ps_drop = 2345, + .ps_ifdrop = 9876, + .ps_what = 1345, + .ps_more = 333}; + current = {.ps_recv = 12, + .ps_drop = 2345, + .ps_ifdrop = 9876, + .ps_what = 1345, + .ps_more = 333}; + + // function call continuation + function_call( + with, many, params, that, will spill, over, eventually, iff, max); + function_call(with, many, params, that, will spill, over, eventually, iff, + we, keep, on, adding); +} + +struct Bitfields { + // clang 11: AlignConsecutiveBitFields + int aaaa : 1; + int b : 12; + int ccc : 8; +}; + +static void wrapping_literals() +{ + // string literal is too long. Continuation is from "string literal start", + // not ContinuationIndentWith, due to reasons? + SCLogInfo("running in 'auto' checksum mode. Detection of interface " + "state will require " xstr(CHECKSUM_SAMPLE_COUNT) " packets"); + + // Same as above but with additional parameter using ContinuationIndentWith + SCLogInfo("running in 'auto' checksum mode. Detection of interface " + "state will require " xstr(CHECKSUM_SAMPLE_COUNT) " packets %d", + someValue); + + // Just params use ContinuationIndentWith + SCLogError(SC_ERR_STAT, "(%s) Failed to get pcap_stats: %s", tv->name, + pcap_geterr(ptv->pcap_handle)); + + // string literal param that fits on continuation line uses + // ContinuationIndentWith + SCLogError(SC_ERR_INITIALIZATION, + "Error getting context for Prelude. \"initdata\" argument NULL"); + if (unlikely(initdata == NULL)) { + // string literal param does not fit on continuation line + // Continuation is from "string literal start", not + // ContinuationIndentWith, due to reasons + SCLogError(SC_ERR_INITIALIZATION, "Error getting context for Prelude. " + "\"initdata\" argument NULL"); + SCReturnInt(TM_ECODE_FAILED); + } + + // function call inside if needs breaking apart and uses + // ContinuationIndentWith starting at function. Nice. + if (DetectContentDataParse("fileext", str, &fileext->ext, &fileext->len, + &fileext->flags) == -1) { + goto error; + } +} + +//--- foreach handling as "for loop" --- +void foreach_handling() +{ + // ForEachMacros + + // json_object_foreach and json_array_foreach are "foreach" functions + if (json_is_object(value)) { + json_object_foreach (value, key_js, value_js) { + bla() + } + } else if (json_is_array(value)) { + json_array_foreach (value, index, value_js) { + bla() + } + } else if (json_is_integer(value)) { + ret = AddIntData(alert, key, json_integer_value(value)); + } + + // These are foreach macros but apart from the odd exception, they + // use start parens on next line if they are used in code. + // It would be trivial to also handle them like "for loops" + + // tree.h: SLIST_FOREACH, SLIST_FOREACH_PREVPTR, LIST_FOREACH, + // SIMPLEQ_FOREACH, TAILQ_FOREACH, TAILQ_FOREACH_SAFE, + // TAILQ_FOREACH_REVERSE, CIRCLEQ_FOREACH, CIRCLEQ_FOREACH_REVERSE, + // CIRCLEQ_FOREACH_SAFE, CIRCLEQ_FOREACH_REVERSE_SAFE + + TAILQ_FOREACH(child, &node->head, next) + { + name[level] = SCStrdup(child->name); + /* ... */ + SCFree(name[level]); + } + + // queue.h: SPLAY_FOREACH, RB_FOREACH, RB_FOREACH_FROM, RB_FOREACH_SAFE, + // RB_FOREACH_REVERSE, RB_FOREACH_REVERSE_FROM, RB_FOREACH_REVERSE_SAFE + RB_FOREACH_REVERSE_FROM(tree_seg, TCPSEG, s) + { + if (tree_seg == seg) + continue; + /* ... */ + } +} + +//--- comment wrapping --- +void multi_line_comments() +{ + // ReflowComments: false does not trim comments to ColumnLimit chars + + // TODO: This is a long comment that allows you to understand how long + // comments will be trimmed. Here should be deep thought but it's just not + // right time for this + + /* TODO: This is a long comment that allows you to understand how long + * comments will be trimmed. Here should be deep thought but it's just not + * right time for this + */ +} + +//--- macros --- +#define BIT_MASK 0xDEADBEAF + +// alignment of macro values +// AlignConsecutiveMacros +#define ACTION_ALERT 0x01 +#define ACTION_DROP 0x02 +#define ACTION_REJECT 0x04 +#define ACTION_REJECT_DST 0x08 +#define ACTION_REJECT_BOTH 0x10 +#define ACTION_PASS 0x20 + +// multi-line macros (alignment of backslash can be changed) +// AlignEscapedNewlines: DontAlign, Left, Right +#define MULTILINE_DEF(a, b) \ + if ((a) > 2) { \ + auto temp = (b) / 2; \ + (b) += 10; \ + someFunctionCall((a), (b)); \ + } + +// Formatting of macros cannot be separately configured +#define TAILQ_INIT(head) \ + do { \ + (head)->tqh_first = NULL; \ + (head)->tqh_last = &(head)->tqh_first; \ + } while (0) + +#define SLIST_INIT(head) \ + { \ + SLIST_FIRST(head) = SLIST_END(head); \ + } + +#define APP_LAYER_INCOMPLETE(c, n) \ + (AppLayerResult) \ + { \ + 1, (c), (n) \ + } +// but... +#define APP_LAYER_INCOMPLETE(c, n) ((AppLayerResult){1, (c), (n)}) + +// Only solution is to clang-format on/off if it does not please +/* clang-format off */ +#define APP_LAYER_INCOMPLETE(c, n) (AppLayerResult){1, (c), (n)} +/* clang-format on */ + +//--- disabling formatting --- +// yes, it formats/indents the actual clang-format off/on comment +void disable_formatting() +{ + /* clang-format off */ +int a = 16; +int32_t b = whatever( "I wanna have my own style" + , "with some" +, "params all crooked"); + /* clang-format on */ + + // this is formatted again: + int32_t b = whatever( + "I wanna have my own style", "with some", "params all crooked"); +} + +//--- EOF --- +// clang-format removes trailing empty lines From af5cbd831a146d6b0d7c1a02c9a0cc14729eada4 Mon Sep 17 00:00:00 2001 From: Roland Fischer Date: Fri, 3 Jul 2020 00:22:03 -0400 Subject: [PATCH 2/4] Add dev code-style --- doc/devguide/codebase/code-style.rst | 480 +++++++++++++++++++++++++-- 1 file changed, 460 insertions(+), 20 deletions(-) diff --git a/doc/devguide/codebase/code-style.rst b/doc/devguide/codebase/code-style.rst index 0c55cc99d8ef..ba2d5a3168ae 100644 --- a/doc/devguide/codebase/code-style.rst +++ b/doc/devguide/codebase/code-style.rst @@ -6,12 +6,165 @@ Suricata uses a fairly strict coding style. This document describes it. Formatting ~~~~~~~~~~ +clang-format +^^^^^^^^^^^^ + +.. Argh, github does not support admonitions such as .. note:: + ++-------------------------------------------------------+ +| **Note** | +| | +| The ``.clang-format`` file requires clang 9 or newer. | ++-------------------------------------------------------+ + +``clang-format`` is configured to help you with formatting C code. + + +Use ``git clang-format`` to only format the changes in your branch. + +Format your Changes +******************* +Before opening a pull request, please also try to ensure it is formatted +properly. We use ``clang-format`` for this, which has git integration through the +``git-clang-format`` script. On some systems, it may already be installed (or +be installable via your package manager). If so, you can simply run it – the +following command will format only the code changed in the most recent commit: + +.. code-block:: bash + + $ git clang-format HEAD~1 # or HEAD^ + +Note that this modifies the files, but doesn’t commit them – you’ll likely want to run + +.. code-block:: bash + + $ git commit --amend -a + +in order to update the last commit with all pending changes. + +Use make targets +"""""""""""""""" +Make targets are provided for easier usage, especially if you have multiple +commits to format. + +Check if you branch changes' formatting is correct with: + +.. code-block:: bash + + $ make check-style-branch + +Format changes in git staging and unstaged changes, i.e. code you are currently +editing. + +.. code-block:: bash + + $ make style + # Or with git clang-format + $ git clang-format --force + +Format every commits in your branch off master and rewrite history using the +existing commit metadata. Tip: Create a new version of your branch first and run +this off the new version to have the existing branch around "just in case". + +.. code-block:: bash + + $ make style-rewrite-branch + +Format all changes in your branch. Allows you to add the formatting as an +additional commit "at the end". Prefer to use make style-rewrite-branch instead. + +.. code-block:: bash + + $ make style-branch + +Formatting a whole file +""""""""""""""""""""""" + +.. Argh, github does not support admonitions such as .. note:: + ++--------------------------------------------------------------------+ +| **Note** | +| | +| Do not reformat whole files by default, i.e. do not use | +| clang-format proper in general. | ++--------------------------------------------------------------------+ + +If you were ever to do so, formatting changes of existing code with clang-format +shall be a different commit and must not be mixed with actual code changes. + +.. code-block:: bash + + $ clang-format -i {file} + +Disabling clang-format +********************** + +There might be times, where the clang-format's formatting might not please. +This might mostly happen with macros, arrays (single or multi-dimensional ones), +struct initialization, or where one manually formatted code. + +You can always disable clang-format. + +.. code-block:: c + + /* clang-format off */ + #define APP_LAYER_INCOMPLETE(c, n) (AppLayerResult){1, (c), (n)} + /* clang-format on */ + +Installing clang-format and git-clang-format +******************************************** +clang-format 9 is required. + +TODO - REMOVEME: We could possibly provide an "install-clang-format" script if we wanted to. +It's essentially a decision of maintaining the install script vs support questions about +"hey I have ubuntu xyz, what do I need to do?". + +On ubuntu 18.04: + +- It is sufficient to only install clang-format, e.g. + + .. code-block:: bash + + $ wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add - + $ sudo add-apt-repository "deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-9 main" + $ sudo apt-get update + $ sudo apt-get install -y clang-format-9 + +- see http://apt.llvm.org for other releases + +On fedora: + +- Install the ``clang`` and ``git-clang-format`` packages with + + .. code-block:: bash + + $ sudo dnf install clang git-clang-format + + Line length ^^^^^^^^^^^ -There is a soft limit of ~80 characters. +There is a soft limit of ~80 characters. There is a hard limit of 100. + +When wrapping lines that are too long, they should be indented at least 8 +spaces from previous line. You should attempt to wrap the minimal portion of +the line to meet the 80 character limit. + +TODO - REMOVEME: 80 vs 100 width? @victorjulien mentioned maybe longer? +Even linux is on 100 these days. However, keeping it to 80 would induce the +fewest changes if existing code gets reformatted. + +TODO - REMOVEME: We should remove the "soft limit" as clang-format only has a +hard limit. + +TODO - REMOVEME: Reflow comments, i.e. also adjust comments to length? Yup, for +@jasonish so I set it to do so. + +clang-format: + - ColumnLimit: 80 + - ContinuationIndentWidth: 8 + - ReflowComments: true -When wrapping lines that are too long, they should be indented at least 8 spaces from previous line. You should attempt to wrap the minimal portion of the line to meet the 80 character limit. Indent ^^^^^^ @@ -21,7 +174,7 @@ We use 4 space indentation. .. code-block:: c int DecodeEthernet(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p, - uint8_t *pkt, uint16_t len, PacketQueue *pq) + uint8_t *pkt, uint16_t len, PacketQueue *pq) { SCPerfCounterIncr(dtv->counter_eth, tv->sc_perf_pca); @@ -30,6 +183,19 @@ We use 4 space indentation. return TM_ECODE_FAILED; } +Note, use 8 space indentation when wrapping function parameters. + +NOTE - REMOVEME: tab default width of 8, not 4, as that's most editors default + +NOTE - REMOVEME: Old sample code function parameter indentation was 4. Indentation for next line of function parameters follows +ContinuationIndentWidth as we have AlignAfterOpenBracket: DontAlign + +clang-format: + - IndentWidth: 4 + - UseTab: Never [llvm]_ + - TabWidth: 8 [llvm]_ + - AlignAfterOpenBracket: DontAlign + Braces ^^^^^^ @@ -42,8 +208,9 @@ Functions should have the opening brace on a newline: DoSomething(); } +Note: this is a fairly new requirement, so you'll encounter a lot of non-compliant code. -Control statements should have the opening brace on the same line: +Control and loop statements should have the opening brace on the same line: .. code-block:: c @@ -52,6 +219,16 @@ Control statements should have the opening brace on the same line: return TM_ECODE_FAILED; } + for (ascii_code = 0; ascii_code < 256; ascii_code++) { + ctx->goto_table[ctx->state_count][ascii_code] = SC_AC_FAIL; + } + + while (funcs != NULL) { + temp = funcs; + funcs = funcs->next; + SCFree(temp); + } + Opening and closing braces go on the same line as as the _else_ (also known as a "cuddled else"). .. code-block:: c @@ -62,6 +239,41 @@ Opening and closing braces go on the same line as as the _else_ (also known as a DoThat(); } +Structs, unions and enums should have the opening brace on the same line: + +.. code-block:: c + + union { + TCPVars tcpvars; + ICMPV4Vars icmpv4vars; + ICMPV6Vars icmpv6vars; + } l4vars; + + struct { + uint8_t type; + uint8_t code; + } icmp_s; + + enum { + DETECT_TAG_TYPE_SESSION, + DETECT_TAG_TYPE_HOST, + DETECT_TAG_TYPE_MAX + }; + +clang-format: + - BreakBeforeBraces: Custom [breakbeforebraces]_ + - BraceWrapping: + + - AfterClass: true + - AfterControlStatement: false + - AfterEnum: false + - AfterFunction: true + - AfterStruct: false + - AfterUnion: false + - AfterExternBlock: true + - BeforeElse: false + - IndentBraces: false + Flow ~~~~ @@ -74,6 +286,24 @@ Don't use conditions and statements on the same line. E.g. if (a) b = a; // <- right + for (int i = 0; i < 32; ++i) f(i); // <- wrong + + for (int i = 0; i < 32; ++i) + f(i); // <- right + +Don't put short or empty functions and structs on one line. + +.. code-block:: c + + void empty_function(void) + { + } + + int short_function(void) + { + return 1; + } + Don't use unnecessary branching. E.g.: .. code-block:: c @@ -94,6 +324,91 @@ Can be written as: } a = b; +clang-format: + - AllowShortBlocksOnASingleLine: false [llvm]_ + - AllowShortBlocksOnASingleLine: Never [llvm]_ (breaking change in clang 10!) [clang10]_ + - AllowShortEnumsOnASingleLine: false [clang11]_ + - AllowShortFunctionsOnASingleLine: None + - AllowShortIfStatementsOnASingleLine: Never [llvm]_ + - AllowShortLoopsOnASingleLine: false [llvm]_ + - BreakBeforeBraces: Custom [breakbeforebraces]_ + - BraceWrapping: + + - SplitEmptyFunction: true + - SplitEmptyRecord: true + +Alignment +~~~~~~~~~ + +Pointers +^^^^^^^^ +Pointers shall be right aligned. + +.. code-block:: c + + void *ptr; + void f(int *a, const char *b); + void (*foo)(int *); + +clang-format: + - PointerAlignment: Right + - DerivePointerAlignment: false + +Declarations and Comments +^^^^^^^^^^^^^^^^^^^^^^^^^ +Trailing comments should be aligned for consecutive lines. + +.. code-block:: c + + struct bla { + int a; /* comment */ + unsigned bb; /* comment */ + int *ccc; /* comment */ + }; + + void alignment() + { + // multiple consecutive vars + int a = 13; /* comment */ + int32_t abc = 1312; /* comment */ + int abcdefghikl = 13; /* comment */ + + // AlwaysBreakBeforeMultilineStrings + aaaa = "bbbb" + "ccc"; + ... + } + +.. code-block:: c + + //vvv--- REMOVEME + // FORMAT OPTION: AlignTrailingComments (bool) + // If true, aligns trailing comments. + true: false: + int a; // My comment a vs. int a; // My comment a + int b = 2; // comment b int b = 2; // comment about b + + // FORMAT OPTION: AlignConsecutiveDeclarations (bool) + // If true, aligns consecutive declarations. + // This will align the declaration names of consecutive lines. This will result in formattings like + int aaaa = 12; + float b = 23; + std::string ccc = 23; + float * b = 23; // clang-format feature/bug with right-aligned ptr + + // FORMAT OPTION: AlignConsecutiveAssignments (bool) + // If true, aligns consecutive assignments. + // This will align the assignment operators of consecutive lines. This will result in formattings like + int aaaa = 12; + int b = 23; + int ccc = 23; + //^^^--- REMOVEME + +clang-format: + - AlignConsecutiveAssignments: false + - AlignConsecutiveDeclarations: false + - AlignTrailingComments: true + Functions ~~~~~~~~~ @@ -121,20 +436,6 @@ inline The inlining of functions should be used only in critical paths. -curly braces / brackets -^^^^^^^^^^^^^^^^^^^^^^^ - -Functions should have the opening bracket on a newline: - -.. code-block:: c - - int SomeFunction(void) - { - DoSomething(); - } - -Note: this is a fairly new requirement, so you'll encounter a lot of non-compliant code. - Variables ~~~~~~~~~ @@ -159,7 +460,59 @@ TODO Macros ~~~~~~ -TODO +Macro names are ALL_CAPS_WITH_UNDERSCORES. +Enclose parameters in parens on each usage inside the macro. + +Align macro values on consecutive lines. + +.. code-block:: c + + #define ACTION_ALERT 0x01 + #define ACTION_DROP 0x02 + #define ACTION_REJECT 0x04 + #define ACTION_REJECT_DST 0x08 + #define ACTION_REJECT_BOTH 0x10 + #define ACTION_PASS 0x20 + +Align escape for multi-line macros left-most. + +.. code-block:: c + + #define MULTILINE_DEF(a, b) \ + if ((a) > 2) { \ + auto temp = (b) / 2; \ + (b) += 10; \ + someFunctionCall((a), (b)); \ + } + +.. code-block:: c + + //vvv--- REMOVEME + // FORMAT OPTION: AlignEscapedNewlines + // Options for aligning backslashes in escaped newlines. + + // DontAlign: Don’t align escaped newlines. + #define A \ + int aaaa; \ + int b; \ + int dddddddddd; + + // Left: Align escaped newlines as far left as possible. + #define A \ + int aaaa; \ + int b; \ + int dddddddddd; + + // Right: Align escaped newlines in the right-most column. + #define A \ + int aaaa; \ + int b; \ + int dddddddddd; + //^^^--- REMOVEME + +clang-format: + - AlignConsecutiveMacros: true [clang9]_ + - AlignEscapedNewlines: Left Comments ~~~~~~~~ @@ -205,7 +558,24 @@ Some cases have a multi-layer prefix, e.g. ``util-mpm-ac.c`` Enums ~~~~~ -TODO +Use a common prefix for all enum values. Value names are ALL_CAPS_WITH_UNDERSCORES. + +Put each enum values on a separate line. +Tip: Add a trailing comma to the last element to force "one-value-per-line" +formatting in clang-format. + +.. code-block:: c + + enum { VALUE_ONE, VALUE_TWO }; // <- wrong + + // right + enum { + VALUE_ONE, + VALUE_TWO, // <- force one-value-per-line + }; + +clang-format: + - AllowShortEnumsOnASingleLine: false [clang11]_ Structures and typedefs ~~~~~~~~~~~~~~~~~~~~~~~ @@ -241,6 +611,34 @@ Fall through cases will be commented with ``/* fall through */``. E.g.: default: default_packet_size = DEFAULT_PACKET_SIZE; + +Do not put short case labels on one line. +Put opening brace on same line as case statement. + +.. code-block:: c + + switch (a) { + case 13: { + int a = bla(); + break; + } + case 15: + blu(); + break; + default: + gugus(); + } + + +clang-format: + - IndentCaseLabels: true + - IndentCaseBlocks: false [clang11]_ + - AllowShortCaseLabelsOnASingleLine: false [llvm]_ + - BreakBeforeBraces: Custom [breakbeforebraces]_ + - BraceWrapping: + + - AfterCaseLabel: false (default) + const ~~~~~ @@ -275,6 +673,40 @@ Goto statements should be used with care. Generally, we use it primarily for err return NULL; } +Nested goto labels are indented. + +.. code-block:: c + + int goto_style_nested() + { + if (foo()) { + label1: + bar(); + } + + label2: + return 1; + } + +TODO - REMOVEME: This is only configurable to left-most as of clang 10 so we +could just leave the "nested goto" out to "not overload things". + +clang-format: + - IndentGotoLabels: true (default) [clang10]_ + +Includes +~~~~~~~~ + +TODO + +A .c file shall include it's own header first. + +TODO - REMOVEME: clang-format could sort includes and group them if configured +to do so. This might break compilation until fixed. + +clang-format: + - SortIncludes: false + Unittests ~~~~~~~~~ @@ -332,3 +764,11 @@ Banned functions Also, check the existing code. If yours is wildly different, it's wrong. Example: https://github.com/oisf/suricata/blob/master/src/decode-ethernet.c + +.. rubric:: Footnotes + +.. [llvm] Default LLVM clang-format Style +.. [clang9] Requires clang 9 +.. [clang10] Requires clang 10 +.. [clang11] Requires clang 11 +.. [breakbeforebraces] BreakBeforeBraces: Mozilla is closest, but does not split empty functions/structs \ No newline at end of file From 7df3813d2b0b532bb795d13335484091cd426473 Mon Sep 17 00:00:00 2001 From: Roland Fischer Date: Fri, 3 Jul 2020 00:22:35 -0400 Subject: [PATCH 3/4] Add make support for clang-format --- Makefile.am | 34 ++ scripts/clang-format.sh | 591 +++++++++++++++++++++++++++++++ scripts/git-clang-format-custom | 600 ++++++++++++++++++++++++++++++++ 3 files changed, 1225 insertions(+) create mode 100755 scripts/clang-format.sh create mode 100755 scripts/git-clang-format-custom diff --git a/Makefile.am b/Makefile.am index c920180fa664..b1f9e5cd5b47 100644 --- a/Makefile.am +++ b/Makefile.am @@ -53,3 +53,37 @@ endif @echo "For more information please see:" @echo " https://suricata.readthedocs.io/en/latest/rule-management/index.html" @echo "" + +#--- clang-formatting --- +# Check if branch changes formatting is correct +.PHONY: check-style-branch +check-style-branch: + ./scripts/clang-format.sh check-branch --make + +# Check if branch changes formatting is correct and print diff +.PHONY: diff-style-branch +diff-style-branch: + ./scripts/clang-format.sh check-branch --make --diff + +# Check if branch changes formatting is correct and print diffstat, i.e. files +.PHONY: diffstat-style-branch +diffstat-style-branch: + ./scripts/clang-format.sh check-branch --make --diffstat --show-commits + +# Format changes in git staging +.PHONY: style +style: + ./scripts/clang-format.sh cached + +# Reformat all changes in branch. Allows you to add the formatting as a separate +# commit "at the end". +.PHONY: style-branch +style-branch: + ./scripts/clang-format.sh branch + +# Reformat all commits in branch off master and rewrite history using the +# existing commit metadata. +.PHONY: style-rewrite-branch +style-rewrite-branch: + ./scripts/clang-format.sh rewrite-branch + diff --git a/scripts/clang-format.sh b/scripts/clang-format.sh new file mode 100755 index 000000000000..ce436b445b32 --- /dev/null +++ b/scripts/clang-format.sh @@ -0,0 +1,591 @@ +#!/bin/bash +# +# Script to clang-format suricata C code changes +# +# Rewriting branch parts of it is inspired by +# https://www.thetopsites.net/article/53885283.shtml + +#set -e +#set -x + +EXIT_CODE_ERROR=2 +EXIT_CODE_FORMATTING_REQUIRED=1 +EXIT_CODE_OK=0 + +PRINT_DEBUG=0 +# Debug output if PRINT_DEBUG is 1 +function Debug { + if [ $PRINT_DEBUG -ne 0 ]; then + echo "DEBUG: $@" + fi +} + +# ignore text formatting by default +bold= +normal= +italic= +# $TERM is set to dumb when calling scripts in github actions. +if [ -n "$TERM" -a "$TERM" != "dumb" ]; then + Debug "TERM: '$TERM'" + # tput, albeit unlikely, might not be installed + command -v tput >/dev/null 2>&1 # built-in which + if [ $? -eq 0 ]; then + Debug "Setting text formatting" + bold=$(tput bold) + normal=$(tput sgr0) + italic=$(echo -e '\E[3m') + fi +else + Debug "No text formatting" +fi + +EXEC=$(basename $0) +pushd . >/dev/null # we might change dir - save so that we can revert + +USAGE=$(cat << EOM +usage: $EXEC --help + $EXEC help + $EXEC [] + +Format selected changes using clang-format. + +Note: This does ONLY format the changed code, not the whole file! It +uses ${italic}git-clang-format${normal} for the actual formatting. If you want to format +whole files, use ${italic}clang-format -i ${normal}. + + +Commands used in various situations: + +Formatting branch changes (compared to master): + branch Format all changes in branch as additional commit + rewrite-branch Format every commit in branch and rewrite history + +Formatting single changes: + cached Format changes in git staging + +Checking if formatting is correct: + check-branch Checks if formatting for branch changes is correct + +More info an a command: + help Display more info for a particular +EOM +) + +HELP_BRANCH=$(cat << EOM +${bold}NAME${normal} + $EXEC branch - Format all changes in branch as additional commit + +${bold}SYNOPSIS${normal} + $EXEC branch [--force] + +${bold}DESCRIPTION${normal} + Format all changes in your branch enabling you to add it as an additional + formatting commit. + + Requires that all changes are committed unless --force is provided. + + You will need to commit the reformatted code. + +${bold}OPTIONS${normal} + -f, --force + Allow changes to unstaged files. + +${bold}EXAMPLES${normal} + On your branch whose changes you want to reformat: + + $ $EXEC branch + +${bold}EXIT STATUS${normal} + $EXEC exits with a status of zero if the changes were successfully + formatted, or if no formatting change was required. A status of two will + be returned if any errors were encountered. +EOM +) + +HELP_CACHED=$(cat << EOM +${bold}NAME${normal} + $EXEC cached - Format changes in git staging + +${bold}SYNOPSIS${normal} + $EXEC cached [--force] + +${bold}DESCRIPTION${normal} + Format staged changes using clang-format. + + You will need to commit the reformatted code. + +${bold}OPTIONS${normal} + -f, --force + Allow changes to unstaged files. + +${bold}EXAMPLES${normal} + Format all changes in staging, i.e. in files added with ${italic}git add ${normal}. + + $ $EXEC cached + +${bold}EXIT STATUS${normal} + $EXEC exits with a status of zero if the changes were successfully + formatted, or if no formatting change was required. A status of two will + be returned if any errors were encountered. +EOM +) + +HELP_CHECK_BRANCH=$(cat << EOM +${bold}NAME${normal} + $EXEC check-branch - Checks if formatting for branch changes is correct + +${bold}SYNOPSIS${normal} + $EXEC check-branch [--show-commits] [--make] [--quiet] + $EXEC check-branch --diff [--show-commits] [--make] [--quiet] + $EXEC check-branch --diffstat [--show-commits] [--make] [--quiet] + +${bold}DESCRIPTION${normal} + Check if all branch changes are correctly formatted. + + Note, it does not check every commit's formatting, but rather the + overall diff between HEAD and master. + + Returns 1 if formatting is off, 0 if it is correct. + +${bold}OPTIONS${normal} + -d, --diff + Print formatting diff, i.e. diff of each file with correct formatting. + -s, --diffstat + Print formatting diffstat output, i.e. files with wrong formatting. + -c, --show-commits + Print branch commits. + -m, --make + Print commands as make calls. If not set, print as clang-format.sh calls. + -q, --quiet + Do not print any error if formatting is off, only set exit code. + +${bold}EXIT STATUS${normal} + $EXEC exits with a status of zero if the formatting is correct. A + status of one will be returned if the formatting is not correct. A status + of two will be returned if any errors were encountered. +EOM +) + +HELP_REWRITE_BRANCH=$(cat << EOM +${bold}NAME${normal} + $EXEC rewrite-branch - Format every commit in branch and rewrite history + +${bold}SYNOPSIS${normal} + $EXEC rewrite-branch + +${bold}DESCRIPTION${normal} + Reformat all commits in branch off master one-by-one. This will ${bold}rewrite + the branch history${normal} using the existing commit metadata! + + This is handy in case you want to format all of your branch commits + while keeping the commits. + + This can also be helpful if you have multiple commits in your branch and + the changed files have been reformatted, i.e. where a git rebase would + fail in many ways over-and-over again. + + You can achieve the same manually on a separate branch by: + ${italic}git checkout -n ${normal}, + ${italic}git clang-format${normal} and ${italic}git commit${normal} for each original commit in your branch. + +${bold}OPTIONS${normal} + None + +${bold}EXAMPLES${normal} + In your branch that you want to reformat. Commit all your changes prior + to calling: + + $ $EXEC rewrite-branch + +${bold}EXIT STATUS${normal} + $EXEC exits with a status of zero if the changes were successfully + formatted, or if no formatting change was required. A status of two will + be returned if any errors were encountered. +EOM +) + +# Error message on stderr +function Error { + echo "${bold}ERROR${normal}: $@" 1>&2 +} + +# Exit program (and reset path) +function ExitWith { + popd >/dev/null # we might have changed dir + + if [ $# -ne 1 ]; then + # Huh? No exit value provided? + Error "Internal: ExitWith requires parameter" + exit $EXIT_CODE_ERROR + else + exit $1 + fi +} + +# Failure exit with error message +function Die { + Error $@ + ExitWith $EXIT_CODE_ERROR +} + +# Ensure required program exists. Exits with failure if not found. +# Call with +# RequireProgram ENVVAR_TO_SET program ... +# One can provide multiple alternative programs. Returns first program found in +# provided list. +function RequireProgram { + if [ $# -lt 2 ]; then + Die "Internal - RequireProgram: Need env and program parameters" + fi + + # eat variable to set + local envvar=$1 + shift + + for program in $@; do + command -v $program >/dev/null 2>&1 # built-in which + if [ $? -eq 0 ]; then + eval "$envvar=$(command -v $program)" + return + fi + done + + if [ $# -eq 1 ]; then + Die "$1 not found" + else + Die "None of $@ found" + fi +} + +# Make sure we are running from the top-level git directory. +# Same approach as for setup-decoder.sh. Good enough. +# We could probably use git rev-parse --show-toplevel to do so, as long as we +# handle the libhtp subfolder correctly. +function SetTopLevelDir { + if [ -e ./src/suricata.c ]; then + # Do nothing. + true + elif [ -e ./suricata.c -o -e ../src/suricata.c ]; then + cd .. + else + Die "This does not appear to be a suricata source directory." + fi +} + +# print help for given command +function HelpCommand { + local help_command=$1 + local HELP_COMMAND=$(echo "HELP_$help_command" | sed "s/-/_/g" | tr [:lower:] [:upper:]) + case $help_command in + branch|cached|check-branch|rewrite-branch) + echo "${!HELP_COMMAND}"; + ;; + + "") + echo "$USAGE"; + ;; + + *) + echo "$USAGE"; + echo ""; + Die "No manual entry for $help_command" + ;; + esac +} + +# Return first commit of branch (off master). +# +# Use $first_commit^ if you need the commit on master we branched off. +# Do not compare with master directly as it will diff with the latest commit +# on master. If our branch has not been rebased on the latest master, this +# would result in including all new commits on master! +function FirstCommitOfBranch { + local first_commit=$(git rev-list origin/master..HEAD | tail -n 1) + echo $first_commit +} + +# Check if branch formatting is correct. +# Compares with master branch as baseline which means it's limited to branches +# other than master. +# Exits with 1 if not, 0 if ok. +function CheckBranch { + # check parameters + local quiet=0 + local from_make=0 + local show_diff=0 + local show_diffstat=0 + local show_commits=0 + local git_clang_format="$GIT_CLANG_FORMAT --diff" + while [[ $# -gt 0 ]] + do + case "$1" in + -q|--quiet) + quiet=1 + shift + ;; + + -m|--make) + from_make=1 + shift + ;; + + -d|--diff) + show_diff=1 + shift + ;; + + -s|--diffstat) + show_diffstat=1 + git_clang_format="$GIT_CLANG_FORMAT_DIFFSTAT --diffstat" + shift + ;; + + -c|--show-commits) + show_commits=1 + shift + ;; + + *) # unknown option + echo "$HELP_CHECK_BRANCH"; + echo ""; + Die "Unknown $command option: $1" + ;; + esac + done + + if [ $show_diffstat -eq 1 -a $show_diff -eq 1 ]; then + echo "$HELP_CHECK_BRANCH"; + echo ""; + Die "Cannot combine $command options --diffstat with --diff" + fi + + # Find first commit on branch. Use $first_commit^ if you need the + # commit on master we branched off. + local first_commit=$(FirstCommitOfBranch) + + # git-clang-format is a python script that does not like SIGPIPE shut down + # by "| head" prematurely. Use work-around with writing to tmpfile first. + local format_changes="$git_clang_format --extensions c,h $first_commit^" + local tmpfile=$(mktemp /tmp/clang-format.check.XXXXXX) + $format_changes > $tmpfile + local changes=$(cat $tmpfile | head -1) + if [ $show_diff -eq 1 -o $show_diffstat -eq 1 ]; then + cat $tmpfile + echo "" + fi + rm $tmpfile + + # Branch commits can help with trouble shooting. Print after diff/diffstat + # as output might be tail'd + if [ $show_commits -eq 1 ]; then + echo "Commits on branch (new -> old):" + git log --oneline $first_commit^..HEAD + echo "" + else + if [ $quiet -ne 1 ]; then + echo "First commit on branch: $first_commit" + fi + fi + + # Exit code of git-clang-format is useless as it's 0 no matter if files + # changed or not. Check actual output. Not ideal, but works. + if [ "${changes}" != "no modified files to format" -a \ + "${changes}" != "clang-format did not modify any files" ]; then + if [ $quiet -ne 1 ]; then + Error "Branch requires formatting" + Debug "View required changes with clang-format: ${italic}$format_changes${normal}" + if [ $from_make -eq 1 ]; then + Error "View required changes with: ${italic}make diff-style-branch${normal}" + Error "Use ${italic}make style-rewrite-branch${normal} or ${italic}make style-branch${normal} to fix formatting" + else + Error "View required changes with: ${italic}$EXEC $command --diff${normal}" + Error "Use ${italic}$EXEC rewrite-branch${normal} or ${italic}$EXEC branch${normal} to fix formatting" + fi + ExitWith $EXIT_CODE_FORMATTING_REQUIRED + else + return $EXIT_CODE_FORMATTING_REQUIRED + fi + else + if [ $quiet -ne 1 ]; then + echo "no modified files to format" + fi + return $EXIT_CODE_OK + fi +} + +# Reformat all changes in branch as a separate commit. +function ReformatBranch { + # check parameters + local with_unstaged= + if [ $# -gt 1 ]; then + echo "$HELP_BRANCH"; + echo ""; + Die "Too many $command options: $1" + elif [ $# -eq 1 ]; then + if [ "$1" == "--force" -o "$1" == "-f" ]; then + with_unstaged='--force' + else + echo "$HELP_BRANCH"; + echo ""; + Die "Unknown $command option: $1" + fi + fi + + # Find first commit on branch. Use $first_commit^ if you need the + # commit on master we branched off. + local first_commit=$(FirstCommitOfBranch) + echo "First commit on branch: $first_commit" + + $GIT_CLANG_FORMAT --style file --extensions c,h $with_unstaged $first_commit^ + if [ $? -ne 0 ]; then + Die "Cannot reformat branch. git clang-format failed" + fi +} + +# Reformat currently staged changes +function ReformatCached { + # check parameters + local with_unstaged= + if [ $# -gt 1 ]; then + echo "$HELP_CACHED"; + echo ""; + Die "Too many $command options: $1" + elif [ $# -eq 1 ]; then + if [ "$1" == "--force" -o "$1" == "-f" ]; then + with_unstaged='--force' + else + echo "$HELP_CACHED"; + echo ""; + Die "Unknown $command option: $1" + fi + fi + + $GIT_CLANG_FORMAT --style file --extensions c,h $with_unstaged + if [ $? -ne 0 ]; then + Die "Cannot reformat staging. git clang-format failed" + fi +} + +# Reformat all commits of a branch (compared with master) and rewrites +# the history with the formatted commits one-by-one. +# This is helpful for quickly reformatting branches with multiple commits, +# or where the master version of a file has been reformatted. +# +# You can achieve the same manually by git checkout -n , git clang-format +# for each commit in your branch. +function ReformatCommits { + # Do not allow rewriting of master. + # CheckBranch below will also tell us there are no changes compared with + # master, but let's make this foolproof and explicit here. + local current_branch=$(git rev-parse --abbrev-ref HEAD) + if [ "$current_branch" == "master" ]; then + Die "Must not rewrite master branch history." + fi + + CheckBranch "--quiet" + if [ $? -eq 0 ]; then + echo "no modified files to format" + else + # Only rewrite if there are changes + # Squelch warning. Our usage of git filter-branch is limited and should be ok. + # Should investigate using git-filter-repo in the future instead. + export FILTER_BRANCH_SQUELCH_WARNING=1 + + # Find first commit on branch. Use $first_commit^ if you need the + # commit on master we branched off. + local first_commit=$(FirstCommitOfBranch) + echo "First commit on branch: $first_commit" + # Use --force in case it's run a second time on the same branch + git filter-branch --force --tree-filter "$GIT_CLANG_FORMAT $first_commit^" -- $first_commit..HEAD + if [ $? -ne 0 ]; then + Die "Cannot rewrite branch. git filter-branch failed" + fi + fi +} + +if [ $# -eq 0 ]; then + echo "$USAGE"; + Die "Missing arguments. Call with one argument" +fi + +SetTopLevelDir + +RequireProgram GIT git +# ubuntu uses clang-format-{version} name for newer versions. fedora not. +RequireProgram GIT_CLANG_FORMAT git-clang-format-11 git-clang-format-10 git-clang-format-9 git-clang-format +GIT_CLANG_FORMAT_BINARY=clang-format +if [[ $GIT_CLANG_FORMAT =~ .*git-clang-format-11$ ]]; then + # default binary is clang-format, specify the correct version. + # Alternative: git config clangformat.binary "clang-format-11" + GIT_CLANG_FORMAT_BINARY="clang-format-11" +elif [[ $GIT_CLANG_FORMAT =~ .*git-clang-format-10$ ]]; then + # default binary is clang-format, specify the correct version. + # Alternative: git config clangformat.binary "clang-format-10" + GIT_CLANG_FORMAT_BINARY="clang-format-10" +elif [[ $GIT_CLANG_FORMAT =~ .*git-clang-format-9$ ]]; then + # default binary is clang-format, specify the correct version. + # Alternative: git config clangformat.binary "clang-format-9" + GIT_CLANG_FORMAT_BINARY="clang-format-9" +elif [[ $GIT_CLANG_FORMAT =~ .*git-clang-format$ ]]; then + Debug "Using regular clang-format" +else + Debug "Internal: unhandled clang-format version" +fi + +# enforce minimal clang-format version as required by .clang-format +clang_format_required_version=9 +clang_format_version=$($GIT_CLANG_FORMAT_BINARY --version | sed 's/.*clang-format version \([0-9]*\.[0-9]*\.[0-9]*\).*/\1/') +echo "Found clang-format version: $clang_format_version" +clang_format_version_major=$(echo $clang_format_version | sed 's/\([0-9]*\)\.\([0-9]*\)\.\([0-9]*\).*/\1/') +Debug "clang-format version major: $clang_format_version_major" +if [ $((clang_format_version_major + 0)) -lt $((clang_format_required_version + 0)) ]; then + Die "Require clang version $clang_format_required_version, found $clang_format_version_major ($clang_format_version)." +fi + +# overwite git-clang-version for --diffstat as upstream does not have that yet +RequireProgram GIT_CLANG_FORMAT_DIFFSTAT scripts/git-clang-format-custom +if [ "$GIT_CLANG_FORMAT_BINARY" != "clang-format" ]; then + GIT_CLANG_FORMAT="$GIT_CLANG_FORMAT --binary $GIT_CLANG_FORMAT_BINARY" + GIT_CLANG_FORMAT_DIFFSTAT="$GIT_CLANG_FORMAT_DIFFSTAT --binary $GIT_CLANG_FORMAT_BINARY" +fi +Debug "Using $GIT_CLANG_FORMAT" +Debug "Using $GIT_CLANG_FORMAT_DIFFSTAT" + +command_rc=0 +command=$1 +case $command in + branch) + shift; + ReformatBranch "$@"; + ;; + + check-branch) + shift; + CheckBranch "$@"; + command_rc=$?; + ;; + + cached) + shift; + ReformatCached "$@"; + ;; + + rewrite-branch) + ReformatCommits + ;; + + help) + shift; + HelpCommand $1; + ;; + + -h|--help) + echo "$USAGE"; + ;; + + *) + Die "$EXEC: '$command' is not a command. See '$EXEC --help'" + ;; +esac + +ExitWith $command_rc diff --git a/scripts/git-clang-format-custom b/scripts/git-clang-format-custom new file mode 100755 index 000000000000..c7223eab1cfd --- /dev/null +++ b/scripts/git-clang-format-custom @@ -0,0 +1,600 @@ +#!/usr/bin/env python +# Copy of https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format +# Adds a --diffstat option to show the files needing formatting. +# This change will be upstreamed, but the current git-clang-format does not +# have it yet. We use it in the internal scripts/clang-format.sh +# +#===- git-clang-format - ClangFormat Git Integration ---------*- python -*--===# +# +# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +# See https://llvm.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +# +#===------------------------------------------------------------------------===# + +r""" +clang-format git integration +============================ + +This file provides a clang-format integration for git. Put it somewhere in your +path and ensure that it is executable. Then, "git clang-format" will invoke +clang-format on the changes in current files or a specific commit. + +For further details, run: +git clang-format -h + +Requires Python 2.7 or Python 3 +""" + +from __future__ import absolute_import, division, print_function +import argparse +import collections +import contextlib +import errno +import os +import re +import subprocess +import sys + +usage = 'git clang-format [OPTIONS] [] [] [--] [...]' + +desc = ''' +If zero or one commits are given, run clang-format on all lines that differ +between the working directory and , which defaults to HEAD. Changes are +only applied to the working directory. + +If two commits are given (requires --diff), run clang-format on all lines in the +second that differ from the first . + +The following git-config settings set the default of the corresponding option: + clangFormat.binary + clangFormat.commit + clangFormat.extensions + clangFormat.style +''' + +# Name of the temporary index file in which save the output of clang-format. +# This file is created within the .git directory. +temp_index_basename = 'clang-format-index' + + +Range = collections.namedtuple('Range', 'start, count') + + +def main(): + config = load_git_config() + + # In order to keep '--' yet allow options after positionals, we need to + # check for '--' ourselves. (Setting nargs='*' throws away the '--', while + # nargs=argparse.REMAINDER disallows options after positionals.) + argv = sys.argv[1:] + try: + idx = argv.index('--') + except ValueError: + dash_dash = [] + else: + dash_dash = argv[idx:] + argv = argv[:idx] + + default_extensions = ','.join([ + # From clang/lib/Frontend/FrontendOptions.cpp, all lower case + 'c', 'h', # C + 'm', # ObjC + 'mm', # ObjC++ + 'cc', 'cp', 'cpp', 'c++', 'cxx', 'hh', 'hpp', 'hxx', # C++ + 'cu', # CUDA + # Other languages that clang-format supports + 'proto', 'protodevel', # Protocol Buffers + 'java', # Java + 'js', # JavaScript + 'ts', # TypeScript + 'cs', # C Sharp + ]) + + p = argparse.ArgumentParser( + usage=usage, formatter_class=argparse.RawDescriptionHelpFormatter, + description=desc) + p.add_argument('--binary', + default=config.get('clangformat.binary', 'clang-format'), + help='path to clang-format'), + p.add_argument('--commit', + default=config.get('clangformat.commit', 'HEAD'), + help='default commit to use if none is specified'), + p.add_argument('--diff', action='store_true', + help='print a diff instead of applying the changes') + p.add_argument('--diffstat', action='store_true', + help='print diffstat instead of applying the changes') + p.add_argument('--extensions', + default=config.get('clangformat.extensions', + default_extensions), + help=('comma-separated list of file extensions to format, ' + 'excluding the period and case-insensitive')), + p.add_argument('-f', '--force', action='store_true', + help='allow changes to unstaged files') + p.add_argument('-p', '--patch', action='store_true', + help='select hunks interactively') + p.add_argument('-q', '--quiet', action='count', default=0, + help='print less information') + p.add_argument('--style', + default=config.get('clangformat.style', None), + help='passed to clang-format'), + p.add_argument('-v', '--verbose', action='count', default=0, + help='print extra information') + # We gather all the remaining positional arguments into 'args' since we need + # to use some heuristics to determine whether or not was present. + # However, to print pretty messages, we make use of metavar and help. + p.add_argument('args', nargs='*', metavar='', + help='revision from which to compute the diff') + p.add_argument('ignored', nargs='*', metavar='...', + help='if specified, only consider differences in these files') + opts = p.parse_args(argv) + + opts.verbose -= opts.quiet + del opts.quiet + + commits, files = interpret_args(opts.args, dash_dash, opts.commit) + if len(commits) > 1: + if not opts.diff: + die('--diff is required when two commits are given') + else: + if len(commits) > 2: + die('at most two commits allowed; %d given' % len(commits)) + changed_lines = compute_diff_and_extract_lines(commits, files) + if opts.verbose >= 1: + ignored_files = set(changed_lines) + filter_by_extension(changed_lines, opts.extensions.lower().split(',')) + if opts.verbose >= 1: + ignored_files.difference_update(changed_lines) + if ignored_files: + print('Ignoring changes in the following files (wrong extension):') + for filename in ignored_files: + print(' %s' % filename) + if changed_lines: + print('Running clang-format on the following files:') + for filename in changed_lines: + print(' %s' % filename) + if not changed_lines: + print('no modified files to format') + return + # The computed diff outputs absolute paths, so we must cd before accessing + # those files. + cd_to_toplevel() + if len(commits) > 1: + old_tree = commits[1] + new_tree = run_clang_format_and_save_to_tree(changed_lines, + revision=commits[1], + binary=opts.binary, + style=opts.style) + else: + old_tree = create_tree_from_workdir(changed_lines) + new_tree = run_clang_format_and_save_to_tree(changed_lines, + binary=opts.binary, + style=opts.style) + if opts.verbose >= 1: + print('old tree: %s' % old_tree) + print('new tree: %s' % new_tree) + if old_tree == new_tree: + if opts.verbose >= 0: + print('clang-format did not modify any files') + elif opts.diff: + print_diff(old_tree, new_tree) + elif opts.diffstat: + print_diffstat(old_tree, new_tree) + else: + changed_files = apply_changes(old_tree, new_tree, force=opts.force, + patch_mode=opts.patch) + if (opts.verbose >= 0 and not opts.patch) or opts.verbose >= 1: + print('changed files:') + for filename in changed_files: + print(' %s' % filename) + + +def load_git_config(non_string_options=None): + """Return the git configuration as a dictionary. + + All options are assumed to be strings unless in `non_string_options`, in which + is a dictionary mapping option name (in lower case) to either "--bool" or + "--int".""" + if non_string_options is None: + non_string_options = {} + out = {} + for entry in run('git', 'config', '--list', '--null').split('\0'): + if entry: + name, value = entry.split('\n', 1) + if name in non_string_options: + value = run('git', 'config', non_string_options[name], name) + out[name] = value + return out + + +def interpret_args(args, dash_dash, default_commit): + """Interpret `args` as "[commits] [--] [files]" and return (commits, files). + + It is assumed that "--" and everything that follows has been removed from + args and placed in `dash_dash`. + + If "--" is present (i.e., `dash_dash` is non-empty), the arguments to its + left (if present) are taken as commits. Otherwise, the arguments are checked + from left to right if they are commits or files. If commits are not given, + a list with `default_commit` is used.""" + if dash_dash: + if len(args) == 0: + commits = [default_commit] + else: + commits = args + for commit in commits: + object_type = get_object_type(commit) + if object_type not in ('commit', 'tag'): + if object_type is None: + die("'%s' is not a commit" % commit) + else: + die("'%s' is a %s, but a commit was expected" % (commit, object_type)) + files = dash_dash[1:] + elif args: + commits = [] + while args: + if not disambiguate_revision(args[0]): + break + commits.append(args.pop(0)) + if not commits: + commits = [default_commit] + files = args + else: + commits = [default_commit] + files = [] + return commits, files + + +def disambiguate_revision(value): + """Returns True if `value` is a revision, False if it is a file, or dies.""" + # If `value` is ambiguous (neither a commit nor a file), the following + # command will die with an appropriate error message. + run('git', 'rev-parse', value, verbose=False) + object_type = get_object_type(value) + if object_type is None: + return False + if object_type in ('commit', 'tag'): + return True + die('`%s` is a %s, but a commit or filename was expected' % + (value, object_type)) + + +def get_object_type(value): + """Returns a string description of an object's type, or None if it is not + a valid git object.""" + cmd = ['git', 'cat-file', '-t', value] + p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + stdout, stderr = p.communicate() + if p.returncode != 0: + return None + return convert_string(stdout.strip()) + + +def compute_diff_and_extract_lines(commits, files): + """Calls compute_diff() followed by extract_lines().""" + diff_process = compute_diff(commits, files) + changed_lines = extract_lines(diff_process.stdout) + diff_process.stdout.close() + diff_process.wait() + if diff_process.returncode != 0: + # Assume error was already printed to stderr. + sys.exit(2) + return changed_lines + + +def compute_diff(commits, files): + """Return a subprocess object producing the diff from `commits`. + + The return value's `stdin` file object will produce a patch with the + differences between the working directory and the first commit if a single + one was specified, or the difference between both specified commits, filtered + on `files` (if non-empty). Zero context lines are used in the patch.""" + git_tool = 'diff-index' + if len(commits) > 1: + git_tool = 'diff-tree' + cmd = ['git', git_tool, '-p', '-U0'] + commits + ['--'] + cmd.extend(files) + p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE) + p.stdin.close() + return p + + +def extract_lines(patch_file): + """Extract the changed lines in `patch_file`. + + The return value is a dictionary mapping filename to a list of (start_line, + line_count) pairs. + + The input must have been produced with ``-U0``, meaning unidiff format with + zero lines of context. The return value is a dict mapping filename to a + list of line `Range`s.""" + matches = {} + for line in patch_file: + line = convert_string(line) + match = re.search(r'^\+\+\+\ [^/]+/(.*)', line) + if match: + filename = match.group(1).rstrip('\r\n') + match = re.search(r'^@@ -[0-9,]+ \+(\d+)(,(\d+))?', line) + if match: + start_line = int(match.group(1)) + line_count = 1 + if match.group(3): + line_count = int(match.group(3)) + if line_count > 0: + matches.setdefault(filename, []).append(Range(start_line, line_count)) + return matches + + +def filter_by_extension(dictionary, allowed_extensions): + """Delete every key in `dictionary` that doesn't have an allowed extension. + + `allowed_extensions` must be a collection of lowercase file extensions, + excluding the period.""" + allowed_extensions = frozenset(allowed_extensions) + for filename in list(dictionary.keys()): + base_ext = filename.rsplit('.', 1) + if len(base_ext) == 1 and '' in allowed_extensions: + continue + if len(base_ext) == 1 or base_ext[1].lower() not in allowed_extensions: + del dictionary[filename] + + +def cd_to_toplevel(): + """Change to the top level of the git repository.""" + toplevel = run('git', 'rev-parse', '--show-toplevel') + os.chdir(toplevel) + + +def create_tree_from_workdir(filenames): + """Create a new git tree with the given files from the working directory. + + Returns the object ID (SHA-1) of the created tree.""" + return create_tree(filenames, '--stdin') + + +def run_clang_format_and_save_to_tree(changed_lines, revision=None, + binary='clang-format', style=None): + """Run clang-format on each file and save the result to a git tree. + + Returns the object ID (SHA-1) of the created tree.""" + def iteritems(container): + try: + return container.iteritems() # Python 2 + except AttributeError: + return container.items() # Python 3 + def index_info_generator(): + for filename, line_ranges in iteritems(changed_lines): + if revision: + git_metadata_cmd = ['git', 'ls-tree', + '%s:%s' % (revision, os.path.dirname(filename)), + os.path.basename(filename)] + git_metadata = subprocess.Popen(git_metadata_cmd, stdin=subprocess.PIPE, + stdout=subprocess.PIPE) + stdout = git_metadata.communicate()[0] + mode = oct(int(stdout.split()[0], 8)) + else: + mode = oct(os.stat(filename).st_mode) + # Adjust python3 octal format so that it matches what git expects + if mode.startswith('0o'): + mode = '0' + mode[2:] + blob_id = clang_format_to_blob(filename, line_ranges, + revision=revision, + binary=binary, + style=style) + yield '%s %s\t%s' % (mode, blob_id, filename) + return create_tree(index_info_generator(), '--index-info') + + +def create_tree(input_lines, mode): + """Create a tree object from the given input. + + If mode is '--stdin', it must be a list of filenames. If mode is + '--index-info' is must be a list of values suitable for "git update-index + --index-info", such as " ". Any other mode + is invalid.""" + assert mode in ('--stdin', '--index-info') + cmd = ['git', 'update-index', '--add', '-z', mode] + with temporary_index_file(): + p = subprocess.Popen(cmd, stdin=subprocess.PIPE) + for line in input_lines: + p.stdin.write(to_bytes('%s\0' % line)) + p.stdin.close() + if p.wait() != 0: + die('`%s` failed' % ' '.join(cmd)) + tree_id = run('git', 'write-tree') + return tree_id + + +def clang_format_to_blob(filename, line_ranges, revision=None, + binary='clang-format', style=None): + """Run clang-format on the given file and save the result to a git blob. + + Runs on the file in `revision` if not None, or on the file in the working + directory if `revision` is None. + + Returns the object ID (SHA-1) of the created blob.""" + clang_format_cmd = [binary] + if style: + clang_format_cmd.extend(['-style='+style]) + clang_format_cmd.extend([ + '-lines=%s:%s' % (start_line, start_line+line_count-1) + for start_line, line_count in line_ranges]) + if revision: + clang_format_cmd.extend(['-assume-filename='+filename]) + git_show_cmd = ['git', 'cat-file', 'blob', '%s:%s' % (revision, filename)] + git_show = subprocess.Popen(git_show_cmd, stdin=subprocess.PIPE, + stdout=subprocess.PIPE) + git_show.stdin.close() + clang_format_stdin = git_show.stdout + else: + clang_format_cmd.extend([filename]) + git_show = None + clang_format_stdin = subprocess.PIPE + try: + clang_format = subprocess.Popen(clang_format_cmd, stdin=clang_format_stdin, + stdout=subprocess.PIPE) + if clang_format_stdin == subprocess.PIPE: + clang_format_stdin = clang_format.stdin + except OSError as e: + if e.errno == errno.ENOENT: + die('cannot find executable "%s"' % binary) + else: + raise + clang_format_stdin.close() + hash_object_cmd = ['git', 'hash-object', '-w', '--path='+filename, '--stdin'] + hash_object = subprocess.Popen(hash_object_cmd, stdin=clang_format.stdout, + stdout=subprocess.PIPE) + clang_format.stdout.close() + stdout = hash_object.communicate()[0] + if hash_object.returncode != 0: + die('`%s` failed' % ' '.join(hash_object_cmd)) + if clang_format.wait() != 0: + die('`%s` failed' % ' '.join(clang_format_cmd)) + if git_show and git_show.wait() != 0: + die('`%s` failed' % ' '.join(git_show_cmd)) + return convert_string(stdout).rstrip('\r\n') + + +@contextlib.contextmanager +def temporary_index_file(tree=None): + """Context manager for setting GIT_INDEX_FILE to a temporary file and deleting + the file afterward.""" + index_path = create_temporary_index(tree) + old_index_path = os.environ.get('GIT_INDEX_FILE') + os.environ['GIT_INDEX_FILE'] = index_path + try: + yield + finally: + if old_index_path is None: + del os.environ['GIT_INDEX_FILE'] + else: + os.environ['GIT_INDEX_FILE'] = old_index_path + os.remove(index_path) + + +def create_temporary_index(tree=None): + """Create a temporary index file and return the created file's path. + + If `tree` is not None, use that as the tree to read in. Otherwise, an + empty index is created.""" + gitdir = run('git', 'rev-parse', '--git-dir') + path = os.path.join(gitdir, temp_index_basename) + if tree is None: + tree = '--empty' + run('git', 'read-tree', '--index-output='+path, tree) + return path + + +def print_diff(old_tree, new_tree): + """Print the diff between the two trees to stdout.""" + # We use the porcelain 'diff' and not plumbing 'diff-tree' because the output + # is expected to be viewed by the user, and only the former does nice things + # like color and pagination. + # + # We also only print modified files since `new_tree` only contains the files + # that were modified, so unmodified files would show as deleted without the + # filter. + subprocess.check_call(['git', 'diff', '--diff-filter=M', old_tree, new_tree, + '--']) + +def print_diffstat(old_tree, new_tree): + """Print the diffstat between the two trees to stdout.""" + # We use the porcelain 'diff' and not plumbing 'diff-tree' because the output + # is expected to be viewed by the user, and only the former does nice things + # like color and pagination. + # + # We also only print modified files since `new_tree` only contains the files + # that were modified, so unmodified files would show as deleted without the + # filter. + subprocess.check_call(['git', 'diff', '--diff-filter=M', '--stat', old_tree, new_tree, + '--']) + + +def apply_changes(old_tree, new_tree, force=False, patch_mode=False): + """Apply the changes in `new_tree` to the working directory. + + Bails if there are local changes in those files and not `force`. If + `patch_mode`, runs `git checkout --patch` to select hunks interactively.""" + changed_files = run('git', 'diff-tree', '--diff-filter=M', '-r', '-z', + '--name-only', old_tree, + new_tree).rstrip('\0').split('\0') + if not force: + unstaged_files = run('git', 'diff-files', '--name-status', *changed_files) + if unstaged_files: + print('The following files would be modified but ' + 'have unstaged changes:', file=sys.stderr) + print(unstaged_files, file=sys.stderr) + print('Please commit, stage, or stash them first.', file=sys.stderr) + sys.exit(2) + if patch_mode: + # In patch mode, we could just as well create an index from the new tree + # and checkout from that, but then the user will be presented with a + # message saying "Discard ... from worktree". Instead, we use the old + # tree as the index and checkout from new_tree, which gives the slightly + # better message, "Apply ... to index and worktree". This is not quite + # right, since it won't be applied to the user's index, but oh well. + with temporary_index_file(old_tree): + subprocess.check_call(['git', 'checkout', '--patch', new_tree]) + index_tree = old_tree + else: + with temporary_index_file(new_tree): + run('git', 'checkout-index', '-a', '-f') + return changed_files + + +def run(*args, **kwargs): + stdin = kwargs.pop('stdin', '') + verbose = kwargs.pop('verbose', True) + strip = kwargs.pop('strip', True) + for name in kwargs: + raise TypeError("run() got an unexpected keyword argument '%s'" % name) + p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + stdin=subprocess.PIPE) + stdout, stderr = p.communicate(input=stdin) + + stdout = convert_string(stdout) + stderr = convert_string(stderr) + + if p.returncode == 0: + if stderr: + if verbose: + print('`%s` printed to stderr:' % ' '.join(args), file=sys.stderr) + print(stderr.rstrip(), file=sys.stderr) + if strip: + stdout = stdout.rstrip('\r\n') + return stdout + if verbose: + print('`%s` returned %s' % (' '.join(args), p.returncode), file=sys.stderr) + if stderr: + print(stderr.rstrip(), file=sys.stderr) + sys.exit(2) + + +def die(message): + print('error:', message, file=sys.stderr) + sys.exit(2) + + +def to_bytes(str_input): + # Encode to UTF-8 to get binary data. + if isinstance(str_input, bytes): + return str_input + return str_input.encode('utf-8') + + +def to_string(bytes_input): + if isinstance(bytes_input, str): + return bytes_input + return bytes_input.encode('utf-8') + + +def convert_string(bytes_input): + try: + return to_string(bytes_input.decode('utf-8')) + except AttributeError: # 'str' object has no attribute 'decode'. + return str(bytes_input) + except UnicodeError: + return str(bytes_input) + +if __name__ == '__main__': + main() From 8429123c25e27dbef76dbf5cfd5c1e9f598c2e9d Mon Sep 17 00:00:00 2001 From: Roland Fischer Date: Tue, 14 Jul 2020 23:43:58 -0400 Subject: [PATCH 4/4] Run formatting check on pull request --- .github/workflows/formatting.yml | 103 +++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 .github/workflows/formatting.yml diff --git a/.github/workflows/formatting.yml b/.github/workflows/formatting.yml new file mode 100644 index 000000000000..6a49c881b294 --- /dev/null +++ b/.github/workflows/formatting.yml @@ -0,0 +1,103 @@ +name: formatting-check + +on: + push: + # Only run on pushes to pull request branches + branches-ignore: + - 'master' + - 'master-*' + pull_request: + +jobs: + + # Checking for correct formatting of branch for C code changes + check-formatting: + name: Formatting Check (clang 9) + runs-on: ubuntu-18.04 + container: ubuntu:18.04 + steps: + + # Cache Rust stuff. + - name: Cache cargo registry + uses: actions/cache@v1 + with: + path: ~/.cargo/registry + key: cargo-registry + + - name: Install dependencies + run: | + apt update + apt -y install \ + libpcre3 \ + libpcre3-dev \ + build-essential \ + autoconf \ + automake \ + git \ + libtool \ + libpcap-dev \ + libnet1-dev \ + libyaml-0-2 \ + libyaml-dev \ + libcap-ng-dev \ + libcap-ng0 \ + libmagic-dev \ + libnetfilter-queue-dev \ + libnetfilter-queue1 \ + libnfnetlink-dev \ + libnfnetlink0 \ + libhiredis-dev \ + libjansson-dev \ + libpython2.7 \ + make \ + python \ + rustc \ + software-properties-common \ + wget \ + zlib1g \ + zlib1g-dev + - name: Install packages for clang-format 9 + run: | + wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | apt-key add - + # no need to install full clang using https://apt.llvm.org/llvm.sh + # using clang 9 + add-apt-repository "deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-9 main" + apt-get update + apt-get install -y clang-format-9 + - name: Install cbindgen + run: cargo install --force --debug --version 0.14.1 cbindgen + - run: echo "::add-path::$HOME/.cargo/bin" + # If master has newer commits, github adds a merge commit from master into + # our branch. + # This screws up git clang-format as it'll format the merge as well and + # reports any formatting issues in those commits on master as well. + # We don't care about them here. So let's use the real branch HEAD commit. + - name: Checkout HEAD on branch, not merge commit! + uses: actions/checkout@v1 + with: + ref: ${{ github.head_ref }} +# ref: ${{ github.event.pull_request.head.sha }} + - run: git clone https://github.com/OISF/libhtp -b 0.5.x + - run: ./autogen.sh + - run: ./configure --enable-unittests + - name: Check formatting + run: | + # Do not use make as it hides the script's exit code + # make diffstat-style-branch >> check_formatting_log.txt 2>&1 + ./scripts/clang-format.sh check-branch --make --diffstat --show-commits >> check_formatting_log.txt 2>&1 + rc=$? + if [ $rc -eq 0 ]; then + cat check_formatting_log.txt + echo "Formatting is following code style guide" + elif [ $rc -eq 1 ]; then + # limit output as it might be a lot in the worst case + tail -n 100 check_formatting_log.txt + echo "::warning ::Formatting is not following code style guide!" + else + cat check_formatting_log.txt + # use last output line as error + last_line=$(tail -n 1 check_formatting_log.txt) + echo "::error ::$last_line" + exit 1 + fi + shell: bash {0}