On 27.10.23 12:29, Eelco Chaudron wrote: > > On 26 Oct 2023, at 14:34, Jakob Meng wrote: > >> On 26.10.23 14:21, Robin Jarry wrote: >>> Jakob Meng, Oct 26, 2023 at 14:17: >>>> On 26.10.23 13:52, Robin Jarry wrote: >>>>> , Oct 26, 2023 at 13:07: >>>>>> From: Jakob Meng <c...@jakobmeng.de> >>>>>> >>>>>> Wildcard sections [*] and [**] are unsafe because properties cannot be >>>>>> applied safely to any filetype in general. For example, IDEs like >>>>>> Visual Studio Code and KDevelop store configuration files in subfolders >>>>>> like .vscode or .kdev4. Properties from wildcard sections also apply to >>>>>> those files which is not safe in general. >>>>>> Another example are patches created with 'git format-patch' which can >>>>>> contain trailing whitespaces. When editing a patch, e.g. to fix a typo >>>>>> in the title, trailing whitespaces should not be removed. >>>>>> >>>>>> Property trim_trailing_whitespace should not be defined at all because >>>>>> it is interpreted differently by editors. Some wipe whitespaces from >>>>>> the whole file, others remove them from edited lines only and a few >>>>>> change their behavior between releases [0]. Limiting the property to a >>>>>> subset of files like *.c/*.h will not mitigate the issue: >>>>>> >>>>>> Multiple definitions of a whitespace exist. Unicode considers a form >>>>>> feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt >>>>>> follows this definition, causing the Kate editor identify a form feed >>>>>> as a trailing whitespace and removing it from sources [3]. This breaks >>>>>> patches when editors remove form feeds and thus causing broken patches >>>>>> which cannot be applied cleanly. >>>>>> >>>>>> Removing trim_trailing_whitespace will be a minor inconvienence, in >>>>>> particular because utilities/checkpatch.py and thus 0-day Robot will >>>>>> prevent trailing whitespaces for our definition of a whitespace. >>>>>> >>>>>> [0] >>>>>> https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295 >>>>>> [1] https://en.wikipedia.org/wiki/Whitespace_character >>>>>> [2] >>>>>> https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554 >>>>>> [3] >>>>>> https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643 >>>>>> >>>>>> Fixes: 07f6d6a0cb51 ("Add editorconfig file.") >>>>>> >>>>>> Signed-off-by: Jakob Meng <c...@jakobmeng.de> >>>>>> --- >>>>>> .editorconfig | 34 +++++++++++++++++++++++++++++----- >>>>>> 1 file changed, 29 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/.editorconfig b/.editorconfig >>>>>> index 685c72750..aebcf3a72 100644 >>>>>> --- a/.editorconfig >>>>>> +++ b/.editorconfig >>>>>> @@ -2,47 +2,71 @@ >>>>>> >>>>>> root = true >>>>>> >>>>>> -[*] >>>>>> -end_of_line = lf >>>>>> -insert_final_newline = true >>>>>> -trim_trailing_whitespace = true >>>>>> -charset = utf-8 >>>>> Hi Jakob, >>>>> >>>>> I think you could keep these two options: >>>>> >>>>> end_of_line = lf >>>>> charset = utf-8 >>>>> >>>> You cannot decide this in general for all possible filetypes across all >>>> possible dev platforms. Again, those properties in [*] would also apply to >>>> non-ovs-owned files inside the source tree, e.g. created by IDEs. Unsafe. >>>> Please don't. >>> Ok fair enough. >>> >>>>> And probably adding insert_final_newline = true is not necessary. > >>>>> checkpatch should complain if the final newline is missing. >>>> I left it in .editorconfig because it is not causing trouble. But I can >>>> remove it, if you want. >>> I don't think it is important you can leave it or remove it. >>> >>> However I just realized that you could simply copy the settings in the >>> [*.{c,h}] section as other sections will inherit from it unless they >>> override something. >> Oh, nice! IIUC this is actually covered by the EditorConfig spec [0]: >> >> "All found EditorConfig files are searched for sections with section names >> matching the given filename." >> >> and >> >> "Files are read top to bottom and the most recent rules found take >> precedence. If multiple EditorConfig files have matching sections, the rules >> from the closer EditorConfig file are read last, so pairs in closer files >> take precedence." >> >> and >> >> "For any pair, a value of unset removes the effect of that pair, even if it >> has been set before. For example, add indent_size = unset to undefine the >> indent_size pair (and use editor defaults)." >> >> The latter would not make sense if you could not inherit properties from >> other sections. >> >> [0] https://spec.editorconfig.org/ > So I guess we can expect a new rev. Will mark this as changes requested for > now.
Updated patch is out: https://patchwork.ozlabs.org/project/openvswitch/patch/20231027121040.26751-1-jm...@redhat.com/ Only change is that properties get inherited as discussed above. > >>>>> Your patch should only remove insert_final_newline and > >>>>> trim_trailing_whitespace from the default section. >>>> >>>>>> +# No wildcard sections [*] and [**] because properties cannot be >>>>>> +# applied safely to any filetype in general. >>>>>> + >>>>>> +# Property trim_trailing_whitespace should not be defined at all >>>>>> +# because it is interpreted differently by editors. >>>>>> >>>>>> [*.{c,h}] >>>>>> +charset = utf-8 >>>>>> +end_of_line = lf >>>>>> indent_style = space >>>>>> indent_size = 4 >>>>>> +insert_final_newline = true >>>>>> max_line_length = 79 >>>>>> >>>>>> [include/linux/**.h] >>>>>> +charset = utf-8 >>>>>> +end_of_line = lf >>>>>> indent_style = tab >>>>>> indent_size = tab >>>>>> +insert_final_newline = true >>>>>> tab_width = 8 >>>>>> >>>>>> [include/sparse/rte_*.h] >>>>>> +charset = utf-8 >>>>>> +end_of_line = lf >>>>>> indent_style = tab >>>>>> +insert_final_newline = true >>>>>> tab_width = 8 >>>>>> >>>>>> [include/windows/getopt.h] >>>>>> +charset = utf-8 >>>>>> +end_of_line = lf >>>>>> indent_style = tab >>>>>> indent_size = tab >>>>>> +insert_final_newline = true >>>>>> tab_width = 8 >>>>>> >>>>>> [include/windows/netinet/{icmp6,ip6}.h] >>>>>> +charset = utf-8 >>>>>> +end_of_line = lf >>>>>> indent_style = tab >>>>>> indent_size = tab >>>>>> +insert_final_newline = true >>>>>> tab_width = 8 >>>>>> >>>>>> [lib/getopt_long.c] >>>>>> +charset = utf-8 >>>>>> +end_of_line = lf >>>>>> indent_style = tab >>>>>> indent_size = tab >>>>>> +insert_final_newline = true >>>>>> tab_width = 8 >>>>>> >>>>>> [lib/sflow*.{c,h}] >>>>>> +charset = utf-8 >>>>>> +end_of_line = lf >>>>>> indent_style = tab >>>>>> indent_size = tab >>>>>> +insert_final_newline = true >>>>>> tab_width = 8 >>>>>> >>>>>> [lib/strsep.c] >>>>>> +charset = utf-8 >>>>>> +end_of_line = lf >>>>>> indent_style = tab >>>>>> indent_size = tab >>>>>> +insert_final_newline = true >>>>>> tab_width = 8 >>>>>> -- >>>>>> 2.39.2 >>>>> >>>>> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
