Re: [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF

[Junio C Hamano <gitster@pobox.com>]

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> A following commit will fix a bug in the ref-filter API that causes
> commit and tag messages containing CRLF to be incorrectly parsed and
> displayed.
>
> Add a test library (t/lib-crlf-messages.sh) that creates refs with such
> commit messages, so that we can easily test that this bug does not
> appear in other commands in the future.
> ...
> The function `test_crlf_subject_body_and_contents` can be used to test
> that the `--format` option of `branch`, `tag`, `for-each-ref` and
> `log` correctly displays the subject, body and raw content of commits and
> tag messages.

I am not sure about the wisdom of this arrangement.  Surely you do
not want to write duplicated set-up for (existing) test scripts for
for-each-ref, branch and tag subcommands, assuming that these test
scripts are separated for subcommands they test.

But you can have a single test script, that is differentiated from
all other test scripts by what it tests: having to deal with commits
that use CRLF.  Then we do not have to add dot-includable test
library that lets various tests to create these same funny commits.
Instead, we can just do these as normal set-up step(s) for that
single test scripts, and then in that test scripts, verify what is
shown by various commands that share the underlying ref-filter
machinery.  No?


> diff --git a/t/lib-crlf-messages.sh b/t/lib-crlf-messages.sh
> new file mode 100644
> index 0000000000..10a2b57280
> --- /dev/null
> +++ b/t/lib-crlf-messages.sh
> @@ -0,0 +1,90 @@
> +# Setup refs with commit and tag messages containing CRLF
> +
> +LIB_CRLF_BRANCHES=""
> +
> +create_crlf_ref () {
> +	message="$1" &&
> +	subject="$2" &&
> +	body="$3" &&
> +	branch="$4" &&
> +	printf "${message}" >.crlf-message-${branch}.txt &&
> +	printf "${subject}" >.crlf-subject-${branch}.txt &&
> +	printf "${body}" >.crlf-body-${branch}.txt &&
> +	LIB_CRLF_BRANCHES="${LIB_CRLF_BRANCHES} ${branch}"
> +	test_tick &&
> +	hash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&
> +	git branch ${branch} ${hash} &&
> +	git tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim
> +}
> +
> +create_crlf_refs () {
> +	message="Subject first line\r\n\r\nBody first line\r\nBody second line\r\n" &&
> +	body="Body first line\r\nBody second line\r\n" &&
> +	subject="Subject first line" &&
> +	branch="crlf" &&
> +	create_crlf_ref "${message}" "${subject}" "${body}" "${branch}" &&
> +	message="Subject first line\r\n\r\n\r\nBody first line\r\nBody second line\r\n" &&
> +	branch="crlf-empty-lines-after-subject" &&
> +	create_crlf_ref "${message}" "${subject}" "${body}" "${branch}" &&
> +	message="Subject first line\r\nSubject second line\r\n\r\nBody first line\r\nBody second line\r\n" &&
> +	subject="Subject first line Subject second line" &&
> +	branch="crlf-two-line-subject" &&
> +	create_crlf_ref "${message}" "${subject}" "${body}" "${branch}" &&
> +	message="Subject first line\r\nSubject second line" &&
> +	subject="Subject first line Subject second line" &&
> +	body="" &&
> +	branch="crlf-two-line-subject-no-body" &&
> +	create_crlf_ref "${message}" "${subject}" "${body}" "${branch}" &&
> +	message="Subject first line\r\nSubject second line\r\n" &&
> +	branch="crlf-two-line-subject-no-body-trailing-newline" &&
> +	create_crlf_ref "${message}" "${subject}" "${body}" "${branch}" &&
> +	message="Subject first line\r\nSubject second line\r\n\r" &&
> +	branch="crlf-two-line-subject-no-body-trailing-newline2" &&
> +	create_crlf_ref "${message}" "${subject}" "${body}" "${branch}"
> +}
> +
> +test_create_crlf_refs () {
> +	test_expect_success 'setup refs with CRLF commit messages' '
> +		create_crlf_refs
> +	'
> +}
> +
> +cleanup_crlf_refs () {
> +	for branch in ${LIB_CRLF_BRANCHES}; do
> +		git branch -D ${branch} &&
> +		git tag -d tag-${branch} &&
> +		rm .crlf-message-${branch}.txt &&
> +		rm .crlf-subject-${branch}.txt &&
> +		rm .crlf-body-${branch}.txt
> +	done
> +}
> +
> +test_cleanup_crlf_refs () {
> +	test_expect_success 'cleanup refs with CRLF commit messages' '
> +		cleanup_crlf_refs
> +	'
> +}
> +
> +test_crlf_subject_body_and_contents() {

It does not excempt a script from being subject to the coding
guidelines to be a test library.

> +	command_and_args="$@" &&
> +	command=$1 &&
> +	if [ ${command} = "branch" ] || [ ${command} = "for-each-ref" ] || [ ${command} = "tag" ]; then
> +		atoms="(contents:subject) (contents:body) (contents)"
> +	elif [ ${command} = "log" ] || [ ${command} = "show" ]; then
> +		atoms="s b B"
> +	fi &&

This is the part that made me react to the organization.  Even
though this helper "library" pretends to be generic, it needs to
actually know exactly what subcommands are going to be tested with
the helper.  It is probably easier to read and understand if these
helper functions are defined in the same script as the one that
tests these various commands but for one specific aspect of these
commands (i.e. how the log message with funny line ending convention
are split into subject and body).

> +	files="subject body message" &&
> +	while  [ -n "${atoms}" ]; do
> +		set ${atoms} && atom=$1 && shift && atoms="$*" &&
> +		set ${files} &&	file=$1 && shift && files="$*" &&
> +		test_expect_success "${command}: --format='%${atom}' works with CRLF input" "
> +			rm -f expect &&
> +			for ref in ${LIB_CRLF_BRANCHES}; do
> +				cat .crlf-${file}-\"\${ref}\".txt >>expect &&
> +				printf \"\n\" >>expect
> +			done &&
> +			git $command_and_args --format=\"%${atom}\" >actual &&
> +			test_cmp expect actual
> +		"
> +	done
> +}