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

Hi,

Rob Hoelz wrote:

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

[...]

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

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?