threads / patch / 33306

patchpush: Alias pushurl from push rewrites

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

## tl;dr

24 messages between Mar 27, 2013 and Mar 29, 2013. Diffs are folded; open one to read it.

replies: 23people: 4as markdown or json

Rob Hoelz· Mar 27, 2013, 17:22 UTC · lore

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

If you use pushurl with an alias that has a pushInsteadOf configuration value, Git does not take advantage of it. For example:

[url "git://github.com/"]
    insteadOf = github:
[url "git://github.com/myuser/"]
    insteadOf = mygithub:
[url "git@github.com:myuser/"]
    pushInsteadOf = mygithub:
[remote "origin"]
    url     = github:organization/project
    pushurl = mygithub:project
With the above configuration, the following occurs:
$ git push origin master
fatal: remote error:
  You can't push to git://github.com/myuser/project.git
  Use git@github.com:myuser/project.git

So you can see that pushurl is being followed (it's not attempting to push to git://github.com/organization/project), but insteadOf values are being used as opposed to pushInsteadOf values for expanding the pushurl alias.

This commit fixes that.
Signed-off-by: Rob Hoelz <rob@hoelz.ro>
---
 remote.c              |  6 +++-
 t/t5516-fetch-push.sh | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 1 deletion(-)
Show changes to 2 files +82 −1

remote.c, t/t5516-fetch-push.sh

diff --git a/remote.c b/remote.c
index e53a6eb..1ea240a 100644
--- 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);
 		}
 		add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
 		for (j = 0; j < remotes[i]->url_nr; j++) {
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c31e5c1..119f043 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -244,6 +244,83 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
 	)
 '
 
+test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + pushInsteadOf does rewrite in this case)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/ro/.insteadOf" ro: &&
+	test_config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
+	test_config remote.r.url ro:wrong &&
+	test_config remote.r.pushurl rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'push without pushInsteadOf and explicit pushurl (pushurl + insteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/ro/.insteadOf" ro: &&
+	test_config "url.file://$TRASH/rw/.insteadOf" rw: &&
+	test_config remote.r.url ro:wrong &&
+	test_config remote.r.pushurl rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'push with pushInsteadOf but without explicit pushurl (url + pushInsteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/ro/.insteadOf" rw: &&
+	test_config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
+	test_config remote.r.url rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'push without pushInsteadOf and without explicit pushurl (url + insteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/rw/.insteadOf" rw: &&
+	test_config remote.r.url rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
+
 test_expect_success 'push with matching heads' '
 
 	mk_test heads/master &&

-- 
1.8.2
Jonathan Nieder· Mar 27, 2013, 18:23 UTC · re: Rob Hoelz · lore

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

Rob Hoelz wrote:
Show 12 quoted lines
> --- 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);
Interesting.
Suppose I configure
	[url "git://anongit.myserver.example.com/"]
		insteadOf = myserver.example.com:
	[url "myserver:"]
		pushInsteadOf = myserver.example.com:

The above code would make the insteadOf rule apply instead of pushInsteadOf, even when pushing. Perhaps something like the following would work?

			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 configuration matched. */
				remotes[i]->pushurl[j] = alias_url(url, &rewrites);
Show 24 quoted lines
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -244,6 +244,83 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
>  	)
>  '
>  
> +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + pushInsteadOf does rewrite in this case)' '
> +	mk_empty &&
> +	rm -rf ro rw &&
> +	TRASH="$(pwd)/" &&
> +	mkdir ro &&
> +	mkdir rw &&
> +	git init --bare rw/testrepo &&
> +	test_config "url.file://$TRASH/ro/.insteadOf" ro: &&
> +	test_config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
> +	test_config remote.r.url ro:wrong &&
> +	test_config remote.r.pushurl rw:testrepo &&
> +	git push r refs/heads/master:refs/remotes/origin/master &&
> +	(
> +		cd rw/testrepo &&
> +		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
> +		git for-each-ref refs/remotes/origin > actual &&
> +		test_cmp expected actual
> +	)

Looks good. The usual style in git tests is to include no space after >redirection operators:

		git for-each-ref refs/remotes/origin >actual &&

Hope that helps, Jonathan

Jonathan Nieder· Mar 27, 2013, 21:15 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
> Rob Hoelz wrote:
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -465,7 +465,11 @@ static void alias_all_urls(void)
[...]
Show 18 quoted lines
>> -			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);
>
> Interesting.
>
> Suppose I configure
> 
> 	[url "git://anongit.myserver.example.com/"]
> 		insteadOf = myserver.example.com:
> 	[url "myserver:"]
> 		pushInsteadOf = myserver.example.com:
>
> The above code would make the insteadOf rule apply instead of
> pushInsteadOf, even when pushing.
Sorry, typo.  The configuration in the example above should have been
	[url "git://anongit.myserver.example.com/"]
		insteadOf = myserver.example.com:
	[url "myserver.example.com:"]
		pushInsteadOf = myserver.example.com:

In other words, suppose I set url.*.insteadof to point to a faster address for fetching alongside url.*.pushinsteadof requesting that the original address should still be used for pushing.

Thanks again for tackling this. Jonathan

Junio C Hamano· Mar 27, 2013, 22:07 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 10 quoted lines
> Sorry, typo.  The configuration in the example above should have been
>
> 	[url "git://anongit.myserver.example.com/"]
> 		insteadOf = myserver.example.com:
> 	[url "myserver.example.com:"]
> 		pushInsteadOf = myserver.example.com:
>
> In other words, suppose I set url.*.insteadof to point to a faster
> address for fetching alongside url.*.pushinsteadof requesting that the
> original address should still be used for pushing.

