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

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Eric,

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

I did not try to do that because I did not think of it. 
However, I think it's clearer using printf, this way '\n' and '\r'
appear clearly on all platforms, whatever editor is in use
and whatever settings this editor is using to hide or not hide
control characters.

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

Yes, that was the reason. Given Junio's comments I'll surely
refactor his library into a script, so there won't be a need for
this indirection.

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

Thanks for the review! (and I'll re-read the shell coding guidelines
before sending v4).

Cheers,
Philippe.