[PATCH] fetch: refuse to fetch into the current branch in a non-bare repository

[PATCH] fetch: refuse to fetch into the current branch in a non-bare repository

[Johannes Schindelin]

Some confusing tutorials suggest that it would be a good idea to call
something like this:

	git pull origin master:master

While it might make sense to store what you want to merge, it typically
is plain wrong.  Especially so when the current branch is "master".

Be at least a little bit helpful by refusing to fetch something into
the current branch.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-fetch.c   |   20 ++++++++++++++++++++
 t/t5505-remote.sh |    2 +-
 t/t5510-fetch.sh  |    6 ++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index ee93d3a..d701550 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -534,6 +534,25 @@ static void find_non_local_tags(struct transport *transport,
 	string_list_clear(&new_refs, 0);
 }
 
+static void check_ref_map(struct ref *ref_map)
+{
+	int flag;
+	unsigned char sha1[20];
+	const char *HEAD;
+
+	if (is_bare_repository())
+		return;
+
+	HEAD = resolve_ref("HEAD", sha1, 1, &flag);
+
+	if (!HEAD || !(flag & REF_ISSYMREF))
+		return;
+
+	for (; ref_map; ref_map = ref_map->next)
+		if (ref_map->peer_ref && !strcmp(HEAD, ref_map->peer_ref->name))
+			die("Refusing to fetch into current branch");
+}
+
 static int do_fetch(struct transport *transport,
 		    struct refspec *refs, int ref_count)
 {
@@ -558,6 +577,7 @@ static int do_fetch(struct transport *transport,
 	}
 
 	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
+	check_ref_map(ref_map);
 
 	for (rm = ref_map; rm; rm = rm->next) {
 		if (rm->peer_ref)
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c449663..0103e1a 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -188,7 +188,7 @@ test_expect_success 'prune --dry-run' '
 test_expect_success 'add --mirror && prune' '
 	(mkdir mirror &&
 	 cd mirror &&
-	 git init &&
+	 git init --bare &&
 	 git remote add --mirror -f origin ../one) &&
 	(cd one &&
 	 git branch -m side2 side) &&
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 9aae496..cd8b550 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -323,4 +323,10 @@ test_expect_success 'auto tag following fetches minimum' '
 	)
 '
 
+test_expect_success 'refuse to fetch into the current branch' '
+
+	test_must_fail git fetch . side:master
+
+'
+
 test_done
-- 
1.6.0.2.749.g0cc32

Re: [PATCH] fetch: refuse to fetch into the current branch in a non-bare repository

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Some confusing tutorials suggest that it would be a good idea to call
> something like this:
>
> 	git pull origin master:master
>
> While it might make sense to store what you want to merge, it typically
> is plain wrong.

I am somewhat confused.

This "confusion" has been there for very long time and (at least the
scripted version of) git-pull/git-fetch pair has supported a workaround in
the form of --update-head-ok option.

Have we broken the workaround ll.127..147 in git-pull.sh recently, or are
you trying to address something else?

Re: [PATCH] fetch: refuse to fetch into the current branch in a non-bare repository

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Some confusing tutorials suggest that it would be a good idea to call
> > something like this:
> >
> > 	git pull origin master:master
> >
> > While it might make sense to store what you want to merge, it typically
> > is plain wrong.
> 
> I am somewhat confused.

The description is confusing, yes.  It should be about git fetch,
not git pull.
 
> This "confusion" has been there for very long time and (at least the
> scripted version of) git-pull/git-fetch pair has supported a workaround in
> the form of --update-head-ok option.

I think "git fetch url side:master" when master is the current branch
and we have omitted --update-head-ok is broken.  Specifically Dscho's
last hunk which adds this test.  The test fails on current master.

Looking at the code in builtin-fetch.c, the only usage of
update_head_ok is for output about the current branch.  I think
it should have been used in at least one other spot, to decide if
the RHS of a refspec is valid for storage.  Dscho's patch tries to
address that.

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 9aae496..cd8b550 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -323,4 +323,10 @@ test_expect_success 'auto tag following fetches minimum' '
        )
 '

+test_expect_success 'refuse to fetch into the current branch' '
+
+	test_must_fail git fetch . side:master
+
+'
+
 test_done


-- 
Shawn.

Re: [PATCH] fetch: refuse to fetch into the current branch in a non-bare repository

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index ee93d3a..d701550 100644
> --- a/builtin-fetch.c
> +++ b/builtin-fetch.c
> @@ -534,6 +534,25 @@ static void find_non_local_tags(struct transport *transport,
>  	string_list_clear(&new_refs, 0);
>  }
>  
> +static void check_ref_map(struct ref *ref_map)
> +{
> +	int flag;
> +	unsigned char sha1[20];
> +	const char *HEAD;
> +
> +	if (is_bare_repository())
> +		return;
> +
> +	HEAD = resolve_ref("HEAD", sha1, 1, &flag);

I'd rather see local variables named lowercase.  Constants should
be the only thing that is all uppercase.

> @@ -558,6 +577,7 @@ static int do_fetch(struct transport *transport,
>  	}
>  
>  	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
> +	check_ref_map(ref_map);

This should only be called if update_head_ok is false.  So maybe:

	if (!update_head_ok)
		check_ref_map(ref_map)
  
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 9aae496..cd8b550 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -323,4 +323,10 @@ test_expect_success 'auto tag following fetches minimum' '
>  	)
>  '
>  
> +test_expect_success 'refuse to fetch into the current branch' '
> +
> +	test_must_fail git fetch . side:master
> +
> +'
> +