I didn't know we were even shooting for supporting the identity mapping:

	url.X.pushinsteadof=X
but that would certainly be nice.

By the way, it seems that the original commit 1c2eafb89bca (Add url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07) wanted to explicitly avoid use of pushInsteadOf aliasing for a pushURL and it is also documented in config.txt from day one.

I think the intent is "You have a normal URL, and a way to override it explicitly with pushURL, or a way to rewrite it via the aliasing the normal URL with pushInsteadOf. Either one or the other, but not both, as having many levels of indirection would be confusing."

Which I can understand and sympathise.

In anay case, the change proposed in this thread seems to change that, so the documentation would need to be updated. Also the tests the original commit adds explicitly checks that pushInsteadOf is ignored, which may have to be updated (unless that test is already broken).

Rob Hoelz· Mar 27, 2013, 22:48 UTC · re: Junio C Hamano · lore

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

On Wed, 27 Mar 2013 15:07:18 -0700 Junio C Hamano <gitster@pobox.com> wrote:

Show 39 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Sorry, typo.  The configuration in the example above should have
> > been
> >
> > 	[url "git://anongit.myserver.example.com/"]
> > 		insteadOf = myserver.example.com:
> > 	[url "myserver.example.com:"]
> > 		pushInsteadOf = myserver.example.com:
> >
> > In other words, suppose I set url.*.insteadof to point to a faster
> > address for fetching alongside url.*.pushinsteadof requesting that
> > the original address should still be used for pushing.
> 
> I didn't know we were even shooting for supporting the identity
> mapping:
> 
> 	url.X.pushinsteadof=X
> 
> but that would certainly be nice.
> 
> By the way, it seems that the original commit 1c2eafb89bca (Add
> url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)
> wanted to explicitly avoid use of pushInsteadOf aliasing for a
> pushURL and it is also documented in config.txt from day one.
> 
> I think the intent is "You have a normal URL, and a way to override
> it explicitly with pushURL, or a way to rewrite it via the aliasing
> the normal URL with pushInsteadOf. Either one or the other, but not
> both, as having many levels of indirection would be confusing."
> 
> Which I can understand and sympathise.
> 
> In anay case, the change proposed in this thread seems to change
> that, so the documentation would need to be updated.  Also the tests
> the original commit adds explicitly checks that pushInsteadOf is
> ignored, which may have to be updated (unless that test is already
> broken).
>

