Re: [PATCH 1/2] t750*: make tests for commit messages more pedantic

On Tue, May 26, 2015 at 2:15 AM, Patryk Obara [off-list ref] 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 <redacted>

More below...

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"

Sneaking in unnecessary whitespace change.

 # Arguments: [<prefix] [<commit message>] [<commit options>]
 check_summary_oneline() {
        test_tick &&