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

> 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" &&

> +       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
> +'

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

>> +       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 &&

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