Re: [PATCH] Fix remote_get so it will return NULL when no remote is found.

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

[nice to go to sleep and see a patch in the morning]

On Thu, 5 Jun 2008, Victor Bogado da Silva Lins wrote:

> >>From 0cf45f264cf7f1b3aa3a8875109fbf4c03d56126 Mon Sep 17 00:00:00 2001
> From: Victor Bogado <victor@bogado.net>
> Date: Thu, 5 Jun 2008 09:36:41 -0300
> Subject: [PATCH] Fix remote_get so it will return NULL when no remote is
> found.

Please imitate what you see on this list, especially by the Git regulars.  
They never include these headers.

> remote_get should return NULL when there is no remote with that name, at 
> least this is what remote.c's rm() function seems to think. As this is a 
> reasonable assumption, and it seems that the function remote_get is 
> acutally trying to do this, I fixed the test so it will test if the URL 
> is equal to the name of the remote.

This is a bit vague, a bit wrong, and it has the subjective "I" in it.  
remote_get() _is_ assumed to return NULL when no remote with that name was 
found.

And the function was fixed (3rd person, passive).  At least that is how I 
remember most of the rest of git.git's commit messages.

> diff --git a/remote.c b/remote.c
> index 91e3b11..62b3611 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -598,7 +598,7 @@ struct remote *remote_get(const char *name)
>  	}
>  	if (!ret->url)
>  		add_url_alias(ret, name);
> -	if (!ret->url)
> +	if (!strcmp(*ret->url,ret->name))
>  		return NULL;

Hmm.  That sounds very dangerous, using a pointer without checking that it 
is NULL.

Besides, I think that the add_url_alias(ret, name) above is at 
fault.  Not the alias part about it, though.  This used to be an add_url() 
from the conception of remote.c, but then it was only used for fetching.

I think the proper solution is to add a parameter, or a function, to force 
NULL if the remote did not exist.

Of course, this involves a careful review of the existing callers, to find 
out which ones rely on getting a newly created remote when none was found.

In any case, thank you very much for finding that bug (the if() I pointed 
out was in remote.c from the very beginning, even then never being 
triggered).

Ciao,
Dscho