[BUG?] remote prune origin interacts badly with clone --mirror and multiple remotes

[BUG?] remote prune origin interacts badly with clone --mirror and multiple remotes

[Dennis Kaarsemaker]

[git version: next as of yesterday afternoon]

If I clone a repo with git clone --mirror, and add other remotes later,
'git remote prune origin' deletes all branches and tags of the other
remotes.

Easily repeatable example:

[core]
	repositoryformatversion = 0
	filemode = true
	bare = true
	logallrefupdates = false
[remote "origin"]
	url = git://github.com/git/git.git
	fetch = +refs/*:refs/*
	mirror = true
[remote "peff"]
	url = git://github.com/peff/git.git
	fetch = +refs/heads/*:refs/remotes/peff/*

'git remote prune origin' will delete all peff's branches in this case.
I'm guessing the wildcards refs/* and refs/remotes/peff/* interact badly
in some place, and I'm trying to understand builtin/remote.c to see if I
can fix it, but haven't gotten very far yet.

git fetch --prune origin and git remote update --prune also show this
behaviour.

git remote prune peff does not delete non-peff branches in this
scenario, further strengthening my belief that the refs/* and
refs/remotes/peff/* wildcards interact badly with prune.

Or is this considered normal behaviour and is what I'm trying to do
simply unsupported? In that case a warning would be welcome when adding
remotes to a --mirror'ed repository.
-- 
Dennis Kaarsemaker
www.kaarsemaker.net

[PATCH] remote: make prune work for mixed mirror/non-mirror repos

[Dennis Kaarsemaker]

When cloning a repo with --mirror, and adding more remotes later,
get_stale_heads for origin would mark all refs from other repos as stale. In
this situation, with refs-src and refs->dst both equal to refs/*, we should
ignore refs/remotes/* when looking for stale refs to prevent this from
happening.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 remote.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index e71f66d..863c183 100644
--- a/remote.c
+++ b/remote.c
@@ -1884,6 +1884,7 @@ struct stale_heads_info {
 	struct ref **stale_refs_tail;
 	struct refspec *refs;
 	int ref_count;
+	int ignore_remotes;
 };
 
 static int get_stale_heads_cb(const char *refname,
@@ -1903,7 +1904,8 @@ static int get_stale_heads_cb(const char *refname,
 	 * remote we consider it to be stale.
 	 */
 	if (!((flags & REF_ISSYMREF) ||
-	      string_list_has_string(info->ref_names, query.src))) {
+	      string_list_has_string(info->ref_names, query.src) ||
+	      (info->ignore_remotes && !prefixcmp(refname, "refs/remotes/")))) {
 		struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
 		hashcpy(ref->new_sha1, sha1);
 	}
@@ -1917,6 +1919,8 @@ struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fet
 	struct ref *ref, *stale_refs = NULL;
 	struct string_list ref_names = STRING_LIST_INIT_NODUP;
 	struct stale_heads_info info;
+	if(!strcmp(refs->src, "refs/*") && !strcmp(refs->dst, "refs/*"))
+		info.ignore_remotes = 1;
 	info.ref_names = &ref_names;
 	info.stale_refs_tail = &stale_refs;
 	info.refs = refs;
