threads / patch / 26624

patch, 2 partspush: better error message when push.default = tracking

Subject: [PATCH 1/2] push: better error message when push.default = tracking

## tl;dr

12 messages between Feb 28, 2011 and Mar 3, 2011. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Matthieu Moy· Feb 28, 2011, 21:14 UTC · lore

[PATCH 0/2] push: better error messages

Since it's likely to become the default in 1.8.0, I've played a bit with "push.default = tracking". I was very happy to see that the flow

git init bla git clone bla git commit git push

was working like a charm (it required a --all or so the first time), but the "create a new branch and push it" is still a bit hard to get for newbies.

[PATCH 1/2] should solve it, by providing a cut-and-paste ready command to do the push.

While I was there, I also improved a bit other messages which gives [PATCH 2/2]. I've kept the changes in separate patches, but they may be squashed once reviewed.

Matthieu Moy (2):
  push: better error message when push.default = tracking
  push: better error messages for detached HEAD and "no destination"
 builtin/push.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)
-- 
1.7.4.1.142.g43604.dirty
Matthieu Moy· Feb 28, 2011, 21:14 UTC · re: Matthieu Moy · lore

A common scenario is to create a new branch and push it (checkout -b && push [--set-upstream]). In this case, the user was getting "The current branch %s has no upstream branch.", which doesn't help much.

Provide the user a command to push the current branch. To avoid the situation in the future, suggest --set-upstream.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 builtin/push.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)
Show changes to builtin/push.c +10 −5
diff --git a/builtin/push.c b/builtin/push.c
index 31da418..210eb36 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -64,14 +64,19 @@ static void set_refspecs(const char **refs, int nr)
 	}
 }
 
