[PATCH] branch: make sure the upstream remote is configured

[PATCH] branch: make sure the upstream remote is configured

[Carlos Martín Nieto]

A command of e.g.

    git push --set-upstream /tmp/t master

will call install_branch_config() with a remote name of "/tmp/t". This
function will set the 'branch.master.remote' key to, which is
nonsensical as there is no remote by that name.

Instead, make sure that the remote given does exist when writing the
configuration and warn if it does not. In order to distinguish named
remotes, introduce REMOTE_NONE as the default origin for remotes,
which the functions reading from the different sources will
overwrite. Thus, an origin of REMOTE_NONE means it has been created at
run-time in order to push to it.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

It's somewhat surprising that there didn't seem to be a way to
distinguish named remotes from those created from a command-line path,
but I guess nobody needed to.

 branch.c                 | 11 +++++++++++
 remote.h                 |  1 +
 t/t5523-push-upstream.sh |  5 +++++
 3 files changed, 17 insertions(+)

diff --git a/branch.c b/branch.c
index c5c6984..cefb8f6 100644
--- a/branch.c
+++ b/branch.c
@@ -53,6 +53,7 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	int remote_is_branch = !prefixcmp(remote, "refs/heads/");
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
+	struct remote *r = remote_get(origin);
 
 	if (remote_is_branch
 	    && !strcmp(local, shortname)
@@ -62,6 +63,16 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 		return;
 	}
 
+	/*
+	 * Make sure that the remote passed is a configured remote, or
+	 * we end up setting 'branch.foo.remote = /tmp/t' which is
+	 * nonsensical.
+	 */
+	if (origin && strcmp(origin, ".") && r->origin == REMOTE_NONE) {
+		warning(_("there is no remote named '%s', no upstream configuration will be set."), origin);
+		return;
+	}
+
 	strbuf_addf(&key, "branch.%s.remote", local);
 	git_config_set(key.buf, origin ? origin : ".");
 
