{"thread":{"id":"27631","subject":"[PATCH] Use correct value when hinting strbuf_read()","startedAt":"2011-06-15T18:08:38Z","lastAt":"2011-06-26T20:32:30Z","messageCount":3,"participants":["Fredrik Gustafsson","Junio C Hamano","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"170077","messageId":"1308161318-25637-1-git-send-email-iveqy@iveqy.com","threadId":"27631","inReplyTo":null,"subject":"[PATCH] Use correct value when hinting strbuf_read()","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2011-06-15T18:08:38Z","receivedAt":"2011-06-15T18:08:38Z","isPatch":true,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"The git strbuf allows for each read to hint about the size of the\nstring. In this case the the string can never be longer than 41\ncharacters, as it cannot contain more than a single hex-sha1 and a\nnewline.\n\nSo let's use 41 instead of 1024 to reduce the memory footprint.\n\nSigned-off-by: Fredrik Gustafsson \nMentored-by: Jens Lehmann \nMentored-by: Heiko Voigt \n---\n submodule.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b6dec70..86baf42 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -326,7 +326,7 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n \t\tcp.no_stdin = 1;\n \t\tcp.out = -1;\n \t\tcp.dir = path;\n-\t\tif (!run_command(&cp) && !strbuf_read(&buf, cp.out, 1024))\n+\t\tif (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))\n \t\t\tis_present = 1;\n \n \t\tclose(cp.out);\n-- \n1.7.5.1.229.g455f\n"},{"id":"170554","messageId":"7vzkl4z8nl.fsf@alter.siamese.dyndns.org","threadId":"27631","inReplyTo":"1308161318-25637-1-git-send-email-iveqy@iveqy.com","subject":"Re: [PATCH] Use correct value when hinting strbuf_read()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-26T19:37:18Z","receivedAt":"2011-06-26T19:37:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Gustafsson writes:\n\n> diff --git a/submodule.c b/submodule.c\n> index b6dec70..86baf42 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -326,7 +326,7 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n> \t\tcp.no_stdin = 1;\n> \t\tcp.out = -1;\n> \t\tcp.dir = path;\n> -\t\tif (!run_command(&cp) && !strbuf_read(&buf, cp.out, 1024))\n> +\t\tif (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))\n> \t\t\tis_present = 1;\n\nThe change is not incorrect per-se. If the original used 41 and a patch\ntried to update it to 1024, we wouldn't accept such a patch, but on the\nother hand, I do not think this patch would make much difference (any\nvalue would do here as it is merely a hint, and if the patch does make a\ndifference, we would have a bigger problem, either by forking too often\nwith run_command(), or by leaking buf.buf every time we do so).\n\nIt however raises a more interesting question.\n\nThis function tries to see, even a commit object name, if it is fully\nconnected to any ref (IOW the tip of a branch or a tag). There are three\noutcomes:\n\n - It is reachable from a ref, and we get nothing from the command;\n\n - It is not, and we get the commit object name back (and nothing else);\n\n - We get something unexpected. Perhaps an error found in the repository\n (strictly speaking I do not think we would catch this as we are not\n capturing stderr at all).\n\nThe first one is what this if() condition catches, but we do not tell the\nsecond and the third apart. I wonder if we should.\n"},{"id":"170557","messageId":"4E07975E.5010900@web.de","threadId":"27631","inReplyTo":"7vzkl4z8nl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Use correct value when hinting strbuf_read()","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-06-26T20:32:30Z","receivedAt":"2011-06-26T20:32:30Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 26.06.2011 21:37, schrieb Junio C Hamano:\n> Fredrik Gustafsson writes:\n> \n>> diff --git a/submodule.c b/submodule.c\n>> index b6dec70..86baf42 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -326,7 +326,7 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n>> \t\tcp.no_stdin = 1;\n>> \t\tcp.out = -1;\n>> \t\tcp.dir = path;\n>> -\t\tif (!run_command(&cp) && !strbuf_read(&buf, cp.out, 1024))\n>> +\t\tif (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))\n>> \t\t\tis_present = 1;\n> \n> The change is not incorrect per-se. If the original used 41 and a patch\n> tried to update it to 1024, we wouldn't accept such a patch, but on the\n> other hand, I do not think this patch would make much difference (any\n> value would do here as it is merely a hint, and if the patch does make a\n> difference, we would have a bigger problem, either by forking too often\n> with run_command(), or by leaking buf.buf every time we do so).\n\nMakes sense.\n\n> It however raises a more interesting question.\n> \n> This function tries to see, even a commit object name, if it is fully\n> connected to any ref (IOW the tip of a branch or a tag). There are three\n> outcomes:\n> \n> - It is reachable from a ref, and we get nothing from the command;\n> \n> - It is not, and we get the commit object name back (and nothing else);\n> \n> - We get something unexpected. Perhaps an error found in the repository\n> (strictly speaking I do not think we would catch this as we are not\n> capturing stderr at all).\n> \n> The first one is what this if() condition catches, but we do not tell the\n> second and the third apart. I wonder if we should.\n\nHmm, maybe we should die() if run_command() doesn't return 0? (The intention\nbehind not catching stderr was that the reason for the failure of rev-list\nshould visible to the user)\n"}]}