[PATCH] Give error when no remote is configured

[PATCH] Give error when no remote is configured

[Daniel Barkalow]

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

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

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

[Bernie Innocenti]

Daniel Barkalow wrote:
> 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/

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

[Junio C Hamano]

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.

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

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)

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

[Daniel Barkalow]

On Mon, 16 Mar 2009, Junio C Hamano wrote:

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

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

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

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

[Jay Soffian]

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.

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

[Giovanni Bajo]

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.

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

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

[Daniel Barkalow]

On Tue, 17 Mar 2009, Giovanni Bajo wrote:

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

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