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.
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().
(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.
I think there is no design level question on this one. I think both are
bugs we currently have (and they may probably not be regressions).
On Sun, 16 Mar 2008, Junio C Hamano wrote:
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.
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".
(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*
Daniel Barkalow [off-list ref] writes:
On Sun, 16 Mar 2008, Junio C Hamano wrote:
...
quoted
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.)
Ok.
As I said, I think either one is valid, and I only mentioned (1) because I
thought refusing duplicates might be more work to get it right. So if
your code does _most of_ (2), that is good. Please document what it is
meant to do in Documentation/pull-fetch-param.txt, so that others can
report deviation from the defined semantics, if any, in the implementation
for us to fix.
quoted
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".
The feeler patch I sent out would be Ok, then. Can you test it, after
updating it with the die() -> error() and message rewording we discussed
in the other message, and send the result in?
quoted
(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.
I tend to agree. I'd propose the semantics for refs/remotes/<name>/HEAD
symref to be like this:
* It is under _local_ control. That means fetch should not update it, and
"remote prune" should not prune it, nor even mention it is prunable.
* It is the means for the user (i.e. the owner of the local repository)
to express which branch from the remote he is most interested in.
I.e., it exists solely to make "<name>" => "refs/remotes/<name>/HEAD"
ref dwimming work as expected.
* It is set up by "git clone" to point at the branch the remote had its
HEAD pointing at when clone happened but that is merely a convenience
feature.
* We would probably want an explicit convenience subcommand "git remote
something <name> <branch>" that switches refs/remotes/<name>/HEAD to
point at a specific remote tracking branch, although you can do that
yourself with symbolic-ref.
* We may want to teach "git remote add <name>" to do the same HEAD
discovery as done by "git clone" (earlier JBF had a patch for it to the
scripted version), to have the same convenience feature as "git clone"
has.
* If we teach "git remote add" to set refs/remotes/<name>/HEAD, we may
also want to teach it an explicit way to let the user say "I want
<name> to mean refs/remotes/<name>/this", not whatever the remote side
currently points at with its HEAD.
* If we teach "git remote add" to do the HEAD discovery, we may also want
to teach "git remote update" a way to let the user request "my
refs/remotes/<name>/HEAD may not be pointing at the branch the remote
currently points at with its HEAD. Please update mine to match
theirs".
When true mirroring configuration "refs/*:refs/*" is employed, neither
"refs/HEAD" nor "refs/heads/HEAD" is needed nor desired on the local side.
On Sun, 16 Mar 2008, Junio C Hamano wrote:
Daniel Barkalow [off-list ref] writes:
quoted
On Sun, 16 Mar 2008, Junio C Hamano wrote:
...
quoted
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.)
Ok.
As I said, I think either one is valid, and I only mentioned (1) because I
thought refusing duplicates might be more work to get it right. So if
your code does _most of_ (2), that is good. Please document what it is
meant to do in Documentation/pull-fetch-param.txt, so that others can
report deviation from the defined semantics, if any, in the implementation
for us to fix.
Oh, sorry, I got the cases backwards. The current code does (1): for each
refspec, it collects all of the matches for that refspec into one big
list, and then it (a) removes items where src and dst match and (b) gives
errors if dst matches but src doesn't (which is obviously a problem: two
refspecs will try to write different things to the same place).
quoted
quoted
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".
The feeler patch I sent out would be Ok, then. Can you test it, after
updating it with the die() -> error() and message rewording we discussed
in the other message, and send the result in?
Sure.
quoted
quoted
(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.
I tend to agree. I'd propose the semantics for refs/remotes/<name>/HEAD
symref to be like this:
* It is under _local_ control. That means fetch should not update it, and
"remote prune" should not prune it, nor even mention it is prunable.
Ah, interesting. Not at all what I'd expected, but much more useful, I
think, than having it controlled by the remote.
* It is the means for the user (i.e. the owner of the local repository)
to express which branch from the remote he is most interested in.
I.e., it exists solely to make "<name>" => "refs/remotes/<name>/HEAD"
ref dwimming work as expected.
* It is set up by "git clone" to point at the branch the remote had its
HEAD pointing at when clone happened but that is merely a convenience
feature.
In other words, a reasonable default since you haven't yet configured
anything.
* We would probably want an explicit convenience subcommand "git remote
something <name> <branch>" that switches refs/remotes/<name>/HEAD to
point at a specific remote tracking branch, although you can do that
yourself with symbolic-ref.
The command would mainly be useful to users as a way of making it clear
that it's theirs to control, rather than as a method for controlling it.
* We may want to teach "git remote add <name>" to do the same HEAD
discovery as done by "git clone" (earlier JBF had a patch for it to the
scripted version), to have the same convenience feature as "git clone"
has.
Right.
* If we teach "git remote add" to set refs/remotes/<name>/HEAD, we may
also want to teach it an explicit way to let the user say "I want
<name> to mean refs/remotes/<name>/this", not whatever the remote side
currently points at with its HEAD.
And likewise clone, which is probably more relevant, actually, because
clone will often also create a local branch based on it and check that
out, which can waste some time if you actually want to use some other
branch.
* If we teach "git remote add" to do the HEAD discovery, we may also want
to teach "git remote update" a way to let the user request "my
refs/remotes/<name>/HEAD may not be pointing at the branch the remote
currently points at with its HEAD. Please update mine to match
theirs".
Right.
When true mirroring configuration "refs/*:refs/*" is employed, neither
"refs/HEAD" nor "refs/heads/HEAD" is needed nor desired on the local side.
Sure.
-Daniel
*This .sig left intentionally blank*