-static void setup_push_upstream(void)
+static void setup_push_upstream(struct remote *remote)
 {
 	struct strbuf refspec = STRBUF_INIT;
 	struct branch *branch = branch_get(NULL);
 	if (!branch)
 		die("You are not currently on a branch.");
 	if (!branch->merge_nr || !branch->merge)
-		die("The current branch %s has no upstream branch.",
+		die("The current branch %s has no upstream branch.\n"
+		    "To push the current branch and set the remote as upstream, use\n"
+		    "\n"
+		    "    git push --set-upstream %s %s\n",
+		    branch->name,
+		    remote->name,
 		    branch->name);
 	if (branch->merge_nr != 1)
 		die("The current branch %s has multiple upstream branches, "
@@ -80,7 +85,7 @@ static void setup_push_upstream(void)
 	add_refspec(refspec.buf);
 }
 
-static void setup_default_push_refspecs(void)
+static void setup_default_push_refspecs(struct remote *remote)
 {
 	switch (push_default) {
 	default:
@@ -89,7 +94,7 @@ static void setup_default_push_refspecs(void)
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream();
+		setup_push_upstream(remote);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
@@ -175,7 +180,7 @@ static int do_push(const char *repo, int flags)
 			refspec = remote->push_refspec;
 			refspec_nr = remote->push_refspec_nr;
 		} else if (!(flags & TRANSPORT_PUSH_MIRROR))
-			setup_default_push_refspecs();
+			setup_default_push_refspecs(remote);
 	}
 	errs = 0;
 	if (remote->pushurl_nr) {

-- 
1.7.4.1.142.g43604.dirty
Matthieu Moy· Feb 28, 2011, 21:14 UTC · re: Matthieu Moy · lore

[PATCH 2/2] push: better error messages for detached HEAD and "no destination"

In both cases, give a hint on what to do next. We mention explicitely "detached HEAD" since this is the keyword to look for in documentations.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 builtin/push.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)
Show changes to builtin/push.c +10 −2
diff --git a/builtin/push.c b/builtin/push.c
index 210eb36..48fb050 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -69,7 +69,8 @@ static void setup_push_upstream(struct remote *remote)
 	struct strbuf refspec = STRBUF_INIT;
 	struct branch *branch = branch_get(NULL);
 	if (!branch)
-		die("You are not currently on a branch.");
+		die("You are not currently on a branch (detached HEAD).\n"
+		    "Please, checkout the branch you want to push first or specify it on the command line.");
 	if (!branch->merge_nr || !branch->merge)
 		die("The current branch %s has no upstream branch.\n"
 		    "To push the current branch and set the remote as upstream, use\n"
@@ -152,7 +153,14 @@ static int do_push(const char *repo, int flags)
 	if (!remote) {
 		if (repo)
 			die("bad repository '%s'", repo);
-		die("No destination configured to push to.");
+		die("No destination configured to push to.\n"
+		    "Either specify the URL from the command line or configure a remote repository using\n"
+		    "\n"
+		    "    git remote add <name> <url>\n"
+		    "\n"
+		    "and then push using the remote name like\n"
+		    "\n"
+		    "    git push <name>\n");
 	}
 
 	if (remote->mirror)

-- 
1.7.4.1.142.g43604.dirty
Jay Soffian· Feb 28, 2011, 22:48 UTC · re: Matthieu Moy · lore

Re: [PATCH 2/2] push: better error messages for detached HEAD and "no destination"

On Mon, Feb 28, 2011 at 4:14 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
Show 10 quoted lines
> ---
>  builtin/push.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
>
> @@ -152,7 +153,14 @@ static int do_push(const char *repo, int flags)
>        if (!remote) {
>                if (repo)
>                        die("bad repository '%s'", repo);
> -               die("No destination configured to push to.");
> +               die("No destination configured to push to.\n"
The dangling preposition is awkward. How about:
  No configured push destination.
Or
  Push destination not configured.
> +                   "Either specify the URL from the command line or configure a remote repository using\n"
Maybe "Either specify the URL explicitly or configure..."
> +                   "\n"
> +                   "    git remote add <name> <url>\n"
> +                   "\n"
> +                   "and then push using the remote name like\n"
s/like//
> +                   "\n"
> +                   "    git push <name>\n");
Well, I just bikeshedded that 3 different colors, didn't I? You're welcome. :-)
j.
Matthieu Moy· Feb 28, 2011, 23:00 UTC · re: Jay Soffian · lore

Re: [PATCH 2/2] push: better error messages for detached HEAD and "no destination"

Jay Soffian <jaysoffian@gmail.com> writes:
>> +                   "Either specify the URL from the command line or configure a remote repository using\n"
>
> Maybe "Either specify the URL explicitly or configure..."

I prefer "from the command-line" to "explicitely", because "explicitely" could mean "in a configuration file", or so.

> Well, I just bikeshedded that 3 different colors, didn't I? You're welcome. :-)
Thanks, I took the 2 other colors.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Feb 28, 2011, 23:29 UTC · re: Matthieu Moy · lore

Re: [PATCH 2/2] push: better error messages for detached HEAD and "no destination"

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 19 quoted lines
> In both cases, give a hint on what to do next. We mention explicitely
> "detached HEAD" since this is the keyword to look for in documentations.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>  builtin/push.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 210eb36..48fb050 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -69,7 +69,8 @@ static void setup_push_upstream(struct remote *remote)
>  	struct strbuf refspec = STRBUF_INIT;
>  	struct branch *branch = branch_get(NULL);
>  	if (!branch)
> -		die("You are not currently on a branch.");
> +		die("You are not currently on a branch (detached HEAD).\n"
> +		    "Please, checkout the branch you want to push first or specify it on the command line.");

I agree that it is a good thing to suggest specifying what to push on the command line, but I don't think this "checkout first" is particularly a good advice, as it requires two-step operation that is quite a roundabout way of doing what the user wanted to do: a very specific checkout followed by re-running of a nonspecific push.

Show 14 quoted lines
> @@ -152,7 +153,14 @@ static int do_push(const char *repo, int flags)
>  	if (!remote) {
>  		if (repo)
>  			die("bad repository '%s'", repo);
> -		die("No destination configured to push to.");
> +		die("No destination configured to push to.\n"
> +		    "Either specify the URL from the command line or configure a remote repository using\n"
> +		    "\n"
> +		    "    git remote add <name> <url>\n"
> +		    "\n"
> +		    "and then push using the remote name like\n"
> +		    "\n"
> +		    "    git push <name>\n");
>  	}
This one would be helpful.
Matthieu Moy· Mar 1, 2011, 09:11 UTC · re: Matthieu Moy · lore

[PATCH 1/2 v2] push: better error messages when push.default = tracking

A common scenario is to create a new branch and push it (checkout -b && push [--set-upstream]). In this case, the user was getting "The current branch %s has no upstream branch.", which doesn't help much.

Provide the user a command to push the current branch. To avoid the situation in the future, suggest --set-upstream.

While we're there, also improve the error message in the "detached HEAD" case. We mention explicitly "detached HEAD" since this is the keyword to look for in documentations.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Compared to v1, the detached HEAD case joined this patch since it
became semantically linked (requires the "remote" extra parameter),
and doesn't mention the possibility to checkout another branch before
pushing it (cf. Junio).
 builtin/push.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)
Show changes to builtin/push.c +15 −6
diff --git a/builtin/push.c b/builtin/push.c
index 31da418..c949187 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -64,14 +64,23 @@ static void set_refspecs(const char **refs, int nr)
 	}
 }
 
