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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 17, 2014, 22:31 UTC
Message-ID: <xmqqr3vx9ad5.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1418825936-18575-1-git-send-email-peter@lekensteyn.nl>

Peter Wu <peter@lekensteyn.nl> writes:

Show 9 quoted lines

> 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.

Show 7 quoted lines

> 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.

Show 10 quoted lines

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

Show 8 quoted lines

> +#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.