Repeat this test, but with --update-head-ok and expect success,
since the check_ref_map logic is conditional on that?

-- 
Shawn.

Re: [PATCH] fetch: refuse to fetch into the current branch in a non-bare repository

[Daniel Barkalow]

On Sat, 11 Oct 2008, Johannes Schindelin wrote:

> Some confusing tutorials suggest that it would be a good idea to call
> something like this:
> 
> 	git pull origin master:master
> 
> While it might make sense to store what you want to merge, it typically
> is plain wrong.  Especially so when the current branch is "master".
> 
> Be at least a little bit helpful by refusing to fetch something into
> the current branch.

I think this is the right thing to do.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin-fetch.c   |   20 ++++++++++++++++++++
>  t/t5505-remote.sh |    2 +-
>  t/t5510-fetch.sh  |    6 ++++++
>  3 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index ee93d3a..d701550 100644
> --- a/builtin-fetch.c
> +++ b/builtin-fetch.c
> @@ -534,6 +534,25 @@ static void find_non_local_tags(struct transport *transport,
>  	string_list_clear(&new_refs, 0);
>  }
>  
> +static void check_ref_map(struct ref *ref_map)
> +{
> +	int flag;
> +	unsigned char sha1[20];
> +	const char *HEAD;
> +
> +	if (is_bare_repository())
> +		return;
> +
> +	HEAD = resolve_ref("HEAD", sha1, 1, &flag);
> +
> +	if (!HEAD || !(flag & REF_ISSYMREF))
> +		return;

remote.h has a function for getting "the current branch", which would save 
5 lines here:

	struct branch *current_branch = branch_get(NULL);
	if (!current_branch || is_bare_repository())
		return;

> +
> +	for (; ref_map; ref_map = ref_map->next)
> +		if (ref_map->peer_ref && !strcmp(HEAD, ref_map->peer_ref->name))

		!strcmp(current_branch->ref_name, ref_map->peer_ref->name)

(untested, and might be off by a "refs/" or something)

> +			die("Refusing to fetch into current branch");
> +}
> +
>  static int do_fetch(struct transport *transport,
>  		    struct refspec *refs, int ref_count)
>  {
> @@ -558,6 +577,7 @@ static int do_fetch(struct transport *transport,
>  	}
>  
>  	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
> +	check_ref_map(ref_map);
>  
>  	for (rm = ref_map; rm; rm = rm->next) {
>  		if (rm->peer_ref)

Re: [PATCH] fetch: refuse to fetch into the current branch in a non-bare repository

[Johannes Schindelin]

Hi,

On Sun, 12 Oct 2008, Shawn O. Pearce wrote:

> Junio C Hamano <gitster@pobox.com> wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > Some confusing tutorials suggest that it would be a good idea to call
> > > something like this:
> > >
> > > 	git pull origin master:master
> > >
> > > While it might make sense to store what you want to merge, it typically
> > > is plain wrong.
> > 
> > I am somewhat confused.
> 
> The description is confusing, yes.  It should be about git fetch,
> not git pull.

However, it was a "git pull origin master:master" that was the issue.  
That's why I wrote it in the commit message.

> > This "confusion" has been there for very long time and (at least the 
> > scripted version of) git-pull/git-fetch pair has supported a 
> > workaround in the form of --update-head-ok option.

Good point.  I completely had forgotten about that feature.

> I think "git fetch url side:master" when master is the current branch 
> and we have omitted --update-head-ok is broken.  Specifically Dscho's 
> last hunk which adds this test.  The test fails on current master.

Thanks for testing; I ran out of my Git time budget yesterday.  Will send 
an updated patch as a reply to Daniel's reply.

Ciao,
Dscho

[PATCH v2] Fix fetch/pull when run without --update-head-ok

[Johannes Schindelin]

Some confusing tutorials suggested that it would be a good idea to fetch
into the current branch with something like this:

	git fetch origin master:master

(or even worse: the same command line with "pull" instead of "fetch").
While it might make sense to store what you want to pull, it typically
is plain wrong when the current branch is "master".

As noticed by Junio, this behavior should be triggered by _not_ passing
the --update-head-ok option, but somewhere along the lines we lost that
behavior.

NOTE: this patch does not completely resurrect the original behavior
without --update-head-ok: the check for the current branch is now _only_
performed in non-bare repositories.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sun, 12 Oct 2008, Daniel Barkalow and Shawn Pearce sent some 
	good suggestions to fix the first version of the patch.

	This is the updated version.  In particular, the test is only 
	performed without the option "--update-head-ok", the function 
	branch_get(NULL) is called to get the current branch, a test was 
	added to verify that --update-head-ok works as expected, and the 
	commit message was modified in the hope that it confuses less now.

	Strangely, some more tests refused to pass this time, because they
	did not use --update-head-ok; this was fixed, too.

 builtin-fetch.c             |   15 +++++++++++++++
 t/t5405-send-pack-rewind.sh |    2 +-
 t/t5505-remote.sh           |    2 +-
 t/t5510-fetch.sh            |   12 ++++++++++++
 t/t9300-fast-import.sh      |    2 +-
 5 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index ee93d3a..57c161d 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -534,6 +534,19 @@ static void find_non_local_tags(struct transport *transport,
 	string_list_clear(&new_refs, 0);
 }
 
+static void check_not_current_branch(struct ref *ref_map)
+{
+	struct branch *current_branch = branch_get(NULL);
+
+	if (is_bare_repository() || !current_branch)
+		return;
+
+	for (; ref_map; ref_map = ref_map->next)
+		if (ref_map->peer_ref && !strcmp(current_branch->refname,
+					ref_map->peer_ref->name))
+			die("Refusing to fetch into current branch");
+}
+
 static int do_fetch(struct transport *transport,
 		    struct refspec *refs, int ref_count)
 {
@@ -558,6 +571,8 @@ static int do_fetch(struct transport *transport,
 	}
 
 	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
+	if (!update_head_ok)
+		check_not_current_branch(ref_map);
 
 	for (rm = ref_map; rm; rm = rm->next) {
 		if (rm->peer_ref)
diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
index 86abc62..cb9aacc 100755
--- a/t/t5405-send-pack-rewind.sh
+++ b/t/t5405-send-pack-rewind.sh
@@ -12,7 +12,7 @@ test_expect_success setup '
 	mkdir another && (
 		cd another &&
 		git init &&
-		git fetch .. master:master
+		git fetch --update-head-ok .. master:master
 	) &&
 
 	>file2 && git add file2 && test_tick &&
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index c449663..0103e1a 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -188,7 +188,7 @@ test_expect_success 'prune --dry-run' '
 test_expect_success 'add --mirror && prune' '
 	(mkdir mirror &&
 	 cd mirror &&
-	 git init &&
+	 git init --bare &&
 	 git remote add --mirror -f origin ../one) &&
 	(cd one &&
 	 git branch -m side2 side) &&
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 9aae496..9e679b4 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -323,4 +323,16 @@ test_expect_success 'auto tag following fetches minimum' '
 	)
 '
 
+test_expect_success 'refuse to fetch into the current branch' '
+
+	test_must_fail git fetch . side:master
+
+'
+
+test_expect_success 'fetch into the current branch with --update-head-ok' '
+
+	git fetch --update-head-ok . side:master
+
+'
+
 test_done
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 328444a..91b5ace 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -983,7 +983,7 @@ test_expect_success \
 	 git checkout subuse1 &&
 	 rm -rf sub && mkdir sub && cd sub &&
 	 git init &&
-	 git fetch .. refs/heads/sub:refs/heads/master &&
+	 git fetch --update-head-ok .. refs/heads/sub:refs/heads/master &&
 	 git checkout master &&
 	 cd .. &&
 	 git submodule init &&
-- 
1.6.0.2.749.g0cc32

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Some confusing tutorials suggested that it would be a good idea to fetch
> into the current branch with something like this:
> 
> 	git fetch origin master:master
> 
> (or even worse: the same command line with "pull" instead of "fetch").
> While it might make sense to store what you want to pull, it typically
> is plain wrong when the current branch is "master".
> 
> As noticed by Junio, this behavior should be triggered by _not_ passing
> the --update-head-ok option, but somewhere along the lines we lost that
> behavior.
> 
> NOTE: this patch does not completely resurrect the original behavior
> without --update-head-ok: the check for the current branch is now _only_
> performed in non-bare repositories.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Acked-by: Shawn O. Pearce <spearce@spearce.org>

> 	Strangely, some more tests refused to pass this time, because they
> 	did not use --update-head-ok; this was fixed, too.

Not strange, --update-head-ok was busted and the tests took advantage
of it.  :-\
 
-- 
Shawn.

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Some confusing tutorials suggested that it would be a good idea to fetch
> into the current branch with something like this:
>
> 	git fetch origin master:master
>
> (or even worse: the same command line with "pull" instead of "fetch").
> While it might make sense to store what you want to pull, it typically
> is plain wrong when the current branch is "master".
>
> As noticed by Junio, this behavior should be triggered by _not_ passing
> the --update-head-ok option, but somewhere along the lines we lost that
> behavior.

Do you mean, by "this behavior should be triggered", "we should allow
updating the current branch head only when --update-head-ok is given", in
other words, "we should error out if the user tries to update the current
head with git-fetch without passing --update-head-ok"?

> NOTE: this patch does not completely resurrect the original behavior
> without --update-head-ok: the check for the current branch is now _only_
> performed in non-bare repositories.

I think that is a sensible improvement.

> 	Strangely, some more tests refused to pass this time, because they
> 	did not use --update-head-ok; this was fixed, too.

We need to look at these changes a bit carefully, as changes to existing
tests can be either (1) fixing those that depended on broken behaviour of
the command, or (2) trying to hide regressions introduced by the patch
under the rug.

>  t/t5405-send-pack-rewind.sh |    2 +-
>  t/t5505-remote.sh           |    2 +-
>  t/t5510-fetch.sh            |   12 ++++++++++++
>  t/t9300-fast-import.sh      |    2 +-

I suspect all of these offending tests came after b888d61 (Make fetch a
builtin, 2007-09-10) which lacked the necessary check in do_fetch() to
cause the regression you are fixing (iow, I am suspecting that the
brokenness of the tests were hidden by the breakage you are fixing).  The
parts of the tests you fixed came from these:

    6738c81 (send-pack: segfault fix on forced push, 2007-11-08)
    4ebc914 (builtin-remote: prune remotes correctly ..., 2008-02-29)
    4942025 (t5510: test "git fetch" following tags minimally, 2008-09-21)
    03db452 (Support gitlinks in fast-import., 2008-07-19)
    
all of which are indeed descendants of b888d61.

With these verified, I think I should move the "Strangely" comment to the
commit log message proper (after stripping "Strangely" part -- it is not
strange anymore after we understand why).

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Daniel Barkalow]

On Mon, 13 Oct 2008, Johannes Schindelin wrote:

> 
> Some confusing tutorials suggested that it would be a good idea to fetch
> into the current branch with something like this:
> 
> 	git fetch origin master:master
> 
> (or even worse: the same command line with "pull" instead of "fetch").
> While it might make sense to store what you want to pull, it typically
> is plain wrong when the current branch is "master".
> 
> As noticed by Junio, this behavior should be triggered by _not_ passing
> the --update-head-ok option, but somewhere along the lines we lost that
> behavior.
> 
> NOTE: this patch does not completely resurrect the original behavior
> without --update-head-ok: the check for the current branch is now _only_
> performed in non-bare repositories.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> With these verified, I think I should move the "Strangely" comment to the
> commit log message proper (after stripping "Strangely" part -- it is not
> strange anymore after we understand why).

Proposed amendment to the commit log message is:

    Fix fetch/pull when run without --update-head-ok
    
    Some confusing tutorials suggested that it would be a good idea to fetch
    into the current branch with something like this:
    
    	git fetch origin master:master
    
    (or even worse: the same command line with "pull" instead of "fetch").
    While it might make sense to store what you want to pull, it typically is
    plain wrong when the current branch is "master".  This should only be
    allowed when (an incorrect) "git pull origin master:master" tries to work
    around by giving --update-head-ok to underlying "git fetch", and otherwise
    we should refuse it, but somewhere along the lines we lost that behavior.
    
    The check for the current branch is now _only_ performed in non-bare
    repositories, which is an improvement from the original behaviour.
    
    Some newer tests were depending on the broken behaviour of "git fetch"
    this patch fixes, and have been adjusted.
    
    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    Acked-by: Shawn O. Pearce <spearce@spearce.org>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks for the fix.

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Johannes Schindelin]

Hi,

On Mon, 13 Oct 2008, Shawn O. Pearce wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > 
> > 	Strangely, some more tests refused to pass this time, because they 
> > 	did not use --update-head-ok; this was fixed, too.
> 
> Not strange, --update-head-ok was busted and the tests took advantage of 
> it.  :-\

Heh.  My "this time" was meant as "since the first revision of the patch".  
I really tested the first revision, promise!  And those tests did not fail 
back then.  Or I am even stupider than I am prepared to admit.

Ciao,
Dscho

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Johannes Schindelin]

