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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 13, 2008, 14:23 UTC
Message-ID: <7vod1obmlh.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.0810131129110.22125@pacific.mpi-cbg.de.mpi-cbg.de>

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