[v8,3/6] t3321: add test cases about the notes stripspace behavior

Commit Message

Teng Long April 25, 2023, 1:34 p.m. UTC

From: Teng Long <[e-mail]>

Signed-off-by: Teng Long <[e-mail]>
---
 t/t3321-notes-stripspace.sh | 291 ++++++++++++++++++++++++++++++++++++
 1 file changed, 291 insertions(+)
 create mode 100755 t/t3321-notes-stripspace.sh

Comments

Junio C Hamano April 25, 2023, 4:25 p.m. UTC | #1

Teng Long <[e-mail]> writes:

> From: Teng Long <[e-mail]>
>
> Signed-off-by: Teng Long <[e-mail]>
> ---
>  t/t3321-notes-stripspace.sh | 291 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 291 insertions(+)
>  create mode 100755 t/t3321-notes-stripspace.sh

Looks quite thorough.

> diff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh
> new file mode 100755
> index 00000000..7c26b184
> --- /dev/null
> +++ b/t/t3321-notes-stripspace.sh
> @@ -0,0 +1,291 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2007 Teng Long

It's 2023 now.

> +test_expect_success 'add note by editor' '
> +	test_when_finished "git notes remove" &&
> +	cat >expect <<-EOF &&
> +		first-line
> +
> +		second-line
> +	EOF
> ...
> +test_expect_success 'append note by specifying multiple "-m"' '
> +	test_when_finished "git notes remove" &&
> +	cat >expect <<-EOF &&
> +	first-line
> +
> +	second-line
> +	EOF

Inconsistent indentation confuses readers if the author meant
something unexplained by the difference between the two.  Stick to
one style (I personally prefer the "indent to the same level as the
starting 'cat' and ending 'EOF'" but it is OK to pick the other one,
as long as it is consistent within a single test script).

> +	cat >note-file <<-EOF &&
> +		${LF}
> +		first-line
> +		${MULTI_LF}
> +		second-line
> +		${LF}
> +	EOF

This is a bit misleading, as there are TWO blank lines before the
"first-line" (one from the here text itself, the other is from
${LF}).  

I do not think it matters too much, because the point of stripspace
is to remove any number of leading or trailing blank lines, and
squash one or more blank lines elsewhere into one, so having two
blank lines at the beginning or at the end of the file is just as
good an example as having a single blank line.  I am mentioning it
primarily because I had to spend some time thinking about ways to
make it less misleading.

Teng Long April 27, 2023, 3:47 a.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

>> ---
>>  t/t3321-notes-stripspace.sh | 291 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 291 insertions(+)
>>  create mode 100755 t/t3321-notes-stripspace.sh
>
>Looks quite thorough.

I'm not sure if I've covered everything, but I've tried to addi
some basic test cases to give us a chance to add on top of that later.

>> diff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh
>> new file mode 100755
>> index 00000000..7c26b184
>> --- /dev/null
>> +++ b/t/t3321-notes-stripspace.sh
>> @@ -0,0 +1,291 @@
>> +#!/bin/sh
>> +#
>> +# Copyright (c) 2007 Teng Long
>
>It's 2023 now.

Will fix.

>> +test_expect_success 'add note by editor' '
>> +	test_when_finished "git notes remove" &&
>> +	cat >expect <<-EOF &&
>> +		first-line
>> +
>> +		second-line
>> +	EOF
>> ...
>> +test_expect_success 'append note by specifying multiple "-m"' '
>> +	test_when_finished "git notes remove" &&
>> +	cat >expect <<-EOF &&
>> +	first-line
>> +
>> +	second-line
>> +	EOF
>
>Inconsistent indentation confuses readers if the author meant
>something unexplained by the difference between the two.  Stick to
>one style (I personally prefer the "indent to the same level as the
>starting 'cat' and ending 'EOF'" but it is OK to pick the other one,
>as long as it is consistent within a single test script).

I will follow as your prefer format, thanks.

>> +	cat >note-file <<-EOF &&
>> +		${LF}
>> +		first-line
>> +		${MULTI_LF}
>> +		second-line
>> +		${LF}
>> +	EOF
>
>This is a bit misleading, as there are TWO blank lines before the
>"first-line" (one from the here text itself, the other is from
>${LF}).  
>
>I do not think it matters too much, because the point of stripspace
>is to remove any number of leading or trailing blank lines, and
>squash one or more blank lines elsewhere into one, so having two
>blank lines at the beginning or at the end of the file is just as
>good an example as having a single blank line.  I am mentioning it
>primarily because I had to spend some time thinking about ways to
>make it less misleading.

Yes, a little bit, I don't found a more clealy so far. It works for
now but I'm willing to optimize if there's a better way.

Thanks.

Patch

