Re: [PATCH 2/4] t7502: demonstrate breakage with a commit message with trailing newlines

2013-02-18 Thread Jonathan Nieder
Brandon Casey wrote:

> This test attempts to verify that a commit message supplied to 'git
> commit' via the -m switch was used in full as the commit message for a
> commit when --cleanup=verbatim was used.
[...]
> The test was able to complete successfully since internally, git appends
> two newlines to each string supplied via the -m switch.
[...]
> Mark this test as failing, since it is not handled correctly by git.
> As described above, git appends two extra newlines to every string
> supplied via -m.

Good catch.  This is an old one, triggered by a combination of

 v1.5.4-rc0~78^2~23 builtin-commit: resurrect behavior for multiple -m
options, 2007-11-11

and

 v1.5.4-rc2~3^2 Allow selection of different cleanup modes for commit
messages, 2007-12-22

The patch makes sense and makes the test easier to read, so
Reviewed-by: Jonathan Nieder 

(Patch left unsnipped for reference.)

> Signed-off-by: Brandon Casey 
> ---
>  t/t7502-commit.sh | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 9040f8a..39e55f8 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -177,10 +177,18 @@ test_expect_success 'verbose respects diff config' '
>   git config --unset color.diff
>  '
>  
> +mesg_with_comment_and_newlines='
> +# text
> +
> +'
> +
> +test_expect_success 'prepare file with comment line and trailing newlines'  '
> + printf "%s" "$mesg_with_comment_and_newlines" >expect
> +'
> +
>  test_expect_success 'cleanup commit messages (verbatim option,-t)' '
>  
>   echo >>negative &&
> - { echo;echo "# text";echo; } >expect &&
>   git commit --cleanup=verbatim --no-status -t expect -a &&
>   git cat-file -p HEAD |sed -e "1,/^\$/d" >actual &&
>   test_cmp expect actual
> @@ -196,10 +204,10 @@ test_expect_success 'cleanup commit messages (verbatim 
> option,-F)' '
>  
>  '
>  
> -test_expect_success 'cleanup commit messages (verbatim option,-m)' '
> +test_expect_failure 'cleanup commit messages (verbatim option,-m)' '
>  
>   echo >>negative &&
> - git commit --cleanup=verbatim -m "$(cat expect)" -a &&
> + git commit --cleanup=verbatim -m "$mesg_with_comment_and_newlines" -a &&
>   git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
>   test_cmp expect actual
>  
> -- 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/4] t7502: demonstrate breakage with a commit message with trailing newlines

2013-02-18 Thread Brandon Casey
This test attempts to verify that a commit message supplied to 'git
commit' via the -m switch was used in full as the commit message for a
commit when --cleanup=verbatim was used.

But, this test has been broken since it was introduced.  Since the
commit message containing trailing newlines was supplied to 'git commit'
using a command substitution, the trailing newlines were removed by the
shell.  This means that a string without any trailing newlines was
actually supplied to 'git commit'.

The test was able to complete successfully since internally, git appends
two newlines to each string supplied via the -m switch.  So, the two
newlines removed by the shell were then re-added by git, and the
resulting commit matched what was expected.

So, let's move the initial creation of the commit message string out
from within a previous test so that it stands alone.  Assign the desired
commit message to a variable using literal newlines.  Then populate the
expect file from the contents of the commit message variable.  This way
the shell variable becomes the authoritative source of the commit
message and can be supplied via the -m switch with the trailing newlines
intact.

Mark this test as failing, since it is not handled correctly by git.
As described above, git appends two extra newlines to every string
supplied via -m.

Signed-off-by: Brandon Casey 
---
 t/t7502-commit.sh | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 9040f8a..39e55f8 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -177,10 +177,18 @@ test_expect_success 'verbose respects diff config' '
git config --unset color.diff
 '
 
+mesg_with_comment_and_newlines='
+# text
+
+'
+
+test_expect_success 'prepare file with comment line and trailing newlines'  '
+   printf "%s" "$mesg_with_comment_and_newlines" >expect
+'
+
 test_expect_success 'cleanup commit messages (verbatim option,-t)' '
 
echo >>negative &&
-   { echo;echo "# text";echo; } >expect &&
git commit --cleanup=verbatim --no-status -t expect -a &&
git cat-file -p HEAD |sed -e "1,/^\$/d" >actual &&
test_cmp expect actual
@@ -196,10 +204,10 @@ test_expect_success 'cleanup commit messages (verbatim 
option,-F)' '
 
 '
 
-test_expect_success 'cleanup commit messages (verbatim option,-m)' '
+test_expect_failure 'cleanup commit messages (verbatim option,-m)' '
 
echo >>negative &&
-   git commit --cleanup=verbatim -m "$(cat expect)" -a &&
+   git commit --cleanup=verbatim -m "$mesg_with_comment_and_newlines" -a &&
git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
test_cmp expect actual
 
-- 
1.8.1.3.638.g372f416.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html