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

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

[Stefan Beller]

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++) {
-- 
1.8.3.3.1135.ge2c9e63

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

[Junio C Hamano]

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).

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

[Stefan Beller]

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

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

[Junio C Hamano]

Stefan Beller <stefanbeller@googlemail.com> writes:

> On 07/23/2013 08:32 PM, Junio C Hamano wrote:
>> Interesting. This is ancient and dates back to 7612a1ef (git-rm:
>> honor -n flag., 2006-06-08).
> 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.

That is why I blamed the bug to 7612a1ef.  Before that, without pathspec,
directory traversal function were told not to report which ones were
seen and which ones were not by passing seen=NULL.