Hi Junio, On 21/11/2022 00:37, Junio C Hamano wrote: > Philip Oakley <philipoakley@iee.email> writes: > >> Ping, Hopefully an easy single patch review for someone on-list. >> >> Potential review points: >> >> Is the commit message sufficient? >> Are the tests: Sufficient, Complete, Correct ? >> Is `qz_to_tab_space` conversion applied correctly? > Is the feature and the design sensible? > > Are the tests checking interesting cases? The underlying mechanism > uses strbuf_utf8_replace() because there are character strings whose > display columns do not match their byte length (otherwise you can > just use strlen() and chomp at byte boundary), so a test whose > result would be different if strbuf_utf8_replace() were replaced > with a more naive strbuf_splice() would be valuable and meaningful. The tests do include those utf8 cases. They are in the existing t/txxxx tests that specifically cover the utf8 non-English characters. I did check how they were constructed and what they were doing to confirm expectations. Were you thinking that an extra comment would be useful in the commit message to confirm the completeness of the existing tests? -- Philip
