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.
