{"thread":{"id":"22781","subject":"[PATCH] pull: fix 'git pull --all' when current branch is tracking remote that is not last in the list of remotes","startedAt":"2010-02-23T22:55:31Z","lastAt":"2010-02-24T17:05:39Z","messageCount":7,"participants":["Michael Lukashov","Junio C Hamano","Paolo Bonzini"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"135490","messageId":"1266965731-4208-1-git-send-email-michael.lukashov@gmail.com","threadId":"22781","inReplyTo":null,"subject":"[PATCH] pull: fix 'git pull --all' when current branch is tracking remote that is not last in the list of remotes","fromName":"Michael Lukashov","fromEmail":"michael.lukashov@gmail.com","sentAt":"2010-02-23T22:55:31Z","receivedAt":"2010-02-23T22:55:31Z","isPatch":true,"sender":{"key":"michael.lukashov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/890439?v=4"},"body":"Steps to reproduce the bug:\n\n\t1. Create repository and add more than one remote\n\t2. Make sure current branch is tracking branch from the remote AND this remote\n\t   is not last in the list of remotes\n\t3. 'git pull --all' exits with error message:\n\nYou asked to pull from the remote '--all', but did not specify\na branch. Because this is not the default configured remote\nfor your current branch, you must specify a branch on the command line.\n\nA minimal test case is added that reproduces the problem.\nTested under Windows and Debian GNU/Linux.\n\nSigned-off-by: Michael Lukashov <michael.lukashov@gmail.com>\n---\n builtin-fetch.c         |    6 +++++-\n git-pull.sh             |    6 +++++-\n t/t5521-pull-options.sh |   39 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 49 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex d3b9d8a..8e54c5a 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -784,13 +784,17 @@ static int add_remote_or_group(const char *name, struct string_list *list)\n static int fetch_multiple(struct string_list *list)\n {\n \tint i, result = 0;\n-\tconst char *argv[] = { \"fetch\", NULL, NULL, NULL, NULL, NULL, NULL };\n+\tconst char *argv[] = { \"fetch\", NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL };\n \tint argc = 1;\n \n \tif (dry_run)\n \t\targv[argc++] = \"--dry-run\";\n \tif (prune)\n \t\targv[argc++] = \"--prune\";\n+\tif (append)\n+\t\targv[argc++] = \"--append\";\n+\tif (update_head_ok)\n+\t\targv[argc++] = \"--update-head-ok\";\n \tif (verbosity >= 2)\n \t\targv[argc++] = \"-v\";\n \tif (verbosity >= 1)\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 38331a8..fcde096 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -214,7 +214,11 @@ test true = \"$rebase\" && {\n \tdone\n }\n orig_head=$(git rev-parse -q --verify HEAD)\n-git fetch $verbosity --update-head-ok \"$@\" || exit 1\n+if test -e \"$GIT_DIR\"/FETCH_HEAD\n+then\n+\trm \"$GIT_DIR\"/FETCH_HEAD 2>/dev/null\n+fi\n+git fetch $verbosity --update-head-ok --append \"$@\" || exit 1\n \n curr_head=$(git rev-parse -q --verify HEAD)\n if test -n \"$orig_head\" && test \"$curr_head\" != \"$orig_head\"\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 83e2e8a..2665caa 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -4,6 +4,17 @@ test_description='pull options'\n \n . ./test-lib.sh\n \n+setup_repository () {\n+\tmkdir \"$1\" && (\n+\tcd \"$1\" &&\n+\tgit init &&\n+\t>file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m \"Initial\"\n+\t)\n+}\n+\n D=`pwd`\n \n test_expect_success 'setup' '\n@@ -57,4 +68,32 @@ test_expect_success 'git pull -q -v' '\n \ttest -s out\n '\n \n+cd \"$D\"\n+\n+test_expect_success 'git pull --all' '\n+\tmkdir pullall &&\n+\tcd pullall &&\n+\tsetup_repository remote1 &&\n+\tsetup_repository remote2 &&\n+\tmkdir test &&\n+\tcd test &&\n+\tgit init &&\n+\tgit remote add remote1 \"$D/pullall/remote1\" &&\n+\tgit remote add remote2 \"$D/pullall/remote2\" &&\n+\t(\n+\t\t# \"git pull remote1\" should print error message\n+\t\t# because there is no local branch that is tracking remote repo\n+\t\tgit pull remote1\n+\t\ttest $? = 1\n+\t) &&\n+\t(\n+\t\t# \"git pull --all\" should not print error message\n+\t\t# when current branch is tracking remote repo and that remote\n+\t\t# is not last in the list of remotes\n+\t\tgit checkout -b remote1master remote1/master\n+\t\tgit pull --all\n+\t\ttest $? = 0\n+\t)\n+'\n+\n test_done\n-- \n1.7.0.1706.g00cdbe\n"},{"id":"135492","messageId":"7vtyt75zdo.fsf@alter.siamese.dyndns.org","threadId":"22781","inReplyTo":"1266965731-4208-1-git-send-email-michael.lukashov@gmail.com","subject":"Re: [PATCH] pull: fix 'git pull --all' when current branch is tracking remote that is not last in the list of remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-23T23:02:59Z","receivedAt":"2010-02-23T23:02:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lukashov <michael.lukashov@gmail.com> writes:\n\n> diff --git a/git-pull.sh b/git-pull.sh\n> index 38331a8..fcde096 100755\n> --- a/git-pull.sh\n> +++ b/git-pull.sh\n> @@ -214,7 +214,11 @@ test true = \"$rebase\" && {\n>  \tdone\n>  }\n>  orig_head=$(git rev-parse -q --verify HEAD)\n> -git fetch $verbosity --update-head-ok \"$@\" || exit 1\n> +if test -e \"$GIT_DIR\"/FETCH_HEAD\n> +then\n> +\trm \"$GIT_DIR\"/FETCH_HEAD 2>/dev/null\n> +fi\n\nWhen is it sane to ignore an error from this \"rm\", especially after you\nmade sure that it exists?\n"},{"id":"135502","messageId":"63cde7731002231544k4140d0d8u65e8c7250a8ff42c@mail.gmail.com","threadId":"22781","inReplyTo":"7vtyt75zdo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] pull: fix 'git pull --all' when current branch is tracking remote that is not last in the list of remotes","fromName":"Michael Lukashov","fromEmail":"michael.lukashov@gmail.com","sentAt":"2010-02-23T23:44:28Z","receivedAt":"2010-02-23T23:44:28Z","isPatch":true,"sender":{"key":"michael.lukashov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/890439?v=4"},"body":"On Wed, Feb 24, 2010 at 2:02 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Michael Lukashov <michael.lukashov@gmail.com> writes:\n>\n>> diff --git a/git-pull.sh b/git-pull.sh\n>> index 38331a8..fcde096 100755\n>> --- a/git-pull.sh\n>> +++ b/git-pull.sh\n>> @@ -214,7 +214,11 @@ test true = \"$rebase\" && {\n>>       done\n>>  }\n>>  orig_head=$(git rev-parse -q --verify HEAD)\n>> -git fetch $verbosity --update-head-ok \"$@\" || exit 1\n>> +if test -e \"$GIT_DIR\"/FETCH_HEAD\n>> +then\n>> +     rm \"$GIT_DIR\"/FETCH_HEAD 2>/dev/null\n>> +fi\n>\n> When is it sane to ignore an error from this \"rm\", especially after you\n> made sure that it exists?\n>\n\nThe file \"$GIT_DIR\"/FETCH_HEAD is rewritten\nin subsequent call to 'git fetch', thus it is safe to ignore all errors.\n"},{"id":"135508","messageId":"7vzl2zxz20.fsf@alter.siamese.dyndns.org","threadId":"22781","inReplyTo":"63cde7731002231544k4140d0d8u65e8c7250a8ff42c@mail.gmail.com","subject":"Re: [PATCH] pull: fix 'git pull --all' when current branch is tracking remote that is not last in the list of remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-24T00:22:31Z","receivedAt":"2010-02-24T00:22:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lukashov <michael.lukashov@gmail.com> writes:\n\n> On Wed, Feb 24, 2010 at 2:02 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Michael Lukashov <michael.lukashov@gmail.com> writes:\n>>\n>>> diff --git a/git-pull.sh b/git-pull.sh\n>>> index 38331a8..fcde096 100755\n>>> --- a/git-pull.sh\n>>> +++ b/git-pull.sh\n>>> @@ -214,7 +214,11 @@ test true = \"$rebase\" && {\n>>>       done\n>>>  }\n>>>  orig_head=$(git rev-parse -q --verify HEAD)\n>>> -git fetch $verbosity --update-head-ok \"$@\" || exit 1\n>>> +if test -e \"$GIT_DIR\"/FETCH_HEAD\n>>> +then\n>>> +     rm \"$GIT_DIR\"/FETCH_HEAD 2>/dev/null\n>>> +fi\n>>\n>> When is it sane to ignore an error from this \"rm\", especially after you\n>> made sure that it exists?\n>\n> The file \"$GIT_DIR\"/FETCH_HEAD is rewritten\n> in subsequent call to 'git fetch', thus it is safe to ignore all errors.\n\nYou are not answering my question.\n\nYou found out that the thing exists, and you want to overwrite it later.\nYou _need_ that file to either not exist, or at least be empty, because\nyou will be _appending_ to it, unlike the earlier code.\n\nNow, you expected you would be able to remove it, and that is why you\ncalled \"rm\".  Suppose that removal has failed for some reason.  The file\nstays.  It is not emptied, either.\n\nWhy is it sane to ignore that error and let fetch --append to run, as if\nit is starting from either non-existing file or an empty one?  You already\ndiagnosed that the file is in some _funny_ state.  It is not sensible to\ncontinue further at that point, knowing that there is something wrong.\n\nIf the new code you introduced were\n\n\trm -f \"$GIT_DIR/FETCH_HEAD\" || exit\n\nthen I would understand it.  But your patch doesn't make sense to me;\nneither your \"thus it is safe\".\n"},{"id":"135567","messageId":"1267016842-3380-1-git-send-email-michael.lukashov@gmail.com","threadId":"22781","inReplyTo":"7vzl2zxz20.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] pull: fix 'git pull --all' when current branch is tracking remote that is not last in the list of remotes","fromName":"Michael Lukashov","fromEmail":"michael.lukashov@gmail.com","sentAt":"2010-02-24T13:07:22Z","receivedAt":"2010-02-24T13:07:22Z","isPatch":true,"sender":{"key":"michael.lukashov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/890439?v=4"},"body":"Steps to reproduce the bug:\n\n\t1. Create repository and add more than one remote\n\t2. Make sure current branch is tracking branch from the remote AND this remote\n\t   is not last in the list of remotes\n\t3. 'git pull --all' exits with error message:\n\n\t\tYou asked to pull from the remote '--all', but did not specify\n\t\ta branch. Because this is not the default configured remote\n\t\tfor your current branch, you must specify a branch on the command line.\n\nAfter 'git pull --all' you need to run 'git pull' to update current branch.\nThis is annoying.\n\nAfter this patch, 'git pull --all' does what it should do - fetches all changes\nfrom all remotes and then updates current branch, if there were changes.\n\nA minimal test case is added that reproduces the problem.\nTested under Windows and Debian GNU/Linux.\n\nSigned-off-by: Michael Lukashov <michael.lukashov@gmail.com>\n---\n builtin-fetch.c         |    6 +++++-\n git-pull.sh             |    6 +++++-\n t/t5521-pull-options.sh |   39 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 49 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex d3b9d8a..8e54c5a 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -784,13 +784,17 @@ static int add_remote_or_group(const char *name, struct string_list *list)\n static int fetch_multiple(struct string_list *list)\n {\n \tint i, result = 0;\n-\tconst char *argv[] = { \"fetch\", NULL, NULL, NULL, NULL, NULL, NULL };\n+\tconst char *argv[] = { \"fetch\", NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL };\n \tint argc = 1;\n \n \tif (dry_run)\n \t\targv[argc++] = \"--dry-run\";\n \tif (prune)\n \t\targv[argc++] = \"--prune\";\n+\tif (append)\n+\t\targv[argc++] = \"--append\";\n+\tif (update_head_ok)\n+\t\targv[argc++] = \"--update-head-ok\";\n \tif (verbosity >= 2)\n \t\targv[argc++] = \"-v\";\n \tif (verbosity >= 1)\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 38331a8..2fbee42 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -214,7 +214,11 @@ test true = \"$rebase\" && {\n \tdone\n }\n orig_head=$(git rev-parse -q --verify HEAD)\n-git fetch $verbosity --update-head-ok \"$@\" || exit 1\n+if test -e \"$GIT_DIR\"/FETCH_HEAD\n+then\n+\trm -f \"$GIT_DIR\"/FETCH_HEAD || exit\n+fi\n+git fetch $verbosity --update-head-ok --append \"$@\" || exit 1\n \n curr_head=$(git rev-parse -q --verify HEAD)\n if test -n \"$orig_head\" && test \"$curr_head\" != \"$orig_head\"\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 83e2e8a..2665caa 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -4,6 +4,17 @@ test_description='pull options'\n \n . ./test-lib.sh\n \n+setup_repository () {\n+\tmkdir \"$1\" && (\n+\tcd \"$1\" &&\n+\tgit init &&\n+\t>file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m \"Initial\"\n+\t)\n+}\n+\n D=`pwd`\n \n test_expect_success 'setup' '\n@@ -57,4 +68,32 @@ test_expect_success 'git pull -q -v' '\n \ttest -s out\n '\n \n+cd \"$D\"\n+\n+test_expect_success 'git pull --all' '\n+\tmkdir pullall &&\n+\tcd pullall &&\n+\tsetup_repository remote1 &&\n+\tsetup_repository remote2 &&\n+\tmkdir test &&\n+\tcd test &&\n+\tgit init &&\n+\tgit remote add remote1 \"$D/pullall/remote1\" &&\n+\tgit remote add remote2 \"$D/pullall/remote2\" &&\n+\t(\n+\t\t# \"git pull remote1\" should print error message\n+\t\t# because there is no local branch that is tracking remote repo\n+\t\tgit pull remote1\n+\t\ttest $? = 1\n+\t) &&\n+\t(\n+\t\t# \"git pull --all\" should not print error message\n+\t\t# when current branch is tracking remote repo and that remote\n+\t\t# is not last in the list of remotes\n+\t\tgit checkout -b remote1master remote1/master\n+\t\tgit pull --all\n+\t\ttest $? = 0\n+\t)\n+'\n+\n test_done\n-- \n1.7.0.1706.g00cdbe\n"},{"id":"135568","messageId":"4B852AC6.8040508@gnu.org","threadId":"22781","inReplyTo":"1267016842-3380-1-git-send-email-michael.lukashov@gmail.com","subject":"Re: [PATCH v2] pull: fix 'git pull --all' when current branch is tracking remote that is not last in the list of remotes","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2010-02-24T13:33:58Z","receivedAt":"2010-02-24T13:33:58Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 02/24/2010 02:07 PM, Michael Lukashov wrote:\n> +if test -e \"$GIT_DIR\"/FETCH_HEAD\n> +then\n> +\trm -f \"$GIT_DIR\"/FETCH_HEAD || exit\n> +fi\n\nYou do not need the if.\n\nPaolo\n"},{"id":"135580","messageId":"7vpr3uwom4.fsf@alter.siamese.dyndns.org","threadId":"22781","inReplyTo":"1267016842-3380-1-git-send-email-michael.lukashov@gmail.com","subject":"Re: [PATCH v2] pull: fix 'git pull --all' when current branch is tracking remote that is not last in the list of remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-24T17:05:39Z","receivedAt":"2010-02-24T17:05:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lukashov <michael.lukashov@gmail.com> writes:\n\n> Steps to reproduce the bug:\n>\n> \t1. Create repository and add more than one remote\n> \t2. Make sure current branch is tracking branch from the remote AND this remote\n> \t   is not last in the list of remotes\n> \t3. 'git pull --all' exits with error message:\n>\n> \t\tYou asked to pull from the remote '--all', but did not specify\n> \t\ta branch. Because this is not the default configured remote\n> \t\tfor your current branch, you must specify a branch on the command line.\n>\n> After 'git pull --all' you need to run 'git pull' to update current branch.\n> This is annoying.\n\nI started to rewrite the commit log message to present the backstory and\nthe assumptions existing code makes, _why_ things break, and then explain\nwhat the proposed solution is and why it is the right fix to the problem\n(by the way, presenting the log message this way helps you to think things\nthrough).  And I had to stop immediately after the description of _why_\nthings break:\n\n    \"git fetch\" learned \"--all\" option and it has become tempting for\n    users to say \"git pull --all\", even though it may not be absolutely\n    necessary to pull from many remotes that are not involved in the merge\n    about to happen to the current branch.\n\n    \"git fetch --all\" however clears the list of fetched branches every\n    time it contacts a different remote.  Unless the current branch is\n    configured to merge with a branch from a remote that happens to be the\n    last in the list of remotes \"fetch --all\" contacts with, \"git pull\n    --all\" will not be able to find the branch it should be merging with.\n\nNotice that presented this way, it becomes clear that it not a bug in\n\"pull --all\" at all. \"fetch --all\" should be doing the clearing of\nFETCH_HEAD at the very beginning (unless --append is given from the\ncommand line, in which case it should just use FETCH_HEAD as-is), and then\nrun in the --append mode even when --append is not given.\n\nSo I think you identified a valid issue to address, but the patch is\nsolving it in a wrong way.  The commit log message I started above would\nbe concluded with something like the following explanation of what the\nproposed solution is and why it is the right fix:\n\n    Make \"fetch --all\" to clear FETCH_HEAD (unless --append is given) and\n    then append the list of branches fetched to it (even when --apend is\n    not given).  That way, \"pull --all\" will be able to find the data for\n    the branch being merged in FETCH_HEAD no matter where the remote\n    appears in the list of remotes to be contacted by \"git fetch\".\n\nand the patch would touch fetch, not pull.\n"}]}