From: Jan Hudec <hidden> Date: 2016-06-15 22:50:03
Hello all,
I have a repository populated with git-svn. For backup I have
a mirror remote set up. Today I ran 'git push backup' on one
terminal and before it finished (it's just on a network
filesystem, so it's kind of slow), I ran 'git svn fetch' on
another. And than I didn't see any results of that fetch.
What happened is that the push took the values of all the
refs -- including those in refs/remotes/svn as it's a mirror
for pushing them to the backup. Meanwhile the fetch udpated
them. But when the push finished with the remote repo, it
updated the local refs back to the values it pushed, undoing
the effects of that fetch.
The repository was created with simple:
git remote add --mirror backup /mnt/server/path/to/repo.git
which created configuration:
[remote "backup"]
url = /mnt/server/path/to/repo.git
fetch = +refs/*:refs/*
mirror = true
So, should the push be more careful when updating the refs,
not simulate the pull back when doing a --mirror, or the
git remote add not add the 'fetch = +refs/*:refs/*' line?
Thanks,
Jan
--
- Jan Hudec [off-list ref]
From: Jeff King <hidden> Date: 2016-06-15 22:50:04
On Thu, Nov 18, 2010 at 08:39:17AM +0100, Jan Hudec wrote:
What happened is that the push took the values of all the
refs -- including those in refs/remotes/svn as it's a mirror
for pushing them to the backup. Meanwhile the fetch udpated
them. But when the push finished with the remote repo, it
updated the local refs back to the values it pushed, undoing
the effects of that fetch.
Hrm. There are actually two issues here, I think.
What is happening, I believe, is that push is trying to
opportunistically update your local tracking branches.
So the first issue is that you do not have the usual branches-in-heads,
tracking-branches-in-remotes setup. Instead it is looking at your fetch
refspec:
and trying to update everything in refs/* with what it just pushed.
Usually this is a no-op, since it is the same as the value we just
pushed, but as you found out, it is in a race with concurrent commands.
I think we don't want to be doing the opportunistic update in this case.
But what is the correct rule for deciding not to do it? I can think of a
few possibilities:
1. When the mirror option is used. But this doesn't help people who
have a broad fetch refspec they have configured without mirror.
2. When the RHS of a fetch refspec is something that is being pushed.
But this doesn't cover the case of pushing local "refs/heads/foo" to
remote "refs/heads/bar", and then having it update "refs/heads/bar"
locally.
3. When the ref to be updated is not in refs/remotes. This feels a
little hack-ish, but I think would work the best in practice. The
refs/remotes hierarchy is supposed to just be a cache of remote
state, so really it is the only place such an opportunistic update
should be safe. People who are doing exotic things like fetching
directly into refs/heads will have to live without the opportunistic
update.
The second issue I mentioned is that transport_update_tracking_ref does
not actually check the old sha1 of the ref it is updating. The usual
practice in git to avoid holding long locks is:
1. lock ref, read sha1, unlock ref
2. do stuff to make a new sha1, remembering old sha1
3. lock ref, read sha1, check that it equals old sha1, write new sha1,
unlock
We don't do that here. It is tempting to do something like:
@@ -605,7 +605,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int vdelete_ref(rs.dst,NULL,0);}elseupdate_ref("update by push",rs.dst,-ref->new_sha1,NULL,0,0);+ref->new_sha1,ref->old_sha1,0,0);free(rs.dst);}}
but that is not right. That is saying "if we updated the remote ref R
from A to B, update the tracking ref of R to B only if it is at A".
However, our tracking ref of R is not necessarily at A; it might be
stale with respect to upstream.
So really we would need to read the current value of the tracking ref at
the beginning of the push. But that is inefficient, and it is not
actually atomic with the push we are doing.
So I think it is OK to keep this the way it is, and assume that
update_tracking_ref is about overwriting whatever is there. The real
problem in your case is that the things it is overwriting are actually
precious heads, not just a remote cache.
-Peff
I think we don't want to be doing the opportunistic update in this case.
But what is the correct rule for deciding not to do it? I can think of a
few possibilities:
Thinking on this more, perhaps it really is the fetch refspec there that
is the problem (as you initially suggested).
It seems to me there are really two kinds of mirrors: one where you will
fetch everything from the remote, and one where you will push everything
to the remote.
You have the latter kind, and the fetch refspec is just causing
problems. Removing it would solve not only this issue, but also the fact
that you would never want to run "git fetch backup", even accidentally,
in your repo, as it would overwrite your local work.
So I think we need --mirror=push, or something similar.
-Peff
From: Jan Hudec <hidden> Date: 2016-06-15 22:50:04
On Thu, Nov 18, 2010 at 12:50:08 -0500, Jeff King wrote:
On Thu, Nov 18, 2010 at 08:39:17AM +0100, Jan Hudec wrote:
quoted
What is happening, I believe, is that push is trying to
opportunistically update your local tracking branches.
Indeed.
So the first issue is that you do not have the usual branches-in-heads,
tracking-branches-in-remotes setup. Instead it is looking at your fetch
refspec:
and trying to update everything in refs/* with what it just pushed.
Usually this is a no-op, since it is the same as the value we just
pushed, but as you found out, it is in a race with concurrent commands.
I think we don't want to be doing the opportunistic update in this case.
But what is the correct rule for deciding not to do it? I can think of a
few possibilities:
1. When the mirror option is used. But this doesn't help people who
have a broad fetch refspec they have configured without mirror.
The above config is what is created by default by 'git remote add --mirror'.
So I expect the problem to be somewhat common with mirror and a lot rarer
without.
Which brings the yet another question, namely whether it actually makes sense
to set the fetch for a mirror remote. Note that any call to fetch will almost
inevitably abort with "reusing to pull to checked out ref in non-bare
repository" error.
2. When the RHS of a fetch refspec is something that is being pushed.
But this doesn't cover the case of pushing local "refs/heads/foo" to
remote "refs/heads/bar", and then having it update "refs/heads/bar"
locally.
3. When the ref to be updated is not in refs/remotes. This feels a
little hack-ish, but I think would work the best in practice. The
refs/remotes hierarchy is supposed to just be a cache of remote
state, so really it is the only place such an opportunistic update
should be safe. People who are doing exotic things like fetching
directly into refs/heads will have to live without the opportunistic
update.
In my case it wouldn't actually help. The race was between push to mirror and
fetch from actual upstream (which happened to be svn via git-svn, but it
would happen with git upstream too) and the incorrectly rewound ref was
'refs/remotes/svn/trunk'.
A combination of 2 *and* 3 would work. I.e. update only remotes and only if
they are not being pushed.
What would work on the other hand -- and be very conservative approach --
would be to only do oportunistic update if the fetch *option* has
'refs/remotes/<something>' on the right side.
The second issue I mentioned is that transport_update_tracking_ref does
not actually check the old sha1 of the ref it is updating. The usual
practice in git to avoid holding long locks is:
1. lock ref, read sha1, unlock ref
2. do stuff to make a new sha1, remembering old sha1
3. lock ref, read sha1, check that it equals old sha1, write new sha1,
unlock
We don't do that here.
[...]
So really we would need to read the current value of the tracking ref at
the beginning of the push. But that is inefficient, and it is not
actually atomic with the push we are doing.
Indeed, it does not sound reasonable. Plus I don't think it would actually do
what we want. In the case of pushing 'refs/heads/foo' -> 'refs/heads/bar' and
updating local 'refs/heads/bar', it's not clear whether it should be updated
or not.
In fact the problem is not in the race, but in the fact, that push updates
refs, that may have other purpose than tracking the particular remote. The
problem is in some cases we don't know whether a ref is purely tracking
*that* remote or not.
So I think it is OK to keep this the way it is, and assume that
update_tracking_ref is about overwriting whatever is there. The real
problem in your case is that the things it is overwriting are actually
precious heads, not just a remote cache.
Well, in my case it actually was a remote cache. But of different remote.
There are two common cases:
1. The mirror case, where we don't want to do the oportunistic update at
all.
2. The regular case of remote tracking branches, in which case the
'remote.<name>.fetch' option matches ".*:refs/remotes/[^*]+/.*"
and than there is a see of various strange hand-crafted setups, where it's
not obvious whether user actually wants the oportunistic update or not.
Thus I see two options to change the oportunistic update:
1. Don't do oportunistic update with mirror. That keeps the other cases work
as they do now. Hopefuly users are aware of the behaviour when they
hand-craft such setups.
2. Only do oportunistic update when the fetch specification matches
".*:refs/remotes/[^*]+/.*". That way oportunistic update will only happen
if the remote has it's own section in refs/remotes, so we can assume
nothing else is touching it.
and the third option (similar to the first, but done at different point):
3. Don't set 'fetch' for mirror remotes in non-bare repositories, since
non-bare repositories can't be treated as mirrors of something.
--
Jan 'Bulb' Hudec [off-list ref]
From: Jan Hudec <hidden> Date: 2016-06-15 22:50:04
On Thu, Nov 18, 2010 at 12:58:11 -0500, Jeff King wrote:
It seems to me there are really two kinds of mirrors: one where you will
fetch everything from the remote, and one where you will push everything
to the remote.
You have the latter kind, and the fetch refspec is just causing
problems. Removing it would solve not only this issue, but also the fact
that you would never want to run "git fetch backup", even accidentally,
in your repo, as it would overwrite your local work.
Accidentally did it already. Fortunately it just died with something like
"refusing to pull to checked out branch of non-bare repository"
and did nothing at all.
So I think we need --mirror=push, or something similar.
Does it *ever* make sense to have a non-bare pull mirror. I think it does
not.
--
Jan 'Bulb' Hudec [off-list ref]
From: Jeff King <hidden> Date: 2016-06-15 22:50:04
On Thu, Nov 18, 2010 at 07:42:41PM +0100, Jan Hudec wrote:
The above config is what is created by default by 'git remote add --mirror'.
So I expect the problem to be somewhat common with mirror and a lot rarer
without.
Agreed, and I think just turning off the behavior with "mirror" might be
OK in practice. But I do want to consider whether we can make other
corner cases more sensible at the same time.
Which brings the yet another question, namely whether it actually makes sense
to set the fetch for a mirror remote. Note that any call to fetch will almost
inevitably abort with "reusing to pull to checked out ref in non-bare
repository" error.
Hmm. Yeah, of the "fetch vs push mirror" distinction I made earlier, it
really only makes sense to push from a non-bare repo, and to fetch into
a bare repo.
[skip some thoughtful analysis which I agree with, but I think ends up
not being relevant]
In fact the problem is not in the race, but in the fact, that push updates
refs, that may have other purpose than tracking the particular remote. The
problem is in some cases we don't know whether a ref is purely tracking
*that* remote or not.
Yeah, you're right. I think the real problem is that we generally assume
that by putting something on the RHS of a fetch refspec, it is used just
for tracking the particular remote (especially when there is a "+" on
the front!).
So the real solution is not having that fetch line.
and the third option (similar to the first, but done at different point):
3. Don't set 'fetch' for mirror remotes in non-bare repositories, since
non-bare repositories can't be treated as mirrors of something.
Of all the options, this is my favorite. It does what we want in the
common cases, it's simple, and it still allows people to hand-config
crazy stuff if they want to.
It doesn't un-break people's existing repos, but I think we can accept
that (actually, the docs say that --mirror only makes sense in bare
repositories. Which I think is not true, as you demonstrate, but perhaps
it dissuaded people from creating broken push mirrors in the past :) ).
That does still leave one slight corner case, which is a bare repo that
is used for both fetch and push mirrors. E.g., a repo that straddles the
border between two networks might do:
git init --bare
git remote add --mirror network1 host.network1:foo.git
git remote add --mirror network2 host.network2:foo.git
git fetch network1
git push network2
to relay commits. Both remotes will have the fetch refspec, as they are
in a bare repo. But only the first one wants it. In the second one, we
will update the heads as tracking refs. A simultaneous fetch/push would
be in conflict.
That is such an unlikely case that we can probably just leave it to be
hand-configured by anybody who really wants it. Or we can have:
# adds fetch = refs/*:refs/*
git remote add --mirror=fetch network1 host.network1:foo.git
# adds push = refs/*:refs/*
git remote add --mirror=push network2 host.network2:foo.git
and the default for --mirror (with no type) can be "fetch" in a bare repo
and "push" in a non-bare one.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:50:04
On Thu, Nov 18, 2010 at 07:49:04PM +0100, Jan Hudec wrote:
quoted
So I think we need --mirror=push, or something similar.
Does it *ever* make sense to have a non-bare pull mirror. I think it does
not.
I don't think so. But it may make sense to have a bare push mirror, as I
mention in my other email. So we may still want to make it easy for the
user to specify.
-Peff