diff --git a/remote.h b/remote.h
index cf56724..92f6e33 100644
--- a/remote.h
+++ b/remote.h
@@ -2,6 +2,7 @@
 #define REMOTE_H
 
 enum {
+	REMOTE_NONE,
 	REMOTE_CONFIG,
 	REMOTE_REMOTES,
 	REMOTE_BRANCHES
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 3683df1..e84c2f8 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -71,6 +71,11 @@ test_expect_success 'push -u HEAD' '
 	check_config headbranch upstream refs/heads/headbranch
 '
 
+test_expect_success 'push -u <url>' '
+        git push -u parent HEAD 2>err &&
+        grep "no upstream configuration will be set" err
+'
+
 test_expect_success TTY 'progress messages go to tty' '
 	ensure_fresh_upstream &&
 
-- 
1.8.3

Re: [PATCH] branch: make sure the upstream remote is configured

[Jeff King]

On Fri, Jul 26, 2013 at 07:39:37PM +0200, Carlos Martín Nieto wrote:

> A command of e.g.
> 
>     git push --set-upstream /tmp/t master
> 
> will call install_branch_config() with a remote name of "/tmp/t". This
> function will set the 'branch.master.remote' key to, which is
> nonsensical as there is no remote by that name.

Is it nonsensical? It does not make sense for the @{upstream} magic
token, because we will not have a branch in tracking branch refs/remotes
to point to. But the configuration would still affect how "git pull"
chooses a branch to fetch and merge.

I.e., you can currently do:

  git push --set-upstream /tmp/t master
  git pull ;# pulls from /tmp/t master

-Peff

Re: [PATCH] branch: make sure the upstream remote is configured

[Jeff King]

On Fri, Jul 26, 2013 at 02:43:11PM -0400, Jeff King wrote:

> On Fri, Jul 26, 2013 at 07:39:37PM +0200, Carlos Martín Nieto wrote:
> 
> > A command of e.g.
> > 
> >     git push --set-upstream /tmp/t master
> > 
> > will call install_branch_config() with a remote name of "/tmp/t". This
> > function will set the 'branch.master.remote' key to, which is
> > nonsensical as there is no remote by that name.
> 
> Is it nonsensical? It does not make sense for the @{upstream} magic
> token, because we will not have a branch in tracking branch refs/remotes

Eh, I am incapable of typing (and proofreading). That should be "not
have a tracking branch in refs/remotes".

-Peff

Re: [PATCH] branch: make sure the upstream remote is configured

[Carlos Martín Nieto]

On Fri, 2013-07-26 at 14:43 -0400, Jeff King wrote:
> On Fri, Jul 26, 2013 at 07:39:37PM +0200, Carlos Martín Nieto wrote:
> 
> > A command of e.g.
> > 
> >     git push --set-upstream /tmp/t master
> > 
> > will call install_branch_config() with a remote name of "/tmp/t". This
> > function will set the 'branch.master.remote' key to, which is
> > nonsensical as there is no remote by that name.
> 
> Is it nonsensical? It does not make sense for the @{upstream} magic
> token, because we will not have a branch in tracking branch refs/remotes

This was the main point, yes; the only time I've seen it used is by
mistake/misunderstanding, and thinking that you wouldn't want to do
something like what's below.

You are also unable to do this kind of thing through git-branch, and as
it seemed to be an oversight, I wanted to tighten it up.

> to point to. But the configuration would still affect how "git pull"
> chooses a branch to fetch and merge.
> 
> I.e., you can currently do:
> 
>   git push --set-upstream /tmp/t master
>   git pull ;# pulls from /tmp/t master

Interestingly, this actually fetches the right branch from the remote. I
wasn't expecting something like this to work at all.

Somewhat doubtful that this usage is something you'd really want to do,
I see that it does behave properly.

   cmn

Re: [PATCH] branch: make sure the upstream remote is configured

[Jeff King]

On Sat, Jul 27, 2013 at 12:29:47AM +0200, Carlos Martín Nieto wrote:

> > Is it nonsensical? It does not make sense for the @{upstream} magic
> > token, because we will not have a branch in tracking branch refs/remotes
> 
> This was the main point, yes; the only time I've seen it used is by
> mistake/misunderstanding, and thinking that you wouldn't want to do
> something like what's below.

If that is what you want to prevent, I do not think checking for a named
remote is sufficient. You can also be pushing to a branch on a named
remote that is not part of your fetch refspec, in which case you do not
have a tracking branch. I.e.:

  git clone $URL repo.git
  cd repo.git
  git push --set-upstream HEAD:refs/foo/whatever

For that matter, I wonder what "--set-upstream" would do if used with
"refs/tags/foo". You would not do that in general, but what about:

  git push --set-upstream master:master master:v1.0

I didn't test.

> > to point to. But the configuration would still affect how "git pull"
> > chooses a branch to fetch and merge.
> > 
> > I.e., you can currently do:
> > 
> >   git push --set-upstream /tmp/t master
> >   git pull ;# pulls from /tmp/t master
> 
> Interestingly, this actually fetches the right branch from the remote. I
> wasn't expecting something like this to work at all.
> 
> Somewhat doubtful that this usage is something you'd really want to do,
> I see that it does behave properly.

I do not claim to have used it myself. Tightening the "--set-upstream"
behavior would not hurt people who want to configure such a thing
manually, and it might catch errors from people doing it accidentally.

So even though the config it generates is not nonsensical, there is a
reasonable chance it was an error, and tightening may make sense. But I
think you would not want the condition to be "this is a named remote",
but rather "the generated configuration actually has an @{upstream}".

-Peff

Re: [PATCH] branch: make sure the upstream remote is configured

[Jeff King]

On Fri, Jul 26, 2013 at 07:12:11PM -0400, Jeff King wrote:

> If that is what you want to prevent, I do not think checking for a named
> remote is sufficient. You can also be pushing to a branch on a named
> remote that is not part of your fetch refspec, in which case you do not
> have a tracking branch. I.e.:
> 
>   git clone $URL repo.git
>   cd repo.git
>   git push --set-upstream HEAD:refs/foo/whatever
> 
> For that matter, I wonder what "--set-upstream" would do if used with
> "refs/tags/foo". You would not do that in general, but what about:
> 
>   git push --set-upstream master:master master:v1.0
> 
> I didn't test.

Ah, nevermind. We already catch the case of non-heads (on both the local
and remote sides) and abort.

So that makes me more confident that your change is a reasonable one; we
are already disallowing a subset of what's possible via "--set-upstream"
in the name of preventing weird accidental configurations. This is just
fixing another such loophole.

-Peff