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.
> # Arguments: [<prefix] [<commit message>] [<commit options>]
> check_summary_oneline() {
> test_tick &&
--
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
