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