| Message ID | 20231026110729.21961-1-jmeng@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [ovs-dev,v2] editorconfig: Remove [*] section and trim_trailing_whitespace. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
, Oct 26, 2023 at 13:07: > From: Jakob Meng <code@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 <code@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 And probably adding insert_final_newline = true is not necessary. checkpatch should complain if the final newline is missing. Your patch should only remove insert_final_newline and trim_trailing_whitespace from the default section. What do you think? > +# 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
On 26.10.23 13:52, Robin Jarry wrote: > , Oct 26, 2023 at 13:07: >> From: Jakob Meng <code@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 <code@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. > 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. > 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 > > >
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 <code@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 <code@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. > > 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 > > > > > >
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 <code@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 <code@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/ > >> > 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 >> > >> > >> > >
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 <code@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 <code@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. >> >>>> 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 >>>> >>>> >>>> >>
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 <code@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 <code@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-jmeng@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 >>>>> >>>>>
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 +# 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