threads / patch / 33976

v2, 3 partsEarly HEAD resolution in push.default = current

Subject: [PATCH v2 0/3] Early HEAD resolution in push.default = current

## tl;dr

8 messages between May 29, 2013 and May 29, 2013. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Ramkumar Ramachandra· May 29, 2013, 19:21 UTC · lore

The commit message in [3/3] is rewritten although I've mentioned the original motivation at the end.

No other changes.
Ramkumar Ramachandra (3):
  push: factor out the detached HEAD error message
  push: fail early with detached HEAD and current
  push: make push.default = current resolve HEAD early
 builtin/push.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
-- 
1.8.3.12.gbd56588
Ramkumar Ramachandra· May 29, 2013, 19:21 UTC · re: Ramkumar Ramachandra · lore

[PATCH 1/3] push: factor out the detached HEAD error message

With push.default set to upstream or simple, and a detached HEAD, git push prints the following error:

  $ git push
  fatal: You are not currently on a branch.
  To push the history leading to the current (detached HEAD)
  state now, use
    git push ram HEAD:<name-of-remote-branch>

This error is not unique to upstream or simple: current cannot push with a detached HEAD either. So, factor out the error string in preparation for using it in current.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
Show changes to builtin/push.c +8 −6
diff --git a/builtin/push.c b/builtin/push.c
index 909c34d..ef3aa97 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -113,17 +113,19 @@ static NORETURN int die_push_simple(struct branch *branch, struct remote *remote
 	    remote->name, branch->name, advice_maybe);
 }
 
