Re: [PATCH] push: Alias pushurl from push rewrites

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Rob Hoelz wrote:

> git push currently doesn't consider pushInsteadOf when
> using pushurl; this test tests that.

I'd leave out this paragraph, since it is redundant next to the rest
of the commit message (except that you have added tests, which ideally
every bugfix patch would do :)).

[...]
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2142,8 +2142,7 @@ url.<base>.pushInsteadOf::
>  	automatically use an appropriate URL to push, even for a
>  	never-before-seen repository on the site.  When more than one
>  	pushInsteadOf strings match a given URL, the longest match is
> -	used.  If a remote has an explicit pushurl, Git will ignore this
> -	setting for that remote.
> +	used.

Old-timers used to the previous behavior might not guess immediately
how this interacts with pushurl.  (If I understand the initial
discussion at [1] correctly, an earlier, unreleased version of the
feature would push to the rewritten fetch url *in addition to* the
unmodified push url.  So there's more than one possible behavior here.)

How about:

	url.<base>.pushInsteadOf

		Any URL that starts with this value will not be pushed to;
		instead, it will be rewritten ... even for a
		never-before-seen repository on the site.

		This rewriting takes place even for explicit push URLs
		set using the `remote.<name>.pushurl` configuration
		variable.

		When more than one pushInsteadOf string matches a given
		URL, the longest match is used.

[1] http://thread.gmane.org/gmane.comp.version-control.git/127889

> --- a/remote.c
> +++ b/remote.c
> @@ -465,7 +465,11 @@ static void alias_all_urls(void)
>  		if (!remotes[i])
>  			continue;
>  		for (j = 0; j < remotes[i]->pushurl_nr; j++) {
> -			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
> +			char *copy = xstrdup(remotes[i]->pushurl[j]);
> +			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
> +			if (!strcmp(copy, remotes[i]->pushurl[j]))
> +				remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
> +			free(copy);

This is relying on !strcmp to detect that no pushInsteadOf rule
matched the URL.  By contrast, the existing pushInsteadOf support
does the following:

	const char *pushurl = alias_url(url, &rewrites_push);
	if (pushurl != url)
		add_pushurl(remote, pushurl);

Because it compares pointers, not strings, it is able to correctly
treat the identity substitution as a real match.  It also avoids some
allocation churn.  This new alias_url() call site should follow the
same convention.

Looking at that existing code also made me worry "Are we applying
the pushinsteadof subsitution twice?"

So let's see what happens:

	Caller calls into remote_get() to learn about remote "origin".
	... which in turn calls read_config()
	... which sets the config machinery in motion with callback handle_config()
	... which stores rewrite rules in 'rewrites' and 'rewrites_push' and
	    unmodified URLs in remotes[i]->url[], remotes[i]->pushurl[]

	Now read_config() calls alias_all_urls() to tweak the url and
	pushurl fields in place.  For each remote:
	 1. If a pushurl matches an *.insteadof rewrite rule, rewrite it.
	 2. Check if any pushurls exist.
	 3. If a url matches a *.pushinsteadof rule and no raw pushurls
	    existed, use the rewritten url as a push url.

	    If a url matches a *.insteadof rewrite rule, rewrite it.

With your tweak, step (1) above just also checks for *.pushinsteadof
in addition to *.insteadof, which should be safe (modulo the string
comparison vs pointer comparison detail mentioned above)

There's also the legacy .git/remotes and .git/branches code paths,
which are basically the same except there's no place for a pushurl.

How about the below, for squashing in?

diff --git i/Documentation/config.txt w/Documentation/config.txt
index 665c0de..25565ca 100644
--- i/Documentation/config.txt
+++ w/Documentation/config.txt
@@ -2150,9 +2150,14 @@ url.<base>.pushInsteadOf::
 	access methods, some of which do not allow push, this feature
 	allows people to specify a pull-only URL and have Git
 	automatically use an appropriate URL to push, even for a
-	never-before-seen repository on the site.  When more than one
-	pushInsteadOf strings match a given URL, the longest match is
-	used.
+	never-before-seen repository on the site.
++
+This rewriting takes place even for explicit push URLs set
+using the `remote.<name>.pushurl` configuration variable.
++
+When more than one pushInsteadOf string matches a given URL,
+the longest match is used.  If no pushInsteadOf strings match
+the URL, Git falls back to insteadOf strings.
 
 user.email::
 	Your email address to be recorded in any newly created commits.
diff --git i/remote.c w/remote.c
index cfcb77a..7255926 100644
--- i/remote.c
+++ w/remote.c
@@ -466,11 +466,14 @@ static void alias_all_urls(void)
 		if (!remotes[i])
 			continue;
 		for (j = 0; j < remotes[i]->pushurl_nr; j++) {
-			char *copy = xstrdup(remotes[i]->pushurl[j]);
-			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
-			if (!strcmp(copy, remotes[i]->pushurl[j]))
-				remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
-			free(copy);
+			const char *url = remotes[i]->pushurl[j];
+			remotes[i]->pushurl[j] = alias_url(url, &rewrites_push);
+			if (remotes[i]->pushurl[j] == url)
+				/*
+				 * No url.*.pushinsteadof string matched.
+				 * Apply url.*.insteadof.
+				 */
+				remotes[i]->pushurl[j] = alias_url(url, &rewrites);
 		}
 		add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
 		for (j = 0; j < remotes[i]->url_nr; j++) {