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

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Christian Couder-2
On Thu, Apr 28, 2016 at 10:00 PM, Rafal Klys <[hidden email]> 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 <[hidden email]>
> ---
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Eric Sunshine
In reply to this post by Rafal Klys
On Thu, Apr 28, 2016 at 4:00 PM, Rafal Klys <[hidden email]> 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 <[hidden email]>
> ---
> 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) {
> --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Eric Sunshine <[hidden email]> 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).


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html