From 13fc2c1877a50dde5ea43e2f37420ebfb2f7dad1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Mar 2011 15:52:52 -0400 Subject: [PATCH 1/3] remote: disallow some nonsensical option combinations It doesn't make sense to use "-m" on a mirror, since "-m" sets up the HEAD symref in the remotes namespace, but with mirror, we are by definition not using a remotes namespace. Similarly, it does not make much sense to specify refspecs with --mirror. For a mirror you plan to push to, those refspecs will be ignored. For a mirror you are fetching from, there is no point in mirroring, since the refspec specifies everything you want to grab. There is one case where "--mirror -t " would be useful. Because is used as-is in the refspec, and because we append it to to refs/, you could mirror a subset of the hierarchy by doing: git remote add --mirror -t 'tags/*' But using anything besides a single branch as an argument to "-t" is not documented and only happens to work, so closing it off is not a serious regression. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/remote.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/builtin/remote.c b/builtin/remote.c index cb26080956..952be2e27b 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -161,6 +161,11 @@ static int add(int argc, const char **argv) if (argc < 2) usage_with_options(builtin_remote_add_usage, options); + if (mirror && master) + die("specifying a master branch makes no sense with --mirror"); + if (mirror && track.nr) + die("specifying branches to track makes no sense with --mirror"); + name = argv[0]; url = argv[1]; From a9f5a3558dcf83440c60ae5a2e2b56c80d65bb0b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Mar 2011 15:53:19 -0400 Subject: [PATCH 2/3] remote: separate the concept of push and fetch mirrors git-remote currently has one option, "--mirror", which sets up mirror configuration which can be used for either fetching or pushing. It looks like this: [remote "mirror"] url = wherever fetch = +refs/*:refs/* mirror = true However, a remote like this can be dangerous and confusing. Specifically: 1. If you issue the wrong command, it can be devastating. You are not likely to "push" when you meant to "fetch", but "git remote update" will try to fetch it, even if you intended the remote only for pushing. In either case, the results can be quite destructive. An unintended push will overwrite or delete remote refs, and an unintended fetch can overwrite local branches. 2. The tracking setup code can produce confusing results. The fetch refspec above means that "git checkout -b new master" will consider refs/heads/master to come from the remote "mirror", even if you only ever intend to push to the mirror. It will set up the "new" branch to track mirror's refs/heads/master. 3. The push code tries to opportunistically update tracking branches. If you "git push mirror foo:bar", it will see that we are updating mirror's refs/heads/bar, which corresponds to our local refs/heads/bar, and will update our local branch. To solve this, we split the concept into "push mirrors" and "fetch mirrors". Push mirrors set only remote.*.mirror, solving (2) and (3), and making an accidental fetch write only into FETCH_HEAD. Fetch mirrors set only the fetch refspec, meaning an accidental push will not force-overwrite or delete refs on the remote end. The new syntax is "--mirror=". For compatibility, we keep "--mirror" as-is, setting up both types simultaneously. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/git-remote.txt | 19 ++++++--- builtin/remote.c | 51 +++++++++++++++++------ t/t5505-remote.sh | 78 ++++++++++++++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 19 deletions(-) diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt index c258ea48db..79e38fedd7 100644 --- a/Documentation/git-remote.txt +++ b/Documentation/git-remote.txt @@ -10,7 +10,7 @@ SYNOPSIS -------- [verse] 'git remote' [-v | --verbose] -'git remote add' [-t ] [-m ] [-f] [--tags|--no-tags] [--mirror] +'git remote add' [-t ] [-m ] [-f] [--tags|--no-tags] [--mirror=] 'git remote rename' 'git remote rm' 'git remote set-head' (-a | -d | ) @@ -67,11 +67,18 @@ multiple branches without grabbing all branches. With `-m ` option, `$GIT_DIR/remotes//HEAD` is set up to point at remote's `` branch. See also the set-head command. + -In mirror mode, enabled with `\--mirror`, the refs will not be stored -in the 'refs/remotes/' namespace, but in 'refs/heads/'. This option -only makes sense in bare repositories. If a remote uses mirror -mode, furthermore, `git push` will always behave as if `\--mirror` -was passed. +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. ++ +When a push mirror is created with `\--mirror=push`, then `git push` +will always behave as if `\--mirror` was passed. ++ +The option `\--mirror` (with no type) sets up both push and fetch +mirror configuration. It is kept for historical purposes, and is +probably not what you want. 'rename':: diff --git a/builtin/remote.c b/builtin/remote.c index 952be2e27b..f9522d5d8b 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -9,7 +9,7 @@ static const char * const builtin_remote_usage[] = { "git remote [-v | --verbose]", - "git remote add [-t ] [-m ] [-f] [--mirror] ", + "git remote add [-t ] [-m ] [-f] [--mirror=] ", "git remote rename ", "git remote rm ", "git remote set-head (-a | -d | )", @@ -117,6 +117,11 @@ enum { TAGS_SET = 2 }; +#define MIRROR_NONE 0 +#define MIRROR_FETCH 1 +#define MIRROR_PUSH 2 +#define MIRROR_BOTH (MIRROR_FETCH|MIRROR_PUSH) + static int add_branch(const char *key, const char *branchname, const char *remotename, int mirror, struct strbuf *tmp) { @@ -131,9 +136,26 @@ static int add_branch(const char *key, const char *branchname, return git_config_set_multivar(key, tmp->buf, "^$", 0); } +static int parse_mirror_opt(const struct option *opt, const char *arg, int not) +{ + unsigned *mirror = opt->value; + if (not) + *mirror = MIRROR_NONE; + else if (!arg) + *mirror = MIRROR_BOTH; + else if (!strcmp(arg, "fetch")) + *mirror = MIRROR_FETCH; + else if (!strcmp(arg, "push")) + *mirror = MIRROR_PUSH; + else + return error("unknown mirror argument: %s", arg); + return 0; +} + static int add(int argc, const char **argv) { - int fetch = 0, mirror = 0, fetch_tags = TAGS_DEFAULT; + int fetch = 0, fetch_tags = TAGS_DEFAULT; + unsigned mirror = MIRROR_NONE; struct string_list track = STRING_LIST_INIT_NODUP; const char *master = NULL; struct remote *remote; @@ -151,7 +173,9 @@ static int add(int argc, const char **argv) OPT_CALLBACK('t', "track", &track, "branch", "branch(es) to track", opt_parse_track), OPT_STRING('m', "master", &master, "branch", "master branch"), - OPT_BOOLEAN(0, "mirror", &mirror, "no separate remotes"), + { OPTION_CALLBACK, 0, "mirror", &mirror, "push|fetch", + "set up remote as a mirror to push to or fetch from", + PARSE_OPT_OPTARG, parse_mirror_opt }, OPT_END() }; @@ -182,18 +206,19 @@ static int add(int argc, const char **argv) if (git_config_set(buf.buf, url)) return 1; - strbuf_reset(&buf); - strbuf_addf(&buf, "remote.%s.fetch", name); - - if (track.nr == 0) - string_list_append(&track, "*"); - for (i = 0; i < track.nr; i++) { - if (add_branch(buf.buf, track.items[i].string, - name, mirror, &buf2)) - return 1; + if (!mirror || mirror & MIRROR_FETCH) { + strbuf_reset(&buf); + strbuf_addf(&buf, "remote.%s.fetch", name); + if (track.nr == 0) + string_list_append(&track, "*"); + for (i = 0; i < track.nr; i++) { + if (add_branch(buf.buf, track.items[i].string, + name, mirror, &buf2)) + return 1; + } } - if (mirror) { + if (mirror & MIRROR_PUSH) { strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.mirror", name); if (git_config_set(buf.buf, "true")) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index d189add2d0..4e69c907d8 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -304,6 +304,84 @@ test_expect_success 'add --mirror && prune' ' git rev-parse --verify refs/heads/side) ' +test_expect_success 'add --mirror=fetch' ' + mkdir mirror-fetch && + git init mirror-fetch/parent && + (cd mirror-fetch/parent && + test_commit one) && + git init --bare mirror-fetch/child && + (cd mirror-fetch/child && + git remote add --mirror=fetch -f parent ../parent) +' + +test_expect_success 'fetch mirrors act as mirrors during fetch' ' + (cd mirror-fetch/parent && + git branch new && + git branch -m master renamed + ) && + (cd mirror-fetch/child && + git fetch parent && + git rev-parse --verify refs/heads/new && + git rev-parse --verify refs/heads/renamed + ) +' + +test_expect_success 'fetch mirrors can prune' ' + (cd mirror-fetch/child && + git remote prune parent && + test_must_fail git rev-parse --verify refs/heads/master + ) +' + +test_expect_success 'fetch mirrors do not act as mirrors during push' ' + (cd mirror-fetch/parent && + git checkout HEAD^0 + ) && + (cd mirror-fetch/child && + git branch -m renamed renamed2 && + git push parent + ) && + (cd mirror-fetch/parent && + git rev-parse --verify renamed && + test_must_fail git rev-parse --verify refs/heads/renamed2 + ) +' + +test_expect_success 'add --mirror=push' ' + mkdir mirror-push && + git init --bare mirror-push/public && + git init mirror-push/private && + (cd mirror-push/private && + test_commit one && + git remote add --mirror=push public ../public + ) +' + +test_expect_success 'push mirrors act as mirrors during push' ' + (cd mirror-push/private && + git branch new && + git branch -m master renamed && + git push public + ) && + (cd mirror-push/private && + git rev-parse --verify refs/heads/new && + git rev-parse --verify refs/heads/renamed && + test_must_fail git rev-parse --verify refs/heads/master + ) +' + +test_expect_success 'push mirrors do not act as mirrors during fetch' ' + (cd mirror-push/public && + git branch -m renamed renamed2 && + git symbolic-ref HEAD refs/heads/renamed2 + ) && + (cd mirror-push/private && + git fetch public && + git rev-parse --verify refs/heads/renamed && + test_must_fail git rev-parse --verify refs/heads/renamed2 + ) +' + test_expect_success 'add alt && prune' ' (mkdir alttst && cd alttst && From 099024861021830f9d4c7db4c64c844bf9d5ebd9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Mar 2011 15:53:39 -0400 Subject: [PATCH 3/3] remote: deprecate --mirror The configuration created by plain --mirror is dangerous and useless, and we now have --mirror=fetch and --mirror=push to replace it. Let's warn the user. One alternative to this is to try to guess which type the user wants. In a non-bare repository, a fetch mirror doesn't make much sense, since it would overwrite local commits. But in a bare repository, you might use either type, or even both (e.g., if you are acting as an intermediate drop-point across two disconnected networks). So rather than try for complex heuristics, let's keep it simple. The user knows what they're trying to do, so let them tell us. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/git-remote.txt | 4 ---- builtin/remote.c | 8 +++++++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt index 79e38fedd7..ddcbcc00ae 100644 --- a/Documentation/git-remote.txt +++ b/Documentation/git-remote.txt @@ -75,10 +75,6 @@ because a fetch would overwrite any local commits. + When a push mirror is created with `\--mirror=push`, then `git push` will always behave as if `\--mirror` was passed. -+ -The option `\--mirror` (with no type) sets up both push and fetch -mirror configuration. It is kept for historical purposes, and is -probably not what you want. 'rename':: diff --git a/builtin/remote.c b/builtin/remote.c index f9522d5d8b..eb1229d689 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -136,13 +136,19 @@ static int add_branch(const char *key, const char *branchname, return git_config_set_multivar(key, tmp->buf, "^$", 0); } +static const char mirror_advice[] = +"--mirror is dangerous and deprecated; please\n" +"\t use --mirror=fetch or --mirror=push instead"; + static int parse_mirror_opt(const struct option *opt, const char *arg, int not) { unsigned *mirror = opt->value; if (not) *mirror = MIRROR_NONE; - else if (!arg) + else if (!arg) { + warning("%s", mirror_advice); *mirror = MIRROR_BOTH; + } else if (!strcmp(arg, "fetch")) *mirror = MIRROR_FETCH; else if (!strcmp(arg, "push"))