{"thread":{"id":"41737","subject":"[PATCH v3] builtin/apply: handle parse_binary() failure","startedAt":"2016-03-18T12:30:41Z","lastAt":"2016-04-01T19:31:55Z","messageCount":4,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"281142","messageId":"1458304241-537-1-git-send-email-chriscool@tuxfamily.org","threadId":"41737","inReplyTo":null,"subject":"[PATCH v3] builtin/apply: handle parse_binary() failure","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-18T12:30:41Z","receivedAt":"2016-03-18T12:30:41Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In parse_binary() there is:\n\n\tforward = parse_binary_hunk(&buffer, &size, &status, &used);\n\tif (!forward && !status)\n\t\t/* there has to be one hunk (forward hunk) */\n\t\treturn error(_(\"unrecognized binary patch at line %d\"), linenr-1);\n\nso parse_binary() can return -1, because that's what error() returns.\n\nAlso parse_binary_hunk() sets \"status\" to -1 in case of error and\nparse_binary() does \"if (status) return status;\".\n\nIn this case parse_chunk() should not add -1 to the patchsize it computes.\nIt is better for future libification efforts to make it just return -1.\n\nSigned-off-by: Christian Couder \n---\nOnly the title of the patch changed in this version compared to v2.\n\n builtin/apply.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 42c610e..c399c97 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1872,6 +1872,11 @@ static struct fragment *parse_binary_hunk(char **buf_p,\n \treturn NULL;\n }\n \n+/*\n+ * Returns:\n+ * -1 in case of error,\n+ * the length of the parsed binary patch otherwise\n+ */\n static int parse_binary(char *buffer, unsigned long size, struct patch *patch)\n {\n \t/*\n@@ -2017,6 +2022,8 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)\n \t\t\tlinenr++;\n \t\t\tused = parse_binary(buffer + hd + llen,\n \t\t\t\t\t size - hd - llen, patch);\n+\t\t\tif (used < 0)\n+\t\t\t\treturn -1;\n \t\t\tif (used)\n \t\t\t\tpatchsize = used + llen;\n \t\t\telse\n-- \n2.8.0.rc2.56.gc9044db\n"},{"id":"282403","messageId":"CAP8UFD2vx17KDf5HgSYYZudbXiq7CYDg0=hXbJuJrLp9QDfJUg@mail.gmail.com","threadId":"41737","inReplyTo":"1458304241-537-1-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH v3] builtin/apply: handle parse_binary() failure","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-31T22:26:26Z","receivedAt":"2016-03-31T22:26:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Mar 18, 2016 at 1:30 PM, Christian Couder\n wrote:\n> In parse_binary() there is:\n>\n> forward = parse_binary_hunk(&buffer, &size, &status, &used);\n> if (!forward && !status)\n> /* there has to be one hunk (forward hunk) */\n> return error(_(\"unrecognized binary patch at line %d\"), linenr-1);\n>\n> so parse_binary() can return -1, because that's what error() returns.\n>\n> Also parse_binary_hunk() sets \"status\" to -1 in case of error and\n> parse_binary() does \"if (status) return status;\".\n>\n> In this case parse_chunk() should not add -1 to the patchsize it computes.\n> It is better for future libification efforts to make it just return -1.\n>\n> Signed-off-by: Christian Couder \n> ---\n> Only the title of the patch changed in this version compared to v2.\n\nIt looks like this patch is not in pu. Maybe it has fallen through the cracks?\n"},{"id":"282486","messageId":"xmqqvb41i705.fsf@gitster.mtv.corp.google.com","threadId":"41737","inReplyTo":"CAP8UFD2vx17KDf5HgSYYZudbXiq7CYDg0=hXbJuJrLp9QDfJUg@mail.gmail.com","subject":"Re: [PATCH v3] builtin/apply: handle parse_binary() failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-01T17:16:58Z","receivedAt":"2016-04-01T17:16:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Fri, Mar 18, 2016 at 1:30 PM, Christian Couder\n> wrote:\n>> In parse_binary() there is:\n>>\n>> forward = parse_binary_hunk(&buffer, &size, &status, &used);\n>> if (!forward && !status)\n>> /* there has to be one hunk (forward hunk) */\n>> return error(_(\"unrecognized binary patch at line %d\"), linenr-1);\n>>\n>> so parse_binary() can return -1, because that's what error() returns.\n>>\n>> Also parse_binary_hunk() sets \"status\" to -1 in case of error and\n>> parse_binary() does \"if (status) return status;\".\n>>\n>> In this case parse_chunk() should not add -1 to the patchsize it computes.\n>> It is better for future libification efforts to make it just return -1.\n>>\n>> Signed-off-by: Christian Couder \n>> ---\n>> Only the title of the patch changed in this version compared to v2.\n>\n> It looks like this patch is not in pu. Maybe it has fallen through the cracks?\n\nYup, it indeed was ignored (giving priority to work towards 2.8\nduring the freeze) and then was forgotten.\n\nMy comment on the first one that exited mentions \"your follow-up\npatch\", but I cannot quite tell which one, as there was no threading\nin your original patches. Does this change need to come before\nsomething else that I already have?\n"},{"id":"282500","messageId":"CAP8UFD0RCECwy4DXM0PHe+K-XTiCbtur0dEhk6Z37wCKLDwfKQ@mail.gmail.com","threadId":"41737","inReplyTo":"xmqqvb41i705.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] builtin/apply: handle parse_binary() failure","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-04-01T19:31:55Z","receivedAt":"2016-04-01T19:31:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Apr 1, 2016 at 1:16 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>>\n>> It looks like this patch is not in pu. Maybe it has fallen through the cracks?\n>\n> Yup, it indeed was ignored (giving priority to work towards 2.8\n> during the freeze) and then was forgotten.\n>\n> My comment on the first one that exited mentions \"your follow-up\n> patch\", but I cannot quite tell which one, as there was no threading\n> in your original patches. Does this change need to come before\n> something else that I already have?\n\nNo, I think by \"your follow-up patch\" you meant the other one that had\nalso been forgotten and that I also replied to, that is:\n\n[PATCH] builtin/apply: free patch when parse_chunk() fails\n\nI had first sent both of them at around the same time.\n\nThanks,\nChristian.\n"}]}