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. -- Štěpán
