Re: [PATCH nft 1/3] tests: shell: README: copy edit

[Pablo Neira Ayuso <pablo@netfilter.org>]

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.