PR: 270266
Approved by: jrm (mentor), emaste, karels
MFC after: 1 month
Relnotes: yes
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| usr.sbin/periodic/periodic.conf | ||
|---|---|---|
| 34 | With whitespace characters displayed.  | |
Comment Actions
| usr.sbin/periodic/periodic.conf | ||
|---|---|---|
| 34 | That is on purpose. This review introduce the variable only. It doe not change the actual behavior. So it can be back ported. See Ed's request. | |
| usr.sbin/periodic/periodic.conf | ||
|---|---|---|
| 34 | This review does change the behavior. It added -u to the diff flags on the gpart, gmirror, and zfs scripts, and that is not the same as the default. It adds header lines and context. Trying to quote correctly: 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 If this is MFC'd without the other change, it will make the review more verbose for 3 scripts. I think those 3 should just be removed from the change. And moving -u to the conf file doesn't seem to serve much purpose. | |
| usr.sbin/periodic/periodic.conf | ||
|---|---|---|
| 34 | Indeed, you are right. Unfortunately, diff(1) does not offer the option to drop the header. I see no way to reconcile that unfortunately, maybe with sed(1), but I guess an provement request do diff(1) would also be approriate? So you are proposing to MFC both changes instead of just this one? | |
| usr.sbin/periodic/periodic.conf | ||
|---|---|---|
| 34 | No. If I were doing it, I would add diff flags with -U 0 in one change, which could be MFC'ed so that new scripts would work if back ported. I would change the scripts to use the new flags in a second change, which would not be MFC'ed. I see no reason to create a version using -u, which is a pessimization compared to diff with no options. And it's not just the header lines, but the 3 lines of context. I would definitely not MFC a change that makes some scripts start to use -u that didn't already. | |
| usr.sbin/periodic/periodic.conf | ||
|---|---|---|
| 34 | Alright, I see your point. Let me rework that. Let me also ask you the following two questions:
$ diff -b -U0 -L foo -L foo update-certs.sh update-certs-pkg.sh --- foo +++ foo @@ -1,5 +1,2 @@ -kinit -k $(hostname -s | tr a-z a-z)'$' -cd /usr/ports-overlays/ldadw-custom -svn up --trust-server-cert-failures=unknown-ca --non-interactive -(cd security/nss-siemens-cacerts-java/; make clean reinstall clean) -(cd security/siemens-cacerts/; make clean reinstall clean) +kinit -k $(hostname -s | tr a-z A-Z)'$' +pkg upgrade -y
| |
| usr.sbin/periodic/periodic.conf | ||
|---|---|---|
| 34 | Answers: I don't think I'd use -L. I think the number of lines is more important than the length of the lines, and -L used that way obscures which is new vs old. I have no idea whether people have machine-parsing of this output; it keeps changing though. I wouldn't MFC any format changes for a while, just in case. Personally, I'm fine with the script change only on 15. I don't think I'd put it in 13 in any case, but maybe 14 (aimed at 14.1). Others may have different opinions. | |
Comment Actions
@karels, addressed your comments. Please have a look. I will rework the other review.
Folks, please have a look, I'd to know your opinion what, how and where to merge.
Comment Actions
I'm sorry to belabor this review longer. However, this commit *in isolation* seems strange to me. We are adding a variable and saying it controls how some periodic scripts are run, but that's not true because we are only defining (and documenting) an unused variable.
Comment Actions
If we want to MFC anything soon, I prefer this approach. I wouldn't say that the other part should never be MFCed, but I wouldn't do it anytime soon. The daily output may not be easily machine-parseable, but that doesn't mean there aren't people doing it. Going from plain diff to -U 0 shouldn't lose information (whereas -u to -U 0 can, if context means anything). If this change is separate, any of the scripts in the other set could be merged individually, or new scripts could use daily_diff_flags and still be MFCed. Finally, it is good form to split changes into steps when possible.
@emaste what do you think? I was hoping for more input before approving, but my requested change has been made.
Comment Actions
I don't want to hold this up any longer, so don't let me stand in the way. That said, do we want to add an unused variable and document that it's doing something that it doesn't? In other words, this commit doesn't stand alone, unless I'm missing something.
Comment Actions
Let me try stating my point another way, and please correct me if I'm wrong. People will see the new text in periodic.conf(5) and assume that they can customize daily_diff_flags, but when they do, it will have no effect.
Comment Actions
I am not happy with this as well, though @karels wanted me to split. See his argument. I still stand by my opinion that both can go in one commit into main and MFC. Alternatively, security_diff_flags is modified and MFC'ed. Separately, daily_diff_flags is introduced, applied and MFC'ed as well.
Comment Actions
Re-reading the man page addition, it is probably overly specific even when the scripts are brought into the picture; it makes it sounds like it is every diff invoked by a daily script. Something more accurate would be good, e.g. referring to "some scripts" or something like that.
I didn't initiate the idea of splitting the changes, but I agree with it.
Comment Actions
My intent in asking to split is that we could have a change that adds the option for different output but does not make any change in the output by default, and we could MFC that. Then the second change introduces the behaviour change by default, which we would not necessarily be merged (we can separately decide whether or not we merge).
E.g.
13.2 - only supports old mode
13.3 - existing style by default, user can override
main - new style by default, user can override
At the time I made the suggestion I didn't realize that the existing diff invocations already have varying options. We could perhaps address this by having daily_diff_flags="" by default (at least in what gets merged back) and use ${daily_diff_flags:-<whatever>} -- i.e. keep the existing behaviour if daily_diff_flags is unset, but use it otherwise. I haven't looked to see how workable that idea would be though.
If it's not feasible to merge back the support but exclude the default change, then I think one commit to main is best, and we later decide on MFCing nothing, or MFCing including the default behaviour change.
Comment Actions
+1 to this because we introduce a variable that has an effect when customized, but in this commit, we do not change the default behaviour for any diff call.
Comment Actions
I wouldn't object to this. Scripts that previously used no options to diff could just use ${daily_diff_flags}.
Comment Actions
Folks, I think this addresses your concerns. Introduces and empty flag which
does *not* change any behavior. You may even override given '-u'. This can be
logically back ported. With a subsequent review the default behavior can change.
Let me know what you think.
| usr.sbin/periodic/etc/daily/200.backup-passwd | ||
|---|---|---|
| 43 | If someone were to customize daily_diff_flags, the -u -I '^#' would still be present. Should we do something like above so that the default value remains the same, but users can also fully customize the value? The same comment applies to the other diff calls with daily_diff_flags and hardcoded options. | |
| usr.sbin/periodic/etc/daily/200.backup-passwd | ||
|---|---|---|
| 43 | I have done this on purpose:
I agree that ${daily_diff_flags:--u} looks better but -u ${...} will at the end produce the same result. Should I change those invocations? I am fine with either one. | |
| usr.sbin/periodic/etc/daily/200.backup-passwd | ||
|---|---|---|
| 43 | I don't have strong opinions either way about hardcoding the -I '^#'. Is there a scenario in which users would want to see changes in comments? If we use -u ${...}, users who customize with something like daily_diff_flags=-U0 will get an odd diff invocation, no? | |
| usr.sbin/periodic/etc/daily/200.backup-passwd | ||
|---|---|---|
| 43 |
Me neither, that is excluded silently by default
Here, it makes not differences, but when reading the man page -u is mutually exclusive with -c and others. I will rewrite to avoid this case. Good point. | |