Re: [PATCH 1/2] t750*: make tests for commit messages more pedantic
Eric Sunshine sunsh...@sunshineco.com writes: On Tue, May 26, 2015 at 2:15 AM, Patryk Obara patryk.ob...@gmail.com wrote: Currently messages are compared with --pretty=format:%s%b which does not retain raw format of commit message. In result it's not clear what part of expected commit msg is subject and what part is body. Also, it's impossible to test if messages with multiple lines are handled correctly, which may be significant when using nondefault --cleanup. Makes sense. ... +test_expect_success 'template without newline before eof should work with --status' ' It's not clear what should work means. I suppose you mean that the end result should have exactly one newline after the template. Perhaps the test title could indicate the intent more clearly. I agree that what should work in this title is unclear. Because there is nothing wrong in the current system, if a follow-up patch plans to change the established behaviour, the tests in this currently we do not test blank lines, so add tests for them patch should limit themselves to document the current behaviour. Then a follow-up patch that modifies the behaviour can show how the updated behaviour is different and illustrate in what way it is better than the current behaviour. That would be one way to justify the change. -- 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
Re: [PATCH 1/2] t750*: make tests for commit messages more pedantic
On Tue, May 26, 2015 at 2:15 AM, Patryk Obara patryk.ob...@gmail.com wrote:
Currently messages are compared with --pretty=format:%s%b which does
not retain raw format of commit message. In result it's not clear what
part of expected commit msg is subject and what part is body. Also, it's
impossible to test if messages with multiple lines are handled
correctly, which may be significant when using nondefault --cleanup.
Makes sense.
Change commit_msg_is function to use raw message format in log and
interpret escaped sequences in expected message. This way it's possible
to test exactly what commit message text was saved.
These changes would be less daunting to review if split into multiple
patches; one per logical change. So, for instance, patch 1 would make
this change and adjust tests accordingly.
Add test to verify, that no additional content is appended to template
message, which uncovers tiny bug in --status handling - new line is
always appended before status message. If template file ended with
newline (which is default for many popular text editors, e.g. vim)
then blank line appears before status (which is very annoying when
template ends with line starting with '#'). On the other hand, this
newline needs to be appended if template file didn't end with newline
(which is default for e.g. emacs) - otherwise first line of status
message may be not cleaned up.
This could be patch 2.
Add explicit test to verify if \n is kept unexpanded in commit message -
this used to be part of unrelated template test.
And patch 3, and so on...
Modify add-content-and-comment fake editor to include both comments and
whitespace, so --cleanup=whitespace is now actually tested.
Modify expected value of test cleanup commit messages (t7502), which
shouldn't be passing, because template and logfiles are unnecessarily
stripped before placing them into editor.
Your cover letter correctly states that with this patch is applied, a
number of tests fail. Tests which are expected to fail should be
declared test_expect_failure rather than test_expect_success. The
patch which fixes the failures should flip them to
test_expect_success.
Signed-off-by: Patryk Obara patryk.ob...@gmail.com
More below...
---
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 116885a..fd1bf71 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -13,8 +13,8 @@ commit_msg_is () {
expect=commit_msg_is.expect
actual=commit_msg_is.actual
- printf %s $(git log --pretty=format:%s%b -1) $actual
- printf %s $1 $expect
+ git log --pretty=format:%B -1 $actual
+ printf %b $1 $expect
test_i18ncmp $expect $actual
}
@@ -329,4 +329,47 @@ test_expect_success 'invalid message options when using
--fixup' '
test_must_fail git commit --fixup HEAD~1 -F log
'
+test_expect_success 'no blank lines appended after template with --status' '
+ echo template line $TEMPLATE
Style: Modern code omits the space after the redirection operator
($TEMPLATE), however, it's also important to match existing style.
Unfortunately, this file has an equal mixture of both 'blap' and '
blap', so it's difficult to know which style to match. As this is new
code, it'd probably be best to omit the space.
+ echo changes foo
+ git add foo
+ test_set_editor $TEST_DIRECTORY/t7500/add-content
+ git commit -e -t $TEMPLATE --status
+ commit_msg_is template line\ncommit message\n
+'
+
+test_expect_success 'template without newline before eof should work with
--status' '
It's not clear what should work means. I suppose you mean that the
end result should have exactly one newline after the template. Perhaps
the test title could indicate the intent more clearly.
+ printf %s template line $TEMPLATE
+ echo changes foo
+ git add foo
+ test_set_editor $TEST_DIRECTORY/t7500/add-content
+ git commit -e -t $TEMPLATE --status
+ commit_msg_is template line\ncommit message\n
+'
+
+test_expect_success 'logfile without newline before eof should work with
--status' '
Ditto: Unclear should work
+ printf %s log line log-file
+ echo changes foo
+ git add foo
+ test_set_editor $TEST_DIRECTORY/t7500/add-content
+ git commit -e -F log-file --status
+ commit_msg_is log line\ncommit message\n
+'
test_done
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 051489e..d2203ed 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -8,11 +8,12 @@ commit_msg_is () {
expect=commit_msg_is.expect
actual=commit_msg_is.actual
- printf %s $(git log --pretty=format:%s%b -1) $actual
- printf %s $1 $expect
- test_i18ncmp $expect $actual
+ git log --pretty=format:%B -1 $actual
+ printf %b $1 $expect
+ test_i18ncmp $expect $actual
}
+
Sneaking in unnecessary whitespace change.
#
[PATCH 1/2] t750*: make tests for commit messages more pedantic
Currently messages are compared with --pretty=format:%s%b which does
not retain raw format of commit message. In result it's not clear what
part of expected commit msg is subject and what part is body. Also, it's
impossible to test if messages with multiple lines are handled
correctly, which may be significant when using nondefault --cleanup.
Change commit_msg_is function to use raw message format in log and
interpret escaped sequences in expected message. This way it's possible
to test exactly what commit message text was saved.
Add test to verify, that no additional content is appended to template
message, which uncovers tiny bug in --status handling - new line is
always appended before status message. If template file ended with
newline (which is default for many popular text editors, e.g. vim)
then blank line appears before status (which is very annoying when
template ends with line starting with '#'). On the other hand, this
newline needs to be appended if template file didn't end with newline
(which is default for e.g. emacs) - otherwise first line of status
message may be not cleaned up.
Add explicit test to verify if \n is kept unexpanded in commit message -
this used to be part of unrelated template test.
Modify add-content-and-comment fake editor to include both comments and
whitespace, so --cleanup=whitespace is now actually tested.
Modify expected value of test cleanup commit messages (t7502), which
shouldn't be passing, because template and logfiles are unnecessarily
stripped before placing them into editor.
Signed-off-by: Patryk Obara patryk.ob...@gmail.com
---
t/t7500-commit.sh | 91 ++---
t/t7500/add-content-and-comment | 4 ++
t/t7502-commit.sh | 29 +++--
t/t7504-commit-msg-hook.sh | 15 ---
4 files changed, 97 insertions(+), 42 deletions(-)
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 116885a..fd1bf71 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -13,8 +13,8 @@ commit_msg_is () {
expect=commit_msg_is.expect
actual=commit_msg_is.actual
- printf %s $(git log --pretty=format:%s%b -1) $actual
- printf %s $1 $expect
+ git log --pretty=format:%B -1 $actual
+ printf %b $1 $expect
test_i18ncmp $expect $actual
}
@@ -76,7 +76,7 @@ test_expect_success 'adding real content to a template should
commit' '
test_set_editor $TEST_DIRECTORY/t7500/add-content
git commit --template $TEMPLATE
)
- commit_msg_is template linecommit message
+ commit_msg_is template line\ncommit message\n
'
test_expect_success '-t option should be short for --template' '
@@ -87,7 +87,7 @@ test_expect_success '-t option should be short for
--template' '
test_set_editor $TEST_DIRECTORY/t7500/add-content
git commit -t $TEMPLATE
)
- commit_msg_is short templatecommit message
+ commit_msg_is short template\ncommit message\n
'
test_expect_success 'config-specified template should commit' '
@@ -99,7 +99,7 @@ test_expect_success 'config-specified template should commit'
'
test_set_editor $TEST_DIRECTORY/t7500/add-content
git commit
)
- commit_msg_is new templatecommit message
+ commit_msg_is new template\ncommit message\n
'
test_expect_success 'explicit commit message should override template' '
@@ -107,7 +107,7 @@ test_expect_success 'explicit commit message should
override template' '
git add foo
GIT_EDITOR=$TEST_DIRECTORY/t7500/add-content git commit --template
$TEMPLATE \
-m command line msg
- commit_msg_is command line msg
+ commit_msg_is command line msg\n
'
test_expect_success 'commit message from file should override template' '
@@ -118,7 +118,7 @@ test_expect_success 'commit message from file should
override template' '
test_set_editor $TEST_DIRECTORY/t7500/add-content
git commit --template $TEMPLATE --file -
)
- commit_msg_is standard input msg
+ commit_msg_is standard input msg\n
'
cat $TEMPLATE \EOF
@@ -132,7 +132,7 @@ test_expect_success 'commit message from template with
whitespace issue' '
git add foo
GIT_EDITOR=$TEST_DIRECTORY/t7500/add-whitespaced-content git commit \
--template $TEMPLATE
- commit_msg_is commit message
+ commit_msg_is commit message\n
'
test_expect_success 'using alternate GIT_INDEX_FILE (1)' '
@@ -187,7 +187,7 @@ test_expect_success 'commit message from file (1)' '
cd subdir
git commit --allow-empty -F log
)
- commit_msg_is Log in sub directory
+ commit_msg_is Log in sub directory\n
'
test_expect_success 'commit message from file (2)' '
@@ -197,7 +197,7 @@ test_expect_success 'commit message from file (2)' '
cd subdir
