Cosmetic style(9) fixes space vs tab.
ClosedPublic

Authored by araujo on Jun 12 2018, 5:02 AM.

Details

Summary

Fixes space vs tab.

Test Plan

Rebuild world and launched a Fedora workstation VM using virtio-scsi and ahci-hd.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
araujo created this revision.Jun 12 2018, 5:02 AM

Please seperate this into 2 reviews, one for the white space/tab cleanup and one for the SPDX tag addition.

Thank you

Please seperate this into 2 reviews, one for the white space/tab cleanup and one for the SPDX tag addition.

Thank you

Please seperate this into 2 reviews, one for the white space/tab cleanup and one for the SPDX tag addition.

Thank you

I think it is unnecessary, it will give unnecessary extra work and as I have touched each file the changes can be done at once as it is more cosmetic and there is no functional changes.

Cheers,

It is normal to separate all whitespace changes from other. It is preferable to separate all SPDX changes from others when it is sweeping. I strongly assert that as the person now cleaning up a 180k lines and 16 commits of diff involving SPDX tags that this separation occur before this is commited.

It is normal to separate all whitespace changes from other. It is preferable to separate all SPDX changes from others when it is sweeping. I strongly assert that as the person now cleaning up a 180k lines and 16 commits of diff involving SPDX tags that this separation occur before this is commited.

Sorry, I strongly disagree, I would need to make two reviews and two commits that can be done at once and there is no functional changes.

The commit log easily can describe these changes:

  • Fix style(9) space vs tab.
  • Add SPDX tag on those files that didn't have it.

I'm inclined to listen to the reviewers advice that I added, if that is mandatory, I will do, otherwise I can't see any problem to have these changes in one commit.

araujo added a reviewer: grehan.
eadler resigned from this revision.Jun 12 2018, 7:15 AM
eadler added a subscriber: eadler.Jun 12 2018, 7:25 AM

It is normal to separate all whitespace changes from other. It is preferable to separate all SPDX changes from others when it is sweeping. I strongly assert that as the person now cleaning up a 180k lines and 16 commits of diff involving SPDX tags that this separation occur before this is commited.

Sorry, I strongly disagree, I would need to make two reviews and two commits that can be done at once and there is no functional changes.

The commit log easily can describe these changes:

  • Fix style(9) space vs tab.
  • Add SPDX tag on those files that didn't have it.

    I'm inclined to listen to the reviewers advice that I added, if that is mandatory, I will do, otherwise I can't see any problem to have these changes in one commit.

I agree with @rgrimes here. Separating out changes makes it much easier to merge later on as well as deal with merge conflicts. While we may not have many, there are also downstream consumers of our code that would appreciate small, self-contained changes.

mav added a comment.EditedJun 12 2018, 2:20 PM

I like the space cleanup, thanks, and SPDX part is also fine. But I also agree that mixing together unrelated changes is not a very good idea. While in this particular case I would personally not create much churn about it (much worse when formatting changes are mixed with code changes, which is not really a case here), once other people complained, I think Marcelo you could faster do the requested separation rather then argue indefinitely from weak position.

pfg requested changes to this revision.Jun 13 2018, 2:20 AM

I agree with other reviewers: The SPDX changes should be split.

This revision now requires changes to proceed.Jun 13 2018, 2:20 AM
pfg added a comment.Jun 13 2018, 2:22 AM

As a sidenote: one file in bhyve has no license. This is a problem.

In D15768#333595, @pfg wrote:

As a sidenote: one file in bhyve has no license. This is a problem.

In D15768#333595, @pfg wrote:

As a sidenote: one file in bhyve has no license. This is a problem.

I'm aware of that! I'm in contact with the author of that part and will add LICENSE soon.

Tks,

araujo updated this revision to Diff 43687.Jun 13 2018, 3:32 AM
  • Remove SPDX-License-Identifier changes already committed at: r335025
araujo retitled this revision from Cosmetic style(9) fixes space vs tab and add SPDX-License-Identifier to Cosmetic style(9) fixes space vs tab..Jun 13 2018, 3:32 AM
araujo edited the summary of this revision. (Show Details)
jhb accepted this revision.Jun 13 2018, 6:01 PM

FWIW, I sometimes post "combined" patches for review and once it is approved split it up into commits. It looks like you've already pared this down, but as general idea I don't think that's terrible for review so long as the commits are split up.

I think the whitespace changes are generally ok.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 14 2018, 1:35 AM
Closed by commit rS335104: Fix style(9) space vs tab. (authored by araujo, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.