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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 5, 2008, 13:26 UTC
Message-ID: <alpine.DEB.1.00.0806051415000.21190@racer>
In-Reply-To: <1212670091.30293.3.camel@omicron.ep.petrobras.com.br>

Hi,

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

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 11 quoted lines

> 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