Re: [RFC 03/11] Allow programs to not depend on remotes having urls

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Jul 27, 2009, 19:33 UTC
Message-ID: <alpine.LNX.2.00.0907271501300.2147@iabervon.org>
In-Reply-To: <7v1vo2j6qo.fsf@alter.siamese.dyndns.org>

On Mon, 27 Jul 2009, Junio C Hamano wrote:

Show 19 quoted lines

> Johan Herland <johan@herland.net> writes:
> 
> > diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
> > index 78a88f7..4c6fc58 100644
> > --- a/builtin-ls-remote.c
> > +++ b/builtin-ls-remote.c
> > @@ -87,9 +87,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> >  		}
> >  	}
> >  	remote = nongit ? NULL : remote_get(dest);
> > -	if (remote && !remote->url_nr)
> > +	if (!nongit && !remote)
> >  		die("remote %s has no configured URL", dest);
> 
> It appears to me that remote_get() calls make_remote() that never returns
> NULL, thus this check will never trigger.  At least, the die() message
> needs to be updated, since remote_get() may return NULL for some reason
> other than "no configured URL", but the condition the message says is not
> what this code checks.

The previous patch in the series causes remote_get() to return NULL if the remote configuration is invalid. You're right, however, that the message should be something more directly accurate.

Show 5 quoted lines

> It is Ok if the new world order is that the URL does not have to be known
> before transport_get() is called, but if that is the case, it should be
> made clear by removing this check from here (and possibly having a new
> check elsewhere where it really matters, e.g. somewhere in transport
> code).

This spot is trying to make sure that the remote was valid, according to remote.c

Show 9 quoted lines

> > -	transport = transport_get(remote, remote ? remote->url[0] : dest);
> > +	transport = transport_get(remote, remote ? NULL : dest);
> >  	if (uploadpack != NULL)
> >  		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>