Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes
Dennis Kaarsemaker 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. -- 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 1/3] remote: Add warnings about mixin --mirror and other remotes
On zo, 2013-06-23 at 15:33 -0700, Junio C Hamano wrote: > Dennis Kaarsemaker writes: > > > On zo, 2013-06-23 at 14:22 -0700, Junio C Hamano wrote: > >> Dennis Kaarsemaker 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 -- 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 1/3] remote: Add warnings about mixin --mirror and other remotes
Dennis Kaarsemaker writes: > On zo, 2013-06-23 at 14:22 -0700, Junio C Hamano wrote: >> Dennis Kaarsemaker 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. -- 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 1/3] remote: Add warnings about mixin --mirror and other remotes
On zo, 2013-06-23 at 14:22 -0700, Junio C Hamano wrote: > Dennis Kaarsemaker 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 -- 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 1/3] remote: Add warnings about mixin --mirror and other remotes
Dennis Kaarsemaker 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 -- 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 1/3] remote: Add warnings about mixin --mirror and other remotes
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
--
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 1/3] remote: Add warnings about mixin --mirror and other remotes
Dennis Kaarsemaker 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
> ---
> 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 ` 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);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
[PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes
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
---
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 ` 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
--
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
