[PATCH] remote.<name>.pushurl does not consider aliases when pushing

[PATCH] remote.<name>.pushurl does not consider aliases when pushing

[Rob Hoelz]

Hi everyone!  I found a bug in Git today and wrote up a fix; I did my best to conform to the rules layed out in Documentation/SubmittingPatches, but please let me know if I need to change anything to get my work merged. =)  I have CC'ed Josh Triplet, as
he was the last one to touch the line I modified.  I hope my commit messages explain the problem I encountered well enough; if not,
I can always go back and amend them.

Patches follow.

-Rob

>From 5007b11e86c0835807632cb41e6cfa75ce9a1aa1 Mon Sep 17 00:00:00 2001
From: Rob Hoelz <rob@hoelz.ro>
Date: Sun, 17 Mar 2013 21:49:20 +0100
Subject: [PATCH 1/2] Add test for pushInsteadOf + pushurl

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

Signed-off-by: Rob Hoelz <rob@hoelz.ro>
---
 t/t5500-push-pushurl.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 t/t5500-push-pushurl.sh

diff --git a/t/t5500-push-pushurl.sh b/t/t5500-push-pushurl.sh
new file mode 100644
index 0000000..74d4ff6
--- /dev/null
+++ b/t/t5500-push-pushurl.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+#
+# Copyright (c) 2013 Rob Hoelz
+#
+
+test_description='Test that remote.<name>.pushurl uses pushInsteadOf'
+. ./test-lib.sh
+
+wd=$(pwd)
+test_expect_success 'setup test repositories' '
+	mkdir ro &&
+	mkdir rw &&
+
+	git init --bare ro/repo &&
+	git init --bare rw/repo &&
+	git init test-repo
+'
+
+test_expect_success 'setup remote config' "
+	cd test-repo &&
+	git config 'url.file://$wd/ro/.insteadOf' herero: &&
+	git config 'url.file://$wd/rw/.pushInsteadOf' hererw: &&
+	git remote add origin herero:repo &&
+	git config remote.origin.pushurl hererw:repo
+"
+
+test_expect_success 'test commit and push' '
+	test_commit one &&
+	git push origin master:master
+'
+
+test_expect_success 'check for commits in rw repo' '
+	cd ../rw/repo &&
+	git log --pretty=oneline | grep -q .
+'
+
+test_done
-- 
1.8.2

>From 0cbd1aab6bdc0c2f8893ed8b9a8e3eb0126917d1 Mon Sep 17 00:00:00 2001
From: Rob Hoelz <rob@hoelz.ro>
Date: Sun, 17 Mar 2013 16:34:35 +0100
Subject: [PATCH 2/2] push: Alias pushurl from push rewrites

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

This commit fixes that.

Signed-off-by: Rob Hoelz <rob@hoelz.ro>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 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++) {
-- 
1.8.2

Re: [PATCH] remote.<name>.pushurl does not consider aliases when pushing

[Junio C Hamano]

Rob Hoelz <rob@hoelz.ro> writes:

> Hi everyone!  I found a bug in Git today and wrote up a fix; I did my best to conform to the rules layed out in Documentation/SubmittingPatches, but please let me know if I need to change anything to get my work merged. =)  I have CC'ed Josh Triplet, as
> he was the last one to touch the line I modified.  I hope my commit messages explain the problem I encountered well enough; if not,
> I can always go back and amend them.
>
> Patches follow.
>
> -Rob


Please read Documentation/SubmittingPatches and follow it.  The
above is most likely to be the cover letter of a two-patch series
(meaning you will be sending three pieces of e-mail messages), or
perhaps out of band comment below the three-dash line of a single
patch (you will send only one piece of e-mail message).

See recent patches on the list from list regulars for good examples,
e.g.

    http://thread.gmane.org/gmane.comp.version-control.git/218350
    http://thread.gmane.org/gmane.comp.version-control.git/218177/focus=218361

> From 5007b11e86c0835807632cb41e6cfa75ce9a1aa1 Mon Sep 17 00:00:00 2001
> From: Rob Hoelz <rob@hoelz.ro>
> Date: Sun, 17 Mar 2013 21:49:20 +0100
> Subject: [PATCH 1/2] Add test for pushInsteadOf + pushurl
>
> git push currently doesn't consider pushInsteadOf when
> using pushurl; this test tests that.
>
> Signed-off-by: Rob Hoelz <rob@hoelz.ro>
> ---
>  t/t5500-push-pushurl.sh | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 t/t5500-push-pushurl.sh

