periodic: Make daily diff(1) output as small is possible

[michaelo]

Summary

Make, by default, daily diff(1) ignore whitespace changes and the unified output a
context of zero (0) lines. This reduces output of unrelated lines in e-mails
delivered to root.

PR: 270266
Approved by: jrm (mentor), emaste, karels
MFC after: 1 month
Relnotes: yes

[michaelo]

Add tab

[michaelo]

Please add someone additionally for review you think is approriate.

[michaelo]

Folks, you have some spare cycles to look at this?

[jrm]

Seems fine to ignore these kinds of whitespace changes and have shorter emails by not providing diff context, but let's hear from a few others who have touched these files.

[emaste]

I might suggest that the change in periodic.conf be done in a separate commit - i.e., add the infrastructure for daily_diff_flags first, then make use of it.

share/man/man5/periodic.conf.5

26

update prior to commit

[michaelo]

In D42762#978355, @emaste wrote:

I might suggest that the change in periodic.conf be done in a separate commit - i.e., add the infrastructure for daily_diff_flags first, then make use of it.

I thought of this as well. Will split.

[emaste]

Will split.

Great. That will allow us to have the infrastructure in stable/14 and stable/13, but not change the default within the branch.

Also make sure there's a Relnotes: YES

[• karels]

Seems OK. I wouldn't object to -U 1 either, but for most of these things context is probably unimportant.

[michaelo]

Split into two commits^

[michaelo]

This one is to be merged to stable branches since it does not change default
behavior.

[michaelo]

Add flag

[michaelo]

Modify flags

[michaelo]

Split

[michaelo]

.

[michaelo]

.

[• karels]

Looks like this changes behavior by adding -u to the last 3 daily scripts. But I'm not sure why this should be MFCed. Or you could omit daily_diff_flags from /etc/defaults/rc.conf and set defaults in the scripts that currently use -u if the variable hadn't been set.

[michaelo]

In D42762#978367, @emaste wrote:

Will split.

Great. That will allow us to have the infrastructure in stable/14 and stable/13, but not change the default within the branch.

Also make sure there's a Relnotes: YES

Split as expected. One can go into all branches and the change will remain on main (15).

[michaelo]

In D42762#978457, @karels wrote:

Looks like this changes behavior by adding -u to the last 3 daily scripts. But I'm not sure why this should be MFCed. Or you could omit daily_diff_flags from /etc/defaults/rc.conf and set defaults in the scripts that currently use -u if the variable hadn't been set.

This one won't be MFC'd. But diff(1) without an option will default to -u as far as I can see. This change is intended for 15 only. The introduction of the flag for all branches.

[jrm]

Looks good (assuming the commit title is updated). Thanks.

[jrm]

[otis]

[• karels]

In D42762#978459, @michaelo wrote:

This one won't be MFC'd. But diff(1) without an option will default to -u as far as I can see. This change is intended for 15 only. The introduction of the flag for all branches.

mike@pughole$ diff /tmp/[xy]
1a2

hi

mike@pughole$ diff -u /tmp/[xy]

• /tmp/x 2023-12-04 15:35:49.864258000 -0600

+++ /tmp/y 2023-12-04 15:35:56.220759000 -0600
@@ -1 +1,2 @@
here
+hi

[michaelo]

Rebase

[michaelo]

In D42762#978495, @karels wrote:

In D42762#978459, @michaelo wrote:

This one won't be MFC'd. But diff(1) without an option will default to -u as far as I can see. This change is intended for 15 only. The introduction of the flag for all branches.

mike@pughole$ diff /tmp/[xy]
1a2

hi

mike@pughole$ diff -u /tmp/[xy]

• /tmp/x 2023-12-04 15:35:49.864258000 -0600

+++ /tmp/y 2023-12-04 15:35:56.220759000 -0600
@@ -1 +1,2 @@
here
+hi

Can you reformat your message? I cannot read it or I do not understand what you are trying to say.

[• karels]

Sorry, apparently parts were interpreted as markdown, but it was the output of diff and diff -u. My point was that -u is not the default, and is more verbose than the default. WIth the current change, it no longer matters.

[michaelo]

In D42762#978592, @karels wrote:

Sorry, apparently parts were interpreted as markdown, but it was the output of diff and diff -u. My point was that -u is not the default, and is more verbose than the default. WIth the current change, it no longer matters.

