On Thu, Dec 09, 2021 at 12:11:00AM -0500, Eric Sunshine wrote: > Several tests assign the output of `$(...)` command substitution to an > "expect" variable, taking advantage of the fact that `$(...)` folds out > the final line terminator while leaving internal line terminators > intact. They do this because the "actual" string with which "expect" > will be compared is shaped the same way. However, this intent (having > internal line terminators, but no final line terminator) is not > necessarily obvious at first glance and may confuse casual readers. The > intent can be made more obvious by using `printf` instead, with which > line termination is stated clearly: > > printf "sixth\nthird" > > In fact, many other tests in this script already use `printf` for > precisely this purpose, thus it is an established pattern. Therefore, > convert these tests to employ `printf`, as well. Seems reasonable. I don't think these tests actually care about the lack of trailing newline, so another option is to use tformat. Or its shorter cousin, --format. E.g.: > - actual=$(git log --pretty="format:%s" --diff-filter=M HEAD) && > - expect=$(echo second) && > - verbose test "$actual" = "$expect" > + git log --pretty="format:%s" --diff-filter=M HEAD >actual && > + printf "second" >expect && > + test_cmp expect actual becomes: git log --format=%s --diff-filter=M HEAD >actual && echo second >expect && test_cmp expect actual which is even less magical. But if the existing pattern is there in nearby tests, I don't have any problem with following it. > While at it, modernize the tests to use test_cmp() to compare the > expected and actual output rather than using the semi-deprecated > `verbose test "$x" = "$y"`. Yay. Happy to see more "verbose" calls cleaned up. -Peff