The number 5500 is already taken.  Please do not add a duplicate.

I also wonder if we need to waste a new test number for this;
perhaps adding new tests to 5516 that already tests insteadOf might
be a better fit, but I didn't carefully read it.

> diff --git a/t/t5500-push-pushurl.sh b/t/t5500-push-pushurl.sh
> new file mode 100644

Test scripts are supposed to be executable.

> +test_expect_success 'test commit and push' '
> +	test_commit one &&
> +	git push origin master:master
> +'
> +
> +test_expect_success 'check for commits in rw repo' '
> +	cd ../rw/repo &&
> +	git log --pretty=oneline | grep -q .
> +'

Are both expected to succeed in patch 1/2 without any code change?

If you were doing a large code change, it is a good series structure
to have tests first that are marked as "expect_failure" in an early
patch, and then in a later patch that changes the code to fix it,
update the tests that start to pass to "expect_success".

I personally do not think you need such a two-step approach for
something small like this; instead you can just have a single patch
that adds a set of tests that expect success and code change.

Thanks.

Re: [PATCH] remote.<name>.pushurl does not consider aliases when pushing

[Rob Hoelz]

On Sun, 17 Mar 2013 15:14:32 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Rob Hoelz <rob@hoelz.ro> writes:
> 
> > Hi everyone!  I found a bug in Git today and wrote up a fix; I did
> > my best to conform to the rules layed out in
> > Documentation/SubmittingPatches, but please let me know if I need
> > to change anything to get my work merged. =)  I have CC'ed Josh
> > Triplet, as he was the last one to touch the line I modified.  I
> > hope my commit messages explain the problem I encountered well
> > enough; if not, I can always go back and amend them.
> >
> > Patches follow.
> >
> > -Rob
> 
> 
> Please read Documentation/SubmittingPatches and follow it.  The
> above is most likely to be the cover letter of a two-patch series
> (meaning you will be sending three pieces of e-mail messages), or
> perhaps out of band comment below the three-dash line of a single
> patch (you will send only one piece of e-mail message).
> 
> See recent patches on the list from list regulars for good examples,
> e.g.
> 
>     http://thread.gmane.org/gmane.comp.version-control.git/218350
>     http://thread.gmane.org/gmane.comp.version-control.git/218177/focus=218361
> 
> > From 5007b11e86c0835807632cb41e6cfa75ce9a1aa1 Mon Sep 17 00:00:00
> > 2001 From: Rob Hoelz <rob@hoelz.ro>
> > Date: Sun, 17 Mar 2013 21:49:20 +0100
> > Subject: [PATCH 1/2] Add test for pushInsteadOf + pushurl
> >
> > git push currently doesn't consider pushInsteadOf when
> > using pushurl; this test tests that.
> >
> > Signed-off-by: Rob Hoelz <rob@hoelz.ro>
> > ---
> >  t/t5500-push-pushurl.sh | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644 t/t5500-push-pushurl.sh
> 
> The number 5500 is already taken.  Please do not add a duplicate.
> 
> I also wonder if we need to waste a new test number for this;
> perhaps adding new tests to 5516 that already tests insteadOf might
> be a better fit, but I didn't carefully read it.
> 
> > diff --git a/t/t5500-push-pushurl.sh b/t/t5500-push-pushurl.sh
> > new file mode 100644
> 
> Test scripts are supposed to be executable.
> 
> > +test_expect_success 'test commit and push' '
> > +	test_commit one &&
> > +	git push origin master:master
> > +'
> > +
> > +test_expect_success 'check for commits in rw repo' '
> > +	cd ../rw/repo &&
> > +	git log --pretty=oneline | grep -q .
> > +'
> 
> Are both expected to succeed in patch 1/2 without any code change?
> 
> If you were doing a large code change, it is a good series structure
> to have tests first that are marked as "expect_failure" in an early
> patch, and then in a later patch that changes the code to fix it,
> update the tests that start to pass to "expect_success".
> 
> I personally do not think you need such a two-step approach for
> something small like this; instead you can just have a single patch
> that adds a set of tests that expect success and code change.
> 
> Thanks.
> 

Thanks for the feeback; another reply with the new patch follows.