{"thread":{"id":"34516","subject":"[PATCH] rm: do not set a variable twice without intermediate reading.","startedAt":"2013-07-23T09:19:22Z","lastAt":"2013-07-23T19:01:33Z","messageCount":4,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"223976","messageId":"1374571162-22881-1-git-send-email-stefanbeller@googlemail.com","threadId":"34516","inReplyTo":null,"subject":"[PATCH] rm: do not set a variable twice without intermediate reading.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-23T09:19:22Z","receivedAt":"2013-07-23T09:19:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Just the next line assigns a non-null value to seen.\n\nSigned-off-by: Stefan Beller \n---\n builtin/rm.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 5b63d3f..df85f98 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -316,7 +316,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tparse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);\n \trefresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);\n \n-\tseen = NULL;\n \tseen = xcalloc(pathspec.nr, 1);\n \n \tfor (i = 0; i < active_nr; i++) {\n-- \n1.8.3.3.1135.ge2c9e63\n"},{"id":"223993","messageId":"7v4nbl86us.fsf@alter.siamese.dyndns.org","threadId":"34516","inReplyTo":"1374571162-22881-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] rm: do not set a variable twice without intermediate reading.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-23T18:32:43Z","receivedAt":"2013-07-23T18:32:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Just the next line assigns a non-null value to seen.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/rm.c | 1 -\n> 1 file changed, 1 deletion(-)\n>\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 5b63d3f..df85f98 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -316,7 +316,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> \tparse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);\n> \trefresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);\n> \n> -\tseen = NULL;\n> \tseen = xcalloc(pathspec.nr, 1);\n> \n> \tfor (i = 0; i < active_nr; i++) {\n\nInteresting. This is ancient and dates back to 7612a1ef (git-rm:\nhonor -n flag., 2006-06-08).\n"},{"id":"223994","messageId":"51EECFBB.3050406@googlemail.com","threadId":"34516","inReplyTo":"7v4nbl86us.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rm: do not set a variable twice without intermediate reading.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-23T18:47:23Z","receivedAt":"2013-07-23T18:47:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/23/2013 08:32 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n> \n>> Just the next line assigns a non-null value to seen.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> builtin/rm.c | 1 -\n>> 1 file changed, 1 deletion(-)\n>>\n>> diff --git a/builtin/rm.c b/builtin/rm.c\n>> index 5b63d3f..df85f98 100644\n>> --- a/builtin/rm.c\n>> +++ b/builtin/rm.c\n>> @@ -316,7 +316,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n>> \tparse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);\n>> \trefresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);\n>> \n>> -\tseen = NULL;\n>> \tseen = xcalloc(pathspec.nr, 1);\n>> \n>> \tfor (i = 0; i < active_nr; i++) {\n> \n> Interesting. This is ancient and dates back to 7612a1ef (git-rm:\n> honor -n flag., 2006-06-08).\n> \n\nWell the removed line itself maybe, but the next line ... as well.\n\nThe next line \"seen = xcalloc(...)\" was introduced in 29211a93c14\n(2013-07-14) and was changed 4 or 5 times before (changing the\nthe malloc function to xmalloc and then to xcalloc.\nI suppose these changes did not pay attention to the local area\naround, but rather were interested in making the memory allocation\nfast or safe in many places.\n\n\nOriginally it comes from d9b814cc97 (by Linus), which introduced:\n+\tseen = NULL;\n+\tif (pathspec) {\n+\t\tfor (i = 0; pathspec[i] ; i++)\n+\t\t\t/* nothing */;\n+\t\tseen = xmalloc(i);\n+\t\tmemset(seen, 0, i);\n+\t}\n\nThen in 7612a1efdb0c the second seen assignment was made unconditional.\nAnd since then it has been not noticed. ;)\n\nHowever that being said, I am currently playing around with different\ncode analyzers (find dead code, possible null pointers, and such),\nand the coding style of git is very different to what I am used to\n(and what the tools are used to as well, lots of false positives).\n\nPersonally the coding style of git often reminds me to 'C as a\nmacro-assembler' rather than 'C as a high level programming\nlanguage'.\n\nStefan\n"},{"id":"223995","messageId":"7vzjtd6qya.fsf@alter.siamese.dyndns.org","threadId":"34516","inReplyTo":"51EECFBB.3050406@googlemail.com","subject":"Re: [PATCH] rm: do not set a variable twice without intermediate reading.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-23T19:01:33Z","receivedAt":"2013-07-23T19:01:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On 07/23/2013 08:32 PM, Junio C Hamano wrote:\n>> Interesting. This is ancient and dates back to 7612a1ef (git-rm:\n>> honor -n flag., 2006-06-08).\n> Originally it comes from d9b814cc97 (by Linus), which introduced:\n> +\tseen = NULL;\n> +\tif (pathspec) {\n> +\t\tfor (i = 0; pathspec[i] ; i++)\n> +\t\t\t/* nothing */;\n> +\t\tseen = xmalloc(i);\n> +\t\tmemset(seen, 0, i);\n> +\t}\n>\n> Then in 7612a1efdb0c the second seen assignment was made unconditional.\n\nThat is why I blamed the bug to 7612a1ef. Before that, without pathspec,\ndirectory traversal function were told not to report which ones were\nseen and which ones were not by passing seen=NULL.\n"}]}