threads / patch / 15858

patchfetch: refuse to fetch into the current branch in a non-bare repository

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

## tl;dr

24 messages between Oct 11, 2008 and Oct 14, 2008. Diffs are folded; open one to read it.

replies: 23people: 4as markdown or json

Johannes Schindelin· Oct 11, 2008, 11:38 UTC · lore

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(-)
Show changes to 3 files +27 −1

builtin-fetch.c, t/t5505-remote.sh, t/t5510-fetch.sh

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
Junio C Hamano· Oct 11, 2008, 21:44 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 7 quoted lines
> 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?

Shawn O. Pearce· Oct 12, 2008, 18:47 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> 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.

Show changes to t/t5510-fetch.sh +6 −0
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.
Johannes Schindelin· Oct 13, 2008, 09:28 UTC · re: Shawn O. Pearce · lore

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

Hi,
On Sun, 12 Oct 2008, Shawn O. Pearce wrote:
Show 15 quoted lines
> 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

Shawn O. Pearce· Oct 12, 2008, 18:52 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 18 quoted lines
> 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.

Show 5 quoted lines
> @@ -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)
Show 14 quoted lines
> 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.
Daniel Barkalow· Oct 12, 2008, 20:37 UTC · re: Johannes Schindelin · lore

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

On Sat, 11 Oct 2008, Johannes Schindelin wrote:
Show 10 quoted lines
> 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.
Show 28 quoted lines
> 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)
Show 14 quoted lines
> +			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)
Johannes Schindelin· Oct 13, 2008, 09:36 UTC · re: Daniel Barkalow · lore

[PATCH v2] 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".

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(-)
Show changes to 5 files +30 −3

builtin-fetch.c, t/t5405-send-pack-rewind.sh, t/t5505-remote.sh, t/t5510-fetch.sh, t/t9300-fast-import.sh

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
Shawn O. Pearce· Oct 13, 2008, 14:09 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 19 quoted lines
> 
> 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.
Johannes Schindelin· Oct 13, 2008, 17:57 UTC · re: Shawn O. Pearce · lore

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

Hi,
On Mon, 13 Oct 2008, Shawn O. Pearce wrote:
Show 7 quoted lines
> 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

Junio C Hamano· Oct 13, 2008, 14:23 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
> 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).

Junio C Hamano· Oct 13, 2008, 17:30 UTC · re: Junio C Hamano · lore

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

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.
Johannes Schindelin· Oct 13, 2008, 18:12 UTC · re: Junio C Hamano · lore

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

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

Daniel Barkalow· Oct 13, 2008, 20:05 UTC · re: Johannes Schindelin · lore

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

On Mon, 13 Oct 2008, Johannes Schindelin wrote:
Show 9 quoted lines
> 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*
Johannes Schindelin· Oct 14, 2008, 09:49 UTC · re: Daniel Barkalow · lore

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

Hi,
On Mon, 13 Oct 2008, Daniel Barkalow wrote:
Show 19 quoted lines
> 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

Shawn O. Pearce· Oct 14, 2008, 15:02 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 10 quoted lines
> 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.
Daniel Barkalow· Oct 14, 2008, 16:04 UTC · re: Shawn O. Pearce · lore

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

On Tue, 14 Oct 2008, Shawn O. Pearce wrote:
Show 16 quoted lines
> 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*
Johannes Schindelin· Oct 14, 2008, 16:15 UTC · re: Shawn O. Pearce · lore

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

Hi,
On Tue, 14 Oct 2008, Shawn O. Pearce wrote:
Show 16 quoted lines
> 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

Daniel Barkalow· Oct 14, 2008, 15:57 UTC · re: Johannes Schindelin · lore

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

On Tue, 14 Oct 2008, Johannes Schindelin wrote:
Show 40 quoted lines
> 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*
Johannes Schindelin· Oct 14, 2008, 16:17 UTC · re: Daniel Barkalow · lore

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

Hi,
On Tue, 14 Oct 2008, Daniel Barkalow wrote:
Show 43 quoted lines
> 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

Daniel Barkalow· Oct 14, 2008, 16:52 UTC · re: Johannes Schindelin · lore

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

On Tue, 14 Oct 2008, Johannes Schindelin wrote:
Show 49 quoted lines
> 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*
Daniel Barkalow· Oct 14, 2008, 17:02 UTC · re: Daniel Barkalow · lore

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

On Tue, 14 Oct 2008, Daniel Barkalow wrote:
Show 62 quoted lines
> 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*
Junio C Hamano· Oct 14, 2008, 22:07 UTC · re: Daniel Barkalow · lore

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

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 10 quoted lines
> 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(-)
Show changes to diff +2 −2
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
Junio C Hamano· Oct 14, 2008, 22:53 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +14 −2

git-pull.sh, t/t5520-pull.sh

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
Daniel Barkalow· Oct 13, 2008, 17:08 UTC · re: Johannes Schindelin · lore

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

On Mon, 13 Oct 2008, Johannes Schindelin wrote:
Show 19 quoted lines
> 
> 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>

← back to recent threads