Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html