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

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 <redacted>
---
 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

Johannes Schindelin [off-list ref] wrote:

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

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

	if (!update_head_ok)
		check_ref_map(ref_map)
  

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

-- 
Shawn.

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.

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)

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 <redacted>
---

	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

Johannes Schindelin [off-list ref] 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 <redacted>

Acked-by: Shawn O. Pearce <redacted>

	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.

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 <redacted>

Acked-by: Daniel Barkalow <redacted>

Hi,

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

Johannes Schindelin [off-list ref] wrote:

quoted

	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