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

From: Junio C Hamano <gitster@pobox.com>
Date: May 2, 2016, 18:13 UTC
Message-ID: <xmqqy47stjjg.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPig+cT=Ca+Jd_SN_SM=iOmLnYtAt82dW7ammW4-AKdRReFfPA@mail.gmail.com>

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.

Show 6 quoted lines

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

Show 26 quoted lines

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

Show 14 quoted lines

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