Re: [PATCH] rm: do not set a variable twice without intermediate reading.

[Stefan Beller <stefanbeller@googlemail.com>]

On 07/23/2013 08:32 PM, Junio C Hamano wrote:
> Stefan Beller <stefanbeller@googlemail.com> writes:
> 
>> Just the next line assigns a non-null value to seen.
>>
>> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
>> ---
>>  builtin/rm.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/builtin/rm.c b/builtin/rm.c
>> index 5b63d3f..df85f98 100644
>> --- a/builtin/rm.c
>> +++ b/builtin/rm.c
>> @@ -316,7 +316,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>  	parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);
>>  	refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);
>>  
>> -	seen = NULL;
>>  	seen = xcalloc(pathspec.nr, 1);
>>  
>>  	for (i = 0; i < active_nr; i++) {
> 
> Interesting. This is ancient and dates back to 7612a1ef (git-rm:
> honor -n flag., 2006-06-08).
> 

Well the removed line itself maybe, but the next line ... as well.

The next line "seen = xcalloc(...)" was introduced in 29211a93c14
(2013-07-14) and was changed 4 or 5 times before (changing the
the malloc function to xmalloc and then to xcalloc.
I suppose these changes did not pay attention to the local area
around, but rather were interested in making the memory allocation
fast or safe in many places.


Originally it comes from d9b814cc97 (by Linus), which introduced:
+	seen = NULL;
+	if (pathspec) {
+		for (i = 0; pathspec[i] ; i++)
+			/* nothing */;
+		seen = xmalloc(i);
+		memset(seen, 0, i);
+	}

Then in 7612a1efdb0c the second seen assignment was made unconditional.
And since then it has been not noticed. ;)

However that being said, I am currently playing around with different
code analyzers (find dead code, possible null pointers, and such),
and the coding style of git is very different to what I am used to
(and what the tools are used to as well, lots of false positives).

Personally the coding style of git often reminds me to 'C as a
macro-assembler' rather than 'C as a high level programming
language'.

Stefan