diff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh
new file mode 100755
index 00000000..7c26b184
--- /dev/null
+++ b/t/t3321-notes-stripspace.sh
@@ -0,0 +1,291 @@ 
+#!/bin/sh
+#
+# Copyright (c) 2007 Teng Long
+#
+
+test_description='Test commit notes with stripspace behavior'
+
+. ./test-lib.sh
+
+MULTI_LF="$LF$LF$LF"
+write_script fake_editor <<\EOF
+echo "$MSG" >"$1"
+echo "$MSG" >&2
+EOF
+GIT_EDITOR=./fake_editor
+export GIT_EDITOR
+
+test_expect_success 'setup the commit' '
+	test_commit 1st
+'
+
+test_expect_success 'add note by editor' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+		first-line
+
+		second-line
+	EOF
+
+	MSG="${LF}first-line${MULTI_LF}second-line${LF}" git notes add  &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'add note by specifying single "-m"' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+		first-line
+
+		second-line
+	EOF
+
+	git notes add -m "${LF}first-line${MULTI_LF}second-line${LF}" &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'add note by specifying multiple "-m"' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+	first-line
+
+	second-line
+	EOF
+
+	git notes add -m "${LF}" \
+		      -m "first-line" \
+		      -m "${MULTI_LF}" \
+		      -m "second-line" \
+		      -m "${LF}" &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+
+test_expect_success 'append note by editor' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+		first-line
+
+		second-line
+	EOF
+
+	git notes add -m "first-line" &&
+	MSG="${MULTI_LF}second-line${LF}" git notes append  &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'append note by specifying single "-m"' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+		first-line
+
+		second-line
+	EOF
+
+	git notes add -m "${LF}first-line" &&
+	git notes append -m "${MULTI_LF}second-line${LF}" &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'append note by specifying multiple "-m"' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+	first-line
+
+	second-line
+	EOF
+
+	git notes add -m "${LF}first-line" &&
+	git notes append -m "${MULTI_LF}" \
+		      -m "second-line" \
+		      -m "${LF}" &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'add note by specifying single "-F"' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+		first-line
+
+		second-line
+	EOF
+
+	cat >note-file <<-EOF &&
+		${LF}
+		first-line
+		${MULTI_LF}
+		second-line
+		${LF}
+	EOF
+
+	git notes add -F note-file &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'add notes by specifying multiple "-F"' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+		file-1-first-line
+
+		file-1-second-line
+
+		file-2-first-line
+
+		file-2-second-line
+	EOF
+
+	cat >note-file-1 <<-EOF &&
+		${LF}
+		file-1-first-line
+		${MULTI_LF}
+		file-1-second-line
+		${LF}
+	EOF
+
+	cat >note-file-2 <<-EOF &&
+		${LF}
+		file-2-first-line
+		${MULTI_LF}
+		file-2-second-line
+		${LF}
+	EOF
+
+	git notes add -F note-file-1 -F note-file-2 &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'append note by specifying single "-F"' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+		initial-line
+
+		first-line
+
+		second-line
+	EOF
+
+	cat >note-file <<-EOF &&
+		${LF}
+		first-line
+		${MULTI_LF}
+		second-line
+		${LF}
+	EOF
+
+	git notes add -m "initial-line" &&
+	git notes append -F note-file &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'append notes by specifying multiple "-F"' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+		initial-line
+
+		file-1-first-line
+
+		file-1-second-line
+
+		file-2-first-line
+
+		file-2-second-line
+	EOF
+
+	cat >note-file-1 <<-EOF &&
+		${LF}
+		file-1-first-line
+		${MULTI_LF}
+		file-1-second-line
+		${LF}
+	EOF
+
+	cat >note-file-2 <<-EOF &&
+		${LF}
+		file-2-first-line
+		${MULTI_LF}
+		file-2-second-line
+		${LF}
+	EOF
+
+	git notes add -m "initial-line" &&
+	git notes append -F note-file-1 -F note-file-2 &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'add notes with empty messages' '
+	rev=$(git rev-parse HEAD) &&
+	git notes add -m "${LF}" \
+		      -m "${MULTI_LF}" \
+		      -m "${LF}" >actual 2>&1 &&
+	test_i18ngrep "Removing note for object" actual
+'
+
+test_expect_success 'add note by specifying "-C" , do not stripspace is the default behavior' '
+	test_when_finished "git notes remove" &&
+	cat >expect <<-EOF &&
+		${LF}
+		first-line
+		${MULTI_LF}
+		second-line
+		${LF}
+	EOF
+
+	cat expect | git hash-object -w --stdin >blob &&
+	git notes add -C $(cat blob) &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'add notes with "-C" and "-m", "-m" will stripspace all together' '
+	test_when_finished "git notes remove" &&
+	cat >data <<-EOF &&
+		${LF}
+		first-line
+		${MULTI_LF}
+		second-line
+		${LF}
+	EOF
+
+	cat >expect <<-EOF &&
+		first-line
+
+		second-line
+
+		third-line
+	EOF
+
+	cat data | git hash-object -w --stdin >blob &&
+	git notes add -C $(cat blob) -m "third-line" &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'add notes with "-m" and "-C", "-C" will not stripspace all together' '
+	test_when_finished "git notes remove" &&
+	cat >data <<-EOF &&
+
+		second-line
+	EOF
+
+	cat >expect <<-EOF &&
+		first-line
+		${LF}
+		second-line
+	EOF
+
+	cat data | git hash-object -w --stdin >blob &&
+	git notes add -m "first-line" -C $(cat blob)  &&
+	git notes show >actual &&
+	test_cmp expect actual
+'
+
+test_done