-static void setup_push_upstream(void)
+static void setup_push_upstream(struct remote *remote)
 {
 	struct strbuf refspec = STRBUF_INIT;
 	struct branch *branch = branch_get(NULL);
 	if (!branch)
-		die("You are not currently on a branch.");
+		die("You are not currently on a branch (detached HEAD).\n"
+		    "To push a specific branch and set the remote as upstream, use\n"
+		    "\n"
+		    "    git push --set-upstream %s <branch-name>\n",
+		    remote->name);
 	if (!branch->merge_nr || !branch->merge)
-		die("The current branch %s has no upstream branch.",
+		die("The current branch %s has no upstream branch.\n"
+		    "To push the current branch and set the remote as upstream, use\n"
+		    "\n"
+		    "    git push --set-upstream %s %s\n",
+		    branch->name,
+		    remote->name,
 		    branch->name);
 	if (branch->merge_nr != 1)
 		die("The current branch %s has multiple upstream branches, "
@@ -80,7 +89,7 @@ static void setup_push_upstream(void)
 	add_refspec(refspec.buf);
 }
 
-static void setup_default_push_refspecs(void)
+static void setup_default_push_refspecs(struct remote *remote)
 {
 	switch (push_default) {
 	default:
@@ -89,7 +98,7 @@ static void setup_default_push_refspecs(void)
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream();
+		setup_push_upstream(remote);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
@@ -175,7 +184,7 @@ static int do_push(const char *repo, int flags)
 			refspec = remote->push_refspec;
 			refspec_nr = remote->push_refspec_nr;
 		} else if (!(flags & TRANSPORT_PUSH_MIRROR))
-			setup_default_push_refspecs();
+			setup_default_push_refspecs(remote);
 	}
 	errs = 0;
 	if (remote->pushurl_nr) {

-- 
1.7.4.1.142.ge03e1
Matthieu Moy· Mar 1, 2011, 09:11 UTC · re: Matthieu Moy · lore

[PATCH 2/2 v2] push: better error message when no remote configured

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
"No configured push destination." (Jay Soffian) and reword.
 builtin/push.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
Show changes to builtin/push.c +8 −1
diff --git a/builtin/push.c b/builtin/push.c
index c949187..b4fba7a 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -156,7 +156,14 @@ static int do_push(const char *repo, int flags)
 	if (!remote) {
 		if (repo)
 			die("bad repository '%s'", repo);
-		die("No destination configured to push to.");
+		die("No configured push destination.\n"
+		    "Either specify the URL from the command-line or configure a remote repository using\n"
+		    "\n"
+		    "    git remote add <name> <url>\n"
+		    "\n"
+		    "and then push using the remote name\n"
+		    "\n"
+		    "    git push <name>\n");
 	}
 
 	if (remote->mirror)

-- 
1.7.4.1.142.ge03e1
Junio C Hamano· Mar 1, 2011, 18:34 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/2 v2] push: better error messages when push.default = tracking

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 20 quoted lines
> diff --git a/builtin/push.c b/builtin/push.c
> index 31da418..c949187 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -64,14 +64,23 @@ static void set_refspecs(const char **refs, int nr)
>  	}
>  }
>  
> -static void setup_push_upstream(void)
> +static void setup_push_upstream(struct remote *remote)
>  {
>  	struct strbuf refspec = STRBUF_INIT;
>  	struct branch *branch = branch_get(NULL);
>  	if (!branch)
> -		die("You are not currently on a branch.");
> +		die("You are not currently on a branch (detached HEAD).\n"
> +		    "To push a specific branch and set the remote as upstream, use\n"
> +		    "\n"
> +		    "    git push --set-upstream %s <branch-name>\n",
> +		    remote->name);

For all the other cases covered in this patch, the sequence that lead to this situation would be like this:

	git checkout somebranch
        hack hack hack including commits
        git push

and it is very clear that the user wants to push the current branch to the corresponding place but the user is getting an error because there is no "corresponding place" mapping established yet.

I agree "push --set-upstream" is a very good advice to give under that scenario---it would push the history s/he wanted to push right now, while establishing the mapping for later use, both at the same time with a single command.

However, I don't think that applies to this case with detached HEAD; it is more likely that the user came here this way:

	git checkout somebranch~4 ;# the tip 3 are not quite ready
        hack hack quickfix to do only the sure part of the tip 3 did
        commit and test
        git push

Maybe the user needed to quickly push out a minimum fix out of the more elaborate work, in which case, what would follow in the workflow is first to:

	git push origin HEAD:somebranch

in order to unblock others. This will then be followed by a more leisurely:

	git rebase HEAD somebranch
to get back to the more elaborate work that is not yet presentable.
It is not likely that the end user wanted to:
	git checkout $not_a_branch_tip ;# detached
        hack hack hack including commits
        git push origin an_unrelated_branch

and wanted to omit "where and what" part. We are talking about "push the current branch only to corresponding destination" people, so if that unrelated branch were already ready for external consumption, they would have already pushed it out at the end of the session when they were on that branch (and seen the other advice you are adding in this patch).

That is why I suggested to advice an explicit push, without checkout nor set upstream, in my original review message. IOW, I think the message should instead suggest:

	If you want to push the history leading to the current (detached)
	state now, use
	    git push $remote HEAD:the-branch-you-want-to-push-to
Matthieu Moy· Mar 2, 2011, 20:12 UTC · re: Junio C Hamano · lore

[PATCH 1/2 v3] push: better error messages when push.default = tracking

A common scenario is to create a new branch and push it (checkout -b && push [--set-upstream]). In this case, the user was getting "The current branch %s has no upstream branch.", which doesn't help much.

Provide the user a command to push the current branch. To avoid the situation in the future, suggest --set-upstream.

While we're there, also improve the error message in the "detached HEAD" case. We mention explicitly "detached HEAD" since this is the keyword to look for in documentations.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

I applied Junio's suggestion to suggest pushing HEAD in detached HEAD state. I don't care very much either way indeed (and I didn't want to make the message too heavy, just give the user a way to do something).

hope that's OK for inclusion.
 builtin/push.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)
Show changes to builtin/push.c +16 −6
diff --git a/builtin/push.c b/builtin/push.c
index 31da418..1b493fb 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -64,14 +64,24 @@ static void set_refspecs(const char **refs, int nr)
 	}
 }
 