Hi,

thanks for doing the due diligence that I should have done (but I ran out 
of time).

On Mon, 13 Oct 2008, Junio C Hamano wrote:

> The parts of the tests you fixed came from these:
> 
>     6738c81 (send-pack: segfault fix on forced push, 2007-11-08)

This really wants to make sure that no objects are shared or hard-linked 
between the repository in "trash directory/" and the one in its 
subdirectory "another/".  It predates "test_must_fail", too, it seems.

It never touches the working directory "another/", so using 
--update-head-ok is okay.

>     4ebc914 (builtin-remote: prune remotes correctly ..., 2008-02-29)

This tests "git remote add"'s --mirror option.

It never touches the working directory either.

>     4942025 (t5510: test "git fetch" following tags minimally, 2008-09-21)

This test is actually not fixed, but contains two test cases for the issue 
the commit fixes.

>     03db452 (Support gitlinks in fast-import., 2008-07-19)

This creates an empty repository for tests to fast-import, and fetches 
into the current (not yet existing) branch.

I actually understand now why the tests started failing: the change from 
resolve_ref() to get_branch() as requested by Daniel are at fault: 
get_branch() does not check if the branch has an initial commit.

I am actually regretting making this change.  Daniel, do you agree that it 
might be better to change back to resolve_ref(), so that the initial 
complaint (IIRC Han-Wen git pull'ed into a freshly initialized repository 
with that utterly bogus "git pull origin master:master" line) is not 
re-raised?

