# [PATCH] rev-list: use hdr_termination instead of a always using a newline 4 messages from 2016-10-20 to 2016-10-20. Participants: Jacob Keller, Dennis Kaarsemaker, Junio C Hamano. Thread: https://gitlist.dev/t/44340 ## Jacob Keller, 2016-10-20 18:19 Subject: [PATCH] rev-list: use hdr_termination instead of a always using a newline Message-ID: <20161020181930.21084-1-jacob.e.keller@intel.com> URL: https://gitlist.dev/e/20161020181930.21084-1-jacob.e.keller%40intel.com ``` From: Jacob Keller When adding support for prefixing output of log and other commands using --line-prefix, commit 660e113ce118 ("graph: add support for --line-prefix on all graph-aware output", 2016-08-31) accidentally broke rev-list --header output. In order to make the output appear with a line-prefix, the flow was changed to always use the graph subsystem for display. Unfortunately the graph flow in rev-list did not use info->hdr_termination as it was assumed that graph output would never need to putput NULs. Since we now always use the graph code in order to handle the case of line-prefix, simply replace putchar('\n') with putchar(info->hdr_termination) which will correct this issue. Add a test for the --header case to make sure we don't break it in the future. Implement a helper function test_ends_with_nul() to make it more obvious what sort of check we are looking for. Reported-by: Dennis Kaarsemaker Signed-off-by: Jacob Keller --- Here's my solution, with an updated test using a helper function based on using sed (which I think is more portable than tail -n1 ?). The change actually is very simple. I ran the test suite and it appears to be not breaking anyone else since the normal case is hdr_termination="\n" except in the cases where it needs to be NUL. Thanks for the bug report! builtin/rev-list.c | 2 +- t/t6000-rev-list-misc.sh | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 8479f6ed28aa..c43decda7011 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -145,7 +145,7 @@ static void show_commit(struct commit *commit, void *data) */ if (buf.len && buf.buf[buf.len - 1] == '\n') graph_show_padding(revs->graph); - putchar('\n'); + putchar(info->hdr_termination); } else { /* * If the message buffer is empty, just show diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh index 3e752ce03280..e8c6979baf59 100755 --- a/t/t6000-rev-list-misc.sh +++ b/t/t6000-rev-list-misc.sh @@ -4,6 +4,12 @@ test_description='miscellaneous rev-list tests' . ./test-lib.sh +test_ends_with_nul() { + printf "\0" >nul + sed '$!d' "$@" >contents + test_cmp_bin nul contents +} + test_expect_success setup ' echo content1 >wanted_file && echo content2 >unwanted_file && @@ -100,4 +106,9 @@ test_expect_success '--bisect and --first-parent can not be combined' ' test_must_fail git rev-list --bisect --first-parent HEAD ' +test_expect_success '--header shows a NUL after each commit' ' + git rev-list --header --max-count=1 HEAD | sed \$!d >actual && + test_ends_with_nul actual +' + test_done -- 2.10.0.560.g867c144 ``` ## Dennis Kaarsemaker, 2016-10-20 18:49 Subject: Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline Message-ID: <1476989380.28685.8.camel@kaarsemaker.net> URL: https://gitlist.dev/e/1476989380.28685.8.camel%40kaarsemaker.net In-Reply-To: <20161020181930.21084-1-jacob.e.keller@intel.com> ``` On Thu, 2016-10-20 at 11:19 -0700, Jacob Keller wrote: > Here's my solution, with an updated test using a helper function based > on using sed (which I think is more portable than tail -n1 ?). The > change actually is very simple. I ran the test suite and it appears to > be not breaking anyone else since the normal case is > hdr_termination="\n" except in the cases where it needs to be NUL. > > Thanks for the bug report! I like both improvements, and both 'make test' and gitweb are happy with this version. Thanks for the quick fix. D. ``` ## Junio C Hamano, 2016-10-20 18:54 Subject: Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline Message-ID: URL: https://gitlist.dev/e/xmqq4m46hmvq.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20161020181930.21084-1-jacob.e.keller@intel.com> ``` Jacob Keller writes: > diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh > index 3e752ce03280..e8c6979baf59 100755 > --- a/t/t6000-rev-list-misc.sh > +++ b/t/t6000-rev-list-misc.sh > @@ -4,6 +4,12 @@ test_description='miscellaneous rev-list tests' > > . ./test-lib.sh > > +test_ends_with_nul() { > + printf "\0" >nul > + sed '$!d' "$@" >contents > + test_cmp_bin nul contents > +} > + > test_expect_success setup ' > echo content1 >wanted_file && > echo content2 >unwanted_file && > @@ -100,4 +106,9 @@ test_expect_success '--bisect and --first-parent can not be combined' ' > test_must_fail git rev-list --bisect --first-parent HEAD > ' > > +test_expect_success '--header shows a NUL after each commit' ' > + git rev-list --header --max-count=1 HEAD | sed \$!d >actual && > + test_ends_with_nul actual > +' > + > test_done Thanks. The main part of the patch looks good. For "passing NUL to sed", I'd probably work it around like so: t/t6000-rev-list-misc.sh | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh index e8c6979baf..737026c34f 100755 --- a/t/t6000-rev-list-misc.sh +++ b/t/t6000-rev-list-misc.sh @@ -4,12 +4,6 @@ test_description='miscellaneous rev-list tests' . ./test-lib.sh -test_ends_with_nul() { - printf "\0" >nul - sed '$!d' "$@" >contents - test_cmp_bin nul contents -} - test_expect_success setup ' echo content1 >wanted_file && echo content2 >unwanted_file && @@ -107,8 +101,17 @@ test_expect_success '--bisect and --first-parent can not be combined' ' ' test_expect_success '--header shows a NUL after each commit' ' - git rev-list --header --max-count=1 HEAD | sed \$!d >actual && - test_ends_with_nul actual + # We know there is no Q in the true payload; names and + # addresses of the authors and the committers do not have + # any, and object names or header names do not, either. + git rev-list --header --max-count=2 HEAD | + nul_to_q | + grep "^Q" >actual && + cat >expect <<-EOF && + Q$(git rev-parse HEAD~1) + Q + EOF + test_cmp expect actual ' test_done ``` ## Jacob Keller, 2016-10-20 20:05 Subject: Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline Message-ID: URL: https://gitlist.dev/e/CA%2BP7%2Bxp1GevYkrSfDo2OtEve3RCXokawEf6c%3D5gF0daZOUF00Q%40mail.gmail.com In-Reply-To: ``` On Thu, Oct 20, 2016 at 11:54 AM, Junio C Hamano wrote: > > The main part of the patch looks good. For "passing NUL to sed", > I'd probably work it around like so: > Yep. I wasn't sure on the test as it was, because of the portability concern. > t/t6000-rev-list-misc.sh | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh > index e8c6979baf..737026c34f 100755 > --- a/t/t6000-rev-list-misc.sh > +++ b/t/t6000-rev-list-misc.sh > @@ -4,12 +4,6 @@ test_description='miscellaneous rev-list tests' > > . ./test-lib.sh > > -test_ends_with_nul() { > - printf "\0" >nul > - sed '$!d' "$@" >contents > - test_cmp_bin nul contents > -} > - > test_expect_success setup ' > echo content1 >wanted_file && > echo content2 >unwanted_file && > @@ -107,8 +101,17 @@ test_expect_success '--bisect and --first-parent can not be combined' ' > ' > > test_expect_success '--header shows a NUL after each commit' ' > - git rev-list --header --max-count=1 HEAD | sed \$!d >actual && > - test_ends_with_nul actual > + # We know there is no Q in the true payload; names and > + # addresses of the authors and the committers do not have > + # any, and object names or header names do not, either. > + git rev-list --header --max-count=2 HEAD | > + nul_to_q | > + grep "^Q" >actual && > + cat >expect <<-EOF && > + Q$(git rev-parse HEAD~1) > + Q > + EOF > + test_cmp expect actual > ' > > test_done I will squash this in and re-send. Thanks, Jake ```