Alright so this makes sense to you?

[• karels]

In D42762#978597, @michaelo wrote:

In D42762#978592, @karels wrote:

Sorry, apparently parts were interpreted as markdown, but it was the output of diff and diff -u. My point was that -u is not the default, and is more verbose than the default. WIth the current change, it no longer matters.

Alright so this makes sense to you?

Using -U 0 makes sense. I don't see why it should be changed to -u before that though, that is not an improvement

[michaelo]

Rebase

[• karels]

Should this contain the periodic.conf* changes? I would have thought they'd be in the other review only, and now I think they conflict.

[michaelo]

In D42762#979469, @karels wrote:

Should this contain the periodic.conf* changes? I would have thought they'd be in the other review only, and now I think they conflict.

No, this is correct. The other one is a MFC'able preprequisite, but this might not be backported, it does not apply anything, but just introduces the flag for daily diff. This applies minimal diff flags to all scripts, effectively. This review cannot be merged into main until the other one isn't merged.

[• karels]

In D42762#979550, @michaelo wrote:

In D42762#979469, @karels wrote:

Should this contain the periodic.conf* changes? I would have thought they'd be in the other review only, and now I think they conflict.

No, this is correct. The other one is a MFC'able preprequisite, but this might not be backported, it does not apply anything, but just introduces the flag for daily diff. This applies minimal diff flags to all scripts, effectively. This review cannot be merged into main until the other one isn't merged.

Sorry, I hadn't noticed that security_status_diff_flags already existed, so I confused the two flags variables. Now that I see what it is used for, I'm a bit apprehensive about changing it. @emaste, what do you think? Also, the change to security_status_diff_flags is unrelated to the other changes, so should be in a separate commit.

[michaelo]

In D42762#979587, @karels wrote:

In D42762#979550, @michaelo wrote:

In D42762#979469, @karels wrote:

Should this contain the periodic.conf* changes? I would have thought they'd be in the other review only, and now I think they conflict.

No, this is correct. The other one is a MFC'able preprequisite, but this might not be backported, it does not apply anything, but just introduces the flag for daily diff. This applies minimal diff flags to all scripts, effectively. This review cannot be merged into main until the other one isn't merged.

Sorry, I hadn't noticed that security_status_diff_flags already existed, so I confused the two flags variables. Now that I see what it is used for, I'm a bit apprehensive about changing it. @emaste, what do you think? Also, the change to security_status_diff_flags is unrelated to the other changes, so should be in a separate commit.

It is not unrelated. The issue says "diff" output, it does not say "daily diff" output. The point is to consistently reduce the diff output. Security happended to have a flag, daily not. That is why you are/were confused. Do you still think that it should be two commits one containing the word "daily" the other one "security"?

[• karels]

In D42762#979649, @michaelo wrote:

It is not unrelated. The issue says "diff" output, it does not say "daily diff" output. The point is to consistently reduce the diff output. Security happended to have a flag, daily not. That is why you are/were confused. Do you still think that it should be two commits one containing the word "daily" the other one "security"?

The two changes are similar, but one changes the setting of a variable that affects security scripts, the other uses a different, new variable in daily scripts. They don't interact. Yes, I think they should be two commits. For example, if there is objection to the security change, it is easier to revert if it is a separate commit.

[michaelo]

In D42762#979653, @karels wrote:

In D42762#979649, @michaelo wrote:

It is not unrelated. The issue says "diff" output, it does not say "daily diff" output. The point is to consistently reduce the diff output. Security happended to have a flag, daily not. That is why you are/were confused. Do you still think that it should be two commits one containing the word "daily" the other one "security"?

The two changes are similar, but one changes the setting of a variable that affects security scripts, the other uses a different, new variable in daily scripts. They don't interact. Yes, I think they should be two commits. For example, if there is objection to the security change, it is easier to revert if it is a separate commit.

Moved out to a separate review. Will update this one shortly.

[michaelo]

This is now rebased on top of https://reviews.freebsd.org/D42900. Here is variable
modified. This truly introduces a change in behavior.

[• karels]

Does this need rebasing again (-u was moved)? Otherwise looks good.

[michaelo]

In D42762#981942, @karels wrote:

Does this need rebasing again (-u was moved)? Otherwise looks good.

Yes, it does. Will do.

[michaelo]

Rebase