Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes
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.
Show 35 quoted lines
> 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/nextYou 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.
Show 14 quoted lines
> + }
> + }
> + 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);
> }
Dennis Kaarsemaker
