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

[Rob Hoelz <rob@hoelz.ro>]

On Sun, 17 Mar 2013 16:35:59 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Rob Hoelz <rob@hoelz.ro> writes:
> 
> > git push currently doesn't consider pushInsteadOf when
> > using pushurl; this tests and fixes 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
> 
> Incomplete sentence?  For example [this is an example configuration]
> and then what happens?  Something like "with the sample
> configuration, 'git push origin' should follow pushurl and then turn
> it into X, but instead it ends up accessing Y".
> 
> If there is no pushInsteadOf, does it still follow insteadOf?  Is it
> tested already?
> 
> Wouldn't you want to cover all the combinations to negative cases
> (i.e. making sure the codepath to support a new case does not affect
> behaviour of the code outside the new case)?  A remote with and
> without pushurl (two combinations) and a pseudo URL scheme with and
> without pushInsteadOf (again, two combinations) will give you four
> cases.
> 
> 
> Thanks.

I've taken your advice, and an amended patch follows.

> 
> >
> > Signed-off-by: Rob Hoelz <rob@hoelz.ro>
> > ---
> >  remote.c              |  2 +-
> >  t/t5516-fetch-push.sh | 20 ++++++++++++++++++++
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/remote.c b/remote.c
> > index ca1f8f2..de7a915 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -465,7 +465,7 @@ 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);
> > +			remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites_push); }
> >  		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 b5417cc..272e225 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf
> > and explicit pushurl (pushInsteadOf )
> >  '
> >  
> > +test_expect_success 'push with pushInsteadOf and explicit pushurl
> > (pushInsteadOf does rewrite in this case)' '
> > +	mk_empty &&
> > +	TRASH="$(pwd)/" &&
> > +	mkdir ro &&
> > +	mkdir rw &&
> > +	git init --bare rw/testrepo &&
> > +	git config "url.file://$TRASH/ro/.insteadOf" ro: &&
> > +	git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
> > +	git config remote.r.url ro:wrong &&
> > +	git config remote.r.pushurl rw:testrepo &&
> > +	git push r refs/heads/master:refs/remotes/origin/master &&
> > +	(
> > +		cd rw/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)
> > +	)
> > +'
> > +
> >  test_expect_success 'push with matching heads' '
> >  
> >  	mk_test heads/master &&
>