-static void setup_push_upstream(void)
+static void setup_push_upstream(struct remote *remote)
 {
 	struct strbuf refspec = STRBUF_INIT;
 	struct branch *branch = branch_get(NULL);
 	if (!branch)
-		die("You are not currently on a branch.");
+		die("You are not currently on a branch.\n"
+		    "To push the history leading to the current (detached HEAD)\n"
+		    "state now, use\n"
+		    "\n"
+		    "    git push %s HEAD:<name-of-remote-branch>\n",
+		    remote->name);
 	if (!branch->merge_nr || !branch->merge)
-		die("The current branch %s has no upstream branch.",
+		die("The current branch %s has no upstream branch.\n"
+		    "To push the current branch and set the remote as upstream, use\n"
+		    "\n"
+		    "    git push --set-upstream %s %s\n",
+		    branch->name,
+		    remote->name,
 		    branch->name);
 	if (branch->merge_nr != 1)
 		die("The current branch %s has multiple upstream branches, "
@@ -80,7 +90,7 @@ static void setup_push_upstream(void)
 	add_refspec(refspec.buf);
 }
 
-static void setup_default_push_refspecs(void)
+static void setup_default_push_refspecs(struct remote *remote)
 {
 	switch (push_default) {
 	default:
@@ -89,7 +99,7 @@ static void setup_default_push_refspecs(void)
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream();
+		setup_push_upstream(remote);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
@@ -175,7 +185,7 @@ static int do_push(const char *repo, int flags)
 			refspec = remote->push_refspec;
 			refspec_nr = remote->push_refspec_nr;
 		} else if (!(flags & TRANSPORT_PUSH_MIRROR))
-			setup_default_push_refspecs();
+			setup_default_push_refspecs(remote);
 	}
 	errs = 0;
 	if (remote->pushurl_nr) {

-- 
1.7.4.1.176.g6b069.dirty
Matthieu Moy· Mar 2, 2011, 20:12 UTC · re: Matthieu Moy · lore

[PATCH 2/2 v3] push: better error message when no remote configured

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
No change since v2
 builtin/push.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
Show changes to builtin/push.c +8 −1
diff --git a/builtin/push.c b/builtin/push.c
index 1b493fb..c3c2feb 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -157,7 +157,14 @@ static int do_push(const char *repo, int flags)
 	if (!remote) {
 		if (repo)
 			die("bad repository '%s'", repo);
-		die("No destination configured to push to.");
+		die("No configured push destination.\n"
+		    "Either specify the URL from the command-line or configure a remote repository using\n"
+		    "\n"
+		    "    git remote add <name> <url>\n"
+		    "\n"
+		    "and then push using the remote name\n"
+		    "\n"
+		    "    git push <name>\n");
 	}
 
 	if (remote->mirror)

-- 
1.7.4.1.176.g6b069.dirty
Junio C Hamano· Mar 3, 2011, 01:43 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/2 v3] push: better error messages when push.default = tracking

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> I applied Junio's suggestion to suggest pushing HEAD in detached HEAD
> state. I don't care very much either way indeed (and I didn't want to
> make the message too heavy, just give the user a way to do something).

If I were asked my honest opinion, I would have to say that I do not care too deeply about it either, in the sense that I won't claim that the workflow I thought was most likely to get a new user into the situation to get the message on the detached HEAD would be really the most likely one, but I wouldn't spend too much time on coming up with other scenarios myself even though I admit that there may be some other cases that the updated message is not optimal.

But one thing that I would value (and I would like to see others on the list to also value) is that every change we make is backed by solid thinking---at least I would like to see us being able to explain _why_ we thought that the message after applying this is better than the old one today. Others who will wonder why the message advises the action it advises three months down the road should be able to find out the thinking behind the change, and decide to agree or disagree with it. Without such a trace of clear and solid thinking, i.e. "even though we admit we may not have fully thought things through, at least we considered this and that when we came up with this solution", we would end up going circles, wasting everybody's time.

In any case, I've applied them and pushed the results out.  Thanks.

← back to recent threads