| Message ID | 20211020124512.490288-1-snemec@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft,1/3] tests: shell: README: copy edit | expand |
Hi, What you you mean with 'copy edit'? Please make subject line a bit more comprehensible. Also, adding at least a single line of description is mandatory, even if the subject speaks for itself. The actual changes look good to me, though! Thanks, Phil
On Wed, 20 Oct 2021 17:04:02 +0200 Phil Sutter wrote: > What you you mean with 'copy edit'? https://en.wikipedia.org/wiki/Copy_editing > Please make subject line a bit more comprehensible. Would "fix language issues" work for you? Or, could we perhaps keep the original subject, and add "fix language issues" in the body, to address your other concern? > Also, adding at least a single line of description is mandatory, even > if the subject speaks for itself. Commit messages consisting of only the subject and trailers (SOB et al.) are quite common, both in this project and elsewhere. [1] I suppose that as someone responsible for reviewing and applying patches, you're free to install any hoops you see fit for contributors to jump through, but if it is the project's and contributors' best interest you have in mind, I think it would be better if you mentioned the "description is always mandatory" rule explicitly in patch submission guidelines [2] (providing a rationale and being consistent about it in reality would be better still). Thanks, Štěpán [1] To get some picture, you can pipe output of the attached script to 'git log --stdin --no-walk', or further pipe that to 'git shortlog -ns' for a summary. [2] https://wiki.nftables.org/wiki-nftables/index.php/Portal:DeveloperDocs/Patch_submission_guidelines #!/usr/bin/env perl use strict; use warnings; my $log_cmd = "git log -z --pretty=format:'%H\n%b'"; local $/ = "\0"; open(my $log_fh, '-|', $log_cmd) or die "Can't start git log: $!"; while (<$log_fh>) { chomp; my ($hash, $body) = split(/\n/, $_, 2); print($hash, "\n") if ($body =~ /\A\s*(?:[^:\n]+: +[^\n]+\n?)*\s*\Z/); }
Hi, On Thu, Oct 21, 2021 at 10:30:25AM +0200, Štěpán Němec wrote: > On Wed, 20 Oct 2021 17:04:02 +0200 > Phil Sutter wrote: > > > What you you mean with 'copy edit'? > > https://en.wikipedia.org/wiki/Copy_editing Ah, thanks! I didn't know the term. > > Please make subject line a bit more comprehensible. > > Would "fix language issues" work for you? Or, could we perhaps keep the > original subject, and add "fix language issues" in the body, to address > your other concern? Yes, I'm fine with keeping the subject if a description is added. > > Also, adding at least a single line of description is mandatory, even > > if the subject speaks for itself. > > Commit messages consisting of only the subject and trailers (SOB et al.) > are quite common, both in this project and elsewhere. [1] Well yes, roughly 10% of all commits in nftables repo are. In iptables repo it's even worse with close to 50%. Thanks a lot for providing the script, BTW! :) > I suppose that as someone responsible for reviewing and applying > patches, you're free to install any hoops you see fit for contributors > to jump through, but if it is the project's and contributors' best > interest you have in mind, I think it would be better if you mentioned > the "description is always mandatory" rule explicitly in patch > submission guidelines [2] (providing a rationale and being consistent > about it in reality would be better still). Sorry for not checking the guideline but quoting advice I once received from the top of my head instead. Maybe I was incorrect and in obvious cases the description is optional. The relevant text in [2] at least doesn't explicitly state it is, while being pretty verbose about it regarding cover letters. Cheers, Phil > [2] https://wiki.nftables.org/wiki-nftables/index.php/Portal:DeveloperDocs/Patch_submission_guidelines
On Thu, 21 Oct 2021 12:26:44 +0200 Phil Sutter wrote: > Sorry for not checking the guideline but quoting advice I once received > from the top of my head instead. Maybe I was incorrect and in obvious > cases the description is optional. The relevant text in [2] at least > doesn't explicitly state it is, while being pretty verbose about it > regarding cover letters. Does this mean that you retract your requirement? If not, could you please make sure that you and the other maintainers are on the same page about this, and document the requirement? Regarding this patch series (if it is to be resent, in part or as a whole): we've discussed the first patch so far; the other two patches have no description, either. Do they need one? If so, could you provide some suggestions? I can't think of anything sensible that isn't already said in the subject, Fixes:, or the modified README text itself. Thanks, Štěpán
On Thu, Oct 21, 2021 at 01:03:23PM +0200, Štěpán Němec wrote: > On Thu, 21 Oct 2021 12:26:44 +0200 > Phil Sutter wrote: > > > Sorry for not checking the guideline but quoting advice I once received > > from the top of my head instead. Maybe I was incorrect and in obvious > > cases the description is optional. The relevant text in [2] at least > > doesn't explicitly state it is, while being pretty verbose about it > > regarding cover letters. > > Does this mean that you retract your requirement? If not, could you > please make sure that you and the other maintainers are on the same page > about this, and document the requirement? > > Regarding this patch series (if it is to be resent, in part or as a > whole): we've discussed the first patch so far; the other two patches > have no description, either. Do they need one? If so, could you provide > some suggestions? I can't think of anything sensible that isn't already > said in the subject, Fixes:, or the modified README text itself. I also prefer if there is oneline description in the patch. My suggestions: * patch 1/3, not clear to me what "copy edit" means either. * patch 2/3, what is the intention there? a path to the nft executable is most generic way to refer how you use $NFT, right? * patch 3/3, I'd add a terse sentence so I do not need to scroll down and read the update to README to understand what this patch updates. I'd suggest: "Test files are located with find, so they can be placed in any location." Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this to the README file. The test infrastructure is only used for internal development use, this is included in tarballs but distributors do not package this. So please address Phil's feedback. Thanks.
On Wed, 27 Oct 2021 11:06:00 +0200 Pablo Neira Ayuso wrote: > I also prefer if there is oneline description in the patch. My > suggestions: > > * patch 1/3, not clear to me what "copy edit" means either. Description proposal: Grammar, wording, formatting fixes (no substantial change of meaning). > * patch 2/3, what is the intention there? a path to the nft executable > is most generic way to refer how you use $NFT, right? No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly or at least also meant for humans, i.e., that the person reading the commit message and wanting to better understand the change would look at the referenced commit, but if that is a wrong assumption to make, I propose to add the following description: Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g. 'valgrind nft'. > * patch 3/3, I'd add a terse sentence so I do not need to scroll down > and read the update to README to understand what this patch updates. > I'd suggest: "Test files are located with find, so they can be placed > in any location." That text was just split to a separate paragraph, it has nothing to do with the actual change. > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this > to the README file. The test infrastructure is only used for internal > development use, this is included in tarballs but distributors do not > package this. IMO this argument should speak _for_ including the commit reference rather than omitting it, as the developer is more likely to be interested in the commit than the consumer. I thought about making the wording simply describe the current state without any historical explanations, but saying "Test files are executable files matching the pattern <<name_N>>, where N doesn't mean anything." seemed weird. Please advise.
On Wed, Oct 27, 2021 at 11:51:12AM +0200, Štěpán Němec wrote: > On Wed, 27 Oct 2021 11:06:00 +0200 > Pablo Neira Ayuso wrote: > > > I also prefer if there is oneline description in the patch. My > > suggestions: > > > > * patch 1/3, not clear to me what "copy edit" means either. > > Description proposal: > > Grammar, wording, formatting fixes (no substantial change of meaning). > > > * patch 2/3, what is the intention there? a path to the nft executable > > is most generic way to refer how you use $NFT, right? > > No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly > or at least also meant for humans, i.e., that the person reading the > commit message and wanting to better understand the change would look at > the referenced commit, but if that is a wrong assumption to make, I > propose to add the following description: > Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g. > 'valgrind nft'. OK, but why the reader need to know about former behaviour? The git repository already provides the historical information if this is of his interest. To me, the README file should contain the most up to date information that is relevant to run the test infrastructure. > > * patch 3/3, I'd add a terse sentence so I do not need to scroll down > > and read the update to README to understand what this patch updates. > > I'd suggest: "Test files are located with find, so they can be placed > > in any location." > > That text was just split to a separate paragraph, it has nothing to do > with the actual change. OK, then please document every update in your patch. > > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this > > to the README file. The test infrastructure is only used for internal > > development use, this is included in tarballs but distributors do not > > package this. > > IMO this argument should speak _for_ including the commit reference > rather than omitting it, as the developer is more likely to be > interested in the commit than the consumer. > > I thought about making the wording simply describe the current state > without any historical explanations, but saying "Test files are > executable files matching the pattern <<name_N>>, where N doesn't mean > anything." seemed weird. For historical explanations, you can dig into git. Thanks.
On Wed, 27 Oct 2021 12:13:57 +0200 Pablo Neira Ayuso wrote: > On Wed, Oct 27, 2021 at 11:51:12AM +0200, Štěpán Němec wrote: >> > * patch 2/3, what is the intention there? a path to the nft executable >> > is most generic way to refer how you use $NFT, right? >> >> No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly >> or at least also meant for humans, i.e., that the person reading the >> commit message and wanting to better understand the change would look at >> the referenced commit, but if that is a wrong assumption to make, I >> propose to add the following description: > >> Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g. >> 'valgrind nft'. > > OK, but why the reader need to know about former behaviour? The git > repository already provides the historical information if this is of > his interest. To me, the README file should contain the most up to > date information that is relevant to run the test infrastructure. I agree, and that's what the patch does: 'a path to the nft executable' is the former behaviour, arbitrary command is the most up to date information. (The "Since..." text above is my proposal for the commit description, not for the README file.) >> > * patch 3/3, I'd add a terse sentence so I do not need to scroll down >> > and read the update to README to understand what this patch updates. >> > I'd suggest: "Test files are located with find, so they can be placed >> > in any location." >> >> That text was just split to a separate paragraph, it has nothing to do >> with the actual change. > > OK, then please document every update in your patch. >> > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this >> > to the README file. The test infrastructure is only used for internal >> > development use, this is included in tarballs but distributors do not >> > package this. >> >> IMO this argument should speak _for_ including the commit reference >> rather than omitting it, as the developer is more likely to be >> interested in the commit than the consumer. >> >> I thought about making the wording simply describe the current state >> without any historical explanations, but saying "Test files are >> executable files matching the pattern <<name_N>>, where N doesn't mean >> anything." seemed weird. > > For historical explanations, you can dig into git. Would the following README text work for you? Test files are executable files matching the pattern <<name_N>>, where N has no meaning. All tests should return 0 on success. Since they are located with `find', test files can be put in any subdirectory. Maybe saying "where N should be 0 in all new tests" would be less strange? I think both are significantly less helpful than my original version, but at least they aren't wrong like the current text. Proposed commit description, based on your comments: Since commit 4d26b6dd3c4c, test file name suffix no longer reflects expected exit code in all cases. Move the sentence "Since they are located with `find', test files can be put in any subdirectory." to a separate paragraph.
On Wed, Oct 27, 2021 at 01:04:38PM +0200, Štěpán Němec wrote: > On Wed, 27 Oct 2021 12:13:57 +0200 > Pablo Neira Ayuso wrote: > > > On Wed, Oct 27, 2021 at 11:51:12AM +0200, Štěpán Němec wrote: > >> > * patch 2/3, what is the intention there? a path to the nft executable > >> > is most generic way to refer how you use $NFT, right? > >> > >> No, not since 7c8a44b25c22. I've always thought that 'Fixes:' is mostly > >> or at least also meant for humans, i.e., that the person reading the > >> commit message and wanting to better understand the change would look at > >> the referenced commit, but if that is a wrong assumption to make, I > >> propose to add the following description: > > > >> Since commit 7c8a44b25c22, $NFT can contain an arbitrary command, e.g. > >> 'valgrind nft'. > > > > OK, but why the reader need to know about former behaviour? The git > > repository already provides the historical information if this is of > > his interest. To me, the README file should contain the most up to > > date information that is relevant to run the test infrastructure. > > I agree, and that's what the patch does: 'a path to the nft executable' > is the former behaviour, arbitrary command is the most up to date > information. (The "Since..." text above is my proposal for the commit > description, not for the README file.) OK, this slightly more verbose description in the paragraph above LGTM. [...] > >> > Regarding reference to 4d26b6dd3c4c, not sure it is worth to add this > >> > to the README file. The test infrastructure is only used for internal > >> > development use, this is included in tarballs but distributors do not > >> > package this. > >> > >> IMO this argument should speak _for_ including the commit reference > >> rather than omitting it, as the developer is more likely to be > >> interested in the commit than the consumer. > >> > >> I thought about making the wording simply describe the current state > >> without any historical explanations, but saying "Test files are > >> executable files matching the pattern <<name_N>>, where N doesn't mean > >> anything." seemed weird. > > > > For historical explanations, you can dig into git. > > Would the following README text work for you? > > Test files are executable files matching the pattern <<name_N>>, > where N has no meaning. All tests should return 0 on success. > > Since they are located with `find', test files can be put in any > subdirectory. LGTM. > Maybe saying "where N should be 0 in all new tests" would be less > strange? This is also OK, we can probably remove the trailing 0 at some point given it has no meaning anymore. > I think both are significantly less helpful than my original version, > but at least they aren't wrong like the current text. Makes sense. > Proposed commit description, based on your comments: > > Since commit 4d26b6dd3c4c, test file name suffix no longer reflects > expected exit code in all cases. > > Move the sentence "Since they are located with `find', test files can > be put in any subdirectory." to a separate paragraph. LGTM. Thanks.
diff --git a/tests/shell/README b/tests/shell/README index e0279bbdc30c..35f6e3785f0e 100644 --- a/tests/shell/README +++ b/tests/shell/README @@ -1,16 +1,17 @@ -This test-suite is intended to perform tests of higher level than -the other regression test-suite. +This test suite is intended to perform tests on a higher level +than the other regression test suites. -It can run arbitrary executables which can perform any test apart of testing -the nft syntax or netlink code (which is what the regression tests does). +It can run arbitrary executables which can perform any test, not +limited to testing the nft syntax or netlink code (which is what +the regression tests do). To run the test suite (as root): $ cd tests/shell # ./run-tests.sh -Test files are executables files with the pattern <<name_N>>, where N is the -expected return code of the executable. Since they are located with `find', -test-files can be spread in any sub-directories. +Test files are executable files matching the pattern <<name_N>>, +where N is the expected return code of the executable. Since they +are located with `find', test files can be put in any subdirectory. You can turn on a verbose execution by calling: # ./run-tests.sh -v @@ -18,11 +19,11 @@ You can turn on a verbose execution by calling: And generate missing dump files with: # ./run-tests.sh -g <TESTFILE> -Before each call to the test-files, `nft flush ruleset' will be called. -Also, test-files will receive the environment variable $NFT which contains the -path to the nftables binary being tested. +Before each test file invocation, `nft flush ruleset' will be called. +Also, test file process environment will include the variable $NFT +which contains the path to the nft binary being tested. You can pass an arbitrary $NFT value as well: # NFT=/usr/local/sbin/nft ./run-tests.sh -By default the tests are run with the nft binary at '../../src/nft' +By default, the tests are run with the nft binary at '../../src/nft'
Signed-off-by: Štěpán Němec <snemec@redhat.com> --- tests/shell/README | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) base-commit: 2139913694a9850c9160920b2c638aac4828f9bb