{"thread":{"id":"25973","subject":"[PATCH] parse-remote: handle detached HEAD","startedAt":"2010-12-05T23:47:15Z","lastAt":"2010-12-06T17:14:47Z","messageCount":8,"participants":["Santi Béjar","Sverre Rabbelier","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"157373","messageId":"1291592835-29949-1-git-send-email-santi@agolina.net","threadId":"25973","inReplyTo":null,"subject":"[PATCH] parse-remote: handle detached HEAD","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2010-12-05T23:47:15Z","receivedAt":"2010-12-05T23:47:15Z","isPatch":true,"sender":{"key":"santi@agolina.net","avatar":null},"body":"In get_remote_merge_branch 'git for-each-ref' is used to know the\nupstream branch of the current branch ($curr_branch). But $curr_branch\ncan be empty when in detached HEAD, so the call to for-each-ref is\nmade without a pattern.\n\nQuote the $curr_branch variable in the git for-each-ref call to always\nprovide a pattern (the current branch or an empty string) Otherwise it\nwould mean all refs.\n\nThis fixes a bug reported by Sverre Rabbelier. The overall results\nwere correct but not the output text.\n\nSigned-off-by: Santi Béjar \n---\nHi *,\n\nSorry Sverre, but the patch you tested fixed your case but broke all\nthe others :(\n\nHope you can also test it. Now it passes the test suite.\n\nThanks,\nSanti\n git-parse-remote.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-parse-remote.sh b/git-parse-remote.sh\nindex 5f47b18..07060c3 100644\n--- a/git-parse-remote.sh\n+++ b/git-parse-remote.sh\n@@ -68,7 +68,7 @@ get_remote_merge_branch () {\n \t test -z \"$origin\" && origin=$default\n \t curr_branch=$(git symbolic-ref -q HEAD)\n \t [ \"$origin\" = \"$default\" ] &&\n-\t echo $(git for-each-ref --format='%(upstream)' $curr_branch)\n+\t echo $(git for-each-ref --format='%(upstream)' \"$curr_branch\")\n \t ;;\n \t*)\n \t repo=$1\n-- \n1.7.3.3.399.gea47f\n"},{"id":"157374","messageId":"AANLkTi=yfuiFuatshYuS2Q0EV0Ytj-QFKpuXAWeGerQB@mail.gmail.com","threadId":"25973","inReplyTo":"1291592835-29949-1-git-send-email-santi@agolina.net","subject":"Re: [PATCH] parse-remote: handle detached HEAD","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-12-05T23:49:39Z","receivedAt":"2010-12-05T23:49:39Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Dec 6, 2010 at 00:47, Santi Béjar wrote:\n> This fixes a bug reported by Sverre Rabbelier. The overall results\n> were correct but not the output text.\n\nI think we usually use:\n\nReported-by: Sverre Rabbelier \n\nBut since I already verified the fix, perhaps just:\n\nTested-by: Sverre Rabbelier \n\nis enough? Both are fine with me.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"157375","messageId":"1291593517-4406-1-git-send-email-santi@agolina.net","threadId":"25973","inReplyTo":"AANLkTi=yfuiFuatshYuS2Q0EV0Ytj-QFKpuXAWeGerQB@mail.gmail.com","subject":"[PATCHv2] parse-remote: handle detached HEAD","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2010-12-05T23:58:37Z","receivedAt":"2010-12-05T23:58:37Z","isPatch":false,"sender":{"key":"santi@agolina.net","avatar":null},"body":"In get_remote_merge_branch 'git for-each-ref' is used to know the\nupstream branch of the current branch ($curr_branch). But $curr_branch\ncan be empty when in detached HEAD, so the call to for-each-ref is\nmade without a pattern.\n\nQuote the $curr_branch variable in the git for-each-ref call to always\nprovide a pattern (the current branch or an empty string) Otherwise it\nwould mean all refs.\n\nReported-by: Sverre Rabbelier \nSigned-off-by: Santi Béjar \nTested-by: Sverre Rabbelier \n---\nChanges since v1:\n Tags for Reported-by and Tested-by.\n\n git-parse-remote.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-parse-remote.sh b/git-parse-remote.sh\nindex 5f47b18..07060c3 100644\n--- a/git-parse-remote.sh\n+++ b/git-parse-remote.sh\n@@ -68,7 +68,7 @@ get_remote_merge_branch () {\n \t test -z \"$origin\" && origin=$default\n \t curr_branch=$(git symbolic-ref -q HEAD)\n \t [ \"$origin\" = \"$default\" ] &&\n-\t echo $(git for-each-ref --format='%(upstream)' $curr_branch)\n+\t echo $(git for-each-ref --format='%(upstream)' \"$curr_branch\")\n \t ;;\n \t*)\n \t repo=$1\n-- \n1.7.3.3.399.gea47f\n"},{"id":"157378","messageId":"7vfwubtw1g.fsf@alter.siamese.dyndns.org","threadId":"25973","inReplyTo":"1291593517-4406-1-git-send-email-santi@agolina.net","subject":"Re: [PATCHv2] parse-remote: handle detached HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-06T03:33:31Z","receivedAt":"2010-12-06T03:33:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Santi Béjar writes:\n\n> In get_remote_merge_branch 'git for-each-ref' is used to know the\n> upstream branch of the current branch ($curr_branch). But $curr_branch\n> can be empty when in detached HEAD, so the call to for-each-ref is\n> made without a pattern.\n>\n> Quote the $curr_branch variable in the git for-each-ref call to always\n> provide a pattern (the current branch or an empty string) Otherwise it\n> would mean all refs.\n\nWhat output do you want to see in this case? \"Nothing needs to be\nreported because on detached head you are not tracking anything?\"\n\nIf that is the case, shouldn't we be not calling \"echo\" at all to begin\nwith? IOW, shouldn't the code read more like this?\n\n\tcurr_branch=$(git symbolic-ref -q HEAD) &&\n test \"$origin\" = \"$default\" &&\n\techo ...\n\n> Reported-by: Sverre Rabbelier \n> Signed-off-by: Santi Béjar \n> Tested-by: Sverre Rabbelier \n> ---\n> Changes since v1:\n> Tags for Reported-by and Tested-by.\n>\n> git-parse-remote.sh | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-parse-remote.sh b/git-parse-remote.sh\n> index 5f47b18..07060c3 100644\n> --- a/git-parse-remote.sh\n> +++ b/git-parse-remote.sh\n> @@ -68,7 +68,7 @@ get_remote_merge_branch () {\n> \t test -z \"$origin\" && origin=$default\n> \t curr_branch=$(git symbolic-ref -q HEAD)\n> \t [ \"$origin\" = \"$default\" ] &&\n> -\t echo $(git for-each-ref --format='%(upstream)' $curr_branch)\n> +\t echo $(git for-each-ref --format='%(upstream)' \"$curr_branch\")\n> \t ;;\n> \t*)\n> \t repo=$1\n"},{"id":"157397","messageId":"1291630811-16584-1-git-send-email-santi@agolina.net","threadId":"25973","inReplyTo":"7vfwubtw1g.fsf@alter.siamese.dyndns.org","subject":"[PATCHv3] parse-remote: handle detached HEAD","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2010-12-06T10:20:11Z","receivedAt":"2010-12-06T10:20:11Z","isPatch":false,"sender":{"key":"santi@agolina.net","avatar":null},"body":"get_remote_merge_branch with zero or one arguments returns the\nupstream branch. But a detached HEAD does no have an upstream branch,\nas it is not tracking anything. Handle this case testing the exit code\nof \"git symbolic-ref -q HEAD\".\n\nReported-by: Sverre Rabbelier \nSigned-off-by: Santi Béjar \n---\n\n> If that is the case, shouldn't we be not calling \"echo\" at all to begin\n> with? IOW, shouldn't the code read more like this?\n>\n> curr_branch=$(git symbolic-ref -q HEAD) &&\n> test \"$origin\" = \"$default\" &&\n> echo ...\n\nOr course, you are right. I didn't know/think about the exit\ncode... Thanks.\n\nSanti\n\n git-parse-remote.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-parse-remote.sh b/git-parse-remote.sh\nindex 5f47b18..4da72ae 100644\n--- a/git-parse-remote.sh\n+++ b/git-parse-remote.sh\n@@ -66,7 +66,7 @@ get_remote_merge_branch () {\n \t origin=\"$1\"\n \t default=$(get_default_remote)\n \t test -z \"$origin\" && origin=$default\n-\t curr_branch=$(git symbolic-ref -q HEAD)\n+\t curr_branch=$(git symbolic-ref -q HEAD) &&\n \t [ \"$origin\" = \"$default\" ] &&\n \t echo $(git for-each-ref --format='%(upstream)' $curr_branch)\n \t ;;\n-- \n1.7.3.3.399.g0d2be.dirty\n"},{"id":"157402","messageId":"AANLkTik4LLCm3WzcKPkOY44M88vF7oT2nuLrv-S3L22X@mail.gmail.com","threadId":"25973","inReplyTo":"1291630811-16584-1-git-send-email-santi@agolina.net","subject":"Re: [PATCHv3] parse-remote: handle detached HEAD","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2010-12-06T14:32:55Z","receivedAt":"2010-12-06T14:32:55Z","isPatch":false,"sender":{"key":"santi@agolina.net","avatar":null},"body":"On Mon, Dec 6, 2010 at 11:20 AM, Santi Béjar wrote:\n> get_remote_merge_branch with zero or one arguments returns the\n> upstream branch. But a detached HEAD does no have an upstream branch,\n> as it is not tracking anything. Handle this case testing the exit code\n> of \"git symbolic-ref -q HEAD\".\n>\n> Reported-by: Sverre Rabbelier \n> Signed-off-by: Santi Béjar \n> ---\n>\n>> If that is the case, shouldn't we be not calling \"echo\" at all to begin\n>> with?  IOW, shouldn't the code read more like this?\n>>\n>>        curr_branch=$(git symbolic-ref -q HEAD) &&\n>>        test \"$origin\" = \"$default\" &&\n>>        echo ...\n>\n> Or course, you are right. I didn't know/think about the exit\n> code... Thanks.\n\nNow that I think of... the final form of the patch is yours (Junio).\nFeel free to add something like this to the commit message:\n\nFinal patch form by Junio C Hamano\n\nOr alternatively, take ownership of the patch and add something like\n\"Patch handled by Santi Béjar but final patch form by Junio C Hamano\"\nand:\n\nAcked-by: Santi Béjar \n\nSanti\n"},{"id":"157405","messageId":"7vr5dusxb1.fsf@alter.siamese.dyndns.org","threadId":"25973","inReplyTo":"1291630811-16584-1-git-send-email-santi@agolina.net","subject":"Re: [PATCHv3] parse-remote: handle detached HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-06T16:03:46Z","receivedAt":"2010-12-06T16:03:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Santi Béjar writes:\n\n> get_remote_merge_branch with zero or one arguments returns the\n> upstream branch. But a detached HEAD does no have an upstream branch,\n> as it is not tracking anything. Handle this case testing the exit code\n> of \"git symbolic-ref -q HEAD\".\n>\n> Reported-by: Sverre Rabbelier \n> Signed-off-by: Santi Béjar \n> ---\n>\n>> If that is the case, shouldn't we be not calling \"echo\" at all to begin\n>> with? IOW, shouldn't the code read more like this?\n>>\n>> curr_branch=$(git symbolic-ref -q HEAD) &&\n>> test \"$origin\" = \"$default\" &&\n>> echo ...\n>\n> Or course, you are right. I didn't know/think about the exit\n> code... Thanks.\n\nThe calling codepath in git-pull that wants to determine remoteref and\noldremoteref seems to expect get-remote-merge-branch to succeed in order\nto find its $oldremoteref variable, and returning false in detached HEAD\ncase here will change what happens there---it won't run \"rev-list -g\"\nanymore and quits the codepath early, leaving the variable empty.\n\nBut we do want to set the variable to an empty string in this case anyway,\nso there is no harm done (it probably is what we actually want to happen).\n\nSo this should be Ok. Sverre, do you want to do another round of testing\njust to be sure before I apply this?\n\n> Santi\n>\n> git-parse-remote.sh | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-parse-remote.sh b/git-parse-remote.sh\n> index 5f47b18..4da72ae 100644\n> --- a/git-parse-remote.sh\n> +++ b/git-parse-remote.sh\n> @@ -66,7 +66,7 @@ get_remote_merge_branch () {\n> \t origin=\"$1\"\n> \t default=$(get_default_remote)\n> \t test -z \"$origin\" && origin=$default\n> -\t curr_branch=$(git symbolic-ref -q HEAD)\n> +\t curr_branch=$(git symbolic-ref -q HEAD) &&\n> \t [ \"$origin\" = \"$default\" ] &&\n> \t echo $(git for-each-ref --format='%(upstream)' $curr_branch)\n> \t ;;\n> -- \n> 1.7.3.3.399.g0d2be.dirty\n"},{"id":"157407","messageId":"AANLkTi=jf5ZB8Lz3jRN-HJ8mquFt4frhFk0mZ4sCHwZk@mail.gmail.com","threadId":"25973","inReplyTo":"7vr5dusxb1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3] parse-remote: handle detached HEAD","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-12-06T17:14:47Z","receivedAt":"2010-12-06T17:14:47Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Dec 6, 2010 at 17:03, Junio C Hamano wrote:\n> So this should be Ok.  Sverre, do you want to do another round of testing\n> just to be sure before I apply this?\n\nYup:\n\nTested-by: Sverre Rabbelier