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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 12, 2020, 22:47 UTC
Message-ID: <20201012224706.GA4318@flurp.local>
In-Reply-To: <f17d182c3bf5e758490441801423cdb0da17060d.1602526169.git.gitgitgadget@gmail.com>

On Mon, Oct 12, 2020 at 06:09:27PM +0000, Philippe Blain via GitGitGadget wrote:

> 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.

In addition to Junio's review comments...

Show 14 quoted lines

> 
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> diff --git a/t/lib-crlf-messages.sh b/t/lib-crlf-messages.sh
> @@ -0,0 +1,90 @@
> +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}"

Broken &&-chain.

Show 12 quoted lines

> +	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}" &&

This is somewhat onerous to digest and compose. Have you considered making it more automated and easier to read? Perhaps something like this:

    create_crlf_ref () {
        branch=$1
        cat >.crlf-message-$branch.txt &&
        sed -n "1,/^$/p" <.crlf-message-$branch.txt | sed "/^$/d" | append_cr >.crlf-subject-$branch.txt &&
        sed -n "/^$/,\$p" <.crlf-message-$branch.txt | sed "1d" | append_cr >.crlf-body-$branch.txt &&
        ...
    }

    create_crlf_refs () {
        create_crlf_ref crlf <<-\EOF
        Subject first line

        Body first line
        Body second line
        EOF
        ...
    }

Show 5 quoted lines

> +test_create_crlf_refs () {
> +	test_expect_success 'setup refs with CRLF commit messages' '
> +		create_crlf_refs
> +	'
> +}

This almost seems like an unnecessary indirection since callers could just as easily do this on their own, like this:

    test_expect_success 'setup refs with CRLF commit messages' '
        create_crlf_refs
    '

which isn't very burdensome. However, I suppose doing it this way gives consistent test titles between scripts, so not necessarily a strong objection on my part.

> +cleanup_crlf_refs () {
> +	for branch in ${LIB_CRLF_BRANCHES}; do

Our style is to place 'do' on its own line:

    for branch in $LIB_CRLF_BRANCHES
    do
        ...

This would be a syntax error if LIB_CRLF_BRANCHES is empty for some reason, but I suppose we don't really have to worry about it here(?).

Show 22 quoted lines

> +		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() {
> +	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 &&

Style:

    if test "$command" = "branch" || test ...
    then
        ...
    elif test ...
    then
        ...
    fi &&

> +	files="subject body message" &&
> +	while  [ -n "${atoms}" ]; do

Too many spaces after 'while'.

Style:

    while tests -n "..."
    do
        ...

Show 5 quoted lines

> +		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

Style.

Show 8 quoted lines

> +				cat .crlf-${file}-\"\${ref}\".txt >>expect &&
> +				printf \"\n\" >>expect
> +			done &&
> +			git $command_and_args --format=\"%${atom}\" >actual &&
> +			test_cmp expect actual
> +		"
> +	done
> +}