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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 20, 2013, 22:46 UTC
Message-ID: <7vppvgpfib.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1371766304-4601-1-git-send-email-dennis@kaarsemaker.net>

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

Show 5 quoted lines

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

Show 36 quoted lines

> 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;