From: Junio C Hamano <hidden> Date: 2016-06-15 22:45:28
Johannes Schindelin [off-list ref] 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.
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).
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:28
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.
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
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:28
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*
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:29
Hi,
On Mon, 13 Oct 2008, Daniel Barkalow wrote:
On Mon, 13 Oct 2008, Johannes Schindelin wrote:
quoted
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
From: Shawn O. Pearce <hidden> Date: 2016-06-15 22:45:29
Johannes Schindelin [off-list ref] wrote:
On Mon, 13 Oct 2008, Daniel Barkalow wrote:
quoted
On Mon, 13 Oct 2008, Johannes Schindelin wrote:
quoted
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.
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:29
On Tue, 14 Oct 2008, Johannes Schindelin wrote:
Hi,
On Mon, 13 Oct 2008, Daniel Barkalow wrote:
quoted
On Mon, 13 Oct 2008, Johannes Schindelin wrote:
quoted
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*
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:29
On Tue, 14 Oct 2008, Shawn O. Pearce wrote:
Johannes Schindelin [off-list ref] wrote:
quoted
On Mon, 13 Oct 2008, Daniel Barkalow wrote:
quoted
On Mon, 13 Oct 2008, Johannes Schindelin wrote:
quoted
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*
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:29
Hi,
On Tue, 14 Oct 2008, Shawn O. Pearce wrote:
Johannes Schindelin [off-list ref] wrote:
quoted
On Mon, 13 Oct 2008, Daniel Barkalow wrote:
quoted
On Mon, 13 Oct 2008, Johannes Schindelin wrote:
quoted
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
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:29
Hi,
On Tue, 14 Oct 2008, Daniel Barkalow wrote:
On Tue, 14 Oct 2008, Johannes Schindelin wrote:
quoted
On Mon, 13 Oct 2008, Daniel Barkalow wrote:
quoted
On Mon, 13 Oct 2008, Johannes Schindelin wrote:
quoted
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
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:29
On Tue, 14 Oct 2008, Johannes Schindelin wrote:
Hi,
On Tue, 14 Oct 2008, Daniel Barkalow wrote:
quoted
On Tue, 14 Oct 2008, Johannes Schindelin wrote:
quoted
On Mon, 13 Oct 2008, Daniel Barkalow wrote:
quoted
On Mon, 13 Oct 2008, Johannes Schindelin wrote:
quoted
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*
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:29
On Tue, 14 Oct 2008, Daniel Barkalow wrote:
On Tue, 14 Oct 2008, Johannes Schindelin wrote:
quoted
Hi,
On Tue, 14 Oct 2008, Daniel Barkalow wrote:
quoted
On Tue, 14 Oct 2008, Johannes Schindelin wrote:
quoted
On Mon, 13 Oct 2008, Daniel Barkalow wrote:
quoted
On Mon, 13 Oct 2008, Johannes Schindelin wrote:
quoted
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*