> With these verified, I think I should move the "Strangely" comment to 
> the commit log message proper (after stripping "Strangely" part -- it is 
> not strange anymore after we understand why).

The only test that would need fixing after reverting back to resolve_ref() 
would be the "remote add --mirror" thing, which I think should be fine.

Ciao,
Dscho

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Daniel Barkalow]

On Mon, 13 Oct 2008, Johannes Schindelin wrote:

> I actually understand now why the tests started failing: the change from 
> resolve_ref() to get_branch() as requested by Daniel are at fault: 
> get_branch() does not check if the branch has an initial commit.
> 
> I am actually regretting making this change.  Daniel, do you agree that it 
> might be better to change back to resolve_ref(), so that the initial 
> complaint (IIRC Han-Wen git pull'ed into a freshly initialized repository 
> with that utterly bogus "git pull origin master:master" line) is not 
> re-raised?

Is it, in fact, okay to fetch into the current branch if it's "yet to be 
born"? I feel like it shouldn't be, since you'll get exactly the same 
problem that you would if the branch already existed: the index reflects 
the previous state (in this case, it's empty), so git will show that 
you've staged removing all of the files, right? So this would make the 
check for --update-head-ok more strict than before, but I think the 
behavior change is correct.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Johannes Schindelin]

Hi,

