{"thread":{"id":"28450","subject":"git patch-id fails on long lines","startedAt":"2011-09-20T18:07:42Z","lastAt":"2011-09-22T17:17:06Z","messageCount":5,"participants":["Andrew Pimlott","Junio C Hamano","Jeff King","Michael Schubert"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"175877","messageId":"1316541771-sup-9996@pimlott.net","threadId":"28450","inReplyTo":null,"subject":"git patch-id fails on long lines","fromName":"Andrew Pimlott","fromEmail":"andrew@pimlott.net","sentAt":"2011-09-20T18:07:42Z","receivedAt":"2011-09-20T18:07:42Z","isPatch":false,"sender":{"key":"andrew@pimlott.net","avatar":null},"body":"In patch-id.c, get_one_patchid uses a fixed 1000-char buffer to read a line.[1]\nThis causes incorrect results on longer lines. Pasted below is a git commit\n(from git show) that demonstrates the problem. The result of running git\npatch-id on this commit is:\n\n9220f380851be9cab1a760430e3be096dcbee8c6 9b96b6fde8f7df791a1490ae18e1fa75fbab3262\n74b8ede07628a574fd586624e0c77a4b6c9967e0 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n\nThe commit:\n\ncommit 9b96b6fde8f7df791a1490ae18e1fa75fbab3262\nAuthor: Andrew Pimlott \nDate: Tue Sep 20 10:53:25 2011 -0700\n\n 2\n\ndiff --git a/a b/a\nindex e69de29..2e6adac 100644\n--- a/a\n+++ b/a\n@@ -0,0 +1 @@\n+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ndiff --git a/b b/b\nindex e69de29..1425fcc 100644\n--- a/b\n+++ b/b\n@@ -0,0 +1 @@\n+b\n\nAndrew\n\n[1] https://github.com/git/git/blob/master/builtin/patch-id.c\n"},{"id":"175890","messageId":"7vehzb5602.fsf@alter.siamese.dyndns.org","threadId":"28450","inReplyTo":"1316541771-sup-9996@pimlott.net","subject":"Re: git patch-id fails on long lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-20T20:11:09Z","receivedAt":"2011-09-20T20:11:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Pimlott writes:\n\n> In patch-id.c, get_one_patchid uses a fixed 1000-char buffer to read a line.\n\nThanks; builtin/patch-id.c is parhaps one of the more ancient part of the\nsystem and it hasn't been updated to use more modern facility like strbuf\nAPI.\n\nFix should be pretty straightforward. Any takers?\n"},{"id":"175891","messageId":"20110920201808.GA18310@sigill.intra.peff.net","threadId":"28450","inReplyTo":"1316541771-sup-9996@pimlott.net","subject":"Re: git patch-id fails on long lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-20T20:18:08Z","receivedAt":"2011-09-20T20:18:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 20, 2011 at 11:07:42AM -0700, Andrew Pimlott wrote:\n\n> In patch-id.c, get_one_patchid uses a fixed 1000-char buffer to read a line.[1]\n> This causes incorrect results on longer lines. Pasted below is a git commit\n> (from git show) that demonstrates the problem. The result of running git\n> patch-id on this commit is:\n> \n> 9220f380851be9cab1a760430e3be096dcbee8c6 9b96b6fde8f7df791a1490ae18e1fa75fbab3262\n> 74b8ede07628a574fd586624e0c77a4b6c9967e0 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n\nHmm, yeah. My initial impression \"eh, so what, it will just add the\nline contents to the sha1 patch-id in two separate hunks\". But we\nactually treat lines magically based on their beginnings, and it\nseems we accidentally think the \"aaa\" is the start of the next commit\nheader.\n\nI think this can be trivially converted to use strbuf_getwholeline,\nsomething like this (completely untested):\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex f821eb3..99ba2ca 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -58,11 +58,12 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \n static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)\n {\n-\tstatic char line[1000];\n+\tstatic struct strbuf line_buf = STRBUF_INIT;\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n \n-\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n+\twhile (strbuf_getwholeline(&line_buf, stdin, '\\n') != EOF) {\n+\t\tchar *line = line_buf.buf;\n \t\tchar *p = line;\n \t\tint len;\n \n\nThat just reuses the same heap-allocated buffer over and over, and then\neventually leaks it at the end (but then the program exits anyway). You\ncould get fancier and actually pass in the strbuf from generate_id_list,\nand then release it when we're done.\n\n-Peff\n"},{"id":"175943","messageId":"4E79DBAE.5090505@elegosoft.com","threadId":"28450","inReplyTo":"7vehzb5602.fsf@alter.siamese.dyndns.org","subject":"[PATCH] patch-id.c: use strbuf instead of a fixed buffer","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2011-09-21T12:42:22Z","receivedAt":"2011-09-21T12:42:22Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"get_one_patchid() uses a rather dumb heuristic to determine if the\npassed buffer is part of the next commit. Whenever the first 40 bytes\nare a valid hexadecimal sha1 representation, get_one_patchid() returns\nnext_sha1.\nOnce the current line is longer than the fixed buffer, this will break\n(provided the additional bytes make a valid hexadecimal sha1). As a result\npatch-id returns incorrect results. Instead, user strbuf and read one\nline at a time.\n\nHelped-by: Jeff King \nSigned-off-by: Michael Schubert \n---\n builtin/patch-id.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex f821eb3..3cfe02d 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -56,13 +56,13 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \treturn 1;\n }\n \n-static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)\n+static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct strbuf *line_buf)\n {\n-\tstatic char line[1000];\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n \n-\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n+\twhile (strbuf_getwholeline(line_buf, stdin, '\\n') != EOF) {\n+\t\tchar *line = line_buf->buf;\n \t\tchar *p = line;\n \t\tint len;\n \n@@ -133,14 +133,16 @@ static void generate_id_list(void)\n \tunsigned char sha1[20], n[20];\n \tgit_SHA_CTX ctx;\n \tint patchlen;\n+\tstruct strbuf line_buf = STRBUF_INIT;\n \n \tgit_SHA1_Init(&ctx);\n \thashclr(sha1);\n \twhile (!feof(stdin)) {\n-\t\tpatchlen = get_one_patchid(n, &ctx);\n+\t\tpatchlen = get_one_patchid(n, &ctx, &line_buf);\n \t\tflush_current_id(patchlen, sha1, &ctx);\n \t\thashcpy(sha1, n);\n \t}\n+\tstrbuf_release(&line_buf);\n }\n \n static const char patch_id_usage[] = \"git patch-id < patch\";\n-- \n1.7.7.rc2.365.g55c1f\n"},{"id":"176001","messageId":"20110922171706.GB2934@sigill.intra.peff.net","threadId":"28450","inReplyTo":"4E79DBAE.5090505@elegosoft.com","subject":"Re: [PATCH] patch-id.c: use strbuf instead of a fixed buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-22T17:17:06Z","receivedAt":"2011-09-22T17:17:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 21, 2011 at 02:42:22PM +0200, Michael Schubert wrote:\n\n> get_one_patchid() uses a rather dumb heuristic to determine if the\n> passed buffer is part of the next commit. Whenever the first 40 bytes\n> are a valid hexadecimal sha1 representation, get_one_patchid() returns\n> next_sha1.\n> Once the current line is longer than the fixed buffer, this will break\n> (provided the additional bytes make a valid hexadecimal sha1). As a result\n> patch-id returns incorrect results. Instead, user strbuf and read one\n> line at a time.\n\nA minor nit, but I think this is probably broken even if the additional\nbytes don't look like a valid sha1. It would look like cruft after the\ndiff (since the lien doesn't start with plus, minus, or space), which\nmeans it would not be stirred into the sha1 mix.\n\n> builtin/patch-id.c | 10 ++++++----\n> 1 files changed, 6 insertions(+), 4 deletions(-)\n\nThe patch itself looks good to me, though. Thanks.\n\n-Peff\n"}]}