Re: [ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.
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 wrote:
>>> Eelco Chaudron, Oct 25, 2023 at 14:56:
On 25 Oct 2023, at 13:52, jm...@redhat.com wrote:
> From: Jakob Meng
>
> 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
>>> 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
> 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-jm...@redhat.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.
On 25.10.23 15:48, Mike Pattrick wrote:
> On Wed, Oct 25, 2023 at 9:02 AM Robin Jarry wrote:
>> Eelco Chaudron, Oct 25, 2023 at 14:56:
>>> On 25 Oct 2023, at 13:52, jm...@redhat.com wrote:
>>>
From: Jakob Meng
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
>> 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
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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.
On Wed, Oct 25, 2023 at 9:02 AM Robin Jarry wrote:
>
> Eelco Chaudron, Oct 25, 2023 at 14:56:
> > On 25 Oct 2023, at 13:52, jm...@redhat.com wrote:
> >
> > > From: Jakob Meng
> > >
> > > 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
>
> 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
>
> What do you think?
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.
Eelco Chaudron, Oct 25, 2023 at 14:56:
On 25 Oct 2023, at 13:52, jm...@redhat.com wrote:
> From: Jakob Meng
>
> 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
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?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.
On 25 Oct 2023, at 13:52, jm...@redhat.com wrote:
> From: Jakob Meng
>
> 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.
On 25.10.23 13:52, jm...@redhat.com wrote:
> From: Jakob Meng
>
> 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
> ---
> .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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
