On Mon, 16 Mar 2009, Junio C Hamano wrote:
> Daniel Barkalow <barkalow@xxxxxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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)
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