-- 
1.8.3.1-619-gbec0aa7

Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> When cloning a repo with --mirror, and adding more remotes later,
> get_stale_heads for origin would mark all refs from other repos as stale. In
> this situation, with refs-src and refs->dst both equal to refs/*, we should
> ignore refs/remotes/* when looking for stale refs to prevent this from
> happening.

I do not think it is a right solution to single out refs/remotes/*.

Going back to your original example:

    [remote "origin"]
            url = git://github.com/git/git.git
            fetch = +refs/*:refs/*
            mirror = true
    [remote "peff"]
            url = git://github.com/peff/git.git
            fetch = +refs/heads/*:refs/remotes/peff/*

Wouldn't you obtain "refs/remotes/github/html" from your "origin"
via "git pull origin"?  What happens to your local copy of that ref,
when it goes away from the origin and then you try to "fetch --prune
origin" the next time with this patch (and without this patch)?

What should happen?

What if you had this instead of the above version of remote.peff.*?

    [remote "peff"]
            url = git://github.com/peff/git.git
            fetch = +refs/heads/*:refs/remotes/github/*

I think this is an unsolvable problem, and I _think_ the root cause
of the issue is the configuration above that allows the RHS of
different fetch refspecs to overlap.  refs/* is more generic and
covers refs/remotes/peff/* and refs/remotes/github/*.  You cannot
even know, just by looking at "origin" and your local repository,
if refs/remotes/github/html you have should go away or it might have
come from somewhere else.

The best we _could_ do, without contacting all the defined remotes,
is probably to check each ref that we did not see from "origin" (for
example, you find "refs/remotes/peff/frotz" that your origin does
not have) and see if it could match RHS of fetch refspec of somebody
else (e.g. RHS of "refs/heads/*:refs/remotes/peff/*" matches that
ref).  Then we can conclude that refs/remotes/peff/frotz _might_
have come from Peff's repository and not from "origin", and then we
can optionally issue a warning and refrain from removing it.

This inevitably will have false positives and leave something that
did originally came from "origin", because peff may no longer have
'frotz' branch in his repository.  I do not think we can do better
than that, because we are trying to see if we can improve things
without having to contact all the remotes.

But if you go that route, the logic needs to go the same way when
you are pruning against 'peff', and anything that you do not see in
his repository right now but you have in refs/remotes/peff/ cannot
be pruned, because it might have come from your origin via more
generic refs/*:refs/* mapping.  It follows that you could never
prune anything under refs/remotes/peff/* hierarchy.

You could introduce a "assume that more specific mapping never
overlaps with a more generic mapping" rule (i.e. refs/* from RHS of
remote.origin.fetch is more generic than refs/remotes/peff/* from
RHS of remote.peff.fetch, and assume everything that you see in your
local refs/remotes/peff/* came from peff and not from origin, I
think, but at that point, is it worth the possible complexity to
code that rule in the prune codepath and brittleness of that
assumption that your origin will never add a new ref under that
hierarchy, e.g. refs/remotes/peff/xyzzy?

So, I dunno.

> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
>  remote.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index e71f66d..863c183 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1884,6 +1884,7 @@ struct stale_heads_info {
>  	struct ref **stale_refs_tail;
>  	struct refspec *refs;
>  	int ref_count;
> +	int ignore_remotes;
>  };
>  
>  static int get_stale_heads_cb(const char *refname,
> @@ -1903,7 +1904,8 @@ static int get_stale_heads_cb(const char *refname,
>  	 * remote we consider it to be stale.
>  	 */
>  	if (!((flags & REF_ISSYMREF) ||
> -	      string_list_has_string(info->ref_names, query.src))) {
> +	      string_list_has_string(info->ref_names, query.src) ||
> +	      (info->ignore_remotes && !prefixcmp(refname, "refs/remotes/")))) {
>  		struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
>  		hashcpy(ref->new_sha1, sha1);
>  	}
> @@ -1917,6 +1919,8 @@ struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fet
>  	struct ref *ref, *stale_refs = NULL;
>  	struct string_list ref_names = STRING_LIST_INIT_NODUP;
>  	struct stale_heads_info info;
> +	if(!strcmp(refs->src, "refs/*") && !strcmp(refs->dst, "refs/*"))
> +		info.ignore_remotes = 1;
>  	info.ref_names = &ref_names;
>  	info.stale_refs_tail = &stale_refs;
>  	info.refs = refs;

Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos

[Dennis Kaarsemaker]

(Sorry, I sent v2 before seeing this mail)

