On Wednesday 17 December 2014 11:08:07 Torsten Bögershausen wrote:
> On 11/25/2014 12:48 PM, Peter Wu wrote:
> > git remote set-url knew about the '--push' option to update just the
> > pushurl, but it does not have a similar option for "update fetch URL and
> > leave whatever was in place for the push URL".
> >
> > This patch adds support for a '--fetch' option which implements that use
> > case in a backwards compatible way: if no --both, --push or --fetch
> > options are given, then the push URL is modified too if it was not set
> > before. This is the case since the push URL is implicitly based on the
> > fetch URL.
> >
> > A '--both' option is added to make the command independent of previous
> > pushurl settings. For the --add and --delete set operations, it will
> > always set the push and/ or the fetch URLs. For the primary mode of
> > operation (without --add or --delete), it will drop pushurl as the
> > implicit push URL is the (fetch) URL.
> >
> > The documentation has also been updated and a missing '--push' option
> > is added to the 'git remote -h' command.
> >
> > Tests are also added to verify the documented behavior.
> >
> > Signed-off-by: Peter Wu <peter@xxxxxxxxxxxxx>
> > ---
> >
> > v2: fixed test case
> > v3: added --both option, changed --fetch --push behavior, added more tests for
> > --add/--delete cases, refactored to reduce duplication (not special-casing
> > add_mode without oldurl, just skip initially setting oldurl).
> >
> > Translators note: the help text gained more translatable strings and some
> > strings got additional options.
> > ---
> > Documentation/git-remote.txt | 17 ++++--
> > builtin/remote.c | 140 +++++++++++++++++++++++++++----------------
> > t/t5505-remote.sh | 125 ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 228 insertions(+), 54 deletions(-)
> >
> > diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> > index cb103c8..bdd0305 100644
> > --- a/Documentation/git-remote.txt
> > +++ b/Documentation/git-remote.txt
> > @@ -15,9 +15,9 @@ SYNOPSIS
> > 'git remote remove' <name>
> > 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
> > 'git remote set-branches' [--add] <name> <branch>...
> > -'git remote set-url' [--push] <name> <newurl> [<oldurl>]
> > -'git remote set-url --add' [--push] <name> <newurl>
> > -'git remote set-url --delete' [--push] <name> <url>
> > +'git remote set-url' [--both | --fetch | --push] <name> <newurl> [<oldurl>]
> > +'git remote set-url' [--both | --fetch | --push] '--add' <name> <newurl>
> > +'git remote set-url' [--both | --fetch | --push] '--delete' <name> <url>
> > 'git remote' [-v | --verbose] 'show' [-n] <name>...
> > 'git remote prune' [-n | --dry-run] <name>...
> > 'git remote' [-v | --verbose] 'update' [-p | --prune] [(<group> | <remote>)...]
> > @@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote points to matching
> > regex <oldurl> (first URL if no <oldurl> is given) to <newurl>. If
> > <oldurl> doesn't match any URL, error occurs and nothing is changed.
> > +
> > -With '--push', push URLs are manipulated instead of fetch URLs.
> > +With '--both', both the fetch and push URLs are manipulated.
> > ++
> > +With '--fetch', only fetch URLs are manipulated.
> > ++
> > +With '--push', only push URLs are manipulated.
> > ++
> > +If none of the '--both', '--fetch' or --push' options are given, then
> > +'--both' applies only if no push URL was set before. Otherwise '--fetch'
> > +is assumed for historical reasons. This default may change in the
> > +future to '--both' to avoid surprises depending on the configuration.
> > +
> > With '--add', instead of changing some URL, new URL is added.
> > +
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index 7f28f92..a250b23 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {
> > N_("git remote prune [-n | --dry-run] <name>"),
> > N_("git remote [-v | --verbose] update [-p | --prune] [(<group> | <remote>)...]"),
> > N_("git remote set-branches [--add] <name> <branch>..."),
> > - N_("git remote set-url [--push] <name> <newurl> [<oldurl>]"),
> > - N_("git remote set-url --add <name> <newurl>"),
> > - N_("git remote set-url --delete <name> <url>"),
> > + N_("git remote set-url [--both | --fetch | --push] <name> <newurl> [<oldurl>]"),
> > + N_("git remote set-url [--both | --fetch | --push] --add <name> <newurl>"),
> > + N_("git remote set-url [--both | --fetch | --push] --delete <name> <url>"),
> > NULL
> > };
> >
> > @@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] = {
> > };
> >
> > static const char * const builtin_remote_seturl_usage[] = {
> > - N_("git remote set-url [--push] <name> <newurl> [<oldurl>]"),
> > - N_("git remote set-url --add <name> <newurl>"),
> > - N_("git remote set-url --delete <name> <url>"),
> > + N_("git remote set-url [--both | --fetch | --push] <name> <newurl> [<oldurl>]"),
> > + N_("git remote set-url [--both | --fetch | --push] --add <name> <newurl>"),
> > + N_("git remote set-url [--both | --fetch | --push] --delete <name> <url>"),
> > NULL
> > };
> >
> > @@ -1503,21 +1503,35 @@ static int set_branches(int argc, const char **argv)
> > return set_remote_branches(argv[0], argv + 1, add_mode);
> > }
> >
> > +#define MODIFY_TYPE_FETCH (1 << 0)
> > +#define MODIFY_TYPE_PUSH (1 << 1)
> > +#define MODIFY_TYPE_BOTH (MODIFY_TYPE_FETCH | MODIFY_TYPE_PUSH)
> > +#define MODIFY_TYPE_HISTORIC (MODIFY_TYPE_FETCH | (1 << 2))
> > +
> > static int set_url(int argc, const char **argv)
> > {
> > - int i, push_mode = 0, add_mode = 0, delete_mode = 0;
> > + int i, add_mode = 0, delete_mode = 0;
> > + int modify_type = MODIFY_TYPE_HISTORIC;
> > int matches = 0, negative_matches = 0;
> > const char *remotename = NULL;
> > const char *newurl = NULL;
> > const char *oldurl = NULL;
> > struct remote *remote;
> > regex_t old_regex;
> > - const char **urlset;
> > - int urlset_nr;
> > - struct strbuf name_buf = STRBUF_INIT;
> > + struct strbuf name_buf_fetch = STRBUF_INIT;
> > + struct strbuf name_buf_push = STRBUF_INIT;
> > struct option options[] = {
> > - OPT_BOOL('\0', "push", &push_mode,
> > - N_("manipulate push URLs")),
> > + OPT_GROUP(""),
> > + OPT_SET_INT('\0', "fetch", &modify_type,
> > + N_("manipulate just fetch URLs"),
> > + MODIFY_TYPE_FETCH),
> > + OPT_SET_INT('\0', "push", &modify_type,
> > + N_("manipulate just push URLs"),
> > + MODIFY_TYPE_PUSH),
> > + OPT_SET_INT('\0', "both", &modify_type,
> > + N_("manipulate both push and fetch URLs"),
> > + MODIFY_TYPE_BOTH),
> > + OPT_GROUP(""),
> > OPT_BOOL('\0', "add", &add_mode,
> > N_("add URL")),
> > OPT_BOOL('\0', "delete", &delete_mode,
> > @@ -1535,7 +1549,8 @@ static int set_url(int argc, const char **argv)
> >
> > remotename = argv[1];
> > newurl = argv[2];
> > - if (argc > 3)
> > + /* The old URL is only meaningful for the primary non-set operation. */
> > + if (argc > 3 && !add_mode && !delete_mode)
> > oldurl = argv[3];
> >
> > if (delete_mode)
> > @@ -1545,47 +1560,72 @@ static int set_url(int argc, const char **argv)
> > die(_("No such remote '%s'"), remotename);
> > remote = remote_get(remotename);
> >
> > - if (push_mode) {
> > - strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
> > - urlset = remote->pushurl;
> > - urlset_nr = remote->pushurl_nr;
> > - } else {
> > - strbuf_addf(&name_buf, "remote.%s.url", remotename);
> > - urlset = remote->url;
> > - urlset_nr = remote->url_nr;
> > + strbuf_addf(&name_buf_fetch, "remote.%s.url", remotename);
> > + strbuf_addf(&name_buf_push, "remote.%s.pushurl", remotename);
> > +
> > + if (oldurl && !add_mode) {
> > + /* Old URL specified, or deletion. Demand that one matches. */
> > + if (regcomp(&old_regex, oldurl, REG_EXTENDED))
> > + die(_("Invalid old URL pattern: %s"), oldurl);
> > +
> > + if (modify_type & MODIFY_TYPE_FETCH)
> > + for (i = 0; i < remote->url_nr; i++)
> > + if (!regexec(&old_regex, remote->url[i], 0, NULL, 0))
> > + matches++;
> > + else
> > + negative_matches++;
> > + if (delete_mode && !negative_matches && modify_type & MODIFY_TYPE_FETCH)
> > + die(_("Will not delete all non-push URLs"));
> > + if (modify_type & MODIFY_TYPE_PUSH)
> > + for (i = 0; i < remote->pushurl_nr; i++)
> > + if (!regexec(&old_regex, remote->pushurl[i], 0, NULL, 0))
> > + matches++;
> > + else
> > + negative_matches++;
> There are 2 warning "dangling else", line 1570 and 1578
> I think we should use:
>
> for (i = 0; i < remote->pushurl_nr; i++) {
> if (!regexec(&old_regex, remote->pushurl[i], 0, NULL, 0))
> matches++;
> else
> negative_matches++;
> }
>
Good catch, strange enough I did not get any warnings from my compiler
or Clang's static analyzer. I have submitted a new patch which treats
the accumulated comments.
--
Kind regards,
Peter
https://lekensteyn.nl
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html