{"thread":{"id":"15858","subject":"[PATCH] fetch: refuse to fetch into the current branch in a non-bare repository","startedAt":"2008-10-11T11:38:02Z","lastAt":"2008-10-14T22:53:44Z","messageCount":24,"participants":["Johannes Schindelin","Junio C Hamano","Shawn O. Pearce","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"92791","messageId":"alpine.DEB.1.00.0810111336350.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15858","inReplyTo":null,"subject":"[PATCH] fetch: refuse to fetch into the current branch in a non-bare repository","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-11T11:38:02Z","receivedAt":"2008-10-11T11:38:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nSome confusing tutorials suggest that it would be a good idea to call\nsomething like this:\n\n\tgit pull origin master:master\n\nWhile it might make sense to store what you want to merge, it typically\nis plain wrong.  Especially so when the current branch is \"master\".\n\nBe at least a little bit helpful by refusing to fetch something into\nthe current branch.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin-fetch.c   |   20 ++++++++++++++++++++\n t/t5505-remote.sh |    2 +-\n t/t5510-fetch.sh  |    6 ++++++\n 3 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex ee93d3a..d701550 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -534,6 +534,25 @@ static void find_non_local_tags(struct transport *transport,\n \tstring_list_clear(&new_refs, 0);\n }\n \n+static void check_ref_map(struct ref *ref_map)\n+{\n+\tint flag;\n+\tunsigned char sha1[20];\n+\tconst char *HEAD;\n+\n+\tif (is_bare_repository())\n+\t\treturn;\n+\n+\tHEAD = resolve_ref(\"HEAD\", sha1, 1, &flag);\n+\n+\tif (!HEAD || !(flag & REF_ISSYMREF))\n+\t\treturn;\n+\n+\tfor (; ref_map; ref_map = ref_map->next)\n+\t\tif (ref_map->peer_ref && !strcmp(HEAD, ref_map->peer_ref->name))\n+\t\t\tdie(\"Refusing to fetch into current branch\");\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t    struct refspec *refs, int ref_count)\n {\n@@ -558,6 +577,7 @@ static int do_fetch(struct transport *transport,\n \t}\n \n \tref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);\n+\tcheck_ref_map(ref_map);\n \n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tif (rm->peer_ref)\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex c449663..0103e1a 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -188,7 +188,7 @@ test_expect_success 'prune --dry-run' '\n test_expect_success 'add --mirror && prune' '\n \t(mkdir mirror &&\n \t cd mirror &&\n-\t git init &&\n+\t git init --bare &&\n \t git remote add --mirror -f origin ../one) &&\n \t(cd one &&\n \t git branch -m side2 side) &&\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 9aae496..cd8b550 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -323,4 +323,10 @@ test_expect_success 'auto tag following fetches minimum' '\n \t)\n '\n \n+test_expect_success 'refuse to fetch into the current branch' '\n+\n+\ttest_must_fail git fetch . side:master\n+\n+'\n+\n test_done\n-- \n1.6.0.2.749.g0cc32\n"},{"id":"92812","messageId":"7vprm6iz6z.fsf@gitster.siamese.dyndns.org","threadId":"15858","inReplyTo":"alpine.DEB.1.00.0810111336350.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH] fetch: refuse to fetch into the current branch in a non-bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-11T21:44:52Z","receivedAt":"2008-10-11T21:44:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Some confusing tutorials suggest that it would be a good idea to call\n> something like this:\n>\n> \tgit pull origin master:master\n>\n> While it might make sense to store what you want to merge, it typically\n> is plain wrong.\n\nI am somewhat confused.\n\nThis \"confusion\" has been there for very long time and (at least the\nscripted version of) git-pull/git-fetch pair has supported a workaround in\nthe form of --update-head-ok option.\n\nHave we broken the workaround ll.127..147 in git-pull.sh recently, or are\nyou trying to address something else?\n"},{"id":"92866","messageId":"20081012184727.GE4856@spearce.org","threadId":"15858","inReplyTo":"7vprm6iz6z.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fetch: refuse to fetch into the current branch in a non-bare repository","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-10-12T18:47:27Z","receivedAt":"2008-10-12T18:47:27Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > Some confusing tutorials suggest that it would be a good idea to call\n> > something like this:\n> >\n> > \tgit pull origin master:master\n> >\n> > While it might make sense to store what you want to merge, it typically\n> > is plain wrong.\n> \n> I am somewhat confused.\n\nThe description is confusing, yes.  It should be about git fetch,\nnot git pull.\n \n> This \"confusion\" has been there for very long time and (at least the\n> scripted version of) git-pull/git-fetch pair has supported a workaround in\n> the form of --update-head-ok option.\n\nI think \"git fetch url side:master\" when master is the current branch\nand we have omitted --update-head-ok is broken.  Specifically Dscho's\nlast hunk which adds this test.  The test fails on current master.\n\nLooking at the code in builtin-fetch.c, the only usage of\nupdate_head_ok is for output about the current branch.  I think\nit should have been used in at least one other spot, to decide if\nthe RHS of a refspec is valid for storage.  Dscho's patch tries to\naddress that.\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 9aae496..cd8b550 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -323,4 +323,10 @@ test_expect_success 'auto tag following fetches minimum' '\n        )\n '\n\n+test_expect_success 'refuse to fetch into the current branch' '\n+\n+\ttest_must_fail git fetch . side:master\n+\n+'\n+\n test_done\n\n\n-- \nShawn.\n"},{"id":"92867","messageId":"20081012185209.GB5255@spearce.org","threadId":"15858","inReplyTo":"alpine.DEB.1.00.0810111336350.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH] fetch: refuse to fetch into the current branch in a non-bare repository","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-10-12T18:52:09Z","receivedAt":"2008-10-12T18:52:09Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> diff --git a/builtin-fetch.c b/builtin-fetch.c\n> index ee93d3a..d701550 100644\n> --- a/builtin-fetch.c\n> +++ b/builtin-fetch.c\n> @@ -534,6 +534,25 @@ static void find_non_local_tags(struct transport *transport,\n>  \tstring_list_clear(&new_refs, 0);\n>  }\n>  \n> +static void check_ref_map(struct ref *ref_map)\n> +{\n> +\tint flag;\n> +\tunsigned char sha1[20];\n> +\tconst char *HEAD;\n> +\n> +\tif (is_bare_repository())\n> +\t\treturn;\n> +\n> +\tHEAD = resolve_ref(\"HEAD\", sha1, 1, &flag);\n\nI'd rather see local variables named lowercase.  Constants should\nbe the only thing that is all uppercase.\n\n> @@ -558,6 +577,7 @@ static int do_fetch(struct transport *transport,\n>  \t}\n>  \n>  \tref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);\n> +\tcheck_ref_map(ref_map);\n\nThis should only be called if update_head_ok is false.  So maybe:\n\n\tif (!update_head_ok)\n\t\tcheck_ref_map(ref_map)\n  \n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 9aae496..cd8b550 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -323,4 +323,10 @@ test_expect_success 'auto tag following fetches minimum' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'refuse to fetch into the current branch' '\n> +\n> +\ttest_must_fail git fetch . side:master\n> +\n> +'\n> +\n\nRepeat this test, but with --update-head-ok and expect success,\nsince the check_ref_map logic is conditional on that?\n\n-- \nShawn.\n"},{"id":"92872","messageId":"alpine.LNX.1.00.0810121501590.19665@iabervon.org","threadId":"15858","inReplyTo":"alpine.DEB.1.00.0810111336350.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH] fetch: refuse to fetch into the current branch in a non-bare repository","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-10-12T20:37:28Z","receivedAt":"2008-10-12T20:37:28Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 11 Oct 2008, Johannes Schindelin wrote:\n\n> Some confusing tutorials suggest that it would be a good idea to call\n> something like this:\n> \n> \tgit pull origin master:master\n> \n> While it might make sense to store what you want to merge, it typically\n> is plain wrong.  Especially so when the current branch is \"master\".\n> \n> Be at least a little bit helpful by refusing to fetch something into\n> the current branch.\n\nI think this is the right thing to do.\n\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  builtin-fetch.c   |   20 ++++++++++++++++++++\n>  t/t5505-remote.sh |    2 +-\n>  t/t5510-fetch.sh  |    6 ++++++\n>  3 files changed, 28 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin-fetch.c b/builtin-fetch.c\n> index ee93d3a..d701550 100644\n> --- a/builtin-fetch.c\n> +++ b/builtin-fetch.c\n> @@ -534,6 +534,25 @@ static void find_non_local_tags(struct transport *transport,\n>  \tstring_list_clear(&new_refs, 0);\n>  }\n>  \n> +static void check_ref_map(struct ref *ref_map)\n> +{\n> +\tint flag;\n> +\tunsigned char sha1[20];\n> +\tconst char *HEAD;\n> +\n> +\tif (is_bare_repository())\n> +\t\treturn;\n> +\n> +\tHEAD = resolve_ref(\"HEAD\", sha1, 1, &flag);\n> +\n> +\tif (!HEAD || !(flag & REF_ISSYMREF))\n> +\t\treturn;\n\nremote.h has a function for getting \"the current branch\", which would save \n5 lines here:\n\n\tstruct branch *current_branch = branch_get(NULL);\n\tif (!current_branch || is_bare_repository())\n\t\treturn;\n\n> +\n> +\tfor (; ref_map; ref_map = ref_map->next)\n> +\t\tif (ref_map->peer_ref && !strcmp(HEAD, ref_map->peer_ref->name))\n\n\t\t!strcmp(current_branch->ref_name, ref_map->peer_ref->name)\n\n(untested, and might be off by a \"refs/\" or something)\n\n> +\t\t\tdie(\"Refusing to fetch into current branch\");\n> +}\n> +\n>  static int do_fetch(struct transport *transport,\n>  \t\t    struct refspec *refs, int ref_count)\n>  {\n> @@ -558,6 +577,7 @@ static int do_fetch(struct transport *transport,\n>  \t}\n>  \n>  \tref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);\n> +\tcheck_ref_map(ref_map);\n>  \n>  \tfor (rm = ref_map; rm; rm = rm->next) {\n>  \t\tif (rm->peer_ref)\n"},{"id":"92917","messageId":"alpine.DEB.1.00.0810131050420.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15858","inReplyTo":"20081012184727.GE4856@spearce.org","subject":"Re: [PATCH] fetch: refuse to fetch into the current branch in a non-bare repository","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-13T09:28:11Z","receivedAt":"2008-10-13T09:28:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 12 Oct 2008, Shawn O. Pearce wrote:\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> > \n> > > Some confusing tutorials suggest that it would be a good idea to call\n> > > something like this:\n> > >\n> > > \tgit pull origin master:master\n> > >\n> > > While it might make sense to store what you want to merge, it typically\n> > > is plain wrong.\n> > \n> > I am somewhat confused.\n> \n> The description is confusing, yes.  It should be about git fetch,\n> not git pull.\n\nHowever, it was a \"git pull origin master:master\" that was the issue.  \nThat's why I wrote it in the commit message.\n\n> > This \"confusion\" has been there for very long time and (at least the \n> > scripted version of) git-pull/git-fetch pair has supported a \n> > workaround in the form of --update-head-ok option.\n\nGood point.  I completely had forgotten about that feature.\n\n> I think \"git fetch url side:master\" when master is the current branch \n> and we have omitted --update-head-ok is broken.  Specifically Dscho's \n> last hunk which adds this test.  The test fails on current master.\n\nThanks for testing; I ran out of my Git time budget yesterday.  Will send \nan updated patch as a reply to Daniel's reply.\n\nCiao,\nDscho\n"},{"id":"92918","messageId":"alpine.DEB.1.00.0810131129110.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15858","inReplyTo":"alpine.LNX.1.00.0810121501590.19665@iabervon.org","subject":"[PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-13T09:36:52Z","receivedAt":"2008-10-13T09:36:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nSome confusing tutorials suggested that it would be a good idea to fetch\ninto the current branch with something like this:\n\n\tgit fetch origin master:master\n\n(or even worse: the same command line with \"pull\" instead of \"fetch\").\nWhile it might make sense to store what you want to pull, it typically\nis plain wrong when the current branch is \"master\".\n\nAs noticed by Junio, this behavior should be triggered by _not_ passing\nthe --update-head-ok option, but somewhere along the lines we lost that\nbehavior.\n\nNOTE: this patch does not completely resurrect the original behavior\nwithout --update-head-ok: the check for the current branch is now _only_\nperformed in non-bare repositories.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tOn Sun, 12 Oct 2008, Daniel Barkalow and Shawn Pearce sent some \n\tgood suggestions to fix the first version of the patch.\n\n\tThis is the updated version.  In particular, the test is only \n\tperformed without the option \"--update-head-ok\", the function \n\tbranch_get(NULL) is called to get the current branch, a test was \n\tadded to verify that --update-head-ok works as expected, and the \n\tcommit message was modified in the hope that it confuses less now.\n\n\tStrangely, some more tests refused to pass this time, because they\n\tdid not use --update-head-ok; this was fixed, too.\n\n builtin-fetch.c             |   15 +++++++++++++++\n t/t5405-send-pack-rewind.sh |    2 +-\n t/t5505-remote.sh           |    2 +-\n t/t5510-fetch.sh            |   12 ++++++++++++\n t/t9300-fast-import.sh      |    2 +-\n 5 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex ee93d3a..57c161d 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -534,6 +534,19 @@ static void find_non_local_tags(struct transport *transport,\n \tstring_list_clear(&new_refs, 0);\n }\n \n+static void check_not_current_branch(struct ref *ref_map)\n+{\n+\tstruct branch *current_branch = branch_get(NULL);\n+\n+\tif (is_bare_repository() || !current_branch)\n+\t\treturn;\n+\n+\tfor (; ref_map; ref_map = ref_map->next)\n+\t\tif (ref_map->peer_ref && !strcmp(current_branch->refname,\n+\t\t\t\t\tref_map->peer_ref->name))\n+\t\t\tdie(\"Refusing to fetch into current branch\");\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t    struct refspec *refs, int ref_count)\n {\n@@ -558,6 +571,8 @@ static int do_fetch(struct transport *transport,\n \t}\n \n \tref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);\n+\tif (!update_head_ok)\n+\t\tcheck_not_current_branch(ref_map);\n \n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tif (rm->peer_ref)\ndiff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh\nindex 86abc62..cb9aacc 100755\n--- a/t/t5405-send-pack-rewind.sh\n+++ b/t/t5405-send-pack-rewind.sh\n@@ -12,7 +12,7 @@ test_expect_success setup '\n \tmkdir another && (\n \t\tcd another &&\n \t\tgit init &&\n-\t\tgit fetch .. master:master\n+\t\tgit fetch --update-head-ok .. master:master\n \t) &&\n \n \t>file2 && git add file2 && test_tick &&\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex c449663..0103e1a 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -188,7 +188,7 @@ test_expect_success 'prune --dry-run' '\n test_expect_success 'add --mirror && prune' '\n \t(mkdir mirror &&\n \t cd mirror &&\n-\t git init &&\n+\t git init --bare &&\n \t git remote add --mirror -f origin ../one) &&\n \t(cd one &&\n \t git branch -m side2 side) &&\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 9aae496..9e679b4 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -323,4 +323,16 @@ test_expect_success 'auto tag following fetches minimum' '\n \t)\n '\n \n+test_expect_success 'refuse to fetch into the current branch' '\n+\n+\ttest_must_fail git fetch . side:master\n+\n+'\n+\n+test_expect_success 'fetch into the current branch with --update-head-ok' '\n+\n+\tgit fetch --update-head-ok . side:master\n+\n+'\n+\n test_done\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 328444a..91b5ace 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -983,7 +983,7 @@ test_expect_success \\\n \t git checkout subuse1 &&\n \t rm -rf sub && mkdir sub && cd sub &&\n \t git init &&\n-\t git fetch .. refs/heads/sub:refs/heads/master &&\n+\t git fetch --update-head-ok .. refs/heads/sub:refs/heads/master &&\n \t git checkout master &&\n \t cd .. &&\n \t git submodule init &&\n-- \n1.6.0.2.749.g0cc32\n"},{"id":"92932","messageId":"20081013140938.GM4856@spearce.org","threadId":"15858","inReplyTo":"alpine.DEB.1.00.0810131129110.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-10-13T14:09:38Z","receivedAt":"2008-10-13T14:09:38Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> \n> Some confusing tutorials suggested that it would be a good idea to fetch\n> into the current branch with something like this:\n> \n> \tgit fetch origin master:master\n> \n> (or even worse: the same command line with \"pull\" instead of \"fetch\").\n> While it might make sense to store what you want to pull, it typically\n> is plain wrong when the current branch is \"master\".\n> \n> As noticed by Junio, this behavior should be triggered by _not_ passing\n> the --update-head-ok option, but somewhere along the lines we lost that\n> behavior.\n> \n> NOTE: this patch does not completely resurrect the original behavior\n> without --update-head-ok: the check for the current branch is now _only_\n> performed in non-bare repositories.\n> \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nAcked-by: Shawn O. Pearce <spearce@spearce.org>\n\n> \tStrangely, some more tests refused to pass this time, because they\n> \tdid not use --update-head-ok; this was fixed, too.\n\nNot strange, --update-head-ok was busted and the tests took advantage\nof it.  :-\\\n \n-- \nShawn.\n"},{"id":"92933","messageId":"7vod1obmlh.fsf@gitster.siamese.dyndns.org","threadId":"15858","inReplyTo":"alpine.DEB.1.00.0810131129110.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-13T14:23:22Z","receivedAt":"2008-10-13T14:23:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Some confusing tutorials suggested that it would be a good idea to fetch\n> into the current branch with something like this:\n>\n> \tgit fetch origin master:master\n>\n> (or even worse: the same command line with \"pull\" instead of \"fetch\").\n> While it might make sense to store what you want to pull, it typically\n> is plain wrong when the current branch is \"master\".\n>\n> As noticed by Junio, this behavior should be triggered by _not_ passing\n> the --update-head-ok option, but somewhere along the lines we lost that\n> behavior.\n\nDo you mean, by \"this behavior should be triggered\", \"we should allow\nupdating the current branch head only when --update-head-ok is given\", in\nother words, \"we should error out if the user tries to update the current\nhead with git-fetch without passing --update-head-ok\"?\n\n> NOTE: this patch does not completely resurrect the original behavior\n> without --update-head-ok: the check for the current branch is now _only_\n> performed in non-bare repositories.\n\nI think that is a sensible improvement.\n\n> \tStrangely, some more tests refused to pass this time, because they\n> \tdid not use --update-head-ok; this was fixed, too.\n\nWe need to look at these changes a bit carefully, as changes to existing\ntests can be either (1) fixing those that depended on broken behaviour of\nthe command, or (2) trying to hide regressions introduced by the patch\nunder the rug.\n\n>  t/t5405-send-pack-rewind.sh |    2 +-\n>  t/t5505-remote.sh           |    2 +-\n>  t/t5510-fetch.sh            |   12 ++++++++++++\n>  t/t9300-fast-import.sh      |    2 +-\n\nI suspect all of these offending tests came after b888d61 (Make fetch a\nbuiltin, 2007-09-10) which lacked the necessary check in do_fetch() to\ncause the regression you are fixing (iow, I am suspecting that the\nbrokenness of the tests were hidden by the breakage you are fixing).  The\nparts of the tests you fixed came from these:\n\n    6738c81 (send-pack: segfault fix on forced push, 2007-11-08)\n    4ebc914 (builtin-remote: prune remotes correctly ..., 2008-02-29)\n    4942025 (t5510: test \"git fetch\" following tags minimally, 2008-09-21)\n    03db452 (Support gitlinks in fast-import., 2008-07-19)\n    \nall of which are indeed descendants of b888d61.\n\nWith these verified, I think I should move the \"Strangely\" comment to the\ncommit log message proper (after stripping \"Strangely\" part -- it is not\nstrange anymore after we understand why).\n"},{"id":"92944","messageId":"alpine.LNX.1.00.0810131305200.19665@iabervon.org","threadId":"15858","inReplyTo":"alpine.DEB.1.00.0810131129110.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-10-13T17:08:50Z","receivedAt":"2008-10-13T17:08:50Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 13 Oct 2008, Johannes Schindelin wrote:\n\n> \n> Some confusing tutorials suggested that it would be a good idea to fetch\n> into the current branch with something like this:\n> \n> \tgit fetch origin master:master\n> \n> (or even worse: the same command line with \"pull\" instead of \"fetch\").\n> While it might make sense to store what you want to pull, it typically\n> is plain wrong when the current branch is \"master\".\n> \n> As noticed by Junio, this behavior should be triggered by _not_ passing\n> the --update-head-ok option, but somewhere along the lines we lost that\n> behavior.\n> \n> NOTE: this patch does not completely resurrect the original behavior\n> without --update-head-ok: the check for the current branch is now _only_\n> performed in non-bare repositories.\n> \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nAcked-by: Daniel Barkalow <barkalow@iabervon.org>\n"},{"id":"92946","messageId":"7vod1o9zd4.fsf@gitster.siamese.dyndns.org","threadId":"15858","inReplyTo":"7vod1obmlh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-13T17:30:31Z","receivedAt":"2008-10-13T17:30:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> With these verified, I think I should move the \"Strangely\" comment to the\n> commit log message proper (after stripping \"Strangely\" part -- it is not\n> strange anymore after we understand why).\n\nProposed amendment to the commit log message is:\n\n    Fix fetch/pull when run without --update-head-ok\n    \n    Some confusing tutorials suggested that it would be a good idea to fetch\n    into the current branch with something like this:\n    \n    \tgit fetch origin master:master\n    \n    (or even worse: the same command line with \"pull\" instead of \"fetch\").\n    While it might make sense to store what you want to pull, it typically is\n    plain wrong when the current branch is \"master\".  This should only be\n    allowed when (an incorrect) \"git pull origin master:master\" tries to work\n    around by giving --update-head-ok to underlying \"git fetch\", and otherwise\n    we should refuse it, but somewhere along the lines we lost that behavior.\n    \n    The check for the current branch is now _only_ performed in non-bare\n    repositories, which is an improvement from the original behaviour.\n    \n    Some newer tests were depending on the broken behaviour of \"git fetch\"\n    this patch fixes, and have been adjusted.\n    \n    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n    Acked-by: Shawn O. Pearce <spearce@spearce.org>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nThanks for the fix.\n"},{"id":"92952","messageId":"alpine.DEB.1.00.0810131956360.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15858","inReplyTo":"20081013140938.GM4856@spearce.org","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-13T17:57:41Z","receivedAt":"2008-10-13T17:57:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 13 Oct 2008, Shawn O. Pearce wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> > \n> > \tStrangely, some more tests refused to pass this time, because they \n> > \tdid not use --update-head-ok; this was fixed, too.\n> \n> Not strange, --update-head-ok was busted and the tests took advantage of \n> it.  :-\\\n\nHeh.  My \"this time\" was meant as \"since the first revision of the patch\".  \nI really tested the first revision, promise!  And those tests did not fail \nback then.  Or I am even stupider than I am prepared to admit.\n\nCiao,\nDscho\n"},{"id":"92954","messageId":"alpine.DEB.1.00.0810132001230.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15858","inReplyTo":"7vod1obmlh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-13T18:12:43Z","receivedAt":"2008-10-13T18:12:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nthanks for doing the due diligence that I should have done (but I ran out \nof time).\n\nOn Mon, 13 Oct 2008, Junio C Hamano wrote:\n\n> The parts of the tests you fixed came from these:\n> \n>     6738c81 (send-pack: segfault fix on forced push, 2007-11-08)\n\nThis really wants to make sure that no objects are shared or hard-linked \nbetween the repository in \"trash directory/\" and the one in its \nsubdirectory \"another/\".  It predates \"test_must_fail\", too, it seems.\n\nIt never touches the working directory \"another/\", so using \n--update-head-ok is okay.\n\n>     4ebc914 (builtin-remote: prune remotes correctly ..., 2008-02-29)\n\nThis tests \"git remote add\"'s --mirror option.\n\nIt never touches the working directory either.\n\n>     4942025 (t5510: test \"git fetch\" following tags minimally, 2008-09-21)\n\nThis test is actually not fixed, but contains two test cases for the issue \nthe commit fixes.\n\n>     03db452 (Support gitlinks in fast-import., 2008-07-19)\n\nThis creates an empty repository for tests to fast-import, and fetches \ninto the current (not yet existing) branch.\n\nI actually understand now why the tests started failing: the change from \nresolve_ref() to get_branch() as requested by Daniel are at fault: \nget_branch() does not check if the branch has an initial commit.\n\nI am actually regretting making this change.  Daniel, do you agree that it \nmight be better to change back to resolve_ref(), so that the initial \ncomplaint (IIRC Han-Wen git pull'ed into a freshly initialized repository \nwith that utterly bogus \"git pull origin master:master\" line) is not \nre-raised?\n\n> With these verified, I think I should move the \"Strangely\" comment to \n> the commit log message proper (after stripping \"Strangely\" part -- it is \n> not strange anymore after we understand why).\n\nThe only test that would need fixing after reverting back to resolve_ref() \nwould be the \"remote add --mirror\" thing, which I think should be fine.\n\nCiao,\nDscho\n"},{"id":"92961","messageId":"alpine.LNX.1.00.0810131546180.19665@iabervon.org","threadId":"15858","inReplyTo":"alpine.DEB.1.00.0810132001230.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-10-13T20:05:50Z","receivedAt":"2008-10-13T20:05:50Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 13 Oct 2008, Johannes Schindelin wrote:\n\n> I actually understand now why the tests started failing: the change from \n> resolve_ref() to get_branch() as requested by Daniel are at fault: \n> get_branch() does not check if the branch has an initial commit.\n> \n> I am actually regretting making this change.  Daniel, do you agree that it \n> might be better to change back to resolve_ref(), so that the initial \n> complaint (IIRC Han-Wen git pull'ed into a freshly initialized repository \n> with that utterly bogus \"git pull origin master:master\" line) is not \n> re-raised?\n\nIs it, in fact, okay to fetch into the current branch if it's \"yet to be \nborn\"? I feel like it shouldn't be, since you'll get exactly the same \nproblem that you would if the branch already existed: the index reflects \nthe previous state (in this case, it's empty), so git will show that \nyou've staged removing all of the files, right? So this would make the \ncheck for --update-head-ok more strict than before, but I think the \nbehavior change is correct.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"92996","messageId":"alpine.DEB.1.00.0810141145491.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15858","inReplyTo":"alpine.LNX.1.00.0810131546180.19665@iabervon.org","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-14T09:49:06Z","receivedAt":"2008-10-14T09:49:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 13 Oct 2008, Daniel Barkalow wrote:\n\n> On Mon, 13 Oct 2008, Johannes Schindelin wrote:\n> \n> > I actually understand now why the tests started failing: the change from \n> > resolve_ref() to get_branch() as requested by Daniel are at fault: \n> > get_branch() does not check if the branch has an initial commit.\n> > \n> > I am actually regretting making this change.  Daniel, do you agree \n> > that it might be better to change back to resolve_ref(), so that the \n> > initial complaint (IIRC Han-Wen git pull'ed into a freshly initialized \n> > repository with that utterly bogus \"git pull origin master:master\" \n> > line) is not re-raised?\n> \n> Is it, in fact, okay to fetch into the current branch if it's \"yet to be \n> born\"? I feel like it shouldn't be, since you'll get exactly the same \n> problem that you would if the branch already existed: the index reflects \n> the previous state (in this case, it's empty), so git will show that \n> you've staged removing all of the files, right? So this would make the \n> check for --update-head-ok more strict than before, but I think the \n> behavior change is correct.\n\nI think \nhttp://thread.gmane.org/gmane.comp.version-control.git/31351/focus=31544 \nis the best link to see what Han-Wen said.  Granted, it was a \nmisunderstanding on his part, but there have been quite a few people with \nthe same misunderstanding.\n\nSo what they did was\n\n\t$ mkdir just-one-branch\n\t$ cd just-one-branch\n\t$ git init\n\t$ git remote add origin <url>\n\t$ git pull origin master:master\n\nAnd this _will_ work correctly.  Except when using get_branch(NULL) \ninstead of the validating resolve_ref().\n\nWhen we talk about not breaking existing behavior, we have to talk about \nthis behavior, too.\n\nSo, my vote is to revert back to resolve_ref(), even if it needs more \nlines.\n\nThoughts of others?\n\nCiao,\nDscho\n"},{"id":"93008","messageId":"20081014150227.GC4856@spearce.org","threadId":"15858","inReplyTo":"alpine.DEB.1.00.0810141145491.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-10-14T15:02:27Z","receivedAt":"2008-10-14T15:02:27Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> On Mon, 13 Oct 2008, Daniel Barkalow wrote:\n> \n> > On Mon, 13 Oct 2008, Johannes Schindelin wrote:\n> > \n> > > I actually understand now why the tests started failing: the change from \n> > > resolve_ref() to get_branch() as requested by Daniel are at fault: \n> > > get_branch() does not check if the branch has an initial commit.\n> \n> So, my vote is to revert back to resolve_ref(), even if it needs more \n> lines.\n\nYes, I agree, resolve_ref() is the best thing to be using here,\neven if it is more code.  get_branch() validates the commit and we\ndon't want that.  We really just want to know if the current branch\nis going to be updated, we don't care to what/why.\n\n-- \nShawn.\n"},{"id":"93011","messageId":"alpine.LNX.1.00.0810141148010.19665@iabervon.org","threadId":"15858","inReplyTo":"alpine.DEB.1.00.0810141145491.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-10-14T15:57:09Z","receivedAt":"2008-10-14T15:57:09Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 14 Oct 2008, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Mon, 13 Oct 2008, Daniel Barkalow wrote:\n> \n> > On Mon, 13 Oct 2008, Johannes Schindelin wrote:\n> > \n> > > I actually understand now why the tests started failing: the change from \n> > > resolve_ref() to get_branch() as requested by Daniel are at fault: \n> > > get_branch() does not check if the branch has an initial commit.\n> > > \n> > > I am actually regretting making this change.  Daniel, do you agree \n> > > that it might be better to change back to resolve_ref(), so that the \n> > > initial complaint (IIRC Han-Wen git pull'ed into a freshly initialized \n> > > repository with that utterly bogus \"git pull origin master:master\" \n> > > line) is not re-raised?\n> > \n> > Is it, in fact, okay to fetch into the current branch if it's \"yet to be \n> > born\"? I feel like it shouldn't be, since you'll get exactly the same \n> > problem that you would if the branch already existed: the index reflects \n> > the previous state (in this case, it's empty), so git will show that \n> > you've staged removing all of the files, right? So this would make the \n> > check for --update-head-ok more strict than before, but I think the \n> > behavior change is correct.\n> \n> I think \n> http://thread.gmane.org/gmane.comp.version-control.git/31351/focus=31544 \n> is the best link to see what Han-Wen said.  Granted, it was a \n> misunderstanding on his part, but there have been quite a few people with \n> the same misunderstanding.\n> \n> So what they did was\n> \n> \t$ mkdir just-one-branch\n> \t$ cd just-one-branch\n> \t$ git init\n> \t$ git remote add origin <url>\n> \t$ git pull origin master:master\n> \n> And this _will_ work correctly.  Except when using get_branch(NULL) \n> instead of the validating resolve_ref().\n\n\"git pull origin master:master\" invokes \"git fetch\" with --update-head-ok, \nso it doesn't matter for this case what \"git fetch\" does without \n--update-head-ok.\n\nThe check would block:\n\n$ git fetch origin master:master\n\nbut this also would fail to update the working directory and would leave \nthe index as if you're removing everything.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"93012","messageId":"alpine.LNX.1.00.0810141200140.19665@iabervon.org","threadId":"15858","inReplyTo":"20081014150227.GC4856@spearce.org","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-10-14T16:04:43Z","receivedAt":"2008-10-14T16:04:43Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 14 Oct 2008, Shawn O. Pearce wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> > On Mon, 13 Oct 2008, Daniel Barkalow wrote:\n> > \n> > > On Mon, 13 Oct 2008, Johannes Schindelin wrote:\n> > > \n> > > > I actually understand now why the tests started failing: the change from \n> > > > resolve_ref() to get_branch() as requested by Daniel are at fault: \n> > > > get_branch() does not check if the branch has an initial commit.\n> > \n> > So, my vote is to revert back to resolve_ref(), even if it needs more \n> > lines.\n> \n> Yes, I agree, resolve_ref() is the best thing to be using here,\n> even if it is more code.  get_branch() validates the commit and we\n> don't want that.  We really just want to know if the current branch\n> is going to be updated, we don't care to what/why.\n\nIt doesn't validate the commit; it doesn't even validate the symref. The \nresolve_ref()-using code validates the symref, and I think that's an \nerror; we also don't care what state we'd update the current branch from.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"93013","messageId":"alpine.DEB.1.00.0810141808020.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15858","inReplyTo":"20081014150227.GC4856@spearce.org","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-14T16:15:07Z","receivedAt":"2008-10-14T16:15:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 14 Oct 2008, Shawn O. Pearce wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> > On Mon, 13 Oct 2008, Daniel Barkalow wrote:\n> > \n> > > On Mon, 13 Oct 2008, Johannes Schindelin wrote:\n> > > \n> > > > I actually understand now why the tests started failing: the change from \n> > > > resolve_ref() to get_branch() as requested by Daniel are at fault: \n> > > > get_branch() does not check if the branch has an initial commit.\n> > \n> > So, my vote is to revert back to resolve_ref(), even if it needs more \n> > lines.\n> \n> Yes, I agree, resolve_ref() is the best thing to be using here,\n> even if it is more code.  get_branch() validates the commit and we\n> don't want that.  We really just want to know if the current branch\n> is going to be updated, we don't care to what/why.\n\nActually, get_branch() does _not_ validate.  Which is why it thinks that \nthe current branch is \"master\" even if there is no commit yet.\n\nOTOH, resolve_ref() reads the SHA-1 of the ref, which fails in the case \nthat there has not been any commit yet.\n\nStill, I think that it would be nice to allow \"git pull origin \nmaster:master\" in a freshly initied non-bare repository, so I like \nresolve_ref() better.\n\nCiao,\nDscho\n"},{"id":"93014","messageId":"alpine.DEB.1.00.0810141815490.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15858","inReplyTo":"alpine.LNX.1.00.0810141148010.19665@iabervon.org","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-14T16:17:06Z","receivedAt":"2008-10-14T16:17:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 14 Oct 2008, Daniel Barkalow wrote:\n\n> On Tue, 14 Oct 2008, Johannes Schindelin wrote:\n> \n> > On Mon, 13 Oct 2008, Daniel Barkalow wrote:\n> > \n> > > On Mon, 13 Oct 2008, Johannes Schindelin wrote:\n> > > \n> > > > I actually understand now why the tests started failing: the \n> > > > change from resolve_ref() to get_branch() as requested by Daniel \n> > > > are at fault: get_branch() does not check if the branch has an \n> > > > initial commit.\n> > > > \n> > > > I am actually regretting making this change.  Daniel, do you agree \n> > > > that it might be better to change back to resolve_ref(), so that \n> > > > the initial complaint (IIRC Han-Wen git pull'ed into a freshly \n> > > > initialized repository with that utterly bogus \"git pull origin \n> > > > master:master\" line) is not re-raised?\n> > > \n> > > Is it, in fact, okay to fetch into the current branch if it's \"yet \n> > > to be born\"? I feel like it shouldn't be, since you'll get exactly \n> > > the same problem that you would if the branch already existed: the \n> > > index reflects the previous state (in this case, it's empty), so git \n> > > will show that you've staged removing all of the files, right? So \n> > > this would make the check for --update-head-ok more strict than \n> > > before, but I think the behavior change is correct.\n> > \n> > I think \n> > http://thread.gmane.org/gmane.comp.version-control.git/31351/focus=31544 \n> > is the best link to see what Han-Wen said.  Granted, it was a \n> > misunderstanding on his part, but there have been quite a few people \n> > with the same misunderstanding.\n> > \n> > So what they did was\n> > \n> > \t$ mkdir just-one-branch\n> > \t$ cd just-one-branch\n> > \t$ git init\n> > \t$ git remote add origin <url>\n> > \t$ git pull origin master:master\n> > \n> > And this _will_ work correctly.  Except when using get_branch(NULL) \n> > instead of the validating resolve_ref().\n> \n> \"git pull origin master:master\" invokes \"git fetch\" with --update-head-ok, \n\nDoes it?  You're correct.  I do not like it.\n\nCiao,\nDscho\n"},{"id":"93018","messageId":"alpine.LNX.1.00.0810141240510.19665@iabervon.org","threadId":"15858","inReplyTo":"alpine.DEB.1.00.0810141815490.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-10-14T16:52:49Z","receivedAt":"2008-10-14T16:52:49Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 14 Oct 2008, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Tue, 14 Oct 2008, Daniel Barkalow wrote:\n> \n> > On Tue, 14 Oct 2008, Johannes Schindelin wrote:\n> > \n> > > On Mon, 13 Oct 2008, Daniel Barkalow wrote:\n> > > \n> > > > On Mon, 13 Oct 2008, Johannes Schindelin wrote:\n> > > > \n> > > > > I actually understand now why the tests started failing: the \n> > > > > change from resolve_ref() to get_branch() as requested by Daniel \n> > > > > are at fault: get_branch() does not check if the branch has an \n> > > > > initial commit.\n> > > > > \n> > > > > I am actually regretting making this change.  Daniel, do you agree \n> > > > > that it might be better to change back to resolve_ref(), so that \n> > > > > the initial complaint (IIRC Han-Wen git pull'ed into a freshly \n> > > > > initialized repository with that utterly bogus \"git pull origin \n> > > > > master:master\" line) is not re-raised?\n> > > > \n> > > > Is it, in fact, okay to fetch into the current branch if it's \"yet \n> > > > to be born\"? I feel like it shouldn't be, since you'll get exactly \n> > > > the same problem that you would if the branch already existed: the \n> > > > index reflects the previous state (in this case, it's empty), so git \n> > > > will show that you've staged removing all of the files, right? So \n> > > > this would make the check for --update-head-ok more strict than \n> > > > before, but I think the behavior change is correct.\n> > > \n> > > I think \n> > > http://thread.gmane.org/gmane.comp.version-control.git/31351/focus=31544 \n> > > is the best link to see what Han-Wen said.  Granted, it was a \n> > > misunderstanding on his part, but there have been quite a few people \n> > > with the same misunderstanding.\n> > > \n> > > So what they did was\n> > > \n> > > \t$ mkdir just-one-branch\n> > > \t$ cd just-one-branch\n> > > \t$ git init\n> > > \t$ git remote add origin <url>\n> > > \t$ git pull origin master:master\n> > > \n> > > And this _will_ work correctly.  Except when using get_branch(NULL) \n> > > instead of the validating resolve_ref().\n> > \n> > \"git pull origin master:master\" invokes \"git fetch\" with --update-head-ok, \n> \n> Does it?  You're correct.  I do not like it.\n\nThe reason that it runs with --update-head-ok (and the reason that \n--update-head-ok exists in the first place) is that, when you're doing a \npull, if you fetch into the current branch, pull will identify that you've \nactually fast-forwarded the current branch and will update the working \ntree and index accordingly (which it's allowed to do because it's expected \nto perform a merge in the working tree and index).\n\nThat is, it uses --update-head-ok because \"git pull origin master:master\" \nwill work correctly, regardless of whether the local master is \nyet-to-be-born or not.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"93020","messageId":"alpine.LNX.1.00.0810141258050.19665@iabervon.org","threadId":"15858","inReplyTo":"alpine.LNX.1.00.0810141240510.19665@iabervon.org","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-10-14T17:02:35Z","receivedAt":"2008-10-14T17:02:35Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 14 Oct 2008, Daniel Barkalow wrote:\n\n> On Tue, 14 Oct 2008, Johannes Schindelin wrote:\n> \n> > Hi,\n> > \n> > On Tue, 14 Oct 2008, Daniel Barkalow wrote:\n> > \n> > > On Tue, 14 Oct 2008, Johannes Schindelin wrote:\n> > > \n> > > > On Mon, 13 Oct 2008, Daniel Barkalow wrote:\n> > > > \n> > > > > On Mon, 13 Oct 2008, Johannes Schindelin wrote:\n> > > > > \n> > > > > > I actually understand now why the tests started failing: the \n> > > > > > change from resolve_ref() to get_branch() as requested by Daniel \n> > > > > > are at fault: get_branch() does not check if the branch has an \n> > > > > > initial commit.\n> > > > > > \n> > > > > > I am actually regretting making this change.  Daniel, do you agree \n> > > > > > that it might be better to change back to resolve_ref(), so that \n> > > > > > the initial complaint (IIRC Han-Wen git pull'ed into a freshly \n> > > > > > initialized repository with that utterly bogus \"git pull origin \n> > > > > > master:master\" line) is not re-raised?\n> > > > > \n> > > > > Is it, in fact, okay to fetch into the current branch if it's \"yet \n> > > > > to be born\"? I feel like it shouldn't be, since you'll get exactly \n> > > > > the same problem that you would if the branch already existed: the \n> > > > > index reflects the previous state (in this case, it's empty), so git \n> > > > > will show that you've staged removing all of the files, right? So \n> > > > > this would make the check for --update-head-ok more strict than \n> > > > > before, but I think the behavior change is correct.\n> > > > \n> > > > I think \n> > > > http://thread.gmane.org/gmane.comp.version-control.git/31351/focus=31544 \n> > > > is the best link to see what Han-Wen said.  Granted, it was a \n> > > > misunderstanding on his part, but there have been quite a few people \n> > > > with the same misunderstanding.\n> > > > \n> > > > So what they did was\n> > > > \n> > > > \t$ mkdir just-one-branch\n> > > > \t$ cd just-one-branch\n> > > > \t$ git init\n> > > > \t$ git remote add origin <url>\n> > > > \t$ git pull origin master:master\n> > > > \n> > > > And this _will_ work correctly.  Except when using get_branch(NULL) \n> > > > instead of the validating resolve_ref().\n> > > \n> > > \"git pull origin master:master\" invokes \"git fetch\" with --update-head-ok, \n> > \n> > Does it?  You're correct.  I do not like it.\n> \n> The reason that it runs with --update-head-ok (and the reason that \n> --update-head-ok exists in the first place) is that, when you're doing a \n> pull, if you fetch into the current branch, pull will identify that you've \n> actually fast-forwarded the current branch and will update the working \n> tree and index accordingly (which it's allowed to do because it's expected \n> to perform a merge in the working tree and index).\n> \n> That is, it uses --update-head-ok because \"git pull origin master:master\" \n> will work correctly, regardless of whether the local master is \n> yet-to-be-born or not.\n\nIn particular, the --update-head-ok is from b10ac50f, which is what added \nthe check to prohibit fetching into the current branch otherwise, back in \nAugust 2005. There's never been anything preventing updating the current \nbranch using \"pull\".\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"93033","messageId":"7vk5ca7rw3.fsf@gitster.siamese.dyndns.org","threadId":"15858","inReplyTo":"alpine.LNX.1.00.0810141258050.19665@iabervon.org","subject":"Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-14T22:07:08Z","receivedAt":"2008-10-14T22:07:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> On Tue, 14 Oct 2008, Daniel Barkalow wrote:\n> ...\n>> That is, it uses --update-head-ok because \"git pull origin master:master\" \n>> will work correctly, regardless of whether the local master is \n>> yet-to-be-born or not.\n>\n> In particular, the --update-head-ok is from b10ac50f, which is what added \n> the check to prohibit fetching into the current branch otherwise, back in \n> August 2005. There's never been anything preventing updating the current \n> branch using \"pull\".\n\nWhat you wrote above is correct, and it is all that is necessary to make\n\"pull master:master\" (when 'master' is the current branch) work correctly,\nin normal situations.  Dscho's patch is in itself is a good thing to do,\nbut is not necessary when the caller gives --update-head-ok.\n\nNor is it sufficient.  Have you tested the current \"git-pull\" with or\nwithout Dscho's patch applied to \"git-fetch\" when 'master' is an unborn\nbranch?  \"git-pull\" has this piece:\n\n        curr_head=$(git rev-parse --verify HEAD 2>/dev/null)\n        if test \"$curr_head\" != \"$orig_head\"\n        then\n                # The fetch involved updating the current branch.\n\n                # The working tree and the index file is still based on the\n                # $orig_head commit, but we are merging into $curr_head.\n                # First update the working tree to match $curr_head.\n\n                echo >&2 \"Warning: fetch updated the current branch head.\"\n                ...\n        fi\n\nThe above part (written by yours truly) did not care what happens when the\ncurrent branch is unborn.  Back then when git-pull was originally written,\nnobody was crazy enough to pull into an empty branch and expect it to\nwork.  The code to allow that craziness (look for \"initial pull\" in the\nsame script to find a 6-line block near the end) came much later, and the\ncommit that added the \"initial pull\" support should have adjusted the\nabove codeblock if it really wanted to support pulling into an unborn\nbranch, but it forgot to do so.  It also forgot to handle the case where\nthe originally unborn current branch was fetched into.\n\nIf we really care about is \"git pull origin master:master\" into an unborn\nbranch, I think we need the attached patch on top, and this is regardless\nof Dscho's patch that tightens the check when -update-head-ok is not given.\n\n git-pull.sh |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git c/git-pull.sh w/git-pull.sh\nindex 75c3610..664fe34 100755\n--- c/git-pull.sh\n+++ w/git-pull.sh\n@@ -124,7 +124,7 @@ orig_head=$(git rev-parse --verify HEAD 2>/dev/null)\n git fetch --update-head-ok \"$@\" || exit 1\n \n curr_head=$(git rev-parse --verify HEAD 2>/dev/null)\n-if test \"$curr_head\" != \"$orig_head\"\n+if test -n \"$orig_head\" && test \"$curr_head\" != \"$orig_head\"\n then\n \t# The fetch involved updating the current branch.\n \n@@ -172,7 +172,7 @@ esac\n \n if test -z \"$orig_head\"\n then\n-\tgit update-ref -m \"initial pull\" HEAD $merge_head \"\" &&\n+\tgit update-ref -m \"initial pull\" HEAD $merge_head \"$curr_head\" &&\n \tgit read-tree --reset -u HEAD || exit 1\n \texit\n fi\n"},{"id":"93040","messageId":"7vmyh66b5z.fsf@gitster.siamese.dyndns.org","threadId":"15858","inReplyTo":"7vk5ca7rw3.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] pull: allow \"git pull origin $something:$current_branch\" into an unborn branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-14T22:53:44Z","receivedAt":"2008-10-14T22:53:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Some misguided documents floating on the Net suggests this sequence:\n\n    mkdir newdir && cd newdir\n    git init\n    git remote add origin $url\n    git pull origin master:master\n\n\"git pull\" has known about an misguided \"pull\" like this where the\nunderlying fetch updates the current branch for a long time, and it also\nhas known about another form of insanity \"git pull origin master\" into an\nunborn branch, but these two workarounds were not aware of the existence\nof each other and did not work well together.\n\nThis fixes it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This comes on top of js/maint-fetch-update-head.  Passes all the tests,\n   and also its own.\n\n git-pull.sh     |    4 ++--\n t/t5520-pull.sh |   12 ++++++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/git-pull.sh b/git-pull.sh\nindex 75c3610..664fe34 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -124,7 +124,7 @@ orig_head=$(git rev-parse --verify HEAD 2>/dev/null)\n git fetch --update-head-ok \"$@\" || exit 1\n \n curr_head=$(git rev-parse --verify HEAD 2>/dev/null)\n-if test \"$curr_head\" != \"$orig_head\"\n+if test -n \"$orig_head\" && test \"$curr_head\" != \"$orig_head\"\n then\n \t# The fetch involved updating the current branch.\n \n@@ -172,7 +172,7 @@ esac\n \n if test -z \"$orig_head\"\n then\n-\tgit update-ref -m \"initial pull\" HEAD $merge_head \"\" &&\n+\tgit update-ref -m \"initial pull\" HEAD $merge_head \"$curr_head\" &&\n \tgit read-tree --reset -u HEAD || exit 1\n \texit\n fi\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 997b2db..725771f 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -29,6 +29,18 @@ test_expect_success 'checking the results' '\n \tdiff file cloned/file\n '\n \n+test_expect_success 'pulling into void using master:master' '\n+\tmkdir cloned-uho &&\n+\t(\n+\t\tcd cloned-uho &&\n+\t\tgit init &&\n+\t\tgit pull .. master:master\n+\t) &&\n+\ttest -f file &&\n+\ttest -f cloned-uho/file &&\n+\ttest_cmp file cloned-uho/file\n+'\n+\n test_expect_success 'test . as a remote' '\n \n \tgit branch copy master &&\n-- \n1.6.0.2.711.gf1ba4\n"}]}