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 ...
[ message continues ]

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

[ message continues ]

The description is confusing, yes.  It should be about git fetch,

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

[ message continues ]

Hi,


However, it was a "git pull origin master:master" that was the issue.  


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

[ message continues ]

I'd rather see local variables named lowercase.  Constants should

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

	if (!update_head_ok)

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

-- 
Shawn.
--

[ message continues ]

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())

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

--

[ message continues ]

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() || ...
[ message continues ]

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

[ message continues ]

Hi,


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

--

[ message continues ]

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


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

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).
--

[ message continues ]

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

[ message continues ]

Hi,

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


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 

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


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

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 

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

--

[ message continues ]

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

[ message continues ]

Hi,


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

--

[ message continues ]

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

[ message continues ]

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

[ message continues ]

Hi,


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

--

[ message continues ]

"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*
--

[ message continues ]

Hi,


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

Ciao,
Dscho

--

[ message continues ]

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

[ message continues ]

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

[ message continues ]

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
+++ ...
[ message continues ]

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

[ message continues ]