On do, 2013-06-20 at 15:46 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > When cloning a repo with --mirror, and adding more remotes later,
> > get_stale_heads for origin would mark all refs from other repos as stale. In
> > this situation, with refs-src and refs->dst both equal to refs/*, we should
> > ignore refs/remotes/* when looking for stale refs to prevent this from
> > happening.
> 
> I do not think it is a right solution to single out refs/remotes/*.
> 
> Going back to your original example:
> 
>     [remote "origin"]
>             url = git://github.com/git/git.git
>             fetch = +refs/*:refs/*
>             mirror = true
>     [remote "peff"]
>             url = git://github.com/peff/git.git
>             fetch = +refs/heads/*:refs/remotes/peff/*
> 
> Wouldn't you obtain "refs/remotes/github/html" from your "origin"
> via "git pull origin"?  What happens to your local copy of that ref,
> when it goes away from the origin and then you try to "fetch --prune
> origin" the next time with this patch (and without this patch)?

git pull origin gives me refs/html in this case. I did not try fetch
--prune, but prune origin DTRT: if the html branch goes away at the
origin, it goes away locally. Both with and without this patch.

It's refs/remotes/peff/somebranch that in this case *also* goes away
without this patch, but is untouched with this patch

> What should happen?

Exactly that.

> What if you had this instead of the above version of remote.peff.*?
> 
>     [remote "peff"]
>             url = git://github.com/peff/git.git
>             fetch = +refs/heads/*:refs/remotes/github/*

That doesn't change anything.

> I think this is an unsolvable problem, and I _think_ the root cause
> of the issue is the configuration above that allows the RHS of
> different fetch refspecs to overlap.  refs/* is more generic and
> covers refs/remotes/peff/* and refs/remotes/github/*.  You cannot
> even know, just by looking at "origin" and your local repository,
> if refs/remotes/github/html you have should go away or it might have
> come from somewhere else.
> 
> The best we _could_ do, without contacting all the defined remotes,
> is probably to check each ref that we did not see from "origin" (for
> example, you find "refs/remotes/peff/frotz" that your origin does
> not have) and see if it could match RHS of fetch refspec of somebody
> else (e.g. RHS of "refs/heads/*:refs/remotes/peff/*" matches that
> ref).  Then we can conclude that refs/remotes/peff/frotz _might_
> have come from Peff's repository and not from "origin", and then we
> can optionally issue a warning and refrain from removing it.

I like that idea, though I also like the simplicity of simply singling
out "remotes" as that's where normal remotes usually sit. And don't
forget about tags (see patch v2).

> This inevitably will have false positives and leave something that
> did originally came from "origin", because peff may no longer have
> 'frotz' branch in his repository.  I do not think we can do better
> than that, because we are trying to see if we can improve things
> without having to contact all the remotes.

But then the ref would have to be called "refs/remotes/peff/frotz"
upstream. Hmm, that is of course completely possible: cloning something
that's already a clone.

> But if you go that route, the logic needs to go the same way when
> you are pruning against 'peff', and anything that you do not see in
> his repository right now but you have in refs/remotes/peff/ cannot
> be pruned, because it might have come from your origin via more
> generic refs/*:refs/* mapping.  It follows that you could never
> prune anything under refs/remotes/peff/* hierarchy.
> 
> You could introduce a "assume that more specific mapping never
> overlaps with a more generic mapping" rule (i.e. refs/* from RHS of
> remote.origin.fetch is more generic than refs/remotes/peff/* from
> RHS of remote.peff.fetch, and assume everything that you see in your
> local refs/remotes/peff/* came from peff and not from origin, I
> think, but at that point, is it worth the possible complexity to
> code that rule in the prune codepath and brittleness of that
> assumption that your origin will never add a new ref under that
> hierarchy, e.g. refs/remotes/peff/xyzzy?
> 
> So, I dunno.

Yeah, I'm starting to think this is not such a good idea. How about plan
B: issuing a warning when adding a remote with a refspec that also
matches another remote's refspec?

Or plan C: add a per-remote pruneIgnore setting that in this case I
could set to refs/tags/* refs/remotes/* as I know it's correct? Could
even be combined with plan B.

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

>> Going back to your original example:
>> 
>>     [remote "origin"]
>>             url = git://github.com/git/git.git
>>             fetch = +refs/*:refs/*
>>             mirror = true
>>     [remote "peff"]
>>             url = git://github.com/peff/git.git
>>             fetch = +refs/heads/*:refs/remotes/peff/*
>> 
>> Wouldn't you obtain "refs/remotes/github/html" from your "origin"
>> via "git pull origin"?  What happens to your local copy of that ref,
>> when it goes away from the origin and then you try to "fetch --prune
>> origin" the next time with this patch (and without this patch)?
>
> git pull origin gives me refs/html in this case.

Maybe there is a miscommunication.

	$ git ls-remote git://github.com/git/git.git | grep remotes/

shows that that repository, your origin, has refs/remotes/github/html

And you have remote.origin.fetch = +refs/*:refs/* in your local
repository.  So that ref will come to your local repository under
the same name, i.e. refs/remotes/github/html, no?  Not as refs/html.

So this answer

>> What if you had this instead of the above version of remote.peff.*?
>> 
>>     [remote "peff"]
>>             url = git://github.com/peff/git.git
>>             fetch = +refs/heads/*:refs/remotes/github/*
>
> That doesn't change anything.

cannot be correct.  You fetch from your origin and then its
refs/remotes/github/html *will* want to come to your local
repository and be stored there.  If Peff has html branch in that
hypothetical configuration, you fetch from him and his html branch
(i.e. refs/heads/html from his point of view) will want to replace
your local refs/remotes/github/html.  Or Peff may not have html
branch.  The point is that you cannot tell by only looking at your
local ref namespace and what "origin" has.

> Yeah, I'm starting to think this is not such a good idea. How about plan
> B: issuing a warning when adding a remote with a refspec that also
> matches another remote's refspec?

Surely that will make things safer.

> Or plan C: add a per-remote pruneIgnore setting that in this case I
> could set to refs/tags/* refs/remotes/* as I know it's correct? Could
> even be combined with plan B.

As I already said "I dunno", I am not sure if it is worth the effort
to support overlapping RHSs of fetch refspecs, so between B and C, I
would vote for B.

Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos

[Dennis Kaarsemaker]

On do, 2013-06-20 at 16:30 -0700, Junio C Hamano wrote:

> Maybe there is a miscommunication.
> 
> 	$ git ls-remote git://github.com/git/git.git | grep remotes/
> 
> shows that that repository, your origin, has refs/remotes/github/html

Yes, I misunderstood you and see the problem now. Thanks for being
patient with me :)

> > Yeah, I'm starting to think this is not such a good idea. How about plan
> > B: issuing a warning when adding a remote with a refspec that also
> > matches another remote's refspec?
> 
> Surely that will make things safer.
> 
> > Or plan C: add a per-remote pruneIgnore setting that in this case I
> > could set to refs/tags/* refs/remotes/* as I know it's correct? Could
> > even be combined with plan B.
> 
> As I already said "I dunno", I am not sure if it is worth the effort
> to support overlapping RHSs of fetch refspecs, so between B and C, I
> would vote for B.

I'm halfway through cooking up a patch for B, as I agree that it will
make things safer.

I'd really like to have C as well though, would you accept a patch that
implements it?
-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> I'd really like to have C as well though, would you accept a patch that
> implements it?

I already said I dunno, and asking me 5 minutes after that would not
change my answer X-<.  I tend to agree with Peff that it is papering
over the underlying problem, which B solves.

Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos

[Jeff King]

On Thu, Jun 20, 2013 at 03:46:20PM -0700, Junio C Hamano wrote:

> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > When cloning a repo with --mirror, and adding more remotes later,
> > get_stale_heads for origin would mark all refs from other repos as stale. In
> > this situation, with refs-src and refs->dst both equal to refs/*, we should
> > ignore refs/remotes/* when looking for stale refs to prevent this from
> > happening.
> 
> I do not think it is a right solution to single out refs/remotes/*.

Yeah, I agree.

> Going back to your original example:
> 
>     [remote "origin"]
>             url = git://github.com/git/git.git
>             fetch = +refs/*:refs/*
>             mirror = true
>     [remote "peff"]
>             url = git://github.com/peff/git.git
>             fetch = +refs/heads/*:refs/remotes/peff/*

There is a fundamental namespace conflict here: one remote is claiming
the whole refs/* namespace, and another remote is claiming some subset.
"fetch --prune" is only one type of problem you can have; you might also
overwrite stuff from the "peff" remote with stuff from the "origin"
remote (if it happens to have "refs/remotes/peff/foo" itself).

> I think this is an unsolvable problem, and I _think_ the root cause
> of the issue is the configuration above that allows the RHS of
> different fetch refspecs to overlap.  refs/* is more generic and
> covers refs/remotes/peff/* and refs/remotes/github/*.  You cannot
> even know, just by looking at "origin" and your local repository,
> if refs/remotes/github/html you have should go away or it might have
> come from somewhere else.

Exactly.

> The best we _could_ do, without contacting all the defined remotes,
> is probably to check each ref that we did not see from "origin" (for
> example, you find "refs/remotes/peff/frotz" that your origin does
> not have) and see if it could match RHS of fetch refspec of somebody
> else (e.g. RHS of "refs/heads/*:refs/remotes/peff/*" matches that
> ref).  Then we can conclude that refs/remotes/peff/frotz _might_
> have come from Peff's repository and not from "origin", and then we
> can optionally issue a warning and refrain from removing it.

I think this is just papering over the problem in one instance. What
happens when you _do_ have overlapping refs in the "origin" remote, and
you have a true conflict.

I wonder why Dennis wants to "refs/*:refs/*" in the first place. It
is not usually a useful thing to have in a non-bare repository, because
fetches will overwrite local work on branches. If he just wanted the
automatic "git push --mirror" setting, that does not depend on the fetch
refspec.

We made this distinction in "git remote --mirror={fetch,remote}", but I
don't think "git clone --mirror" ever learned about it.

-Peff

Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos

[Dennis Kaarsemaker]

On do, 2013-06-20 at 19:08 -0400, Jeff King wrote:

> I wonder why Dennis wants to "refs/*:refs/*" in the first place. It
> is not usually a useful thing to have in a non-bare repository,
> because fetches will overwrite local work on branches. If he just
> wanted the automatic "git push --mirror" setting, that does not depend
> on the fetch refspec.

I'm not doing that in non-bare repositories, neither do I use it for
pushing. It's for a continuous integration system, which never has any
locally created branches or commits, but does integrate things from
different remotes in some cases. The example with git.git is used
roughly as follows:

* git fetch all remotes (for most projects that will be 1 remote)
* rebuild reflogs from github events (or fetch via http/ssh)
* per push to next, check out to a separate $GIT_WORK_TREE and run make 
  test
* for the last push, also build and publish daily tarball+deb+rpm

Then, for further testing of local requirements:

* cherry-pick your jk/blame_tree branch
* test, build and install package

Given that this system works with clones of what should be considered
canonical copies of repositories, those remotes shouldn't have any
remotes defined themselves, so at least being able to configure prune to
ignore refs/remotes/* and refs/tags/* would help me a lot.

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> I'm not doing that in non-bare repositories, neither do I use it for
> pushing. It's for a continuous integration system, which never has any
> locally created branches or commits, but does integrate things from
> different remotes in some cases.

Perhaps then

	[remote "origin"]
        	url = somewhere
                fetch = +refs/heads/*:refs/heads/*
                fetch = +refs/tags/*:refs/tags/*
	[remote "another"]
               	url = somewhere-else
                fetch = +refs/heads/*:refs/remotes/another/*
	[remote "third"]
               	url = third-place
                fetch = +refs/heads/*:refs/remotes/third/*

would be sufficient?  You may not even need tags, but that is an
independent issue.

[PATCH v2] remote: make prune work for mixed mirror/non-mirror repos

[Dennis Kaarsemaker]

When cloning a repo with --mirror, and adding more remotes later,
get_stale_heads for origin would mark all refs from other repos as stale. In
this situation, with refs-src and refs->dst both equal to refs/*, we should
ignore refs/remotes/* and refs/tags/* when looking for stale refs to
prevent this from happening.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 The previous attempt only ignored refs/remotes, but that's not good enough as
 that will still delete tags. So let's ignore refs/tags too. The downside is
 that tags removed at the origin don't get removed, but prune should only be
 pruning branches anyway if I read the documentation correctly.

 remote.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/remote.c b/remote.c
index e71f66d..efc5481 100644
--- a/remote.c
+++ b/remote.c
@@ -1884,6 +1884,7 @@ struct stale_heads_info {
 	struct ref **stale_refs_tail;
 	struct refspec *refs;
 	int ref_count;
+	int is_mirror;
 };
 
 static int get_stale_heads_cb(const char *refname,
@@ -1896,6 +1897,13 @@ static int get_stale_heads_cb(const char *refname,
 
 	if (query_refspecs(info->refs, info->ref_count, &query))
 		return 0; /* No matches */
