Re: [PATCH v3] remote: add --fetch and --both options to set-url

[Peter Wu <peter@lekensteyn.nl>]

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@lekensteyn.nl>
> > ---
> >
> >   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