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

> Le 12 oct. 2020 à 18:47, Eric Sunshine <sunshine@sunshineco.com> a écrit :
> 
> 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...
> 
>> 
>> 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.
> 
>> +	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
>        ...
>    }

> 
>> +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(?).
> 
>> +		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
>        ...
> 
>> +		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.
> 
>> +				cat .crlf-${file}-\"\${ref}\".txt >>expect &&
>> +				printf \"\n\" >>expect
>> +			done &&
>> +			git $command_and_args --format=\"%${atom}\" >actual &&
>> +			test_cmp expect actual
>> +		"
>> +	done
>> +}