On Thu, Apr 28, 2016 at 4:00 PM, Rafal Klys <rafalklys@xxxxx> 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@xxxxx>
> ---
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html