threads / patch / 18262

patchGive error when no remote is configured

Subject: [PATCH] Give error when no remote is configured

## tl;dr

7 messages between Mar 11, 2009 and Mar 17, 2009. Diffs are folded; open one to read it.

replies: 6people: 5as markdown or json

Daniel Barkalow· Mar 11, 2009, 05:47 UTC · lore

When there's no explicitly-named remote, we use the remote specified for the current branch, which in turn defaults to "origin". But it this case should require the remote to actually be configured, and not fall back to the path "origin".

Possibly, the config file's "remote = something" should require the something to be a configured remote instead of a bare repository URL, but we actually test with a bare repository URL.

In fetch, we were giving the sensible error message when coming up with a URL failed, but this wasn't actually reachable, so move that error up and use it when appropriate.

In push, we need a new error message, because the old one (formerly unreachable without a lot of help) used the repo name, which was NULL.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
I think the main way to reach this in actual usage would be to use git-svn 
to create a repository and then forget that you used it and therefore 
don't have an origin.
 builtin-fetch.c |    6 +++---
 builtin-push.c  |    7 +++++--
 remote.c        |   30 +++++++++++++++++++++++++++---
 3 files changed, 35 insertions(+), 8 deletions(-)
Show changes to 3 files +35 −8

builtin-fetch.c, builtin-push.c, remote.c

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 1e4a3d9..7fb35fc 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -636,6 +636,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	else
 		remote = remote_get(argv[0]);
 
+	if (!remote)
+		die("Where do you want to fetch from today?");
+
 	transport = transport_get(remote, remote->url[0]);
 	if (verbosity >= 2)
 		transport->verbose = 1;
@@ -648,9 +651,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	if (depth)
 		set_option(TRANS_OPT_DEPTH, depth);
 
-	if (!transport->url)
-		die("Where do you want to fetch from today?");
-
 	if (argc > 1) {
 		int j = 0;
 		refs = xcalloc(argc + 1, sizeof(const char *));
diff --git a/builtin-push.c b/builtin-push.c
index 122fdcf..ca36fb1 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -53,8 +53,11 @@ static int do_push(const char *repo, int flags)
 	int i, errs;
 	struct remote *remote = remote_get(repo);
 
-	if (!remote)
-		die("bad repository '%s'", repo);
+	if (!remote) {
+		if (repo)
+			die("bad repository '%s'", repo);
+		die("No destination configured to push to.");
+	}
 
 	if (remote->mirror)
 		flags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);
diff --git a/remote.c b/remote.c
index d7079c6..199830e 100644
--- a/remote.c
+++ b/remote.c
@@ -38,6 +38,7 @@ static int branches_nr;
 
 static struct branch *current_branch;
 static const char *default_remote_name;
+static int explicit_default_remote_name;
 
 static struct rewrite **rewrite;
 static int rewrite_alloc;
@@ -104,6 +105,16 @@ static void add_url_alias(struct remote *remote, const char *url)
 	add_url(remote, alias_url(url));
 }
 
