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++) { -- 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