Re: On fetch refspecs and wildcards

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Mar 16, 2008, 23:03 UTC
Message-ID: <alpine.LNX.1.00.0803161831330.19665@iabervon.org>
In-Reply-To: <7v8x0igvdp.fsf_-_@gitster.siamese.dyndns.org>

On Sun, 16 Mar 2008, Junio C Hamano wrote:

Show 43 quoted lines

> I was looking at t5505 tests and noticed something funny.
> 
> This is a design level question, so I am cc'ing Daniel whose remote.c is
> heavily involved in the new implementation.
> 
> What should this config do:
> 
>     [remote "origin"]
>         url = ../one/.git
>         fetch = +refs/heads/*:refs/remotes/origin/*
>         fetch = refs/heads/master:refs/heads/upstream
> 
> when the other repository (../one/.git) has branches "master" and "side2"?
> 
> I am not sure if the original implementation used to copy master to both
> refs/remotes/origin/master and refs/heads/upstream, but I think that is
> what the users would expect.  
> 
> I think the current one excludes any source that has an explicit
> destination from the wildcard matches.  It is probably Ok as long as we
> reject if the same source has more than one destinations (or matches more
> than one wildcards, for that matter) like this as a configuration error:
> 
>     [remote "origin"]
>         url = ../one/.git
>         fetch = refs/heads/master:refs/heads/upstream
>         fetch = refs/heads/master:refs/heads/another
> 
> If it doesn't, it does feel somewhat inconsistent.
> 
> Fortunately or unfortunately, Documentation/pull-fetch-param.txt does not
> talk about wildcard refspecs (not even the syntax, let alone the
> semantics), so we can define whatever we want right now, and I think both
> 
>     (1) allow duplicated destinations, including wildcard matches; and
> 
>     (2) refuse duplicated destinations for explicit ones, and more than
>         one wildcard patterns that match the same ref, but omit explicitly
>         specified ones from wildcard matches;
> 
> are viable options.  I suspect the current code does not do either.  We
> should pick one semantics, make sure the implementation matches that, and
> document it.

Actually, I think the current code is close to (2). get_fetch_map() returns everything, ref_remove_duplicates() removes any exact matches and gives errors if there's the same destination for two different sources.

(Upon further consideration, there's one slight issue:

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

is not quite the same as:

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

in whether pu will be forced; the forcing flag on the first matching refspec is what matters.)

In any case, the implementation should be easy enough with any of these combinations.

Show 18 quoted lines

> Another topic is what the semantics should be for mirroring configuration,
> like this:
> 
>     [remote "origin"]
>         url = ../one/.git
>         fetch = refs/*:refs/*
> 
> or
> 
>     [remote "origin"]
>         url = ../one/.git
>         fetch = refs/*:refs/remotes/one/*
> 
> The issues are:
> 
>  (1) get_fetch_map() currently insists on refname to be check_ref_format()
>      clean; it even rejects CHECK_REF_FORMAT_ONELEVEL, which means that
>      refs/stash would not be considered Ok and the code will die().

Yes, that's probably wrong. We probably do want to reject people whose servers send us "refs/heads/../../heads/master", but not "refs/stash".

Show 8 quoted lines

>  (2) "git remote prune" seems to cull refs/remotes/one/HEAD if exists.
> 
> Currently we do not have a way to determine where HEAD at the remote
> points at at the protocol level (I've sent a patch to the list earlier for
> the necessary protocol extension on the upload-pack side, but receiver
> side never got implemented in remotes.c).  So we cannot propagate
> refs/HEAD information correctly right now, but when we accept the protocol
> extension to do so, issue (1) will matter also for HEAD.

There's the issue that "HEAD" isn't "refs/HEAD". I'm not at all sure how the user should communicate the desire to update things to match the remote HEAD. FWIW, I was considering moving the code to guess where the remote HEAD points from builtin-clone to remotes.c, until I realized that it's not clear what configuration should control this. I think it'd be necessary to have a special option to say "write HEAD here", but I may be wrong.

The implementation of whatever handles it is slightly non-trivial, since struct ref can't currently represent a symref, but that shouldn't be a major issue.

	-Daniel
*This .sig left intentionally blank*