+static struct remote *get_remote_by_name(const char *name)
+{
+	int i;
+	for (i = 0; i < remotes_nr; i++) {
+		if (!strcmp(name, remotes[i]->name))
+			return remotes[i];
+	}
+	return NULL;
+}
+
 static struct remote *make_remote(const char *name, int len)
 {
 	struct remote *ret;
@@ -330,8 +341,10 @@ static int handle_config(const char *key, const char *value, void *cb)
 			if (!value)
 				return config_error_nonbool(key);
 			branch->remote_name = xstrdup(value);
-			if (branch == current_branch)
+			if (branch == current_branch) {
 				default_remote_name = branch->remote_name;
+				explicit_default_remote_name = 1;
+			}
 		} else if (!strcmp(subkey, ".merge")) {
 			if (!value)
 				return config_error_nonbool(key);
@@ -643,11 +656,22 @@ static int valid_remote_nick(const char *name)
 struct remote *remote_get(const char *name)
 {
 	struct remote *ret;
+	int name_given = 0;
 
 	read_config();
-	if (!name)
+	if (name)
+		name_given = 1;
+	else {
 		name = default_remote_name;
-	ret = make_remote(name, 0);
+		name_given = explicit_default_remote_name;
+	}
+	if (name_given)
+		ret = make_remote(name, 0);
+	else {
+		ret = get_remote_by_name(name);
+		if (!ret)
+			return NULL;
+	}
 	if (valid_remote_nick(name)) {
 		if (!ret->url)
 			read_remotes_file(ret);
-- 
1.6.2.104.g7aeb2.dirty
Bernie Innocenti· Mar 11, 2009, 06:19 UTC · re: Daniel Barkalow · lore

Re: [PATCH] Give error when no remote is configured

Daniel Barkalow wrote:
Show 5 quoted lines
> When there's no explicitly-named remote, we use the remote specified
> for the current branch, which in turn defaults to "origin". But it
> this case should require the remote to actually be configured, and not
> fall back to the path "origin".
> [...]
Thanks, works like a charm!
-- 
   // Bernie Innocenti - http://www.codewiz.org/
 \X/  Sugar Labs       - http://www.sugarlabs.org/
Junio C Hamano· Mar 16, 2009, 07:12 UTC · re: Daniel Barkalow · lore

Re: [PATCH] Give error when no remote is configured

Daniel Barkalow <barkalow@iabervon.org> writes:
> When there's no explicitly-named remote, we use the remote specified
> for the current branch, which in turn defaults to "origin". But it
> this case should require the remote to actually be configured, and not
> fall back to the path "origin".
This is seriously broken.
Show 22 quoted lines
> @@ -643,11 +656,22 @@ static int valid_remote_nick(const char *name)
>  struct remote *remote_get(const char *name)
>  {
>  	struct remote *ret;
> +	int name_given = 0;
>  
>  	read_config();
> -	if (!name)
> +	if (name)
> +		name_given = 1;
> +	else {
>  		name = default_remote_name;
> -	ret = make_remote(name, 0);
> +		name_given = explicit_default_remote_name;
> +	}
> +	if (name_given)
> +		ret = make_remote(name, 0);
> +	else {
> +		ret = get_remote_by_name(name);
> +		if (!ret)
> +			return NULL;
> +	}

When you do not have any config entry to name your remotes but have been using .git/remotes/origin happily, you may have read config already at this point, but when you call get_remote_by_name() you haven't read anything from .git/remotes/* (nor .git/branches/* for that matter). The caller will get NULL in such a case. This happens for both fetch and push.

Because you did not have any test to protect whatever you wanted to "fix" with your patch, I have no way knowing if I am breaking something else you wanted to do with your patch, but the patch below at least fixes the regression for me when running "git pull" in a repository I initialized long time ago that does not use the .git/config file to specify where my remote repositories are.

It applies on top of fa685bd (Give error when no remote is configured, 2009-03-11)

-- >8 --
Subject: Remove total confusion from git-fetch and git-push

The config file is not the only place remotes are defined, and without consulting .git/remotes and .git/branches, you won't know if "origin" is configured by the user. Don't give up too early and insult the user with a wisecrack "Where do you want to fetch from today?"

Insulting is ok, but I personally get really pissed off if a tool is both confused and insulting. At least be _correct_ and insulting.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote.c |   21 ++++-----------------
 1 files changed, 4 insertions(+), 17 deletions(-)
Show changes to remote.c +4 −17
diff --git a/remote.c b/remote.c
index 199830e..9f07dbc 100644
--- a/remote.c
+++ b/remote.c
@@ -105,16 +105,6 @@ static void add_url_alias(struct remote *remote, const char *url)
 	add_url(remote, alias_url(url));
 }
 
-static struct remote *get_remote_by_name(const char *name)
-{
-	int i;
-	for (i = 0; i < remotes_nr; i++) {
-		if (!strcmp(name, remotes[i]->name))
-			return remotes[i];
-	}
-	return NULL;
-}
-
 static struct remote *make_remote(const char *name, int len)
 {
 	struct remote *ret;
@@ -665,19 +655,16 @@ struct remote *remote_get(const char *name)
 		name = default_remote_name;
 		name_given = explicit_default_remote_name;
 	}
-	if (name_given)
-		ret = make_remote(name, 0);
-	else {
-		ret = get_remote_by_name(name);
-		if (!ret)
-			return NULL;
-	}
+
+	ret = make_remote(name, 0);
 	if (valid_remote_nick(name)) {
 		if (!ret->url)
 			read_remotes_file(ret);
 		if (!ret->url)
 			read_branches_file(ret);
 	}
+	if (!name_given && !ret->url)
+		return NULL;
 	if (!ret->url)
 		add_url_alias(ret, name);
 	if (!ret->url)
Daniel Barkalow· Mar 16, 2009, 16:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] Give error when no remote is configured

On Mon, 16 Mar 2009, Junio C Hamano wrote:
Show 38 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > When there's no explicitly-named remote, we use the remote specified
> > for the current branch, which in turn defaults to "origin". But it
> > this case should require the remote to actually be configured, and not
> > fall back to the path "origin".
> 
> This is seriously broken.
> 
> > @@ -643,11 +656,22 @@ static int valid_remote_nick(const char *name)
> >  struct remote *remote_get(const char *name)
> >  {
> >  	struct remote *ret;
> > +	int name_given = 0;
> >  
> >  	read_config();
> > -	if (!name)
> > +	if (name)
> > +		name_given = 1;
> > +	else {
> >  		name = default_remote_name;
> > -	ret = make_remote(name, 0);
> > +		name_given = explicit_default_remote_name;
> > +	}
> > +	if (name_given)
> > +		ret = make_remote(name, 0);
> > +	else {
> > +		ret = get_remote_by_name(name);
> > +		if (!ret)
> > +			return NULL;
> > +	}
> 
> When you do not have any config entry to name your remotes but have been
> using .git/remotes/origin happily, you may have read config already at
> this point, but when you call get_remote_by_name() you haven't read
> anything from .git/remotes/* (nor .git/branches/* for that matter).  The
> caller will get NULL in such a case.  This happens for both fetch and
> push.

That's actually a simple bug; the block that's just after what you quoted should be just before it. I thought we had a test for having "origin" defined by one of the old methods, but I guess not. Your version is better, though; I'd forgotten that using the name as the URL was in remote_get() and not make_remote().

> Because you did not have any test to protect whatever you wanted to "fix"
> with your patch, I have no way knowing if I am breaking something else you
> wanted to do with your patch,

$ git init $ git fetch

Shouldn't try fetching from ./origin/.git; I suppose the best test would be to do something like:

$ mkdir origin $ (cd origin; git init; touch a; git add a; git commit -m "initial") $ git init $ git fetch

With test_must_fail. (But I'm more going for having it not give weird errors in an error situation, which is kind of fluffy to try to test.)

Show 15 quoted lines
> but the patch below at least fixes the
> regression for me when running "git pull" in a repository I initialized
> long time ago that does not use the .git/config file to specify where my
> remote repositories are.
> 
> It applies on top of fa685bd (Give error when no remote is configured,
> 2009-03-11)
> 
> -- >8 --
> Subject: Remove total confusion from git-fetch and git-push
> 
> The config file is not the only place remotes are defined, and without
> consulting .git/remotes and .git/branches, you won't know if "origin" is
> configured by the user.  Don't give up too early and insult the user with
> a wisecrack "Where do you want to fetch from today?"

You actually wrote that message, in 853a3697. I think a better message would probably be something like:

No default remote is configured for your current branch, and the default remote "origin" is not configured either.

I think the message missed being made user-friendly in earlier passes due to being inaccessible at the time.

Show 54 quoted lines
> Insulting is ok, but I personally get really pissed off if a tool is both
> confused and insulting.  At least be _correct_ and insulting.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  remote.c |   21 ++++-----------------
>  1 files changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/remote.c b/remote.c
> index 199830e..9f07dbc 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -105,16 +105,6 @@ static void add_url_alias(struct remote *remote, const char *url)
>  	add_url(remote, alias_url(url));
>  }
>  
> -static struct remote *get_remote_by_name(const char *name)
> -{
> -	int i;
> -	for (i = 0; i < remotes_nr; i++) {
> -		if (!strcmp(name, remotes[i]->name))
> -			return remotes[i];
> -	}
> -	return NULL;
> -}
> -
>  static struct remote *make_remote(const char *name, int len)
>  {
>  	struct remote *ret;
> @@ -665,19 +655,16 @@ struct remote *remote_get(const char *name)
>  		name = default_remote_name;
>  		name_given = explicit_default_remote_name;
>  	}
> -	if (name_given)
> -		ret = make_remote(name, 0);
> -	else {
> -		ret = get_remote_by_name(name);
> -		if (!ret)
> -			return NULL;
> -	}
> +
> +	ret = make_remote(name, 0);
>  	if (valid_remote_nick(name)) {
>  		if (!ret->url)
>  			read_remotes_file(ret);
>  		if (!ret->url)
>  			read_branches_file(ret);
>  	}
> +	if (!name_given && !ret->url)
> +		return NULL;
>  	if (!ret->url)
>  		add_url_alias(ret, name);
>  	if (!ret->url)
> 
Jay Soffian· Mar 16, 2009, 20:01 UTC · re: Daniel Barkalow · lore

Re: [PATCH] Give error when no remote is configured

On Mon, Mar 16, 2009 at 12:55 PM, Daniel Barkalow <barkalow@iabervon.org> wrote:
> No default remote is configured for your current branch, and the default
> remote "origin" is not configured either.
The use of "default" twice is slightly confusing. Perhaps:

No remote is configured for the current branch, and the default remote "origin" is not configured either.

j.
Giovanni Bajo· Mar 17, 2009, 00:12 UTC · re: Jay Soffian · lore

Re: [PATCH] Give error when no remote is configured

On 3/16/2009 9:01 PM, Jay Soffian wrote:
Show 8 quoted lines
> On Mon, Mar 16, 2009 at 12:55 PM, Daniel Barkalow <barkalow@iabervon.org> wrote:
>> No default remote is configured for your current branch, and the default
>> remote "origin" is not configured either.
> 
> The use of "default" twice is slightly confusing. Perhaps:
> 
> No remote is configured for the current branch, and the default
> remote "origin" is not configured either.

I'm a total newbie with git. I must say that the above sentence means absolutely nothing to me (in either version) because of the confusing usage of the word "remote" (twice, one as a substantive, one as an adjective) and the word "origin" which is git jargon which I don't master yet.

My suggestion is that you should at least add a sentence that points to a likely solution. Something like:

   (use "git remote add" to configure a remote URL)

Note that I don't have any clue if this sentece is correct and/or is the correct solution. The above is just an example of a helpful error message.

-- 
Giovanni Bajo
Develer S.r.l.
http://www.develer.com
Daniel Barkalow· Mar 17, 2009, 16:06 UTC · re: Giovanni Bajo · lore

Re: [PATCH] Give error when no remote is configured

On Tue, 17 Mar 2009, Giovanni Bajo wrote:
Show 15 quoted lines
> On 3/16/2009 9:01 PM, Jay Soffian wrote:
> > On Mon, Mar 16, 2009 at 12:55 PM, Daniel Barkalow <barkalow@iabervon.org>
> > wrote:
> > > No default remote is configured for your current branch, and the default
> > > remote "origin" is not configured either.
> > 
> > The use of "default" twice is slightly confusing. Perhaps:
> > 
> > No remote is configured for the current branch, and the default
> > remote "origin" is not configured either.
> 
> I'm a total newbie with git. I must say that the above sentence means
> absolutely nothing to me (in either version) because of the confusing usage of
> the word "remote" (twice, one as a substantive, one as an adjective) and the
> word "origin" which is git jargon which I don't master yet.

Actually, it's not that complicated a sentence. In order to fetch/pull/push, you need to specify some attributes of the other side; at a minimum, this is the location, but you can also specify other things (HTTP gateways if you're using HTTP, for example). You can have multiple of these sets of configuration stored under different names; these are remotes.

When you run fetch, you can specify the remote on the command line. If you don't specify, there are two levels of defaults: the first is a setting in the configuration for whichever branch is current; the second is the constant "origin".

The message is trying to say that it fell back to looking for a configured remote named "origin", but there wasn't one configured.

Show 7 quoted lines
> My suggestion is that you should at least add a sentence that points to a
> likely solution. Something like:
> 
>   (use "git remote add" to configure a remote URL)
> 
> Note that I don't have any clue if this sentece is correct and/or is the
> correct solution. The above is just an example of a helpful error message.

The problem is that there are a number of different sorts of configuration you might want, and we have no way of knowing which it is.

You might mean to explicitly specify URLs every time to fetch or push in this repository; you might mean to have a different URL for each branch you work on; or you might mean to have a general default. It's also possible that you actually meant "git svn fetch", which uses different configuration info.

So I think we can only give pointers to the various things it looked for, and leave it up to the documentation to explain which you want, if any, and how to get it.

	-Daniel
*This .sig left intentionally blank*

← back to recent threads