| Message ID | 20231025115236.719260-1-jmeng@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [ovs-dev] editorconfig: Leave *.patch and *.diff files untouched. | 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 |
On 25.10.23 13:52, jmeng@redhat.com wrote: > From: Jakob Meng <code@jakobmeng.de> > > A patch created with 'git format-patch' can contain trailing spaces. > When editing a patch, e.g. to fix a typo in the title, the trailing > spaces should not be removed. This becomes tricky when editors like > Kate identify a space followed by form feed as a trailing space and > remove both. This will result in a broken patch which cannot be applied > cleanly. Although this case should likely be considered a editor bug, > keeping trailing spaces in a patch is the right thing to do and also > helps mitigating these kinds of editor bugs. Whether this is a editor bug or not is debatable. For example, Kate uses QChar's isSpace() [0] to identify a "trailing space" [1] which considers a form feed character (0x0c) a space. In future Kate releases this issue will surface less often because Kate's interpretation of 'trim_trailing_whitespaces' from .editorconfig has been changed [3]. [0] https://doc.qt.io/qt-5/qchar.html#isSpace [1] https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643 [2] https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554 [3] https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295 > Fixes: 07f6d6a0cb51 ("Add editorconfig file.") > > Signed-off-by: Jakob Meng <code@jakobmeng.de> > --- > .editorconfig | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/.editorconfig b/.editorconfig > index 685c72750..5be5415da 100644 > --- a/.editorconfig > +++ b/.editorconfig > @@ -13,6 +13,10 @@ indent_style = space > indent_size = 4 > max_line_length = 79 > > +[*.{patch,diff}] > +insert_final_newline = false > +trim_trailing_whitespace = false > + > [include/linux/**.h] > indent_style = tab > indent_size = tab
On 25 Oct 2023, at 13:52, jmeng@redhat.com wrote: > From: Jakob Meng <code@jakobmeng.de> > > A patch created with 'git format-patch' can contain trailing spaces. > When editing a patch, e.g. to fix a typo in the title, the trailing > spaces should not be removed. This becomes tricky when editors like > Kate identify a space followed by form feed as a trailing space and > remove both. This will result in a broken patch which cannot be applied > cleanly. Although this case should likely be considered a editor bug, > keeping trailing spaces in a patch is the right thing to do and also > helps mitigating these kinds of editor bugs. > > Fixes: 07f6d6a0cb51 ("Add editorconfig file.") This looks good to me, however as you mention this is more of an editor configuration issue. It looks like leaving out any default value will cause the editor to use its configured value. Acked-by: Eelco Chaudron <echaudro@redhat.com>
Eelco Chaudron, Oct 25, 2023 at 14:56: > On 25 Oct 2023, at 13:52, jmeng@redhat.com wrote: > > > From: Jakob Meng <code@jakobmeng.de> > > > > A patch created with 'git format-patch' can contain trailing spaces. > > When editing a patch, e.g. to fix a typo in the title, the trailing > > spaces should not be removed. This becomes tricky when editors like > > Kate identify a space followed by form feed as a trailing space and > > remove both. This will result in a broken patch which cannot be applied > > cleanly. Although this case should likely be considered a editor bug, > > keeping trailing spaces in a patch is the right thing to do and also > > helps mitigating these kinds of editor bugs. > > > > Fixes: 07f6d6a0cb51 ("Add editorconfig file.") > > This looks good to me, however as you mention this is more of an > editor configuration issue. It looks like leaving out any default > value will cause the editor to use its configured value. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> It seems OK as well. But another safer option would have been to move the trim_trailing_whitespace = true option in specific sections. Or completely remove it since it will be caught by checkpatch. What do you think?
On Wed, Oct 25, 2023 at 9:02 AM Robin Jarry <rjarry@redhat.com> wrote: > > Eelco Chaudron, Oct 25, 2023 at 14:56: > > On 25 Oct 2023, at 13:52, jmeng@redhat.com wrote: > > > > > From: Jakob Meng <code@jakobmeng.de> > > > > > > A patch created with 'git format-patch' can contain trailing spaces. > > > When editing a patch, e.g. to fix a typo in the title, the trailing > > > spaces should not be removed. This becomes tricky when editors like > > > Kate identify a space followed by form feed as a trailing space and > > > remove both. This will result in a broken patch which cannot be applied > > > cleanly. Although this case should likely be considered a editor bug, > > > keeping trailing spaces in a patch is the right thing to do and also > > > helps mitigating these kinds of editor bugs. > > > > > > Fixes: 07f6d6a0cb51 ("Add editorconfig file.") > > > > This looks good to me, however as you mention this is more of an > > editor configuration issue. It looks like leaving out any default > > value will cause the editor to use its configured value. > > > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > It seems OK as well. But another safer option would have been to move > the trim_trailing_whitespace = true option in specific sections. Or > completely remove it since it will be caught by checkpatch. I think it also makes sense to remove trim_trailing_whitespace from the default section, but the current patch makes sense as a standalone change. Acked-by: Mike Pattrick <mkp@redhat.com> > > What do you think? > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 25.10.23 15:48, Mike Pattrick wrote: > On Wed, Oct 25, 2023 at 9:02 AM Robin Jarry <rjarry@redhat.com> wrote: >> Eelco Chaudron, Oct 25, 2023 at 14:56: >>> On 25 Oct 2023, at 13:52, jmeng@redhat.com wrote: >>> >>>> From: Jakob Meng <code@jakobmeng.de> >>>> >>>> A patch created with 'git format-patch' can contain trailing spaces. >>>> When editing a patch, e.g. to fix a typo in the title, the trailing >>>> spaces should not be removed. This becomes tricky when editors like >>>> Kate identify a space followed by form feed as a trailing space and >>>> remove both. This will result in a broken patch which cannot be applied >>>> cleanly. Although this case should likely be considered a editor bug, >>>> keeping trailing spaces in a patch is the right thing to do and also >>>> helps mitigating these kinds of editor bugs. >>>> >>>> Fixes: 07f6d6a0cb51 ("Add editorconfig file.") >>> This looks good to me, however as you mention this is more of an >>> editor configuration issue. It looks like leaving out any default >>> value will cause the editor to use its configured value. >>> >>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> It seems OK as well. But another safer option would have been to move >> the trim_trailing_whitespace = true option in specific sections. Or >> completely remove it since it will be caught by checkpatch. > I think it also makes sense to remove trim_trailing_whitespace from > the default section, but the current patch makes sense as a standalone > change. > > Acked-by: Mike Pattrick <mkp@redhat.com> Thank you all for your feedback! You are right, I will change my patch ☺️ We should completely remove the default section because we cannot set any reasonable defaults for all possible filetypes. For example, IDEs tend to write their own files to a subfolder like .vscode or .kdev4. A default section would apply to files in there, too, which is not safe in general. We also should not use insert_final_newline and trim_trailing_whitespace anywhere in .editorconfig! Editors interpret these lines differently, some will wipe whitespaces across the whole file, others will only remove them from lines being edited and others change their behavior between releases. Limiting those options to a subset like *.c/*.h will not help: As written in my other response, the definition of whitespace is ambiguous. Unicode considers form feed to be a whitespace [0], causing some editors to wipe form feeds when trim_trailing_whitespace is true which is not what we want in OVS. As Robin mentioned, we already have a test for our definition of whitespaces and thus we can avoid this whitespace mess by not using it in .editorconfig. [0] https://en.wikipedia.org/wiki/Whitespace_character
On 25.10.23 20:50, Jakob Meng wrote: > On 25.10.23 15:48, Mike Pattrick wrote: >> On Wed, Oct 25, 2023 at 9:02 AM Robin Jarry <rjarry@redhat.com> wrote: >>> Eelco Chaudron, Oct 25, 2023 at 14:56: >>>> On 25 Oct 2023, at 13:52, jmeng@redhat.com wrote: >>>> >>>>> From: Jakob Meng <code@jakobmeng.de> >>>>> >>>>> A patch created with 'git format-patch' can contain trailing spaces. >>>>> When editing a patch, e.g. to fix a typo in the title, the trailing >>>>> spaces should not be removed. This becomes tricky when editors like >>>>> Kate identify a space followed by form feed as a trailing space and >>>>> remove both. This will result in a broken patch which cannot be applied >>>>> cleanly. Although this case should likely be considered a editor bug, >>>>> keeping trailing spaces in a patch is the right thing to do and also >>>>> helps mitigating these kinds of editor bugs. >>>>> >>>>> Fixes: 07f6d6a0cb51 ("Add editorconfig file.") >>>> This looks good to me, however as you mention this is more of an >>>> editor configuration issue. It looks like leaving out any default >>>> value will cause the editor to use its configured value. >>>> >>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>> It seems OK as well. But another safer option would have been to move >>> the trim_trailing_whitespace = true option in specific sections. Or >>> completely remove it since it will be caught by checkpatch. >> I think it also makes sense to remove trim_trailing_whitespace from >> the default section, but the current patch makes sense as a standalone >> change. >> >> Acked-by: Mike Pattrick <mkp@redhat.com> > Thank you all for your feedback! You are right, I will change my patch ☺️ > > We should completely remove the default section because we cannot set any reasonable defaults for all possible filetypes. For example, IDEs tend to write their own files to a subfolder like .vscode or .kdev4. A default section would apply to files in there, too, which is not safe in general. > > We also should not use insert_final_newline and trim_trailing_whitespace anywhere in .editorconfig! Editors interpret these lines differently, some will wipe whitespaces across the whole file, others will only remove them from lines being edited and others change their behavior between releases. Limiting those options to a subset like *.c/*.h will not help: As written in my other response, the definition of whitespace is ambiguous. Unicode considers form feed to be a whitespace [0], causing some editors to wipe form feeds when trim_trailing_whitespace is true which is not what we want in OVS. As Robin mentioned, we already have a test for our definition of whitespaces and thus we can avoid this whitespace mess by not using it in .editorconfig. > > [0] https://en.wikipedia.org/wiki/Whitespace_character > Updated patch is out 🥂 https://patchwork.ozlabs.org/project/openvswitch/patch/20231026110729.21961-1-jmeng@redhat.com/
diff --git a/.editorconfig b/.editorconfig index 685c72750..5be5415da 100644 --- a/.editorconfig +++ b/.editorconfig @@ -13,6 +13,10 @@ indent_style = space indent_size = 4 max_line_length = 79 +[*.{patch,diff}] +insert_final_newline = false +trim_trailing_whitespace = false + [include/linux/**.h] indent_style = tab indent_size = tab