+static const char message_detached_head_die[] =
+	N_("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");
+
 static void setup_push_upstream(struct remote *remote, int simple)
 {
 	struct strbuf refspec = STRBUF_INIT;
 	struct branch *branch = branch_get(NULL);
 	if (!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);
+		die(_(message_detached_head_die), remote->name);
 	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
 		die(_("The current branch %s has no upstream branch.\n"
 		    "To push the current branch and set the remote as upstream, use\n"
-- 
1.8.3.12.gbd56588
Ramkumar Ramachandra· May 29, 2013, 19:21 UTC · re: Ramkumar Ramachandra · lore

[PATCH 2/3] push: fail early with detached HEAD and current

Setting push.default to current adds the refspec "HEAD" for the transport layer to handle. If "HEAD" doesn't resolve to a branch (and since no refspec rhs is specified), the push fails after some time with a cryptic error message:

  $ git push
  error: unable to push to unqualified destination: HEAD
  The destination refspec neither matches an existing ref on the remote nor
  begins with refs/, and we are unable to guess a prefix based on the source ref.
  error: failed to push some refs to 'git@github.com:artagnon/git'
Fail early with a nicer error message:
  $ git push
  fatal: You are not currently on a branch.
  To push the history leading to the current (detached HEAD)
  state now, use
    git push ram HEAD:<name-of-remote-branch>
Just like in the upstream and simple cases.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to builtin/push.c +4 −0
diff --git a/builtin/push.c b/builtin/push.c
index ef3aa97..a79038c 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -175,6 +175,8 @@ static void warn_unspecified_push_default_configuration(void)
 
 static void setup_default_push_refspecs(struct remote *remote)
 {
+	struct branch *branch = branch_get(NULL);
+
 	switch (push_default) {
 	default:
 	case PUSH_DEFAULT_UNSPECIFIED:
@@ -194,6 +196,8 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
+		if (!branch)
+			die(_(message_detached_head_die), remote->name);
 		add_refspec("HEAD");
 		break;
 
-- 
1.8.3.12.gbd56588
Junio C Hamano· May 29, 2013, 19:34 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/3] push: fail early with detached HEAD and current

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 48 quoted lines
> Setting push.default to current adds the refspec "HEAD" for the
> transport layer to handle.  If "HEAD" doesn't resolve to a branch (and
> since no refspec rhs is specified), the push fails after some time with
> a cryptic error message:
>
>   $ git push
>   error: unable to push to unqualified destination: HEAD
>   The destination refspec neither matches an existing ref on the remote nor
>   begins with refs/, and we are unable to guess a prefix based on the source ref.
>   error: failed to push some refs to 'git@github.com:artagnon/git'
>
> Fail early with a nicer error message:
>
>   $ git push
>   fatal: You are not currently on a branch.
>   To push the history leading to the current (detached HEAD)
>   state now, use
>
>     git push ram HEAD:<name-of-remote-branch>
>
> Just like in the upstream and simple cases.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/push.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index ef3aa97..a79038c 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -175,6 +175,8 @@ static void warn_unspecified_push_default_configuration(void)
>  
>  static void setup_default_push_refspecs(struct remote *remote)
>  {
> +	struct branch *branch = branch_get(NULL);
> +
>  	switch (push_default) {
>  	default:
>  	case PUSH_DEFAULT_UNSPECIFIED:
> @@ -194,6 +196,8 @@ static void setup_default_push_refspecs(struct remote *remote)
>  		break;
>  
>  	case PUSH_DEFAULT_CURRENT:
> +		if (!branch)
> +			die(_(message_detached_head_die), remote->name);
>  		add_refspec("HEAD");
>  		break;

This means well, but I am not sure calling branch_get() for all other cases that do not care about the current branch is a right thing to do. It's not like you need branch variable to free some resource after done, or pass it around to callees from this function.

Would it hurt to do
	if (!branch_get(NULL))
		die(...);
here, without the first hunk?  
Ramkumar Ramachandra· May 29, 2013, 19:37 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] push: fail early with detached HEAD and current

Junio C Hamano wrote:
Show 12 quoted lines
>>       case PUSH_DEFAULT_CURRENT:
>> +             if (!branch)
>> +                     die(_(message_detached_head_die), remote->name);
>>               add_refspec("HEAD");
>>               break;
>
> Would it hurt to do
>
>         if (!branch_get(NULL))
>                 die(...);
>
> here, without the first hunk?
And how would I change the add_refspec() call to take branch->name in [3/3]?
Junio C Hamano· May 29, 2013, 19:59 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/3] push: fail early with detached HEAD and current

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 15 quoted lines
> Junio C Hamano wrote:
>>>       case PUSH_DEFAULT_CURRENT:
>>> +             if (!branch)
>>> +                     die(_(message_detached_head_die), remote->name);
>>>               add_refspec("HEAD");
>>>               break;
>>
>> Would it hurt to do
>>
>>         if (!branch_get(NULL))
>>                 die(...);
>>
>> here, without the first hunk?
>
> And how would I change the add_refspec() call to take branch->name in [3/3]?
By doing something like this, perhaps?
	struct ... branch;
        switch (...) {
        ...
        case it-only-matters-here:
        	branch = branch_get(NULL);
                if (!branch)
			die(...);
	}
Ramkumar Ramachandra· May 29, 2013, 20:04 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] push: fail early with detached HEAD and current

Here: put this at the end of the series and autosquash.
-- 8< --
From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Thu, 30 May 2013 01:29:59 +0530
Subject: [PATCH] fixup! HEAD~2
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to builtin/push.c +2 −1
diff --git a/builtin/push.c b/builtin/push.c
index d819487..2d84d10 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -175,7 +175,7 @@ static void
warn_unspecified_push_default_configuration(void)

 static void setup_default_push_refspecs(struct remote *remote)
 {
-       struct branch *branch = branch_get(NULL);
+       struct branch *branch;

        switch (push_default) {
        default:
@@ -196,6 +196,7 @@ static void setup_default_push_refspecs(struct
remote *remote)
                break;

        case PUSH_DEFAULT_CURRENT:
+               branch = branch_get(NULL);
                if (!branch)
                        die(_(message_detached_head_die), remote->name);
                add_refspec(branch->name);
-- 
1.8.3.12.gbd56588
Ramkumar Ramachandra· May 29, 2013, 19:21 UTC · re: Ramkumar Ramachandra · lore

[PATCH 3/3] push: make push.default = current use resolved HEAD

With this change, the output of the push (with push.default set to current) changes subtly from:

  $ git push
  ...
   * [new branch]      HEAD -> push-current-head
to:
  $ git push
  ...
   * [new branch]      push-current-head -> push-current-head

This patch was written with a different motivation. There is a problem unique to push.default = current:

  # on branch push-current-head
  $ git push
  # on another terminal
  $ git checkout master
  # return to the first terminal
  # the push tried to push master!

This happens because the 'git checkout' on the second terminal races with the 'git push' on the first terminal. Although this patch does not solve the core problem (there is still no guarantee that 'git push' on the first terminal will resolve HEAD before 'git checkout' changes HEAD on the second), it works in practice.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/push.c +1 −1
diff --git a/builtin/push.c b/builtin/push.c
index a79038c..d819487 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -198,7 +198,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 	case PUSH_DEFAULT_CURRENT:
 		if (!branch)
 			die(_(message_detached_head_die), remote->name);
-		add_refspec("HEAD");
+		add_refspec(branch->name);
 		break;
 
 	case PUSH_DEFAULT_NOTHING:
-- 
1.8.3.12.gbd56588

← back to recent threads