My use case is that I use Github for my personal development. I have a prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for fetch, git@github.com:hoelzro/ for push) and one for all other Git repos (github: -> git://github.com/) I have a few projects where I work in a fork, but I want to fetch updates from the original project. So my url for the origin remote is github:org/project, but my pushurl is hoelzro:project. This behavior in Git currently doesn't allow me to work that way. I used to work with two remotes; origin for my repo and base for the official one, but I've found that I prefer this other way.

The test that checked that pushInsteadOf + pushurl shouldn't work as I expect was actually broken; I have removed it, updated the documentation, and sent a new patch to the list.

-Rob
Josh Triplett· Mar 27, 2013, 23:09 UTC · re: Rob Hoelz · lore

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

On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:
Show 56 quoted lines
> On Wed, 27 Mar 2013 15:07:18 -0700
> Junio C Hamano <gitster@pobox.com> wrote:
> 
> > Jonathan Nieder <jrnieder@gmail.com> writes:
> > 
> > > Sorry, typo.  The configuration in the example above should have
> > > been
> > >
> > > 	[url "git://anongit.myserver.example.com/"]
> > > 		insteadOf = myserver.example.com:
> > > 	[url "myserver.example.com:"]
> > > 		pushInsteadOf = myserver.example.com:
> > >
> > > In other words, suppose I set url.*.insteadof to point to a faster
> > > address for fetching alongside url.*.pushinsteadof requesting that
> > > the original address should still be used for pushing.
> > 
> > I didn't know we were even shooting for supporting the identity
> > mapping:
> > 
> > 	url.X.pushinsteadof=X
> > 
> > but that would certainly be nice.
> > 
> > By the way, it seems that the original commit 1c2eafb89bca (Add
> > url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)
> > wanted to explicitly avoid use of pushInsteadOf aliasing for a
> > pushURL and it is also documented in config.txt from day one.
> > 
> > I think the intent is "You have a normal URL, and a way to override
> > it explicitly with pushURL, or a way to rewrite it via the aliasing
> > the normal URL with pushInsteadOf. Either one or the other, but not
> > both, as having many levels of indirection would be confusing."
> > 
> > Which I can understand and sympathise.
> > 
> > In anay case, the change proposed in this thread seems to change
> > that, so the documentation would need to be updated.  Also the tests
> > the original commit adds explicitly checks that pushInsteadOf is
> > ignored, which may have to be updated (unless that test is already
> > broken).
> > 
> 
> My use case is that I use Github for my personal development.  I have a
> prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for
> fetch, git@github.com:hoelzro/ for push) and one for all other Git repos
> (github: -> git://github.com/)  I have a few projects where I work in a
> fork, but I want to fetch updates from the original project.  So my url
> for the origin remote is github:org/project, but my pushurl is
> hoelzro:project.  This behavior in Git currently doesn't allow me to
> work that way.  I used to work with two remotes; origin for my repo and
> base for the official one, but I've found that I prefer this other way.
> 
> The test that checked that pushInsteadOf + pushurl shouldn't work as I
> expect was actually broken; I have removed it, updated the
> documentation, and sent a new patch to the list.

There's an argument for either behavior as valid. My original patch specifically documented and tested for the opposite behavior, namely that pushurl overrides any pushInsteadOf, because I intended pushInsteadOf as a fallback if you don't have an explicit pushurl set. For instance, you could use pushInsteadOf to rewrite a family of anonymous git URLs to corresponding pushable repositories, but then use an explicit pushurl to override that for a specific repository. This change would break the ability to use pushurl for its original intended purpose, namely having a local repository where fetch comes from one remote repo and push goes to another.

One use case of mine: I have a .gitconfig in my git-managed home directory which sets pushInsteadOf so that I can clone via git:// and immediately have working push. I work with a number of systems that don't have inbound access to each other but do have outbound access to the network; on some of these "satellite" boxes, I can't push changes directly to the server pushInsteadOf points to, so I can explicitly set pushurl in .git/config for that repository, which overrides the pushInsteadOf. This change would break that configuration.

- Josh Triplett
Josh Triplett· Mar 27, 2013, 23:17 UTC · re: Josh Triplett · lore

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

On Wed, Mar 27, 2013 at 04:09:43PM -0700, Josh Triplett wrote:
Show 77 quoted lines
> On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:
> > On Wed, 27 Mar 2013 15:07:18 -0700
> > Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > > Jonathan Nieder <jrnieder@gmail.com> writes:
> > > 
> > > > Sorry, typo.  The configuration in the example above should have
> > > > been
> > > >
> > > > 	[url "git://anongit.myserver.example.com/"]
> > > > 		insteadOf = myserver.example.com:
> > > > 	[url "myserver.example.com:"]
> > > > 		pushInsteadOf = myserver.example.com:
> > > >
> > > > In other words, suppose I set url.*.insteadof to point to a faster
> > > > address for fetching alongside url.*.pushinsteadof requesting that
> > > > the original address should still be used for pushing.
> > > 
> > > I didn't know we were even shooting for supporting the identity
> > > mapping:
> > > 
> > > 	url.X.pushinsteadof=X
> > > 
> > > but that would certainly be nice.
> > > 
> > > By the way, it seems that the original commit 1c2eafb89bca (Add
> > > url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)
> > > wanted to explicitly avoid use of pushInsteadOf aliasing for a
> > > pushURL and it is also documented in config.txt from day one.
> > > 
> > > I think the intent is "You have a normal URL, and a way to override
> > > it explicitly with pushURL, or a way to rewrite it via the aliasing
> > > the normal URL with pushInsteadOf. Either one or the other, but not
> > > both, as having many levels of indirection would be confusing."
> > > 
> > > Which I can understand and sympathise.
> > > 
> > > In anay case, the change proposed in this thread seems to change
> > > that, so the documentation would need to be updated.  Also the tests
> > > the original commit adds explicitly checks that pushInsteadOf is
> > > ignored, which may have to be updated (unless that test is already
> > > broken).
> > > 
> > 
> > My use case is that I use Github for my personal development.  I have a
> > prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for
> > fetch, git@github.com:hoelzro/ for push) and one for all other Git repos
> > (github: -> git://github.com/)  I have a few projects where I work in a
> > fork, but I want to fetch updates from the original project.  So my url
> > for the origin remote is github:org/project, but my pushurl is
> > hoelzro:project.  This behavior in Git currently doesn't allow me to
> > work that way.  I used to work with two remotes; origin for my repo and
> > base for the official one, but I've found that I prefer this other way.
> > 
> > The test that checked that pushInsteadOf + pushurl shouldn't work as I
> > expect was actually broken; I have removed it, updated the
> > documentation, and sent a new patch to the list.
> 
> There's an argument for either behavior as valid.  My original patch
> specifically documented and tested for the opposite behavior, namely
> that pushurl overrides any pushInsteadOf, because I intended
> pushInsteadOf as a fallback if you don't have an explicit pushurl set.
> For instance, you could use pushInsteadOf to rewrite a family of
> anonymous git URLs to corresponding pushable repositories, but then use
> an explicit pushurl to override that for a specific repository.  This
> change would break the ability to use pushurl for its original intended
> purpose, namely having a local repository where fetch comes from one
> remote repo and push goes to another.
> 
> One use case of mine: I have a .gitconfig in my git-managed home
> directory which sets pushInsteadOf so that I can clone via git:// and
> immediately have working push.  I work with a number of systems that
> don't have inbound access to each other but do have outbound access to
> the network; on some of these "satellite" boxes, I can't push changes
> directly to the server pushInsteadOf points to, so I can explicitly set
> pushurl in .git/config for that repository, which overrides the
> pushInsteadOf.  This change would break that configuration.

Clarifying this use case a bit: note that it's been a while since I had many such boxes, so I don't actually have any systems currently using that pushurl configuration. Still a regression in defined behavior, though.

Why not just use insteadOf for your personal github prefix hoelzro:, and both insteadOf and pushInsteadOf for github: in general? Then, a repository cloned via github: would work for pull and push (if you have push access), and you can change pushurl to your personal github alias if needed.

Though, as Junio said, the modern push-updates-remote-heads behavior of git means that both of our configurations arguably seem wrong, and we should both just use separate remotes for separate repos.

- Josh Triplett
Jonathan Nieder· Mar 27, 2013, 23:18 UTC · re: Josh Triplett · lore

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

Josh Triplett wrote:
Show 8 quoted lines
>                       I have a .gitconfig in my git-managed home
> directory which sets pushInsteadOf so that I can clone via git:// and
> immediately have working push.  I work with a number of systems that
> don't have inbound access to each other but do have outbound access to
> the network; on some of these "satellite" boxes, I can't push changes
> directly to the server pushInsteadOf points to, so I can explicitly set
> pushurl in .git/config for that repository, which overrides the
> pushInsteadOf.  This change would break that configuration.

Would it? As long as your pushurl does not start with git://, I think your configuration would still work fine.

After this patch, neither pushInsteadOf nor pushUrl overrides the other one. The rule is:

	1. First, get the URL from the remote's configuration, based
	   on whether you are fetching or pushing.
	   (At this step, in your setup git chooses the URL specified
	   with pushurl in your .git/config.)
	
	2. Next, apply the most appropriate url.*.insteadOf or
	   url.*.pushInsteadOf rule, based on whether you are fetching
	   or pushing.
	   (At this step, no rewrite rules apply, so the URL is used
	   as is.)
Junio C Hamano· Mar 28, 2013, 15:52 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
> Josh Triplett wrote:
>
>>                       I have a .gitconfig in my git-managed home
>> directory which sets pushInsteadOf so that I can clone via git:// and
>> immediately have working push.  I work with a number of systems that
>> don't have inbound access to each other but do have outbound access to
>> the network; on some of these "satellite" boxes, I can't push changes
>> directly to the server pushInsteadOf points to, so I can explicitly set
>> pushurl in .git/config for that repository, which overrides the
>> pushInsteadOf.  This change would break that configuration.
>
> Would it?  As long as your pushurl does not start with git://, I think
> your configuration would still work fine.

That is a good point, especially because it is very unlikely that git:// was used for pushURL, given that it would not have been rewritten with pushInsteadOf to an authenticated transport.

Show 15 quoted lines
> After this patch, neither pushInsteadOf nor pushUrl overrides the
> other one.  The rule is:
>
> 	1. First, get the URL from the remote's configuration, based
> 	   on whether you are fetching or pushing.
>
> 	   (At this step, in your setup git chooses the URL specified
> 	   with pushurl in your .git/config.)
> 	
> 	2. Next, apply the most appropriate url.*.insteadOf or
> 	   url.*.pushInsteadOf rule, based on whether you are fetching
> 	   or pushing.
>
> 	   (At this step, no rewrite rules apply, so the URL is used
> 	   as is.)
Josh Triplett· Mar 28, 2013, 16:01 UTC · re: Jonathan Nieder · lore

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

On Wed, Mar 27, 2013 at 04:18:19PM -0700, Jonathan Nieder wrote:
Show 13 quoted lines
> Josh Triplett wrote:
> 
> >                       I have a .gitconfig in my git-managed home
> > directory which sets pushInsteadOf so that I can clone via git:// and
> > immediately have working push.  I work with a number of systems that
> > don't have inbound access to each other but do have outbound access to
> > the network; on some of these "satellite" boxes, I can't push changes
> > directly to the server pushInsteadOf points to, so I can explicitly set
> > pushurl in .git/config for that repository, which overrides the
> > pushInsteadOf.  This change would break that configuration.
> 
> Would it?  As long as your pushurl does not start with git://, I think
> your configuration would still work fine.

I had to think about it for a while, but I think you're right; I inferred a behavior that the patch didn't actually add or have anything to do with, namely having the result of applying pushInsteadOf to the non-push URL override the pushUrl.

OK, I take it back. I *can* imagine configurations that this change would break, since it does change intentional and documented behavior, but I don't have any such configuration. The only such configuration I can imagine involves directly counting on the non-rewriting of pushUrl, by using pushInsteadOf to rewrite urls and then sometimes using pushUrl to override that and point back at the un-rewritten URL. And while supported, that does seem *odd*.

Objection withdrawn; if nobody can come up with a sensible configuration that relies on the documented behavior, I don't particularly care if it changes.

Show 15 quoted lines
> After this patch, neither pushInsteadOf nor pushUrl overrides the
> other one.  The rule is:
> 
> 	1. First, get the URL from the remote's configuration, based
> 	   on whether you are fetching or pushing.
> 
> 	   (At this step, in your setup git chooses the URL specified
> 	   with pushurl in your .git/config.)
> 	
> 	2. Next, apply the most appropriate url.*.insteadOf or
> 	   url.*.pushInsteadOf rule, based on whether you are fetching
> 	   or pushing.
> 
> 	   (At this step, no rewrite rules apply, so the URL is used
> 	   as is.)
Junio C Hamano· Mar 28, 2013, 16:10 UTC · re: Josh Triplett · lore

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

Josh Triplett <josh@joshtriplett.org> writes:
Show 11 quoted lines
> OK, I take it back.  I *can* imagine configurations that this change
> would break, since it does change intentional and documented behavior,
> but I don't have any such configuration.  The only such configuration I
> can imagine involves directly counting on the non-rewriting of pushUrl,
> by using pushInsteadOf to rewrite urls and then sometimes using pushUrl
> to override that and point back at the un-rewritten URL.  And while
> supported, that does seem *odd*.
>
> Objection withdrawn; if nobody can come up with a sensible configuration
> that relies on the documented behavior, I don't particularly care if it
> changes.
I actually do.

Given the popularity of the system, "people involved in this thread cannot imagine a case that existing people may get hurt" is very different from "this is not a regression". After merging this change when people start complaining, you and Rob can hide and ignore them, but we collectively as the Git project have to have a way to help them when it happens.

Josh Triplett· Mar 28, 2013, 16:40 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 28, 2013 at 09:10:59AM -0700, Junio C Hamano wrote:
Show 22 quoted lines
> Josh Triplett <josh@joshtriplett.org> writes:
> 
> > OK, I take it back.  I *can* imagine configurations that this change
> > would break, since it does change intentional and documented behavior,
> > but I don't have any such configuration.  The only such configuration I
> > can imagine involves directly counting on the non-rewriting of pushUrl,
> > by using pushInsteadOf to rewrite urls and then sometimes using pushUrl
> > to override that and point back at the un-rewritten URL.  And while
> > supported, that does seem *odd*.
> >
> > Objection withdrawn; if nobody can come up with a sensible configuration
> > that relies on the documented behavior, I don't particularly care if it
> > changes.
> 
> I actually do.
> 
> Given the popularity of the system, "people involved in this thread
> cannot imagine a case that existing people may get hurt" is very
> different from "this is not a regression".  After merging this
> change when people start complaining, you and Rob can hide and
> ignore them, but we collectively as the Git project have to have a
> way to help them when it happens.

I entirely agree that it represents a regression from documented behavior; I just mean that it no longer matches a specific use case I had in mind with the original change. I agree that we should hesitate to change that documented behavior.

- Josh Triplett
Junio C Hamano· Mar 28, 2013, 15:37 UTC · re: Josh Triplett · lore

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

Josh Triplett <josh@joshtriplett.org> writes:
Show 10 quoted lines
> On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:
> ...
>> The test that checked that pushInsteadOf + pushurl shouldn't work as I
>> expect was actually broken; I have removed it, updated the
>> documentation, and sent a new patch to the list.
>
> There's an argument for either behavior as valid.  My original patch
> specifically documented and tested for the opposite behavior, namely
> that pushurl overrides any pushInsteadOf, because I intended
> pushInsteadOf as a fallback if you don't have an explicit pushurl set.
For only this bit.
I think the test in question is this one from t5516:
test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
	mk_empty &&
	TRASH="$(pwd)/" &&
	git config "url.trash2/.pushInsteadOf" trash/ &&
	git config remote.r.url trash/wrong &&
	git config remote.r.pushurl "$TRASH/testrepo" &&
	git push r refs/heads/master:refs/remotes/origin/master &&
	(
		cd testrepo &&
		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
		test "z$r" = "z$the_commit" &&
		test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
	)
'

It defines a remote "r", with URL "trash/wrong" (used for fetch) and pushURL "$(pwd)/testrepo" (used for push). There is a pushInsteadOf rule to rewrite anything that goes to "trash/*" to be pushed to "trash2/*" instead but that shouldn't be used to rewrite an explicit pushURL.

And then the test pushes to "r" and checks if testrepo gets updated; in other words, it wants to make sure remote.r.pushURL defines the final destination, without pushInsteadOf getting in the way.

But $(pwd)/testrepo does not match trash/* in the first place, so there is no chance for a pushInsteadOf to interfere; it looks to me that it is not testing what it wants to test.

Perhaps we should do something like this? We tell it to push to "testrepo/" with pushURL, and set up a pushInsteadOf to rewrite "testrepo/" to "trash2/", but because for this push it comes from an explicit pushURL, it still goes to "testrepo/".

As we do not have "trash2/" repository, the test not just tests the push goes to "testrepo/", but it also tests that it does not attempt to push to "trash2/", checking both sides of the coin.

 t/t5516-fetch-push.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to t/t5516-fetch-push.sh +2 −3
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index d3dc5df..b5ea32c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -230,10 +230,9 @@ test_expect_success 'push with pushInsteadOf' '
 
 test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
 	mk_empty &&
-	TRASH="$(pwd)/" &&
-	git config "url.trash2/.pushInsteadOf" trash/ &&
+	git config "url.trash2/.pushInsteadOf" testrepo/ &&
 	git config remote.r.url trash/wrong &&
-	git config remote.r.pushurl "$TRASH/testrepo" &&
+	git config remote.r.pushurl "testrepo/" &&
 	git push r refs/heads/master:refs/remotes/origin/master &&
 	(
 		cd testrepo &&
Josh Triplett· Mar 28, 2013, 16:09 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 28, 2013 at 08:37:58AM -0700, Junio C Hamano wrote:
Show 46 quoted lines
> Josh Triplett <josh@joshtriplett.org> writes:
> 
> > On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:
> > ...
> >> The test that checked that pushInsteadOf + pushurl shouldn't work as I
> >> expect was actually broken; I have removed it, updated the
> >> documentation, and sent a new patch to the list.
> >
> > There's an argument for either behavior as valid.  My original patch
> > specifically documented and tested for the opposite behavior, namely
> > that pushurl overrides any pushInsteadOf, because I intended
> > pushInsteadOf as a fallback if you don't have an explicit pushurl set.
> 
> For only this bit.
> 
> I think the test in question is this one from t5516:
> 
> test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
> 	mk_empty &&
> 	TRASH="$(pwd)/" &&
> 	git config "url.trash2/.pushInsteadOf" trash/ &&
> 	git config remote.r.url trash/wrong &&
> 	git config remote.r.pushurl "$TRASH/testrepo" &&
> 	git push r refs/heads/master:refs/remotes/origin/master &&
> 	(
> 		cd testrepo &&
> 		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
> 		test "z$r" = "z$the_commit" &&
> 
> 		test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
> 	)
> '
> 
> It defines a remote "r", with URL "trash/wrong" (used for fetch) and
> pushURL "$(pwd)/testrepo" (used for push).  There is a pushInsteadOf
> rule to rewrite anything that goes to "trash/*" to be pushed to
> "trash2/*" instead but that shouldn't be used to rewrite an explicit
> pushURL.
> 
> And then the test pushes to "r" and checks if testrepo gets updated;
> in other words, it wants to make sure remote.r.pushURL defines the
> final destination, without pushInsteadOf getting in the way.
> 
> But $(pwd)/testrepo does not match trash/* in the first place, so
> there is no chance for a pushInsteadOf to interfere; it looks to me
> that it is not testing what it wants to test.

That test does actually test something important: it tests that the result of applying pushInsteadOf to url does *not* override pushurl. Not the same thing as testing that pushInsteadOf does or does not apply to pushurl.

The relevant sentence of the git-config manpage (in the documentation for pushInsteadOf) says:

> If a remote has an explicit pushurl, git will ignore this setting for
> that remote.

That really meant what I just said above: git will prefer an explicit pushurl over the pushInsteadOf rewrite of url. It says nothing about applying pushInsteadOf to rewrite pushurl.

Show 8 quoted lines
> Perhaps we should do something like this?  We tell it to push to
> "testrepo/" with pushURL, and set up a pushInsteadOf to rewrite
> "testrepo/" to "trash2/", but because for this push it comes from an
> explicit pushURL, it still goes to "testrepo/".
> 
> As we do not have "trash2/" repository, the test not just tests the
> push goes to "testrepo/", but it also tests that it does not attempt
> to push to "trash2/", checking both sides of the coin.

Sensible test, assuming you want to enforce that behavior. I don't strongly care either way about that one, since it only applies if your pushInsteadOf rewrites could apply to your pushurl, and I only ever use pushInsteadOf to rewrite unpushable repos to pushable ones. However...

Show 20 quoted lines
>  t/t5516-fetch-push.sh | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index d3dc5df..b5ea32c 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -230,10 +230,9 @@ test_expect_success 'push with pushInsteadOf' '
>  
>  test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
>  	mk_empty &&
> -	TRASH="$(pwd)/" &&
> -	git config "url.trash2/.pushInsteadOf" trash/ &&
> +	git config "url.trash2/.pushInsteadOf" testrepo/ &&
>  	git config remote.r.url trash/wrong &&
> -	git config remote.r.pushurl "$TRASH/testrepo" &&
> +	git config remote.r.pushurl "testrepo/" &&
>  	git push r refs/heads/master:refs/remotes/origin/master &&
>  	(
>  		cd testrepo &&

...the test you describe should appear in *addition* to this test, not replacing it, because as described above this test does test one critical bit of behavior, namely prefering pushurl over the pushInsteadOf rewrite of url.

- Josh Triplett
Junio C Hamano· Mar 28, 2013, 18:50 UTC · re: Josh Triplett · lore

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

Josh Triplett <josh@joshtriplett.org> writes:
(on url.$base.pushInsteadOf)
>> If a remote has an explicit pushurl, git will ignore this setting for
>> that remote.
> That really meant what I just said above: git will prefer an explicit
> pushurl over the pushInsteadOf rewrite of url.
Very correct.
> It says nothing about
> applying pushInsteadOf to rewrite pushurl.

Incorrect, I think. If you have pushURL, pushInsteadOf is *ignored*. Of course, if you have both URL and pushURL, the ignored pushInsteadOf will not apply to _anything_. It will not apply to URL, and it will certainly not apply to pushURL.

You are correct to point out that with the test we would want to make sure that for a remote with pushURL and URL, a push goes

 - to pushURL;
 - not to URL;
 - not to insteadOf(URL);
 - not to pushInsteadOf(URL);
 - not to insteadOf(pushURL); and
 - not to pushInsteadOf(pushURL).

I do not think it is worth checking all of them, but I agree we should make sure it does not go to pushInsteadOf(URL) which you originally meant to check, and we should also make sure it does not go to pushInsteadOf(pushURL).

Show 5 quoted lines
>>  test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
>>  	mk_empty &&
>> -	TRASH="$(pwd)/" &&
>> -	git config "url.trash2/.pushInsteadOf" trash/ &&
>> +	git config "url.trash2/.pushInsteadOf" testrepo/ &&
Adding
	git config "url.trash3/.pusnInsteadOf" trash/wrong &&

here should be sufficient for that, no? If we mistakenly used URL (i.e. trash/wrong) the push would fail. If we mistakenly used pushInsteadOf(URL), that is rewritten to trash3/ and again the push would fail. pushInsteadOf(pushURL) would go to trash2/ and that would also fail.

We aren't checking insteadOf(URL) and insteadOf(pushURL) but everything else is checked, I think, so we can do without replacing anything. We can just extend it, no?

Show 13 quoted lines
>>  	git config remote.r.url trash/wrong &&
>> -	git config remote.r.pushurl "$TRASH/testrepo" &&
>> +	git config remote.r.pushurl "testrepo/" &&
>>  	git push r refs/heads/master:refs/remotes/origin/master &&
>>  	(
>>  		cd testrepo &&
>
> ...the test you describe should appear in *addition* to this test, not
> replacing it, because as described above this test does test one
> critical bit of behavior, namely prefering pushurl over the
> pushInsteadOf rewrite of url.
>
> - Josh Triplett
Josh Triplett· Mar 28, 2013, 19:03 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 28, 2013 at 11:50:08AM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> Josh Triplett <josh@joshtriplett.org> writes:
> (on url.$base.pushInsteadOf)
> >> If a remote has an explicit pushurl, git will ignore this setting for
> >> that remote.
> > That really meant what I just said above: git will prefer an explicit
> > pushurl over the pushInsteadOf rewrite of url.
> 
> Very correct.
> 
> > It says nothing about
> > applying pushInsteadOf to rewrite pushurl.
> 
> Incorrect, I think.  If you have pushURL, pushInsteadOf is *ignored*.
> Of course, if you have both URL and pushURL, the ignored pushInsteadOf
> will not apply to _anything_.  It will not apply to URL, and it will
> certainly not apply to pushURL.

Debatable whether the documentation sentence above really says that; it certainly doesn't say it very clearly if so. But that does match the actual behavior, making the proposed change a regression from the actual behavior, whether the documentation clearly guarantees that behavior or not.

Show 14 quoted lines
> You are correct to point out that with the test we would want to
> make sure that for a remote with pushURL and URL, a push goes
> 
>  - to pushURL;
>  - not to URL;
>  - not to insteadOf(URL);
>  - not to pushInsteadOf(URL);
>  - not to insteadOf(pushURL); and
>  - not to pushInsteadOf(pushURL).
> 
> I do not think it is worth checking all of them, but I agree we
> should make sure it does not go to pushInsteadOf(URL) which you
> originally meant to check, and we should also make sure it does not
> go to pushInsteadOf(pushURL).
Agreed.

Related to this, as a path forward, I do think it makes sense to have a setting usable as an insteadOf that only applies to pushurl, even though pushInsteadOf won't end up serving that purpose. That way, pushInsteadOf covers the "map read-only repo url to pushable repo url" case, and insteadOfPushOnly covers the "construct a magic prefix that maps to different urls when used in url or pushurl" case.

Show 19 quoted lines
> >>  test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
> >>  	mk_empty &&
> >> -	TRASH="$(pwd)/" &&
> >> -	git config "url.trash2/.pushInsteadOf" trash/ &&
> >> +	git config "url.trash2/.pushInsteadOf" testrepo/ &&
> 
> Adding
> 
> 	git config "url.trash3/.pusnInsteadOf" trash/wrong &&
> 
> here should be sufficient for that, no?  If we mistakenly used URL
> (i.e. trash/wrong) the push would fail.  If we mistakenly used
> pushInsteadOf(URL), that is rewritten to trash3/ and again the push
> would fail.  pushInsteadOf(pushURL) would go to trash2/ and that
> would also fail.
> 
> We aren't checking insteadOf(URL) and insteadOf(pushURL) but
> everything else is checked, I think, so we can do without replacing
> anything.  We can just extend it, no?
That sounds sensible.
- Josh Triplett
Jonathan Nieder· Mar 28, 2013, 19:25 UTC · re: Josh Triplett · lore

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

Josh Triplett wrote:
Show 6 quoted lines
> Related to this, as a path forward, I do think it makes sense to have a
> setting usable as an insteadOf that only applies to pushurl, even though
> pushInsteadOf won't end up serving that purpose.  That way,
> pushInsteadOf covers the "map read-only repo url to pushable repo url"
> case, and insteadOfPushOnly covers the "construct a magic prefix that
> maps to different urls when used in url or pushurl" case.
I hope not.  That would be making configuration even more complicated.

I hope that we can fix the documentation, tests, and change description in the commit message enough to make Rob's patch a no-brainer. If that's not possible, I think the current state is livable, just confusing.

I was happy to see Rob's patch because it brings git's behavior closer to following the principle of least surprise. I am not actually that excited by the use case, except the "avoiding surprise" part of it.

Hope that helps, Jonathan

Rob Hoelz· Mar 29, 2013, 04:53 UTC · re: Jonathan Nieder · lore

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

On Thu, 28 Mar 2013 12:25:07 -0700 Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 24 quoted lines
> Josh Triplett wrote:
> 
> > Related to this, as a path forward, I do think it makes sense to
> > have a setting usable as an insteadOf that only applies to pushurl,
> > even though pushInsteadOf won't end up serving that purpose.  That
> > way, pushInsteadOf covers the "map read-only repo url to pushable
> > repo url" case, and insteadOfPushOnly covers the "construct a magic
> > prefix that maps to different urls when used in url or pushurl"
> > case.
> 
> I hope not.  That would be making configuration even more complicated.
> 
> I hope that we can fix the documentation, tests, and change
> description in the commit message enough to make Rob's patch a
> no-brainer.  If that's not possible, I think the current state is
> livable, just confusing.
> 
> I was happy to see Rob's patch because it brings git's behavior closer
> to following the principle of least surprise.  I am not actually that
> excited by the use case, except the "avoiding surprise" part of it.
> 
> Hope that helps,
> Jonathan
>

Thanks for all of the input, everyone. I personally agree with Jonathon's notion of "principle of least surprise", as I was quite surprised when my configuration with pushInsteadOf + pushurl did not work. However, I also understand Junio's arguments about going back on documented behavior, as well as whether or not it's a good idea to have this "triangular" remote set up.

Honestly, if my workflow here is stupid and not "Git-like" and someone has a better suggestion, I would happy to let my patch go. Using two remotes is an option, but to me, using this triangular setup is just easier.

The only way I see this breaking an existing configuration is if a user has something like url.user@server.com.pushInsteadOf = myserver:, and pushurl = myserver:repo/. If this behavior weren't documented, I would say that such a configuration works because it relies on a bug, and should use ssh://myserver:repo/ instead. I personally feel that the fact that insteadOf + url works and pushInsteadOf + pushurl doesn't is inconsistent and confusing. However, I am one user of many, and this is my first exposure to Git from a project contributor point of view. Therefore, I submit to the wisdom of more seasoned Git developers such as yourselves. =)

-Rob
Junio C Hamano· Mar 29, 2013, 05:29 UTC · re: Rob Hoelz · lore

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

Rob Hoelz <rob@hoelz.ro> writes:
> Honestly, if my workflow here is stupid and not "Git-like" and someone
> has a better suggestion, I would happy to let my patch go.  Using two
> remotes is an option, but to me, using this triangular setup is just
> easier.
I think you are conflating two unrelated things.

Pulling from one repository to integrate others' work with yours (either by merging others' work to yours, or rebasing your work on others'), and pushing the result of your work to another repository to publish, i.e. triangular workflow, is no less "Git-like" than pulling from and pushing to the same repository. Both are valid workflows, and Git supports them.

What is not correct in your set-up is that a single remote with URL and pushURL (or rewritten URL derived from them via pushInsteadOf and insteadOf) that point at two different repositories is *not* the way to express that triangular configuration. You name two remotes, pull from one and push to the other.

If you look at Ram's "triangular workflows" series, cf.
    http://thread.gmane.org/gmane.comp.version-control.git/219387

you can see that a progress is being made to make the "two remotes" configuration easier to use.

The discussion on the earliest iteration of the patch series, cf.
    http://thread.gmane.org/gmane.comp.version-control.git/215702

shows that even I initially made the same "pointing two different repositories with URL and pushURL should be sufficient" mistake, which was corrected by others. The primary issue is "remote tracking branches are designed to keep track of the state of the branches at the named remote"---for this reason alone, you must not name a logically different repository with URL and pushURL for a single remote.

So that is one thing. tl;dr: Triangular workflow is valid. A single remote with URL and pushURL to point at the two remote repositories is not a valid way to express that workflow.

The other thing is if it is worth risking to break the backward compatibility and hurting existing users in order to remove the strange "To an explicit pushURL, insteadOf rewriting will not apply" exception.

The reason I didn't bring up the possible breakage of "documented behaviour" in the earlier review of this series is exactly because that special case was unintuitive, so you do not have to argue it is strange, unintuitive, and would not be a way we would have designed the system if we knew better. I already agree to that point, and I think others do, too. There is a gap between "We would design it differently if we were building it now with what we know" and "We should change it and make it ideal" and the gap is called existing users.

These two are unrelated and independent.

I suspect that Ram's "triangular" series, when it matures, will help your "pull from there, push to another" workflow in a different way. You will just define two remotes for these two places, and you may no longer need "pushInsteadOf is not ignored when you have pushURL" to solve your immediate issue.

But removing the "pushInsteadOf is ignored when explicit pushURL exists" may still be a worthwhile thing to do, even if you do not need it.

Rob Hoelz· Mar 27, 2013, 22:29 UTC · re: Jonathan Nieder · lore

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

On Wed, 27 Mar 2013 11:23:45 -0700 Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 31 quoted lines
> Rob Hoelz wrote:
> 
> > --- 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);
> 
> Interesting.
> 
> Suppose I configure
> 
> 	[url "git://anongit.myserver.example.com/"]
> 		insteadOf = myserver.example.com:
> 	[url "myserver:"]
> 		pushInsteadOf = myserver.example.com:
> 
> The above code would make the insteadOf rule apply instead of
> pushInsteadOf, even when pushing.  Perhaps something like the
> following would work?

Are you sure? I create a copy, and compare the copy to the new URL. If they're the same (pushInsteadOf not found), I then use the insteadOf mapping to perform the alteration. Did I introduce a bug?

Jonathan Nieder· Mar 27, 2013, 22:47 UTC · re: Rob Hoelz · lore

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

Hi,
Rob Hoelz wrote:
> On Wed, 27 Mar 2013 11:23:45 -0700
> Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 12 quoted lines
>> Suppose I configure
>>
>> 	[url "git://anongit.myserver.example.com/"]
>> 		insteadOf = myserver.example.com:
>> 	[url "myserver:"]
>> 		pushInsteadOf = myserver.example.com:
>>
>> The above code would make the insteadOf rule apply instead of
>> pushInsteadOf, even when pushing.  Perhaps something like the
>> following would work?
>
> Are you sure?

The message you are replying to is nonsense, due to a typo while editing. Did you see my followup?

Sorry for the confusion, Jonathan

Rob Hoelz· Mar 27, 2013, 22:53 UTC · re: Jonathan Nieder · lore

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

On Wed, 27 Mar 2013 15:47:35 -0700 Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 25 quoted lines
> Hi,
> 
> Rob Hoelz wrote:
> > On Wed, 27 Mar 2013 11:23:45 -0700
> > Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
> >> Suppose I configure
> >>
> >> 	[url "git://anongit.myserver.example.com/"]
> >> 		insteadOf = myserver.example.com:
> >> 	[url "myserver:"]
> >> 		pushInsteadOf = myserver.example.com:
> >>
> >> The above code would make the insteadOf rule apply instead of
> >> pushInsteadOf, even when pushing.  Perhaps something like the
> >> following would work?
> >
> > Are you sure?
> 
> The message you are replying to is nonsense, due to a typo while
> editing.  Did you see my followup?
> 
> Sorry for the confusion,
> Jonathan
>

My mistake; I had not seen it! I thought you may have found a bug in my implementation, so I wanted to double check. =)

-Rob
Jonathan Nieder· Mar 27, 2013, 22:56 UTC · re: Rob Hoelz · lore

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

Rob Hoelz wrote:
> My mistake; I had not seen it!  I thought you may have found a bug in
> my implementation, so I wanted to double check. =)

Well, I had found an unfortunate consequence of the implementation that uses an unnecessary copy. :)

Will follow up to the updated patch.
Rob Hoelz· Mar 27, 2013, 23:06 UTC · re: Jonathan Nieder · lore

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

On Wed, 27 Mar 2013 15:56:56 -0700 Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 10 quoted lines
> Rob Hoelz wrote:
> 
> > My mistake; I had not seen it!  I thought you may have found a bug
> > in my implementation, so I wanted to double check. =)
> 
> Well, I had found an unfortunate consequence of the implementation
> that uses an unnecessary copy. :)
> 
> Will follow up to the updated patch.
>

I actually wanted to talk about the copy thing. I realize that this could have been avoided by simply saving a pointer to the old string and performing a comparison, but I figured if the implementation for alias_url were changed in the future to use realloc or something, it could potentially return the original char * with its contents altered. So, by copying the string, we can avoid strange bugs in the future.

-Rob

← back to recent threads