+	/*
+	 * If we're pruning a clone that was --mirror'ed, let's ignore refs/tags
+	 * and refs/remotes
+	 */
+	if (info->is_mirror && (!prefixcmp(refname, "refs/tags/") ||
+	    !prefixcmp(refname, "refs/remotes/")))
+		return 0;
 
 	/*
 	 * If we did find a suitable refspec and it's not a symref and
@@ -1917,6 +1925,8 @@ struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fet
 	struct ref *ref, *stale_refs = NULL;
 	struct string_list ref_names = STRING_LIST_INIT_NODUP;
 	struct stale_heads_info info;
+	if(!strcmp(refs->src, "refs/*") && !strcmp(refs->dst, "refs/*"))
+		info.is_mirror = 1;
 	info.ref_names = &ref_names;
 	info.stale_refs_tail = &stale_refs;
 	info.refs = refs;
-- 
1.8.3.1-619-gbec0aa7

[PATCH 0/3] Handling overlapping refspecs slightly smarter

[Dennis Kaarsemaker]

1/3 should be pretty sane, just adding a warning in documentation and 'git
remote add' about overlapping refspecs.

2/3 only makes sense if 3/3 is accepted, as it's a test for that change.

3/3 I'm not 100% sure about, though the approach feels reasonably ok. It changes
get_stale_heads to also detect overlapping refspecs and abort any prune action
if it finds them. What I'm not sure about is whether this is the right place to
do it, or to do it in the callers of get_stale_heads and exit(1) in this
situation.

Both 1/3 and 3/3 ignore exactly matching refspecs, as that's a supported thing
already, another test in t5505 broke before I made both ignore exactly matching
refspecs.


Dennis Kaarsemaker (3):
  remote: Add warnings about mixin --mirror and other remotes
  remote: Add test for prune and mixed --mirror and normal remotes
  remote: don't prune when detecting overlapping refspecs

 Documentation/git-remote.txt |  6 +++++-
 builtin/remote.c             | 17 +++++++++++++++++
 remote.c                     | 23 +++++++++++++++++++++++
 t/t5505-remote.sh            |  9 +++++++++
 4 files changed, 54 insertions(+), 1 deletion(-)

-- 
1.8.3.1-619-gbec0aa7

[PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes

[Dennis Kaarsemaker]

When cloning a repo with --mirror, and adding more remotes later,
get_stale_heads for origin will mark all refs from other repos as stale.
Add a warning to the documentation, and warn the user when we detect
such things during git remote add.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 Documentation/git-remote.txt |  6 +++++-
 builtin/remote.c             | 17 +++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 581bb4c..d7457df 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -71,7 +71,11 @@ When a fetch mirror is created with `--mirror=fetch`, the refs will not
 be stored in the 'refs/remotes/' namespace, but rather everything in
 'refs/' on the remote will be directly mirrored into 'refs/' in the
 local repository. This option only makes sense in bare repositories,
-because a fetch would overwrite any local commits.
+because a fetch would overwrite any local commits. If you want to add extra
+remotes to a repository created with `--mirror=fetch`, you will need to change
+the configuration of the mirrored remote to fetch only `refs/heads/*`,
+otherwise `git remote prune <remote>` will remove all branches for the extra
+remotes.
 +
 When a push mirror is created with `--mirror=push`, then `git push`
 will always behave as if `--mirror` was passed.
diff --git a/builtin/remote.c b/builtin/remote.c
index 5e54d36..a4f9cb8 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -112,6 +112,21 @@ enum {
 #define MIRROR_PUSH 2
 #define MIRROR_BOTH (MIRROR_FETCH|MIRROR_PUSH)
 
+static int check_overlapping_refspec(struct remote *remote, void *priv)
+{
+	char *refspec = priv;
+	int i;
+	for (i = 0; i < remote->fetch_refspec_nr; i++) {
+		if(strcmp(refspec, remote->fetch[i].dst) &&
+		   (!fnmatch(refspec, remote->fetch[i].dst, 0) ||
+		    !fnmatch(remote->fetch[i].dst, refspec, 0))) {
+			warning(_("Overlapping refspecs detected: '%s' and '%s'"),
+			        refspec, remote->fetch[i].dst);
+		}
+	}
+	return 0;
+}
+
 static int add_branch(const char *key, const char *branchname,
 		const char *remotename, int mirror, struct strbuf *tmp)
 {
@@ -123,6 +138,8 @@ static int add_branch(const char *key, const char *branchname,
 	else
 		strbuf_addf(tmp, "refs/heads/%s:refs/remotes/%s/%s",
 				branchname, remotename, branchname);
+
+	for_each_remote(check_overlapping_refspec, strchr((const char*)tmp->buf, ':') + 1);
 	return git_config_set_multivar(key, tmp->buf, "^$", 0);
 }
 
-- 
1.8.3.1-619-gbec0aa7

Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> When cloning a repo with --mirror, and adding more remotes later,

Even though --mirror is not the only way to trigger this, I think it
is good to mention it because it is the most common way to do so.

> get_stale_heads for origin will mark all refs from other repos as stale.

As Peff already said, that is only one symptom.  Another is "git
fetch" from origin and then "git fetch another" can try to overwrite
the same ref in refs/remotes/* hierarchy.  We should say what we are
avoiding upfront, not just one of the consequences of the bad thing
we are avoiding.

	... more remotes later, we would end up with fetch refspecs
	from different remotes trying to overwrite the same ref for
	their remote tracking purposes.

        This can cause confusion (e.g. where did
	"refs/remotes/peff/master" come from?  Is it a copy of the
	master branch of remote 'peff', or does our mirror origin
	have a remote tracking branch for a remote it calls 'peff',
	which may not be related to our 'peff', and we just got a
	straight copy from it?) and can cause "remote prune" to
	misbehave.

or something like that.

> Add a warning to the documentation, and warn the user when we detect
> such things during git remote add.
>
> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
>  Documentation/git-remote.txt |  6 +++++-
>  builtin/remote.c             | 17 +++++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index 581bb4c..d7457df 100644
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
> @@ -71,7 +71,11 @@ When a fetch mirror is created with `--mirror=fetch`, the refs will not
>  be stored in the 'refs/remotes/' namespace, but rather everything in
>  'refs/' on the remote will be directly mirrored into 'refs/' in the
>  local repository. This option only makes sense in bare repositories,
> -because a fetch would overwrite any local commits.
> +because a fetch would overwrite any local commits. If you want to add extra
> +remotes to a repository created with `--mirror=fetch`, you will need to change
> +the configuration of the mirrored remote to fetch only `refs/heads/*`,
> +otherwise `git remote prune <remote>` will remove all branches for the extra
> +remotes.
>  +
>  When a push mirror is created with `--mirror=push`, then `git push`
>  will always behave as if `--mirror` was passed.
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 5e54d36..a4f9cb8 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -112,6 +112,21 @@ enum {
>  #define MIRROR_PUSH 2
>  #define MIRROR_BOTH (MIRROR_FETCH|MIRROR_PUSH)
>  
> +static int check_overlapping_refspec(struct remote *remote, void *priv)

It is customery to call that cb_data (i.e. data for the callback) in
our codebase.

> +{
> +	char *refspec = priv;

You are passing only the RHS of the refspec, so this variable name
is confusing to the reader.  Perhaps "dst", as you will be comparing
with ".dst" which is RHS of the fetch refspec for the remotes?

> +	int i;
> +	for (i = 0; i < remote->fetch_refspec_nr; i++) {
> +		if(strcmp(refspec, remote->fetch[i].dst) &&

Style (have SP between if and open paren).

> +		   (!fnmatch(refspec, remote->fetch[i].dst, 0) ||
> +		    !fnmatch(remote->fetch[i].dst, refspec, 0))) {

Does .dst always exist and is never a NULL?  My quick scan of
remote.c::parse_refspec_internal() tells me otherwise.

Also what are you matching with what?  refs/* against refs/remotes/origin/*?

What if remote->fetch[i] is not a wildcarded refspec, e.g.

	[remote "origin"]
        	fetch = +refs/heads/master:refs/heads/origin
		fetch = +refs/heads/next:refs/remotes/origin/next

You would want to check for equality in such a case against the RHS
of the refspeec you have.

> +			warning(_("Overlapping refspecs detected: '%s' and '%s'"),
> +			        refspec, remote->fetch[i].dst);

We know which remote is the offending one, so we can afford to be
more helpful to the user and say which existing remote's fetch
refspec overlaps with the one the user is attempting to add.

> +		}
> +	}
> +	return 0;
> +}
> +
>  static int add_branch(const char *key, const char *branchname,
>  		const char *remotename, int mirror, struct strbuf *tmp)
>  {
> @@ -123,6 +138,8 @@ static int add_branch(const char *key, const char *branchname,
>  	else
>  		strbuf_addf(tmp, "refs/heads/%s:refs/remotes/%s/%s",
>  				branchname, remotename, branchname);
> +
> +	for_each_remote(check_overlapping_refspec, strchr((const char*)tmp->buf, ':') + 1);

Do you need this cast???

>  	return git_config_set_multivar(key, tmp->buf, "^$", 0);
>  }

Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes

[Dennis Kaarsemaker]

On vr, 2013-06-21 at 11:42 -0700, Junio C Hamano wrote:
> > +                (!fnmatch(refspec, remote->fetch[i].dst, 0) ||
> > +                 !fnmatch(remote->fetch[i].dst, refspec, 0))) {
> 
> Does .dst always exist and is never a NULL?  My quick scan of
> remote.c::parse_refspec_internal() tells me otherwise.
> 
> Also what are you matching with what?  refs/* against
> refs/remotes/origin/*?
> 
> What if remote->fetch[i] is not a wildcarded refspec, e.g.
> 
>         [remote "origin"]
>                 fetch = +refs/heads/master:refs/heads/origin
>                 fetch = +refs/heads/next:refs/remotes/origin/next
> 
> You would want to check for equality in such a case against the RHS
> of the refspeec you have.

Thanks for all the feedback, I've incorporated it all in a reroll that's
in progress, except for the above.

I've added a prefix check, so refs/foo and refs/foo/bar will be
considered clashes, but not yet an equality check. Equality for
wildcards is allowed and tested for, so do we really want to 'outlaw'
equality of non-wildcard refspecs?

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> Equality for
> wildcards is allowed and tested for, so do we really want to 'outlaw'
> equality of non-wildcard refspecs?

I am not sure what you mean by "equality for wildcards is allowed".
Do you mean this pair of remote definition is sane and not warned?

	[remote "one"]
        	fetch = refs/heads/*:refs/remotes/mixed/*

	[remote "two"]
        	fetch = refs/heads/*:refs/remotes/mixed/*

For non-wildcard ones, I think these pairs are both suspects for
possible clashes and want to be warned.

(1) literal-vs-literal

	[remote "one"]
        	fetch = refs/heads/master:refs/heads/origin

	[remote "two"]
        	fetch = refs/heads/master:refs/heads/origin

(2) literal-vs-wildcard

	[remote "one"]
        	fetch = refs/heads/*:refs/remotes/origin/*

	[remote "two"]
        	fetch = refs/heads/master:refs/remotes/origin/master

Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes

[Dennis Kaarsemaker]

On zo, 2013-06-23 at 14:22 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > Equality for
> > wildcards is allowed and tested for, so do we really want to 'outlaw'
> > equality of non-wildcard refspecs?
> 
> I am not sure what you mean by "equality for wildcards is allowed".
> Do you mean this pair of remote definition is sane and not warned?
> 
> 	[remote "one"]
>         	fetch = refs/heads/*:refs/remotes/mixed/*
> 
> 	[remote "two"]
>         	fetch = refs/heads/*:refs/remotes/mixed/*

I personally don't consider them very sane and didn't originally support
that. But this behavior is tested for in t5505-remote.sh test 27, which
started failing until I stopped warning for equal refspecs. This support
for "alt remotes" in prune was added by c175a7ad in 2008. The commit
message for that commit give a plausible reason for using them.

> For non-wildcard ones, I think these pairs are both suspects for
> possible clashes and want to be warned.
> 
> (1) literal-vs-literal
> 
> 	[remote "one"]
>         	fetch = refs/heads/master:refs/heads/origin
> 
> 	[remote "two"]
>         	fetch = refs/heads/master:refs/heads/origin

I agree, but c175a7ad would disagree.

> (2) literal-vs-wildcard
> 
> 	[remote "one"]
>         	fetch = refs/heads/*:refs/remotes/origin/*
> 
> 	[remote "two"]
>         	fetch = refs/heads/master:refs/remotes/origin/master
> 

Agreed and was already covered in v1.
-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> On zo, 2013-06-23 at 14:22 -0700, Junio C Hamano wrote:
>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>> 
>> > Equality for
>> > wildcards is allowed and tested for, so do we really want to 'outlaw'
>> > equality of non-wildcard refspecs?
>> 
>> I am not sure what you mean by "equality for wildcards is allowed".
>> Do you mean this pair of remote definition is sane and not warned?
>> 
>> 	[remote "one"]
>>         	fetch = refs/heads/*:refs/remotes/mixed/*
>> 
>> 	[remote "two"]
>>         	fetch = refs/heads/*:refs/remotes/mixed/*
>
> I personally don't consider them very sane and didn't originally support
> that. But this behavior is tested for in t5505-remote.sh test 27, which
> started failing until I stopped warning for equal refspecs. This support
> for "alt remotes" in prune was added by c175a7ad in 2008. The commit
> message for that commit give a plausible reason for using them.

I actually do not read it that way.  What it wanted to do primarily
was to avoid pruning "refs/remotes/alt/*" based on what it observed
at the remote named "alt", when the refs fetched from that remote is
set to update "refs/remotes/origin/*".

The example in the log message is a special case where two
physically different remotes are actually copies of a single logical
repository, so in that narrow use case, it may be OK, but it is an
unusual thing to do and we should "warn" about it, I think.

In any case, I've been assuming in this discussion "allow" is to
silently accept, and overlaps are "warned" but not "rejected".  So
if you meant by 'outlaw' to die and refuse to run, that is not what
I meant.

Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes

[Dennis Kaarsemaker]

On zo, 2013-06-23 at 15:33 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > On zo, 2013-06-23 at 14:22 -0700, Junio C Hamano wrote:
> >> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> >> 
> >> > Equality for
> >> > wildcards is allowed and tested for, so do we really want to 'outlaw'
> >> > equality of non-wildcard refspecs?
> >> 
> >> I am not sure what you mean by "equality for wildcards is allowed".
> >> Do you mean this pair of remote definition is sane and not warned?
> >> 
> >> 	[remote "one"]
> >>         	fetch = refs/heads/*:refs/remotes/mixed/*
> >> 
> >> 	[remote "two"]
> >>         	fetch = refs/heads/*:refs/remotes/mixed/*
> >
> > I personally don't consider them very sane and didn't originally support
> > that. But this behavior is tested for in t5505-remote.sh test 27, which
> > started failing until I stopped warning for equal refspecs. This support
> > for "alt remotes" in prune was added by c175a7ad in 2008. The commit
> > message for that commit give a plausible reason for using them.
> 
> I actually do not read it that way.  What it wanted to do primarily
> was to avoid pruning "refs/remotes/alt/*" based on what it observed
> at the remote named "alt", when the refs fetched from that remote is
> set to update "refs/remotes/origin/*".
>
> The example in the log message is a special case where two
> physically different remotes are actually copies of a single logical
> repository, so in that narrow use case, it may be OK, but it is an
> unusual thing to do and we should "warn" about it, I think.

Apart from the exactly matching refspecs, does git in any other way
treat this as a special case?

> In any case, I've been assuming in this discussion "allow" is to
> silently accept, and overlaps are "warned" but not "rejected".  So
> if you meant by 'outlaw' to die and refuse to run, that is not what
> I meant.

Well, 1/3 is a warning on add, 3/3 is a warning and refusing to prune.
Should 3/3 do something else instead? Perhaps prompt for confirmation or
require some sort of --force?

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> Apart from the exactly matching refspecs, does git in any other way
> treat this as a special case?

Sorry, I do not quite understand, especially the "exactly matching"
part, which as far as I know is not special (in other words, I am
not sure what kind of "special casing" you are discussing).

>> In any case, I've been assuming in this discussion "allow" is to
>> silently accept, and overlaps are "warned" but not "rejected".  So
>> if you meant by 'outlaw' to die and refuse to run, that is not what
>> I meant.
>
> Well, 1/3 is a warning on add, 3/3 is a warning and refusing to prune.

By "refusing to prune" do you mean "error out with die()"?

I was trying to make sure your "outlaw" was not something else
(e.g. "die without not pruning anything that are not even
overlapping"), which is probably too strong.

I think what your patch did was "keep them around not pruned because
we do not know where they came from" (I just checked $gmane/228589
again); that is in line with "warned but not rejected", so I think
we are OK.

[PATCH 2/3] remote: Add test for prune and mixed --mirror and normal remotes

[Dennis Kaarsemaker]

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 t/t5505-remote.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index dd10ff0..439e996 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -428,6 +428,15 @@ test_expect_success 'add alt && prune' '
 	 git rev-parse --verify refs/remotes/origin/side2)
 '
 
+test_expect_failure 'prune and overlapping refspecs' '
+	(git clone --mirror one prunetst &&
+	 cd prunetst &&
+	 git remote add two ../two &&
+     git fetch two &&
+	 git remote prune origin &&
+     git rev-parse --verify refs/remotes/two/another)
+'
+
 cat >test/expect <<\EOF
 some-tag
 EOF
-- 
1.8.3.1-619-gbec0aa7

[PATCH 3/3] remote: don't prune when detecting overlapping refspecs

[Dennis Kaarsemaker]

When cloning a repo with --mirror, and adding more remotes later,
get_stale_heads for origin would mark all refs from other repos as
stale. There's no "good" way to solve, this so the best thing we can do
is refusing to prune if we detect this and warning the user.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 remote.c          | 23 +++++++++++++++++++++++
 t/t5505-remote.sh |  2 +-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index e71f66d..39a9405 100644
--- a/remote.c
+++ b/remote.c
@@ -1912,11 +1912,34 @@ static int get_stale_heads_cb(const char *refname,
 	return 0;
 }
 
+static int check_overlapping_remotes(struct remote *first, void *priv) {
+	struct remote *second = priv;
+	int i, j;
+	if(!second)
+		return for_each_remote(check_overlapping_remotes, first);
+	if(first == second)
+		return 0;
+	for (i = 0; i < first->fetch_refspec_nr; i++) {
+		for (j = 0; j < second->fetch_refspec_nr; j++) {
+			if(strcmp(first->fetch[i].dst, second->fetch[j].dst) &&
+			   (!fnmatch(first->fetch[i].dst, second->fetch[j].dst, 0) ||
+			    !fnmatch(second->fetch[j].dst, first->fetch[i].dst, 0))) {
+				warning(_("Overlapping refspecs detected: '%s' and '%s', not pruning."),
+					first->fetch[i].dst, second->fetch[j].dst);
+				return 1;
+			}
+		}
+	}
+	return 0;
+}
+
 struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fetch_map)
 {
 	struct ref *ref, *stale_refs = NULL;
 	struct string_list ref_names = STRING_LIST_INIT_NODUP;
 	struct stale_heads_info info;
+	if(for_each_remote(check_overlapping_remotes, NULL))
+		return NULL;
 	info.ref_names = &ref_names;
 	info.stale_refs_tail = &stale_refs;
 	info.refs = refs;
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 439e996..d4ac6ce 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -428,7 +428,7 @@ test_expect_success 'add alt && prune' '
 	 git rev-parse --verify refs/remotes/origin/side2)
 '
 
-test_expect_failure 'prune and overlapping refspecs' '
+test_expect_success 'prune and overlapping refspecs' '
 	(git clone --mirror one prunetst &&
 	 cd prunetst &&
 	 git remote add two ../two &&
-- 
1.8.3.1-619-gbec0aa7

Re: [PATCH 3/3] remote: don't prune when detecting overlapping refspecs

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> +static int check_overlapping_remotes(struct remote *first, void *priv) {
> +	struct remote *second = priv;
> +	int i, j;
> +	if(!second)
> +		return for_each_remote(check_overlapping_remotes, first);
> +	if(first == second)
> +		return 0;
> +	for (i = 0; i < first->fetch_refspec_nr; i++) {
> +		for (j = 0; j < second->fetch_refspec_nr; j++) {
> +			if(strcmp(first->fetch[i].dst, second->fetch[j].dst) &&
> +			   (!fnmatch(first->fetch[i].dst, second->fetch[j].dst, 0) ||
> +			    !fnmatch(second->fetch[j].dst, first->fetch[i].dst, 0))) {
> +				warning(_("Overlapping refspecs detected: '%s' and '%s', not pruning."),
> +					first->fetch[i].dst, second->fetch[j].dst);
> +				return 1;
> +			}
> +		}
> +	}
> +	return 0;
> +}

This codepath essentially needs the same logic as 1/3, no?  Instead
of open code the inner loop here, can't you call the "check RHS of a
single fetch refspec for overlap with refspecs from a remote" helper
you introduced in 1/3?

The logic in the inner loop shares the same issue as the code in
1/3; it needs to be extended to cover non-wildcard respecs and
non-storing refspecs.