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

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

[Rafal Klys]

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 <rafalklys@wp.pl>
---

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

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

[Christian Couder]

On Thu, Apr 28, 2016 at 10:00 PM, Rafal Klys <rafalklys@wp.pl> 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 <rafalklys@wp.pl>
> ---
>
> 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.

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

[Eric Sunshine]

On Thu, Apr 28, 2016 at 4:00 PM, Rafal Klys <rafalklys@wp.pl> 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...

> Signed-off-by: Rafal Klys <rafalklys@wp.pl>
> ---
> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> +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" &&

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...)

> diff --git 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);

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) {
> --

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> 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.

Sounds about right.

>> +       git config core.commentChar x &&
>> +       test_when_finished "git config --unset core.commentChar" &&
>
> The above two lines could be collapsed to:
>
>     test_config core.commentChar x &&

Yes.

> 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...)

This certainly does make the result easier to read through.

>> diff --git 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);
>
> 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.

This is a very good point.

"I wanted the call here for core.commentChar" is better said in the
log message, as that will not stay true forever, as other people
will start depending on being able to access other variables and
generally they would not go back to read this message (to update
it).