On Mon, 13 Oct 2008, Daniel Barkalow wrote:

> On Mon, 13 Oct 2008, Johannes Schindelin wrote:
> 
> > I actually understand now why the tests started failing: the change from 
> > resolve_ref() to get_branch() as requested by Daniel are at fault: 
> > get_branch() does not check if the branch has an initial commit.
> > 
> > I am actually regretting making this change.  Daniel, do you agree 
> > that it might be better to change back to resolve_ref(), so that the 
> > initial complaint (IIRC Han-Wen git pull'ed into a freshly initialized 
> > repository with that utterly bogus "git pull origin master:master" 
> > line) is not re-raised?
> 
> Is it, in fact, okay to fetch into the current branch if it's "yet to be 
> born"? I feel like it shouldn't be, since you'll get exactly the same 
> problem that you would if the branch already existed: the index reflects 
> the previous state (in this case, it's empty), so git will show that 
> you've staged removing all of the files, right? So this would make the 
> check for --update-head-ok more strict than before, but I think the 
> behavior change is correct.

I think 
http://thread.gmane.org/gmane.comp.version-control.git/31351/focus=31544 
is the best link to see what Han-Wen said.  Granted, it was a 
misunderstanding on his part, but there have been quite a few people with 
the same misunderstanding.

So what they did was

	$ mkdir just-one-branch
	$ cd just-one-branch
	$ git init
	$ git remote add origin <url>
	$ git pull origin master:master

And this _will_ work correctly.  Except when using get_branch(NULL) 
instead of the validating resolve_ref().

When we talk about not breaking existing behavior, we have to talk about 
this behavior, too.

So, my vote is to revert back to resolve_ref(), even if it needs more 
lines.

Thoughts of others?

Ciao,
Dscho

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Mon, 13 Oct 2008, Daniel Barkalow wrote:
> 
> > On Mon, 13 Oct 2008, Johannes Schindelin wrote:
> > 
> > > I actually understand now why the tests started failing: the change from 
> > > resolve_ref() to get_branch() as requested by Daniel are at fault: 
> > > get_branch() does not check if the branch has an initial commit.
> 
> So, my vote is to revert back to resolve_ref(), even if it needs more 
> lines.

Yes, I agree, resolve_ref() is the best thing to be using here,
even if it is more code.  get_branch() validates the commit and we
don't want that.  We really just want to know if the current branch
is going to be updated, we don't care to what/why.

-- 
Shawn.

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Daniel Barkalow]

On Tue, 14 Oct 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Mon, 13 Oct 2008, Daniel Barkalow wrote:
> 
> > On Mon, 13 Oct 2008, Johannes Schindelin wrote:
> > 
> > > I actually understand now why the tests started failing: the change from 
> > > resolve_ref() to get_branch() as requested by Daniel are at fault: 
> > > get_branch() does not check if the branch has an initial commit.
> > > 
> > > I am actually regretting making this change.  Daniel, do you agree 
> > > that it might be better to change back to resolve_ref(), so that the 
> > > initial complaint (IIRC Han-Wen git pull'ed into a freshly initialized 
> > > repository with that utterly bogus "git pull origin master:master" 
> > > line) is not re-raised?
> > 
> > Is it, in fact, okay to fetch into the current branch if it's "yet to be 
> > born"? I feel like it shouldn't be, since you'll get exactly the same 
> > problem that you would if the branch already existed: the index reflects 
> > the previous state (in this case, it's empty), so git will show that 
> > you've staged removing all of the files, right? So this would make the 
> > check for --update-head-ok more strict than before, but I think the 
> > behavior change is correct.
> 
> I think 
> http://thread.gmane.org/gmane.comp.version-control.git/31351/focus=31544 
> is the best link to see what Han-Wen said.  Granted, it was a 
> misunderstanding on his part, but there have been quite a few people with 
> the same misunderstanding.
> 
> So what they did was
> 
> 	$ mkdir just-one-branch
> 	$ cd just-one-branch
> 	$ git init
> 	$ git remote add origin <url>
> 	$ git pull origin master:master
> 
> And this _will_ work correctly.  Except when using get_branch(NULL) 
> instead of the validating resolve_ref().

"git pull origin master:master" invokes "git fetch" with --update-head-ok, 
so it doesn't matter for this case what "git fetch" does without 
--update-head-ok.

The check would block:

$ git fetch origin master:master

but this also would fail to update the working directory and would leave 
the index as if you're removing everything.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Daniel Barkalow]

On Tue, 14 Oct 2008, Shawn O. Pearce wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Mon, 13 Oct 2008, Daniel Barkalow wrote:
> > 
> > > On Mon, 13 Oct 2008, Johannes Schindelin wrote:
> > > 
> > > > I actually understand now why the tests started failing: the change from 
> > > > resolve_ref() to get_branch() as requested by Daniel are at fault: 
> > > > get_branch() does not check if the branch has an initial commit.
> > 
> > So, my vote is to revert back to resolve_ref(), even if it needs more 
> > lines.
> 
> Yes, I agree, resolve_ref() is the best thing to be using here,
> even if it is more code.  get_branch() validates the commit and we
> don't want that.  We really just want to know if the current branch
> is going to be updated, we don't care to what/why.

It doesn't validate the commit; it doesn't even validate the symref. The 
resolve_ref()-using code validates the symref, and I think that's an 
error; we also don't care what state we'd update the current branch from.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Johannes Schindelin]

