[PATCH 0/2] push: better error messages

[PATCH 0/2] push: better error messages

[Matthieu Moy]

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

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

[Matthieu Moy]

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

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

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

[Matthieu Moy]

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

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

[Jay Soffian]

On Mon, Feb 28, 2011 at 4:14 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:

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

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

[Matthieu Moy]

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/

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

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

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

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

[Matthieu Moy]

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

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

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

[Matthieu Moy]

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

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> 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

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

[Matthieu Moy]

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

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

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

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
No change since v2

 builtin/push.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

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

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

[Junio C Hamano]

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.