Peter Wu <peter@xxxxxxxxxxxxx> writes:
> 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.
OK. In other words, for those without asymmetric configuration
without a need to define pushURL, this default should be the most
convenient, as it does not have to fiddle with two variables.
> 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
"and/or", I think.
> operation (without --add or --delete), it will drop pushurl as the
> implicit push URL is the (fetch) URL.
I think this description is clear enough without "(if exists)" at
the end.
> While '--both' could be implemented as '--fetch --push', it might also
> be mistaken for "--push overrides --fetch". An option such as
> "--only={fetch|push|both}" was also considered, but it is longer than
> the current options, makes --push redundant and brings the confusing
> option "--only=both". Options such as '--direction=...' is too long and
> '--dir=' is ambiguous ("directory"?). Thus, for brevity three new
> options were introduced.
Sounds sensible.
> @@ -134,7 +134,15 @@ 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.
I am afraid that this part is confusing when read in the wider
context of the document.
The first sentence of this section (outside the patch) is "Changes
URL remote points to"; I expect that the most people would think
that it is talking about "remote.*.url" configuration, and the
wording for "--push" in the original is also clear that it touches
'remote.*.pushurl' instead.
In the updated text, you use words "fetch URL" and "push URL" to
mean "regardless of how these are represented by the configuration
system, the URL to be used for fetching/pushing". In other words,
in the updated vocabulary, 'remote.*.pushurl' is *NOT* called "push
URL" (and 'remote.*.url' is not 'fetch URL').
That may be easier for new people who aren't familiar with the
configuration system (read: I am saying that it may be a good idea
in the longer term), but the phrasing does not make it clear that
they are not referring remote.*.{url,pushURL} variables.
Rephrasing these to "URLs used for fetching" vs "URLs used for
pushing" may make things a bit less confusion-prone.
In any case, we would also need to update Documentation/config.txt
which has these:
remote.<name>.url::
The URL of a remote repository. See linkgit:git-fetch[1] or
linkgit:git-push[1].
remote.<name>.pushurl::
The push URL of a remote repository. See linkgit:git-push[1].
It may be sufficient to rephrase them like so:
remote.<name>.url::
The URL of a remote repository, used for fetching from it
and pushing into it unless a separate remote.<name>.pushURL
is defined. See linkgit:git-fetch[1] and linkgit:git-push[1]
remote.<name>.pushurl::
The URL of a remote repository, used for pushing into it
(if undefined, remote.<name>.url is used to push into it).
See linkgit:git-push[1].
perhaps?
> +For historical reasons, if neither --fetch nor --push is specified then the
> +fetch URL is changed, as well as the push URL if this was not already set. This
> +behavior may change in the future.
This paragraph is unwarranted. As you explained in the second
paragraph in the log message, the traditional behaviour was a good
default for majority of people and I do not think we saw any
demonstrated need to deprecate it.
> +#define MODIFY_TYPE_FETCH (1 << 0)
> +#define MODIFY_TYPE_PUSH (1 << 1)
> +#define MODIFY_TYPE_BOTH (MODIFY_TYPE_FETCH | MODIFY_TYPE_PUSH)
> +/* The historic behavior behaves like --fetch, but does not touch the push URL
> + * configuration (and thereby may appear to change the push URL too if this was
> + * not set before).
> + */
> +#define MODIFY_TYPE_HISTORIC (MODIFY_TYPE_FETCH | (1 << 2))
/*
* Our multi-line comment begins with a slash-asterisk
* without anything else on the remainder of the line
* and ends with an asterisk-slash, possibly indented
* and nothing else on it.
*/
Perhaps s/HISTORIC/TRADITIONAL/ or s/HISTORIC/LITERAL/ (the latter
is for "literally update 'URL' configuration for the named remote,
don't play with 'do we have pushURL?' games").
> + /* Make the implicit push URL explicit if the fetch URL is modified,
> + * except when in the historic mode (then the pushurl configuration is
> + * not changed). */
Likewise.
Thanks.
--
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