Hi,

On Tue, 14 Oct 2008, Shawn O. Pearce wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Mon, 13 Oct 2008, Daniel Barkalow wrote:
> > 
> > > On Mon, 13 Oct 2008, Johannes Schindelin wrote:
> > > 
> > > > I actually understand now why the tests started failing: the change from 
> > > > resolve_ref() to get_branch() as requested by Daniel are at fault: 
> > > > get_branch() does not check if the branch has an initial commit.
> > 
> > So, my vote is to revert back to resolve_ref(), even if it needs more 
> > lines.
> 
> Yes, I agree, resolve_ref() is the best thing to be using here,
> even if it is more code.  get_branch() validates the commit and we
> don't want that.  We really just want to know if the current branch
> is going to be updated, we don't care to what/why.

Actually, get_branch() does _not_ validate.  Which is why it thinks that 
the current branch is "master" even if there is no commit yet.

OTOH, resolve_ref() reads the SHA-1 of the ref, which fails in the case 
that there has not been any commit yet.

Still, I think that it would be nice to allow "git pull origin 
master:master" in a freshly initied non-bare repository, so I like 
resolve_ref() better.

Ciao,
Dscho

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Johannes Schindelin]

Hi,

On Tue, 14 Oct 2008, Daniel Barkalow wrote:

