{"thread":{"id":"42173","subject":"[PATCH v2] trailer: load config to handle core.commentChar","startedAt":"2016-04-28T20:00:58Z","lastAt":"2016-05-02T18:13:39Z","messageCount":4,"participants":["Rafal Klys","Christian Couder","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"284869","messageId":"1461873658-11394-1-git-send-email-rafalklys@wp.pl","threadId":"42173","inReplyTo":null,"subject":"[PATCH v2] trailer: load config to handle core.commentChar","fromName":"Rafal Klys","fromEmail":"rafalklys@wp.pl","sentAt":"2016-04-28T20:00:58Z","receivedAt":"2016-04-28T20:00:58Z","isPatch":true,"sender":{"key":"rafalklys@wp.pl","avatar":null},"body":"Fall throught git_default_config when reading config to update the\ncomment_line_char from default '#' to possible different value set in\ncore.commentChar.\n\nSigned-off-by: Rafal Klys \n---\n\nAdded fallthru instead of reading config third time.\n\nAdded test, updated commit message.\n\nI even tried to change that to only one pass, but looks like it would require a\nbit more coding, so maybe next time.\n\nThanks for feedback, I'm impressed that contributing to Git is so easy for\nnewbies (never sent a patch via email before!) and that the response is so\nquick.\n\n t/t7513-interpret-trailers.sh | 30 ++++++++++++++++++++++++++++++\n trailer.c | 3 ++-\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex aee785c..e6f9d8e 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -267,6 +267,36 @@ test_expect_success 'with message that has comments' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with message that has comments using non-default core.commentChar' '\n+\tgit config core.commentChar x &&\n+\ttest_when_finished \"git config --unset core.commentChar\" &&\n+\tcat basic_message >message_with_comments &&\n+\tsed -e \"s/ Z\\$/ /\" >>message_with_comments <<-\\EOF &&\n+\t\tx comment\n+\n+\t\tx other comment\n+\t\tCc: Z\n+\t\tx yet another comment\n+\t\tReviewed-by: Johan\n+\t\tReviewed-by: Z\n+\t\tx last comment\n+\n+\tEOF\n+\tcat basic_patch >>message_with_comments &&\n+\tcat basic_message >expected &&\n+\tcat >>expected <<-\\EOF &&\n+\t\tx comment\n+\n+\t\tReviewed-by: Johan\n+\t\tCc: Peff\n+\t\tx last comment\n+\n+\tEOF\n+\tcat basic_patch >>expected &&\n+\tgit interpret-trailers --trim-empty --trailer \"Cc: Peff\" message_with_comments >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with message that has an old style conflict block' '\n \tcat basic_message >message_with_comments &&\n \tsed -e \"s/ Z\\$/ /\" >>message_with_comments <<-\\EOF &&\ndiff --git a/trailer.c b/trailer.c\nindex 8e48a5c..2ec0883 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -483,7 +483,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v\n \tconst char *trailer_item, *variable_name;\n \n \tif (!skip_prefix(conf_key, \"trailer.\", &trailer_item))\n-\t\treturn 0;\n+\t\t/* for core.commentChar */\n+\t\treturn git_default_config(conf_key, value, cb);\n \n \tvariable_name = strrchr(trailer_item, '.');\n \tif (!variable_name) {\n-- \n2.8.1.69.g6de72cc\n"},{"id":"284875","messageId":"CAP8UFD20p7D3J05xKpYDviXxV8EZvLFhESMDhrmCPaT=bsRcDA@mail.gmail.com","threadId":"42173","inReplyTo":"1461873658-11394-1-git-send-email-rafalklys@wp.pl","subject":"Re: [PATCH v2] trailer: load config to handle core.commentChar","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-04-28T21:06:36Z","receivedAt":"2016-04-28T21:06:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Apr 28, 2016 at 10:00 PM, Rafal Klys wrote:\n> Fall throught git_default_config when reading config to update the\n> comment_line_char from default '#' to possible different value set in\n> core.commentChar.\n>\n> Signed-off-by: Rafal Klys \n> ---\n>\n> Added fallthru instead of reading config third time.\n>\n> Added test, updated commit message.\n>\n> I even tried to change that to only one pass, but looks like it would require a\n> bit more coding, so maybe next time.\n>\n> Thanks for feedback, I'm impressed that contributing to Git is so easy for\n> newbies (never sent a patch via email before!) and that the response is so\n> quick.\n\nThanks for contributing great patches and for the kind words!\nI am also happy that you find it easy for newbies to contribute.\n"},{"id":"284877","messageId":"CAPig+cT=Ca+Jd_SN_SM=iOmLnYtAt82dW7ammW4-AKdRReFfPA@mail.gmail.com","threadId":"42173","inReplyTo":"1461873658-11394-1-git-send-email-rafalklys@wp.pl","subject":"Re: [PATCH v2] trailer: load config to handle core.commentChar","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-28T21:12:16Z","receivedAt":"2016-04-28T21:12:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 28, 2016 at 4:00 PM, Rafal Klys wrote:\n> trailer: load config to handle core.commentChar\n\nThis subject is describing low-level details of the patch rather than\ngiving a high-level overview. A possible rewrite might be:\n\n trailer: respect core.commentChar\n\n> Fall throught git_default_config when reading config to update the\n> comment_line_char from default '#' to possible different value set in\n> core.commentChar.\n\nSimilarly, this text is pretty much repeating what the patch itself\nalready states more concisely. Instead, you'd probably want to say\nhere what problem the patch is solving, and give an explanation of the\nfix. For example:\n\n git-trailer fails to respect core.commentChar. Fix this oversight\n by invoking git_default_config() which loads core.commentChar.\n\nIn fact, this is such a simple fix that the subject suggested above\nmay itself be a sufficient commit message; any extra text might just\nbe noise since the patch itself contains enough information to\nunderstand the problem and the fix.\n\nBy the way, the above minor comments are probably not worth a re-roll.\nSee below for a few more...\n\n> Signed-off-by: Rafal Klys \n> ---\n> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\n> +test_expect_success 'with message that has comments using non-default core.commentChar' '\n> + git config core.commentChar x &&\n> + test_when_finished \"git config --unset core.commentChar\" &&\n\nThe above two lines could be collapsed to:\n\n test_config core.commentChar x &&\n\n> + cat basic_message >message_with_comments &&\n> + sed -e \"s/ Z\\$/ /\" >>message_with_comments <<-\\EOF &&\n> + x comment\n> +\n> + x other comment\n> + Cc: Z\n> + x yet another comment\n> + Reviewed-by: Johan\n> + Reviewed-by: Z\n> + x last comment\n> +\n> + EOF\n> + cat basic_patch >>message_with_comments &&\n> + cat basic_message >expected &&\n> + cat >>expected <<-\\EOF &&\n> + x comment\n> +\n> + Reviewed-by: Johan\n> + Cc: Peff\n> + x last comment\n> +\n> + EOF\n> + cat basic_patch >>expected &&\n> + git interpret-trailers --trim-empty --trailer \"Cc: Peff\" message_with_comments >actual &&\n> + test_cmp expected actual\n> +'\n\nAs this new test is effectively a copy of the preceding test, another\noption would be to factor out the common code. For instance:\n\n test_comment () {\n cat basic_message >message_with_comments &&\n sed -e \"s/ Z\\$/ /\" >>message_with_comments <<-EOF &&\n $1 comment\n ...\n }\n\n test_expect_success 'with message that has comments' '\n test_comment '#'\n '\n\n test_expect_success 'with message that has custom comment char' '\n test_config core.commentChar x &&\n test_comment x\n '\n\nNote that the backslash is dropped from -\\EOF so that $1 can be\ninterpolated into the here-doc.\n\nSuch a re-factoring would be done as a preparatory patch, thus making\nthis a two-patch series, however, it's probably not worth it for only\ntwo tests sharing common code. (Although, the following test is also\nnearly identical...)\n\n> diff --git a/trailer.c b/trailer.c\n> @@ -483,7 +483,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v\n> const char *trailer_item, *variable_name;\n>\n> if (!skip_prefix(conf_key, \"trailer.\", &trailer_item))\n> - return 0;\n> + /* for core.commentChar */\n> + return git_default_config(conf_key, value, cb);\n\nI'm a bit torn about this comment. On the one hand, it does add a bit\nof value since it's not obvious at a glance what config from the\ndefault set is needed by git-trailer, however, if git-trailer someday\ntakes advantage of some additional config from the default set, then\nthis comment will likely become outdated.\n\nIf I was authoring the patch, I'd probably omit the comment. Someone\nwanting to know the reason for this invocation can always consult the\ncommit message (which does explain its purpose).\n\n> variable_name = strrchr(trailer_item, '.');\n> if (!variable_name) {\n> --\n"},{"id":"285179","messageId":"xmqqy47stjjg.fsf@gitster.mtv.corp.google.com","threadId":"42173","inReplyTo":"CAPig+cT=Ca+Jd_SN_SM=iOmLnYtAt82dW7ammW4-AKdRReFfPA@mail.gmail.com","subject":"Re: [PATCH v2] trailer: load config to handle core.commentChar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-02T18:13:39Z","receivedAt":"2016-05-02T18:13:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> In fact, this is such a simple fix that the subject suggested above\n> may itself be a sufficient commit message; any extra text might just\n> be noise since the patch itself contains enough information to\n> understand the problem and the fix.\n\nSounds about right.\n\n>> + git config core.commentChar x &&\n>> + test_when_finished \"git config --unset core.commentChar\" &&\n>\n> The above two lines could be collapsed to:\n>\n> test_config core.commentChar x &&\n\nYes.\n\n> As this new test is effectively a copy of the preceding test, another\n> option would be to factor out the common code. For instance:\n>\n> test_comment () {\n> cat basic_message >message_with_comments &&\n> sed -e \"s/ Z\\$/ /\" >>message_with_comments <<-EOF &&\n> $1 comment\n> ...\n> }\n>\n> test_expect_success 'with message that has comments' '\n> test_comment '#'\n> '\n>\n> test_expect_success 'with message that has custom comment char' '\n> test_config core.commentChar x &&\n> test_comment x\n> '\n>\n> Note that the backslash is dropped from -\\EOF so that $1 can be\n> interpolated into the here-doc.\n>\n> Such a re-factoring would be done as a preparatory patch, thus making\n> this a two-patch series, however, it's probably not worth it for only\n> two tests sharing common code. (Although, the following test is also\n> nearly identical...)\n\nThis certainly does make the result easier to read through.\n\n>> diff --git a/trailer.c b/trailer.c\n>> @@ -483,7 +483,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v\n>> const char *trailer_item, *variable_name;\n>>\n>> if (!skip_prefix(conf_key, \"trailer.\", &trailer_item))\n>> - return 0;\n>> + /* for core.commentChar */\n>> + return git_default_config(conf_key, value, cb);\n>\n> I'm a bit torn about this comment. On the one hand, it does add a bit\n> of value since it's not obvious at a glance what config from the\n> default set is needed by git-trailer, however, if git-trailer someday\n> takes advantage of some additional config from the default set, then\n> this comment will likely become outdated.\n\nThis is a very good point.\n\n\"I wanted the call here for core.commentChar\" is better said in the\nlog message, as that will not stay true forever, as other people\nwill start depending on being able to access other variables and\ngenerally they would not go back to read this message (to update\nit).\n"}]}