[webkit-dev] Removal of trailing whitespace

[Anthony Ricaud]

(not a stakeholder either)

On top of ignoring whitespace, Git and Github support ignoring commits when performing `git blame`.

With that tool, one commit could remove trailing whitespace over the all project and add some editorconfig or CI checks to prevent them from coming back. Then, a second commit would add the first commit to the ignore-revs-file.

https://git-scm.com/docs/git-blame#Documentation/git-blame.txt---ignore-revs-fileltfilegt

https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

> On 13 Apr 2023, at 08:43, Mathias Bynens via webkit-dev <webkit-dev at lists.webkit.org> wrote:
> 
> (not a stakeholder; just curious)
> 
> Does it help that Git supports the `-w` / `--ignore-all-space` flag to ignore whitespace changes? It works with `git diff`, `git blame`, etc.
> 
> GitHub also supports it — compare e.g.
> 
> https://github.com/WebKit/WebKit/commit/2ca55a3e19df60127dec09f3af22e4d3ab2943ec
> vs.
> https://github.com/WebKit/WebKit/commit/2ca55a3e19df60127dec09f3af22e4d3ab2943ec?w=1
> 
> On Wed, Apr 12, 2023 at 9:35 PM Chris Dumez via webkit-dev <webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>> wrote:
>> 
>> 
>>> On Apr 12, 2023, at 10:23 AM, Ryosuke Niwa via webkit-dev <webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>> wrote:
>>> 
>>> Yeah, enforcing that new or otherwise modified lines don’t have trailing whitespaces would be good.
>> 
>> Yes, I wouldn’t mind that either.
>> 
>> However, https://commits.webkit.org/262879@main has just landed and if you look at the changes to Document.cpp, it is mostly spacing changes :(
>> It makes it harder to review or to identify meaningful changes in a patch after landing. It also pollutes git blame for no great reason.
>> 
>>> 
>>> - R. Niwa
>>> 
>>>> On Apr 12, 2023, at 10:20 AM, Yusuke Suzuki <ysuzuki at apple.com <mailto:ysuzuki at apple.com>> wrote:
>>>> 
>>>> I agree that we should not do it because it pollutes change history of files, git-blame results, and review-diff in PR.
>>>> But at the same time, I think there is no reason to add a new trailing whitespace via a new commit.
>>>> It is nice if we can enforce this rule only for newly added code (via style-checker) not to add new trailing spaces.
>>>> 
>>>> -Yusuke
>>>> 
>>>>> On Apr 12, 2023, at 10:08 AM, Ryosuke Niwa via webkit-dev <webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>> wrote:
>>>>> 
>>>>> WebKi proejctt’s long term policy has been to not do this:
>>>>> https://lists.webkit.org/pipermail/webkit-dev/2009-August/009665.html
>>>>> 
>>>>> I don’t think we should change that.
>>>>> 
>>>>> - R. Niwa
>>>>> 
>>>>>> On Apr 12, 2023, at 9:17 AM, Chris Dumez via webkit-dev <webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>> wrote:
>>>>>> 
>>>>>> I am against this because it adds a lot of noise to patches I am trying to review.
>>>>>> I have seen PRs where white space changes account for more than half the patch I am trying to review.
>>>>>> 
>>>>>> Dropping trailing spaces on the lines you’re modifying is OK but in the whole file is too noisy IMO.
>>>>>> 
>>>>>> Chris.
>>>>>> 
>>>>>>> On Apr 12, 2023, at 1:22 AM, Anne van Kesteren via webkit-dev <webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>> wrote:
>>>>>>> 
>>>>>>> To reduce the overhead of switching between projects with different
>>>>>>> whitespace requirements, I would like to suggest we start being
>>>>>>> lenient when trailing whitespace is removed. In particular when a file
>>>>>>> is being changed to fix a bug.
>>>>>>> 
>>>>>>> I could see going even further and enforcing this via the style
>>>>>>> checker, if there is appetite for that.
>>>>>>> 
>>>>>>> Thanks for considering!
>>>>>>> _______________________________________________
>>>>>>> webkit-dev mailing list
>>>>>>> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
>>>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>>>> 
>>>>>> _______________________________________________
>>>>>> webkit-dev mailing list
>>>>>> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
>>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>>> 
>>>>> _______________________________________________
>>>>> webkit-dev mailing list
>>>>> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>> 
>>> 
>>> _______________________________________________
>>> webkit-dev mailing list
>>> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20230413/402f5eea/attachment.htm>