> On Tue, 14 Oct 2008, Johannes Schindelin wrote:
> 
> > On Mon, 13 Oct 2008, Daniel Barkalow wrote:
> > 
> > > On Mon, 13 Oct 2008, Johannes Schindelin wrote:
> > > 
> > > > I actually understand now why the tests started failing: the 
> > > > change from resolve_ref() to get_branch() as requested by Daniel 
> > > > are at fault: get_branch() does not check if the branch has an 
> > > > initial commit.
> > > > 
> > > > I am actually regretting making this change.  Daniel, do you agree 
> > > > that it might be better to change back to resolve_ref(), so that 
> > > > the initial complaint (IIRC Han-Wen git pull'ed into a freshly 
> > > > initialized repository with that utterly bogus "git pull origin 
> > > > master:master" line) is not re-raised?
> > > 
> > > Is it, in fact, okay to fetch into the current branch if it's "yet 
> > > to be born"? I feel like it shouldn't be, since you'll get exactly 
> > > the same problem that you would if the branch already existed: the 
> > > index reflects the previous state (in this case, it's empty), so git 
> > > will show that you've staged removing all of the files, right? So 
> > > this would make the check for --update-head-ok more strict than 
> > > before, but I think the behavior change is correct.
> > 
> > I think 
> > http://thread.gmane.org/gmane.comp.version-control.git/31351/focus=31544 
> > is the best link to see what Han-Wen said.  Granted, it was a 
> > misunderstanding on his part, but there have been quite a few people 
> > with the same misunderstanding.
> > 
> > So what they did was
> > 
> > 	$ mkdir just-one-branch
> > 	$ cd just-one-branch
> > 	$ git init
> > 	$ git remote add origin <url>
> > 	$ git pull origin master:master
> > 
> > And this _will_ work correctly.  Except when using get_branch(NULL) 
> > instead of the validating resolve_ref().
> 
> "git pull origin master:master" invokes "git fetch" with --update-head-ok, 

Does it?  You're correct.  I do not like it.

Ciao,
Dscho

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Daniel Barkalow]

On Tue, 14 Oct 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Tue, 14 Oct 2008, Daniel Barkalow wrote:
> 
> > On Tue, 14 Oct 2008, Johannes Schindelin wrote:
> > 
> > > On Mon, 13 Oct 2008, Daniel Barkalow wrote:
> > > 
> > > > On Mon, 13 Oct 2008, Johannes Schindelin wrote:
> > > > 
> > > > > I actually understand now why the tests started failing: the 
> > > > > change from resolve_ref() to get_branch() as requested by Daniel 
> > > > > are at fault: get_branch() does not check if the branch has an 
> > > > > initial commit.
> > > > > 
> > > > > I am actually regretting making this change.  Daniel, do you agree 
> > > > > that it might be better to change back to resolve_ref(), so that 
> > > > > the initial complaint (IIRC Han-Wen git pull'ed into a freshly 
> > > > > initialized repository with that utterly bogus "git pull origin 
> > > > > master:master" line) is not re-raised?
> > > > 
> > > > Is it, in fact, okay to fetch into the current branch if it's "yet 
> > > > to be born"? I feel like it shouldn't be, since you'll get exactly 
> > > > the same problem that you would if the branch already existed: the 
> > > > index reflects the previous state (in this case, it's empty), so git 
> > > > will show that you've staged removing all of the files, right? So 
> > > > this would make the check for --update-head-ok more strict than 
> > > > before, but I think the behavior change is correct.
> > > 
> > > I think 
> > > http://thread.gmane.org/gmane.comp.version-control.git/31351/focus=31544 
> > > is the best link to see what Han-Wen said.  Granted, it was a 
> > > misunderstanding on his part, but there have been quite a few people 
> > > with the same misunderstanding.
> > > 
> > > So what they did was
> > > 
> > > 	$ mkdir just-one-branch
> > > 	$ cd just-one-branch
> > > 	$ git init
> > > 	$ git remote add origin <url>
> > > 	$ git pull origin master:master
> > > 
> > > And this _will_ work correctly.  Except when using get_branch(NULL) 
> > > instead of the validating resolve_ref().
> > 
> > "git pull origin master:master" invokes "git fetch" with --update-head-ok, 
> 
> Does it?  You're correct.  I do not like it.

The reason that it runs with --update-head-ok (and the reason that 
--update-head-ok exists in the first place) is that, when you're doing a 
pull, if you fetch into the current branch, pull will identify that you've 
actually fast-forwarded the current branch and will update the working 
tree and index accordingly (which it's allowed to do because it's expected 
to perform a merge in the working tree and index).

That is, it uses --update-head-ok because "git pull origin master:master" 
will work correctly, regardless of whether the local master is 
yet-to-be-born or not.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Daniel Barkalow]

On Tue, 14 Oct 2008, Daniel Barkalow wrote:

> On Tue, 14 Oct 2008, Johannes Schindelin wrote:
> 
> > Hi,
> > 
> > On Tue, 14 Oct 2008, Daniel Barkalow wrote:
> > 
> > > On Tue, 14 Oct 2008, Johannes Schindelin wrote:
> > > 
> > > > On Mon, 13 Oct 2008, Daniel Barkalow wrote:
> > > > 
> > > > > On Mon, 13 Oct 2008, Johannes Schindelin wrote:
> > > > > 
> > > > > > I actually understand now why the tests started failing: the 
> > > > > > change from resolve_ref() to get_branch() as requested by Daniel 
> > > > > > are at fault: get_branch() does not check if the branch has an 
> > > > > > initial commit.
> > > > > > 
> > > > > > I am actually regretting making this change.  Daniel, do you agree 
> > > > > > that it might be better to change back to resolve_ref(), so that 
> > > > > > the initial complaint (IIRC Han-Wen git pull'ed into a freshly 
> > > > > > initialized repository with that utterly bogus "git pull origin 
> > > > > > master:master" line) is not re-raised?
> > > > > 
> > > > > Is it, in fact, okay to fetch into the current branch if it's "yet 
> > > > > to be born"? I feel like it shouldn't be, since you'll get exactly 
> > > > > the same problem that you would if the branch already existed: the 
> > > > > index reflects the previous state (in this case, it's empty), so git 
> > > > > will show that you've staged removing all of the files, right? So 
> > > > > this would make the check for --update-head-ok more strict than 
> > > > > before, but I think the behavior change is correct.
> > > > 
> > > > I think 
> > > > http://thread.gmane.org/gmane.comp.version-control.git/31351/focus=31544 
> > > > is the best link to see what Han-Wen said.  Granted, it was a 
> > > > misunderstanding on his part, but there have been quite a few people 
> > > > with the same misunderstanding.
> > > > 
> > > > So what they did was
> > > > 
> > > > 	$ mkdir just-one-branch
> > > > 	$ cd just-one-branch
> > > > 	$ git init
> > > > 	$ git remote add origin <url>
> > > > 	$ git pull origin master:master
> > > > 
> > > > And this _will_ work correctly.  Except when using get_branch(NULL) 
> > > > instead of the validating resolve_ref().
> > > 
> > > "git pull origin master:master" invokes "git fetch" with --update-head-ok, 
> > 
> > Does it?  You're correct.  I do not like it.
> 
> The reason that it runs with --update-head-ok (and the reason that 
> --update-head-ok exists in the first place) is that, when you're doing a 
> pull, if you fetch into the current branch, pull will identify that you've 
> actually fast-forwarded the current branch and will update the working 
> tree and index accordingly (which it's allowed to do because it's expected 
> to perform a merge in the working tree and index).
> 
> That is, it uses --update-head-ok because "git pull origin master:master" 
> will work correctly, regardless of whether the local master is 
> yet-to-be-born or not.

In particular, the --update-head-ok is from b10ac50f, which is what added 
the check to prohibit fetching into the current branch otherwise, back in 
August 2005. There's never been anything preventing updating the current 
branch using "pull".

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH v2] Fix fetch/pull when run without --update-head-ok

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Tue, 14 Oct 2008, Daniel Barkalow wrote:
> ...
>> That is, it uses --update-head-ok because "git pull origin master:master" 
>> will work correctly, regardless of whether the local master is 
>> yet-to-be-born or not.
>
> In particular, the --update-head-ok is from b10ac50f, which is what added 
> the check to prohibit fetching into the current branch otherwise, back in 
> August 2005. There's never been anything preventing updating the current 
> branch using "pull".

What you wrote above is correct, and it is all that is necessary to make
"pull master:master" (when 'master' is the current branch) work correctly,
in normal situations.  Dscho's patch is in itself is a good thing to do,
but is not necessary when the caller gives --update-head-ok.

Nor is it sufficient.  Have you tested the current "git-pull" with or
without Dscho's patch applied to "git-fetch" when 'master' is an unborn
branch?  "git-pull" has this piece:

        curr_head=$(git rev-parse --verify HEAD 2>/dev/null)
        if test "$curr_head" != "$orig_head"
        then
                # The fetch involved updating the current branch.

                # The working tree and the index file is still based on the
                # $orig_head commit, but we are merging into $curr_head.
                # First update the working tree to match $curr_head.

                echo >&2 "Warning: fetch updated the current branch head."
                ...
        fi

The above part (written by yours truly) did not care what happens when the
current branch is unborn.  Back then when git-pull was originally written,
nobody was crazy enough to pull into an empty branch and expect it to
work.  The code to allow that craziness (look for "initial pull" in the
same script to find a 6-line block near the end) came much later, and the
commit that added the "initial pull" support should have adjusted the
above codeblock if it really wanted to support pulling into an unborn
branch, but it forgot to do so.  It also forgot to handle the case where
the originally unborn current branch was fetched into.

If we really care about is "git pull origin master:master" into an unborn
branch, I think we need the attached patch on top, and this is regardless
of Dscho's patch that tightens the check when -update-head-ok is not given.

 git-pull.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git c/git-pull.sh w/git-pull.sh
index 75c3610..664fe34 100755
--- c/git-pull.sh
+++ w/git-pull.sh
@@ -124,7 +124,7 @@ orig_head=$(git rev-parse --verify HEAD 2>/dev/null)
 git fetch --update-head-ok "$@" || exit 1
 
 curr_head=$(git rev-parse --verify HEAD 2>/dev/null)
-if test "$curr_head" != "$orig_head"
+if test -n "$orig_head" && test "$curr_head" != "$orig_head"
 then
 	# The fetch involved updating the current branch.
 
@@ -172,7 +172,7 @@ esac
 
 if test -z "$orig_head"
 then
-	git update-ref -m "initial pull" HEAD $merge_head "" &&
+	git update-ref -m "initial pull" HEAD $merge_head "$curr_head" &&
 	git read-tree --reset -u HEAD || exit 1
 	exit
 fi

[PATCH] pull: allow "git pull origin $something:$current_branch" into an unborn branch

[Junio C Hamano]

Some misguided documents floating on the Net suggests this sequence:

    mkdir newdir && cd newdir
    git init
    git remote add origin $url
    git pull origin master:master

"git pull" has known about an misguided "pull" like this where the
underlying fetch updates the current branch for a long time, and it also
has known about another form of insanity "git pull origin master" into an
unborn branch, but these two workarounds were not aware of the existence
of each other and did not work well together.

This fixes it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This comes on top of js/maint-fetch-update-head.  Passes all the tests,
   and also its own.

 git-pull.sh     |    4 ++--
 t/t5520-pull.sh |   12 ++++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 75c3610..664fe34 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -124,7 +124,7 @@ orig_head=$(git rev-parse --verify HEAD 2>/dev/null)
 git fetch --update-head-ok "$@" || exit 1
 
 curr_head=$(git rev-parse --verify HEAD 2>/dev/null)
-if test "$curr_head" != "$orig_head"
+if test -n "$orig_head" && test "$curr_head" != "$orig_head"
 then
 	# The fetch involved updating the current branch.
 
@@ -172,7 +172,7 @@ esac
 
 if test -z "$orig_head"
 then
-	git update-ref -m "initial pull" HEAD $merge_head "" &&
+	git update-ref -m "initial pull" HEAD $merge_head "$curr_head" &&
 	git read-tree --reset -u HEAD || exit 1
 	exit
 fi
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 997b2db..725771f 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -29,6 +29,18 @@ test_expect_success 'checking the results' '
 	diff file cloned/file
 '
 
+test_expect_success 'pulling into void using master:master' '
+	mkdir cloned-uho &&
+	(
+		cd cloned-uho &&
+		git init &&
+		git pull .. master:master
+	) &&
+	test -f file &&
+	test -f cloned-uho/file &&
+	test_cmp file cloned-uho/file
+'
+
 test_expect_success 'test . as a remote' '
 
 	git branch copy master &&
-- 
1.6.0.2.711.gf1ba4