{"thread":{"id":"25382","subject":"[PATCH] rev-list: handle %x00 NUL in user format","startedAt":"2010-10-07T18:25:43Z","lastAt":"2010-10-14T00:38:54Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"152900","messageId":"20101007182542.GA20165@sigill.intra.peff.net","threadId":"25382","inReplyTo":null,"subject":"[PATCH] rev-list: handle %x00 NUL in user format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-07T18:25:43Z","receivedAt":"2010-10-07T18:25:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The code paths for showing commits in \"git log\" and \"git\nrev-list --graph\" correctly handle embedded NULs by looking\nonly at the resulting strbuf's length, and never treating it\nas a C string. The code path for regular rev-list, however,\nused printf(\"%s\"), which resulted in truncated output. This\npatch uses fwrite instead, like the --graph code path.\n\nSigned-off-by: Jeff King \n---\nI built this on master, though it is perhaps maint-worthy. The buggy\nline seems to blame back to at least v1.5.3-era.\n\nErik mentioned a potential problem with fwrite() and the way we handle\nANSI emulation for Windows. I think if there is a problem, then the same\nproblem exists in the --graph code, and we should do this, and then fix\nboth on top.\n\n builtin/rev-list.c | 6 ++++--\n t/t6006-rev-list-format.sh | 8 ++++++++\n t/test-lib.sh | 4 ++++\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex efe9360..3b2dca0 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -147,8 +147,10 @@ static void show_commit(struct commit *commit, void *data)\n \t\t\t}\n \t\t} else {\n \t\t\tif (revs->commit_format != CMIT_FMT_USERFORMAT ||\n-\t\t\t buf.len)\n-\t\t\t\tprintf(\"%s%c\", buf.buf, info->hdr_termination);\n+\t\t\t buf.len) {\n+\t\t\t\tfwrite(buf.buf, 1, buf.len, stdout);\n+\t\t\t\tputchar(info->hdr_termination);\n+\t\t\t}\n \t\t}\n \t\tstrbuf_release(&buf);\n \t} else {\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex cccacd4..d918cc0 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -162,6 +162,14 @@ commit 131a310eb913d107dd3c09a65d1651175898735d\n commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873\n EOF\n \n+test_expect_success '%x00 shows NUL' '\n+\techo >expect commit f58db70b055c5718631e5c61528b28b12090cdea &&\n+\techo >>expect fooQbar &&\n+\tgit rev-list -1 --format=foo%x00bar HEAD >actual.nul &&\n+\tnul_to_q actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%ad respects --date=' '\n \techo 2005-04-07 >expect.ad-short &&\n \tgit log -1 --date=short --pretty=tformat:%ad >output.ad-short master &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 2af8f10..bbe79e0 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -248,6 +248,10 @@ test_decode_color () {\n \t\t-e 's/.\\[m//g'\n }\n \n+nul_to_q () {\n+\tperl -pe 'y/\\000/Q/'\n+}\n+\n q_to_nul () {\n \tperl -pe 'y/Q/\\000/'\n }\n-- \n1.7.3.1.203.g81d8.dirty\n"},{"id":"153453","messageId":"7vtykpiwkv.fsf@alter.siamese.dyndns.org","threadId":"25382","inReplyTo":"20101007182542.GA20165@sigill.intra.peff.net","subject":"Re: [PATCH] rev-list: handle %x00 NUL in user format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-13T21:58:40Z","receivedAt":"2010-10-13T21:58:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Erik mentioned a potential problem with fwrite() and the way we handle\n> ANSI emulation for Windows. I think if there is a problem, then the same\n> problem exists in the --graph code, and we should do this, and then fix\n> both on top.\n\nI am quite sure that --graph codepath is used a lot less often than this\ncodepath, and most users do not hit the problem you are fixing---that is\nwhy it took so long to get noticed. I think this printf() has been there\nfrom the beginning, even though the possibility of placing a NUL in\nbuf.buf may have come much later, like when %x00 was introduced).\n\nUse of fwrite() here will affect all Windows users who use color (probably\nmost of them), no? It may be a deliberate regression, but it has a real\ncost associated with it; it will break more usual uses on Windows to help\nan obscure use case on POSIX.\n\nAs to emulating fwrite(3) on Windows, even though there are a lot of\ncallers of fwrite(3) that do not want the payload colored, it is probably\nsufficient to check isatty(fileno(stream)) upfront like all the other\nwinansi_fEMULATED() functions do, and to make ansi_emulate() call the\nsystem fwrite(3) directly.\n\nI'll queue this patch on a side branch and I may even feel like merging it\nto 'next', but we would need to hear from Windows folks how they want to\nproceed soon.\n"},{"id":"153475","messageId":"20101014003853.GA10708@coredump.intra.peff.net","threadId":"25382","inReplyTo":"7vtykpiwkv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rev-list: handle %x00 NUL in user format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-14T00:38:54Z","receivedAt":"2010-10-14T00:38:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 13, 2010 at 02:58:40PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Erik mentioned a potential problem with fwrite() and the way we handle\n> > ANSI emulation for Windows. I think if there is a problem, then the same\n> > problem exists in the --graph code, and we should do this, and then fix\n> > both on top.\n> \n> I am quite sure that --graph codepath is used a lot less often than this\n> codepath, and most users do not hit the problem you are fixing---that is\n> why it took so long to get noticed. I think this printf() has been there\n> from the beginning, even though the possibility of placing a NUL in\n> buf.buf may have come much later, like when %x00 was introduced).\n> \n> Use of fwrite() here will affect all Windows users who use color (probably\n> most of them), no? It may be a deliberate regression, but it has a real\n> cost associated with it; it will break more usual uses on Windows to help\n> an obscure use case on POSIX.\n\nYeah, I don't want to regress Windows users in an actual release. I was\nhoping for you to queue it on a side branch, and then have Windows\npeople apply the proper fix on top before getting it merged in.\n\n-Peff\n"}]}