[PATCH v2] trailer: load config to handle core.commentChar

Fall throught git_default_config when reading config to update the
comment_line_char from default '#' to possible different value set in
core.commentChar.

Signed-off-by: Rafal Klys <redacted>
---

Added fallthru instead of reading config third time.

Added test, updated commit message.

I even tried to change that to only one pass, but looks like it would require a
bit more coding, so maybe next time.

Thanks for feedback, I'm impressed that contributing to Git is so easy for
newbies (never sent a patch via email before!) and that the response is so
quick.

 t/t7513-interpret-trailers.sh | 30 ++++++++++++++++++++++++++++++
 trailer.c                     |  3 ++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index aee785c..e6f9d8e 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh

@@ -267,6 +267,36 @@ test_expect_success 'with message that has comments' '
 	test_cmp expected actual
 '
 
+test_expect_success 'with message that has comments using non-default core.commentChar' '
+	git config core.commentChar x &&
+	test_when_finished "git config --unset core.commentChar" &&
+	cat basic_message >message_with_comments &&
+	sed -e "s/ Z\$/ /" >>message_with_comments <<-\EOF &&
+		x comment
+
+		x other comment
+		Cc: Z
+		x yet another comment
+		Reviewed-by: Johan
+		Reviewed-by: Z
+		x last comment
+
+	EOF
+	cat basic_patch >>message_with_comments &&
+	cat basic_message >expected &&
+	cat >>expected <<-\EOF &&
+		x comment
+
+		Reviewed-by: Johan
+		Cc: Peff
+		x last comment
+
+	EOF
+	cat basic_patch >>expected &&
+	git interpret-trailers --trim-empty --trailer "Cc: Peff" message_with_comments >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'with message that has an old style conflict block' '
 	cat basic_message >message_with_comments &&
 	sed -e "s/ Z\$/ /" >>message_with_comments <<-\EOF &&

diff --git a/trailer.c b/trailer.c
index 8e48a5c..2ec0883 100644
--- a/trailer.c
+++ b/trailer.c

@@ -483,7 +483,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v
 	const char *trailer_item, *variable_name;
 
 	if (!skip_prefix(conf_key, "trailer.", &trailer_item))
-		return 0;
+		/* for core.commentChar */
+		return git_default_config(conf_key, value, cb);
 
 	variable_name = strrchr(trailer_item, '.');
 	if (!variable_name) {

-- 
2.8.1.69.g6de72cc

On Thu, Apr 28, 2016 at 10:00 PM, Rafal Klys [off-list ref] wrote:

Fall throught git_default_config when reading config to update the
comment_line_char from default '#' to possible different value set in
core.commentChar.

Signed-off-by: Rafal Klys <redacted>
---

Added fallthru instead of reading config third time.

Added test, updated commit message.

I even tried to change that to only one pass, but looks like it would require a
bit more coding, so maybe next time.

Thanks for feedback, I'm impressed that contributing to Git is so easy for
newbies (never sent a patch via email before!) and that the response is so
quick.

Thanks for contributing great patches and for the kind words!
I am also happy that you find it easy for newbies to contribute.

On Thu, Apr 28, 2016 at 4:00 PM, Rafal Klys [off-list ref] wrote:

trailer: load config to handle core.commentChar

This subject is describing low-level details of the patch rather than
giving a high-level overview. A possible rewrite might be:

    trailer: respect core.commentChar

Fall throught git_default_config when reading config to update the
comment_line_char from default '#' to possible different value set in
core.commentChar.

Similarly, this text is pretty much repeating what the patch itself
already states more concisely. Instead, you'd probably want to say
here what problem the patch is solving, and give an explanation of the
fix. For example:

    git-trailer fails to respect core.commentChar. Fix this oversight
    by invoking git_default_config() which loads core.commentChar.

In fact, this is such a simple fix that the subject suggested above
may itself be a sufficient commit message; any extra text might just
be noise since the patch itself contains enough information to
understand the problem and the fix.

By the way, the above minor comments are probably not worth a re-roll.
See below for a few more...

The above two lines could be collapsed to:

    test_config core.commentChar x &&

+       cat basic_message >message_with_comments &&
+       sed -e "s/ Z\$/ /" >>message_with_comments <<-\EOF &&
+               x comment
+
+               x other comment
+               Cc: Z
+               x yet another comment
+               Reviewed-by: Johan
+               Reviewed-by: Z
+               x last comment
+
+       EOF
+       cat basic_patch >>message_with_comments &&
+       cat basic_message >expected &&
+       cat >>expected <<-\EOF &&
+               x comment
+
+               Reviewed-by: Johan
+               Cc: Peff
+               x last comment
+
+       EOF
+       cat basic_patch >>expected &&
+       git interpret-trailers --trim-empty --trailer "Cc: Peff" message_with_comments >actual &&
+       test_cmp expected actual
+'

As this new test is effectively a copy of the preceding test, another
option would be to factor out the common code. For instance:

    test_comment () {
        cat basic_message >message_with_comments &&
        sed -e "s/ Z\$/ /" >>message_with_comments <<-EOF &&
            $1 comment
        ...
    }

    test_expect_success 'with message that has comments' '
        test_comment '#'
    '

    test_expect_success 'with message that has custom comment char' '
        test_config core.commentChar x &&
        test_comment x
    '

Note that the backslash is dropped from -\EOF so that $1 can be
interpolated into the here-doc.

Such a re-factoring would be done as a preparatory patch, thus making
this a two-patch series, however, it's probably not worth it for only
two tests sharing common code. (Although, the following test is also
nearly identical...)

I'm a bit torn about this comment. On the one hand, it does add a bit
of value since it's not obvious at a glance what config from the
default set is needed by git-trailer, however, if git-trailer someday
takes advantage of some additional config from the default set, then
this comment will likely become outdated.

If I was authoring the patch, I'd probably omit the comment. Someone
wanting to know the reason for this invocation can always consult the
commit message (which does explain its purpose).

        variable_name = strrchr(trailer_item, '.');
        if (!variable_name) {
--