Re: [PATCH v3] remote: add --fetch and --both options to set-url
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 pe...@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)
Re: [PATCH v3] remote: add --fetch and --both options to set-url
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 pe...@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
};
@@
Re: [PATCH v3] remote: add --fetch and --both options to set-url
On Wed, Dec 17, 2014 at 03:20:58PM +0100, Peter Wu wrote:
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.
Yeah, gcc does not seem to care, but compiling with clang (3.5.0 here)
does notice it.
C's parser matches what your indentation indicates, so it is just a
style fixup, not a bug (but one I agree is worth addressing).
-Peff
--
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 v3] remote: add --fetch and --both options to set-url
On Tue, Nov 25, 2014 at 12:48:26PM +0100, 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 pe...@lekensteyn.nl --- Sorry for the slowness in reviewing. The design of this version makes sense to me (not surprising, I guess, since it was in direct response to my comments). I didn't see anything glaringly wrong in the implementation, though I did find it a little hard to follow, because of this: +#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)) When reading through the code, the distinction between modify_type MODIFY_TYPE_FETCH and modify_type == MODIFY_TYPE_FETCH is significant, because the former matches HISTORIC, while the latter does not. I imagine that a distinct bit value for HISTORIC would make things a bit more verbose (you would have to add an extra OR in many places), but I wonder if it would make the code easier to follow (one of the things I wanted to check was that HISTORIC does the same thing that it always did, and it is very hard to follow the HISTORIC behavior reading the code linearly). I dunno. I don't insist; just noting a difficulty I had while reading it. Maybe you went down that route already during development and found it more painful. -Peff -- 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
[PATCH v3] remote: add --fetch and --both options to set-url
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 pe...@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)
Re: [PATCH v3] remote: add --fetch and --both options to set-url
On Tue, Nov 25, 2014 at 6:48 AM, Peter Wu pe...@lekensteyn.nl 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. I've read (though perhaps not fully digested) the other email thread which led up to this version of the patch, as well as the documentation update in this patch, but I still don't understand the need for the --both option. Intuitively, I would expect that specifying --fetch and --push on the command line would have the same effect as the proposed --both option. Thus, why is a separate (and exclusive) --both option needed? Is it to reduce typing? What am I missing? More below. 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 pe...@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. --- 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 @@ -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. This explanation is somewhat tortuous. Assuming that the --both option is superfluous, perhaps this could be explained more clearly along these lines: --- 8 --- `--fetch` changes fetch URLs, and --push changes push URLs. Specified together, both fetch and push URLs are changed. For historical reasons, if neither --fetch nor --push is specified, then the fetch URL is changed, as well as the push URL if not already set. --- 8 --- + With '--add', instead of changing some URL, new URL is added. + -- 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 v3] remote: add --fetch and --both options to set-url
On Tuesday 25 November 2014 17:09:04 Eric Sunshine wrote: On Tue, Nov 25, 2014 at 6:48 AM, Peter Wu pe...@lekensteyn.nl 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. I've read (though perhaps not fully digested) the other email thread which led up to this version of the patch, as well as the documentation update in this patch, but I still don't understand the need for the --both option. Intuitively, I would expect that specifying --fetch and --push on the command line would have the same effect as the proposed --both option. Thus, why is a separate (and exclusive) --both option needed? Is it to reduce typing? What am I missing? The initial version just added --fetch and let --fetch --push behave like the current --both. Here you can see the most recent discussion leading to the --both option: http://www.spinics.net/lists/git/msg242336.html There was an overlapping discussion about the confusing historic behavior (default vs. --fetch vs. --push --fetch), and an alternative (less verbose form) of --push --fetch. The reason that --both is introduced probably has something to do with it being less verbose. I am not too attached to either option by the way. 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 pe...@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. --- 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 @@ -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. This explanation is somewhat tortuous. Assuming that the --both option is superfluous, perhaps this could be explained more clearly along these lines: --- 8 --- `--fetch` changes fetch URLs, and --push changes push URLs. Specified together, both fetch and push URLs are changed. For historical reasons, if neither --fetch nor --push is specified, then the fetch URL is changed, as well as the push URL if not already set. --- 8 --- Excellent, short and concise. If this path is taken, I would still add the note about the future change. I am also interested in opinions about the suggested default behavior. Should it become --both (--push --fetch)? In that case '--both' will be a pretty useless option in the future (when it becomes the default). + With '--add', instead of changing some URL, new URL is added. + -- 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
