Re: [PATCH 3/3] fetch --prune: Repair branchname DF conflicts
On Wed, Dec 18, 2013 at 07:48:59PM -0600, Tom Miller wrote:
I did not intend to introduce new lingo. I did some searching through
history to see if something like this had been worked on before and
I found a commit by Jeff King that introduced me the the idea of
DF conflicts
I take all the blame. :)
As for the patch itself:
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e50b697..845c687 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -868,11 +868,6 @@ static int do_fetch(struct transport *transport,
if (tags == TAGS_DEFAULT autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, 1);
- if (fetch_refs(transport, ref_map)) {
- free_refs(ref_map);
- retcode = 1;
- goto cleanup;
- }
if (prune) {
/*
* We only prune based on refspecs specified
@@ -888,6 +883,11 @@ static int do_fetch(struct transport *transport,
transport-url);
}
}
+ if (fetch_refs(transport, ref_map)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
I think this is _probably_ a good thing to do, but it does have an
interesting side effect for concurrent operations, and I haven't seen
that mentioned so far in the discussion.
Readers of the ref namespace don't have any sort of transactionally
consistent view of all of the refs. So if a remote has moved a branch
foo to bar and we fetch --prune, there will be a moment where a
simultaneous reader will see one of two states that never existed on the
remote (depending on the order the fetch chooses): either both refs
exist, or neither exists.
Right now fetch creates first and deletes after, so a simultaneous
reader may see both refs. After your change, it may see no refs at all.
Even though both are technically wrong, the current behavior is safer.
If the reader is calculating reachability (e.g., for a repack or git
prune), it is better to have too many references than too few.
I'm not sure to what degree we want to care. This is a race, but it's a
reasonably unlikely one, and the D/F thing bites people in the real
world.
And further confounding this is the fact that even if the writer does
everything correctly, the way we read refs can still cause an odd view
of the whole namespace. For example, consider moving refs/heads/z/foo
to refs/heads/a/foo, while somebody else reads simultaneously. Even
with create-before-delete, we can get the sequence:
1. Reader reads refs/heads/a/ and sees it does not contain foo.
2. Writer writes refs/heads/a/foo.
3. Writer deletes refs/heads/z/foo.
4. Reader reads refs/heads/z, which does not contain foo.
That race can be closed with a double-read of the ref namespaces, but
that has poor performance. A more reasonable fix, IMHO, would be to have
an alternate ref store that represents transactions atomically (keeping
in mind that this really only matters for busy repos with simultaneous
readers and writers, so it would not even need to be the default ref
store). And once you have such a store, that solves the other problem,
too: you can just treat the delete-create as a transaction anyway. It
also solves a similar problem with refs that rewind.
So even leaving it as-is does not make the problem go away, though the
proposed change does exacerbate it somewhat. I wonder how hard it would
be to do the safer thing in the common case that there is no D/F
conflict. That is, do multiple passes at updating the refs:
1. Create/update any refs we can. Those with D/F conflicts are put
aside for the moment.
2. Delete any refs according to the --prune rules.
3. Come back to any D/F conflicts and try them again.
I dunno. As far as I know, this is not a race that people see often in
real life (I do not have any confirmed cases of it yet). So it may
simply not be worth worrying about.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] fetch --prune: Repair branchname DF conflicts
Tom Miller jacker...@gmail.com writes:
But what should happen when we do not give --prune to git fetch in
such a situation? Should it fail, because we still have frotz/nitfol
and we cannot create frotz without losing it?
You talk about this to some extent in an email from 2009. I have linked
it below for your review.
http://article.gmane.org/gmane.comp.version-control.git/132276
I do not think the old discussion talks about the case. It was
about we have remotes/origin/{frotz,nitfol} from the origin from an
earlier fetch, the origin now has updated its frotz and deleted its
nitfol. 'git remote prune' removes our remotes/origin/nitfol
without updating our copy of remotes/origin/frotz, but I do not
think it is sensible. 'git fetch --prune origin' would update both
and make our remote-tracking branches for 'origin' in line with the
reality. It was not about what 'git fetch' without '--prune'
should do.
Your 'git fetch' without '--prune' should be less destrictive is a
good guiding principle. If we have a copy of the 'frotz/nitfol'
branch from the 'origin', removing it so that we can have a new copy
of the 'frotz' branch the 'origin' now has (after it removed
'frotz/nitfol' to make room) is indeed an operation that loses
information. And it probably is the right thing to do to fail such
a fetch. 'git fetch --prune' on the other hand really means I do
not care about the branches' histories my 'origin' discarded; bring
me up to date and give me the same view as my 'origin' has in my
remote-tracking branches, so losing 'frotz/nitfol', which the
'origin' already decided to discard, is what the user wants.
The atomicity issue Peff brings up is an interesting and important
one, but I think that is an orthogonal issue.
With the background information from the previous thread between you
and trast, the patch [3/3] looks good to me.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] fetch --prune: Repair branchname DF conflicts
On Wed, Dec 18, 2013 at 3:54 PM, Junio C Hamano gits...@pobox.com wrote:
Tom Miller jacker...@gmail.com writes:
When a branchname DF conflict occurs during a fetch,
You may have started with a specific case in which you want to
change the behaviour of current Git, so it may be clear what you
meant by branchname DF conflict, but that is true for nobody other
than you who will read this log message. Introducing new lingo is
OK as long as it is necessary, but in a case like this, where you
have to describe what situation you are trying to address anyway,
I do not think you need to add a new word to our vocabulary.
When we have a remote-tracking branch frotz/nitfol from a
previous fetch, and the upstream now has branch frotz, we
used to fail to remove frotz/nitfol and recreate frotz with
git fetch --prune from the upstream.
or something like that?
I did not intend to introduce new lingo. I did some searching through
history to see if something like this had been worked on before and
I found a commit by Jeff King that introduced me the the idea of
DF conflicts
commit fa250759794ab98e6edfbbf2f6aa2cb912e535eb
Author: Jeff King p...@peff.net
Date: Mon May 25 06:40:54 2009 -0400
fetch: report ref storage DF errors more accurately
When we fail to store a fetched ref, we recommend that the
user try running git prune to remove up any old refs that
have been deleted by the remote, which would clear up any DF
conflicts. However, ref storage might fail for other
reasons (e.g., permissions problems) in which case the
advice is useless and misleading.
This patch detects when there is an actual DF situation and
only issues the advice when one is found.
Signed-off-by: Jeff King p...@peff.net
Signed-off-by: Junio C Hamano gits...@pobox.com
I have no issue with rewording the it to be more clear and to try to
remove any new lingo.
But what should happen when we do not give --prune to git fetch in
such a situation? Should it fail, because we still have frotz/nitfol
and we cannot create frotz without losing it?
You talk about this to some extent in an email from 2009. I have linked
it below for your review.
http://article.gmane.org/gmane.comp.version-control.git/132276
In my opinion, if I supply --prune to fetch I expect it to be
destructive. It should be noted that the reflog can *not* be used
to recover pruned branches from a remote.
--prune should
be able to fix it. When fetching with --prune, the fetching process
happens before pruning causing the branchname DF conflict to persist
and report an error. This patch prunes before fetching, thus
correcting DF conflicts during a fetch.
Signed-off-by: Tom Miller jacker...@gmail.com
Tested-by: Thomas Rast t...@thomasrast.ch
I wasn't following previous threads closely (was there a previous
thread???); has this iteration been already tested by trast?
There was a previous thread, but I was just looking for feed back on this
as a WIP. Should I have replied to it with this patchset?
Here is a link to the previous thread.
http://thread.gmane.org/gmane.comp.version-control.git/238530
The commit below should be the same patch he tested. The test was added
by him, and I made it part of this commit. Did I do this wrong?
---
builtin/fetch.c | 10 +-
t/t5510-fetch.sh | 14 ++
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e50b697..845c687 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -868,11 +868,6 @@ static int do_fetch(struct transport *transport,
if (tags == TAGS_DEFAULT autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, 1);
- if (fetch_refs(transport, ref_map)) {
- free_refs(ref_map);
- retcode = 1;
- goto cleanup;
- }
if (prune) {
/*
* We only prune based on refspecs specified
@@ -888,6 +883,11 @@ static int do_fetch(struct transport *transport,
transport-url);
}
}
+ if (fetch_refs(transport, ref_map)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
free_refs(ref_map);
/* if neither --no-tags nor --tags was specified, do automated tag
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 5d4581d..a981125 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -614,4 +614,18 @@ test_expect_success 'all boundary commits are excluded'
'
test_bundle_object_count .git/objects/pack/pack-${pack##pack}.pack
3
'
+test_expect_success 'branchname D/F conflict resolved by --prune' '
+ git branch dir/file
+ git clone . prune-df-conflict
+
Re: [PATCH 3/3] fetch --prune: Repair branchname DF conflicts
Tom Miller jacker...@gmail.com writes:
The commit below should be the same patch he tested. The test was added
by him, and I made it part of this commit. Did I do this wrong?
No, no, no. All my questions were true questions, not complaints
veiled as rhetorical questions. Thanks for many pointers for
clarification.
---
builtin/fetch.c | 10 +-
t/t5510-fetch.sh | 14 ++
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e50b697..845c687 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -868,11 +868,6 @@ static int do_fetch(struct transport *transport,
if (tags == TAGS_DEFAULT autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, 1);
- if (fetch_refs(transport, ref_map)) {
- free_refs(ref_map);
- retcode = 1;
- goto cleanup;
- }
if (prune) {
/*
* We only prune based on refspecs specified
@@ -888,6 +883,11 @@ static int do_fetch(struct transport *transport,
transport-url);
}
}
+ if (fetch_refs(transport, ref_map)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
free_refs(ref_map);
/* if neither --no-tags nor --tags was specified, do automated tag
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 5d4581d..a981125 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -614,4 +614,18 @@ test_expect_success 'all boundary commits are
excluded' '
test_bundle_object_count .git/objects/pack/pack-${pack##pack
}.pack 3
'
+test_expect_success 'branchname D/F conflict resolved by --prune' '
+ git branch dir/file
+ git clone . prune-df-conflict
+ git branch -D dir/file
+ git branch dir
+ (
+ cd prune-df-conflict
+ git fetch --prune
+ git rev-parse origin/dir ../actual
+ )
+ git rev-parse dir expect
+ test_cmp expect actual
+'
+
test_done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
