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

Peter Wu [off-list ref] 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.

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.