{"thread":{"id":"44340","subject":"[PATCH] rev-list: use hdr_termination instead of a always using a newline","startedAt":"2016-10-20T18:19:38Z","lastAt":"2016-10-20T20:06:08Z","messageCount":4,"participants":["Jacob Keller","Dennis Kaarsemaker","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"304495","messageId":"20161020181930.21084-1-jacob.e.keller@intel.com","threadId":"44340","inReplyTo":null,"subject":"[PATCH] rev-list: use hdr_termination instead of a always using a newline","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-10-20T18:19:30Z","receivedAt":"2016-10-20T18:19:38Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nWhen adding support for prefixing output of log and other commands using\n--line-prefix, commit 660e113ce118 (\"graph: add support for\n--line-prefix on all graph-aware output\", 2016-08-31) accidentally\nbroke rev-list --header output.\n\nIn order to make the output appear with a line-prefix, the flow was\nchanged to always use the graph subsystem for display. Unfortunately\nthe graph flow in rev-list did not use info->hdr_termination as it was\nassumed that graph output would never need to putput NULs.\n\nSince we now always use the graph code in order to handle the case of\nline-prefix, simply replace putchar('\\n') with\nputchar(info->hdr_termination) which will correct this issue.\n\nAdd a test for the --header case to make sure we don't break it in the\nfuture. Implement a helper function test_ends_with_nul() to make it more\nobvious what sort of check we are looking for.\n\nReported-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n\nHere's my solution, with an updated test using a helper function based\non using sed (which I think is more portable than tail -n1 ?). The\nchange actually is very simple. I ran the test suite and it appears to\nbe not breaking anyone else since the normal case is\nhdr_termination=\"\\n\" except in the cases where it needs to be NUL.\n\nThanks for the bug report!\n\n builtin/rev-list.c       |  2 +-\n t/t6000-rev-list-misc.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 8479f6ed28aa..c43decda7011 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -145,7 +145,7 @@ static void show_commit(struct commit *commit, void *data)\n \t\t\t */\n \t\t\tif (buf.len && buf.buf[buf.len - 1] == '\\n')\n \t\t\t\tgraph_show_padding(revs->graph);\n-\t\t\tputchar('\\n');\n+\t\t\tputchar(info->hdr_termination);\n \t\t} else {\n \t\t\t/*\n \t\t\t * If the message buffer is empty, just show\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 3e752ce03280..e8c6979baf59 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -4,6 +4,12 @@ test_description='miscellaneous rev-list tests'\n \n . ./test-lib.sh\n \n+test_ends_with_nul() {\n+\tprintf \"\\0\" >nul\n+\tsed '$!d' \"$@\" >contents\n+\ttest_cmp_bin nul contents\n+}\n+\n test_expect_success setup '\n \techo content1 >wanted_file &&\n \techo content2 >unwanted_file &&\n@@ -100,4 +106,9 @@ test_expect_success '--bisect and --first-parent can not be combined' '\n \ttest_must_fail git rev-list --bisect --first-parent HEAD\n '\n \n+test_expect_success '--header shows a NUL after each commit' '\n+\tgit rev-list --header --max-count=1 HEAD | sed \\$!d >actual &&\n+\ttest_ends_with_nul actual\n+'\n+\n test_done\n-- \n2.10.0.560.g867c144\n\n"},{"id":"304498","messageId":"1476989380.28685.8.camel@kaarsemaker.net","threadId":"44340","inReplyTo":"20161020181930.21084-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-10-20T18:49:40Z","receivedAt":"2016-10-20T18:49:47Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Thu, 2016-10-20 at 11:19 -0700, Jacob Keller wrote:\n> Here's my solution, with an updated test using a helper function based\n> on using sed (which I think is more portable than tail -n1 ?). The\n> change actually is very simple. I ran the test suite and it appears to\n> be not breaking anyone else since the normal case is\n> hdr_termination=\"\\n\" except in the cases where it needs to be NUL.\n> \n> Thanks for the bug report!\n\nI like both improvements, and both 'make test' and gitweb are happy\nwith this version. Thanks for the quick fix.\n\nD.\n"},{"id":"304499","messageId":"xmqq4m46hmvq.fsf@gitster.mtv.corp.google.com","threadId":"44340","inReplyTo":"20161020181930.21084-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-20T18:54:33Z","receivedAt":"2016-10-20T18:54:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\n> index 3e752ce03280..e8c6979baf59 100755\n> --- a/t/t6000-rev-list-misc.sh\n> +++ b/t/t6000-rev-list-misc.sh\n> @@ -4,6 +4,12 @@ test_description='miscellaneous rev-list tests'\n>  \n>  . ./test-lib.sh\n>  \n> +test_ends_with_nul() {\n> +\tprintf \"\\0\" >nul\n> +\tsed '$!d' \"$@\" >contents\n> +\ttest_cmp_bin nul contents\n> +}\n> +\n>  test_expect_success setup '\n>  \techo content1 >wanted_file &&\n>  \techo content2 >unwanted_file &&\n> @@ -100,4 +106,9 @@ test_expect_success '--bisect and --first-parent can not be combined' '\n>  \ttest_must_fail git rev-list --bisect --first-parent HEAD\n>  '\n>  \n> +test_expect_success '--header shows a NUL after each commit' '\n> +\tgit rev-list --header --max-count=1 HEAD | sed \\$!d >actual &&\n> +\ttest_ends_with_nul actual\n> +'\n> +\n>  test_done\n\nThanks.\n\nThe main part of the patch looks good.  For \"passing NUL to sed\",\nI'd probably work it around like so:\n\n t/t6000-rev-list-misc.sh | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex e8c6979baf..737026c34f 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -4,12 +4,6 @@ test_description='miscellaneous rev-list tests'\n \n . ./test-lib.sh\n \n-test_ends_with_nul() {\n-\tprintf \"\\0\" >nul\n-\tsed '$!d' \"$@\" >contents\n-\ttest_cmp_bin nul contents\n-}\n-\n test_expect_success setup '\n \techo content1 >wanted_file &&\n \techo content2 >unwanted_file &&\n@@ -107,8 +101,17 @@ test_expect_success '--bisect and --first-parent can not be combined' '\n '\n \n test_expect_success '--header shows a NUL after each commit' '\n-\tgit rev-list --header --max-count=1 HEAD | sed \\$!d >actual &&\n-\ttest_ends_with_nul actual\n+\t# We know there is no Q in the true payload; names and\n+\t# addresses of the authors and the committers do not have\n+\t# any, and object names or header names do not, either.\n+\tgit rev-list --header --max-count=2 HEAD |\n+\tnul_to_q |\n+\tgrep \"^Q\" >actual &&\n+\tcat >expect <<-EOF &&\n+\tQ$(git rev-parse HEAD~1)\n+\tQ\n+\tEOF\n+\ttest_cmp expect actual\n '\n \n test_done\n"},{"id":"304506","messageId":"CA+P7+xp1GevYkrSfDo2OtEve3RCXokawEf6c=5gF0daZOUF00Q@mail.gmail.com","threadId":"44340","inReplyTo":"xmqq4m46hmvq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-10-20T20:05:42Z","receivedAt":"2016-10-20T20:06:08Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Oct 20, 2016 at 11:54 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> The main part of the patch looks good.  For \"passing NUL to sed\",\n> I'd probably work it around like so:\n>\n\nYep. I wasn't sure on the test as it was, because of the portability concern.\n\n>  t/t6000-rev-list-misc.sh | 19 +++++++++++--------\n>  1 file changed, 11 insertions(+), 8 deletions(-)\n>\n> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\n> index e8c6979baf..737026c34f 100755\n> --- a/t/t6000-rev-list-misc.sh\n> +++ b/t/t6000-rev-list-misc.sh\n> @@ -4,12 +4,6 @@ test_description='miscellaneous rev-list tests'\n>\n>  . ./test-lib.sh\n>\n> -test_ends_with_nul() {\n> -       printf \"\\0\" >nul\n> -       sed '$!d' \"$@\" >contents\n> -       test_cmp_bin nul contents\n> -}\n> -\n>  test_expect_success setup '\n>         echo content1 >wanted_file &&\n>         echo content2 >unwanted_file &&\n> @@ -107,8 +101,17 @@ test_expect_success '--bisect and --first-parent can not be combined' '\n>  '\n>\n>  test_expect_success '--header shows a NUL after each commit' '\n> -       git rev-list --header --max-count=1 HEAD | sed \\$!d >actual &&\n> -       test_ends_with_nul actual\n> +       # We know there is no Q in the true payload; names and\n> +       # addresses of the authors and the committers do not have\n> +       # any, and object names or header names do not, either.\n> +       git rev-list --header --max-count=2 HEAD |\n> +       nul_to_q |\n> +       grep \"^Q\" >actual &&\n> +       cat >expect <<-EOF &&\n> +       Q$(git rev-parse HEAD~1)\n> +       Q\n> +       EOF\n> +       test_cmp expect actual\n>  '\n>\n>  test_done\n\nI will squash this in and re-send.\n\nThanks,\nJake\n"}]}