bug: request-pull broken when remote name contains a slash

bug: request-pull broken when remote name contains a slash

[Uwe Kleine-König]

Hello,

the remotes in my linux repo look as follows:

	[remote "ptx/ukl"]
		url = git://git.pengutronix.de/git/ukl/linux-2.6.git
		push = +refs/heads/*:refs/heads/*
		fetch = +refs/heads/*:refs/remotes/ptx/ukl/*

	[remote "ptx/otheruser"]
		url = git://git.pengutronix.de/git/otheruser/linux-2.6.git
		fetch = +refs/heads/*:refs/remotes/ptx/otheruser/*

	[remote "ptx/yetanotheruser"]
		url = git://git.pengutronix.de/git/yetanotheruser/linux-2.6.git
		fetch = +refs/heads/*:refs/remotes/ptx/yetanotheruser/*

unfortunately this makes sending request pulls uncomfortable:

	ukl@octopus:~/gsrc/linux-2.6$ git request-pull HEAD^ ptx/ukl
	The following changes since commit a08948812b30653eb2c536ae613b635a989feb6f:

	  Merge branch 'hwmon-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/staging (2011-01-10 08:57:46 -0800)

	are available in the git repository at:

	  ptx/ukl mxs/for-2.6.38
	...

The reason for that is in git-parse-remote.sh:get_data_source() which
assumes that a remote with a slash is a filename and so get_remote_url
doesn't use $(git config --get "remote.$1.url") but ptx/ukl directly.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] fix git-parse-remote.sh for remotes that contain slashes

[Stefan Naewe]

Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
---
 git-parse-remote.sh |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 5f47b18..7cf204e 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -7,8 +7,12 @@ GIT_DIR=$(git rev-parse -q --git-dir) || :;
 get_data_source () {
 	case "$1" in
 	*/*)
-		echo ''
-		;;
+		if test "$(git config --get "remote.$1.url")"
+		then
+			echo config
+		else
+			echo ''
+		fi ;;
 	.)
 		echo self
 		;;
-- 
1.7.3.5

Re: [PATCH] fix git-parse-remote.sh for remotes that contain slashes

[Junio C Hamano]

Stefan Naewe <stefan.naewe@gmail.com> writes:

> Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
> ---

Thanks, but no explanation?

Imagine somebody who weren't reading this thread (especially the article
you responded to with this patch) sees this in "git log" output stream.
For that matter, imagine yourself doing that in 2012 when the motivation
of this change you all forgot already.

Do you think it is obvious what the problem the patch tried to fix was?
I don't.  "fix" on the subject line gives you 0-bit information for that
purpose.

> diff --git a/git-parse-remote.sh b/git-parse-remote.sh
> index 5f47b18..7cf204e 100644
> --- a/git-parse-remote.sh
> +++ b/git-parse-remote.sh
> @@ -7,8 +7,12 @@ GIT_DIR=$(git rev-parse -q --git-dir) || :;
>  get_data_source () {
>  	case "$1" in
>  	*/*)
> -		echo ''
> -		;;
> +		if test "$(git config --get "remote.$1.url")"
> +		then
> +			echo config
> +		else
> +			echo ''
> +		fi ;;

I suspect that making this case arm trigger not on */* but only on /* and
../* would be a lot more sensible solution.  Otherwise you would still
have the same issue in repositories that use remotes/ and branches/
mechanism.

 git-parse-remote.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 1cc2ba6..8ec33e3 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -6,7 +6,7 @@ GIT_DIR=$(git rev-parse -q --git-dir) || :;
 
 get_data_source () {
 	case "$1" in
-	*/*)
+	../* | /*)
 		echo ''
 		;;
 	.)

[PATCH] get_remote_url(): use the same data source as ls-remote to get remote urls

[Uwe Kleine-König]

The formerly implemented algorithm behaved differently to
remote.c:remote_get() at least for remotes that contain a slash.  While the
former just assumes a/b is a path the latter checks the config for
remote."a/b" first which is more reasonable.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

with this patch git-request-pull.sh (== the only in-tree user of
git-parse-remote.sh:get_remote_url()) could directly use

	git ls-remote --get-url

.  I guess you wouldn't want to remove git-parse-remote.sh:get_remote_url()
though?!  Maybe add a deprecation warning to it?  The same applies to
git-parse-remote.sh:get_data_source() that isn't used anymore already with this
patch.

Best regards
Uwe

 builtin/ls-remote.c |   11 +++++++++++
 git-parse-remote.sh |   24 +-----------------------
 2 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 97eed40..1a1ff87 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -33,6 +33,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int i;
 	const char *dest = NULL;
 	unsigned flags = 0;
+	int get_url = 0;
 	int quiet = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
@@ -69,6 +70,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 				quiet = 1;
 				continue;
 			}
+			if (!strcmp("--get-url", arg)) {
+				get_url = 1;
+				continue;
+			}
 			usage(ls_remote_usage);
 		}
 		dest = arg;
@@ -94,6 +99,12 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	}
 	if (!remote->url_nr)
 		die("remote %s has no configured URL", dest);
+
+	if (get_url) {
+		printf("%s\n", *remote->url);
+		return 0;
+	}
+
 	transport = transport_get(remote, NULL);
 	if (uploadpack != NULL)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 1cc2ba6..4fb778e 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -29,29 +29,7 @@ get_data_source () {
 }
 
 get_remote_url () {
-	data_source=$(get_data_source "$1")
-	case "$data_source" in
-	'')
-		echo "$1"
-		;;
-	self)
-		echo "$1"
-		;;
-	config)
-		git config --get "remote.$1.url"
-		;;
-	remotes)
-		sed -ne '/^URL: */{
-			s///p
-			q
-		}' "$GIT_DIR/remotes/$1"
-		;;
-	branches)
-		sed -e 's/#.*//' "$GIT_DIR/branches/$1"
-		;;
-	*)
-		die "internal error: get-remote-url $1" ;;
-	esac
+	git ls-remote --get-url "$1"
 }
 
 get_default_remote () {
-- 
1.7.2.3

Re: [PATCH] get_remote_url(): use the same data source as ls-remote to get remote urls

[Junio C Hamano]

Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:

> with this patch git-request-pull.sh (== the only in-tree user of
> git-parse-remote.sh:get_remote_url()) could directly use
>
> 	git ls-remote --get-url
>
> .  I guess you wouldn't want to remove git-parse-remote.sh:get_remote_url()
> though?!

If nobody uses it, why not?  I was actually hoping for the day that nobody
uses any of the defined function so taht we can remove that whole file.

Re: [PATCH] get_remote_url(): use the same data source as ls-remote to get remote urls

[Uwe Kleine-König]

On Mon, Feb 28, 2011 at 03:38:26PM -0800, Junio C Hamano wrote:
> Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:
> 
> > with this patch git-request-pull.sh (== the only in-tree user of
> > git-parse-remote.sh:get_remote_url()) could directly use
> >
> > 	git ls-remote --get-url
> >
> > .  I guess you wouldn't want to remove git-parse-remote.sh:get_remote_url()
> > though?!
> 
> If nobody uses it, why not?  I was actually hoping for the day that nobody
> uses any of the defined function so taht we can remove that whole file.
Ok for me.  Probably I'm a bit conservative here because I usually work
on the kernel and people are picky here to remove interfaces.

I can followup with a patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/2] get_remote_url(): use the same data source as ls-remote to get remote urls

[Uwe Kleine-König]

The formerly implemented algorithm behaved differently to
remote.c:remote_get() at least for remotes that contain a slash.  While the
former just assumes a/b is a path the latter checks the config for
remote."a/b" first which is more reasonable.

This removes the last user of git-parse-remote.sh:get_data_source(), so
this function is removed.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

compared with the previous patch this patch removes get_data_source
which is unused now.  The second patch also gets rid of get_remote_url.

 builtin/ls-remote.c |   11 +++++++++++
 git-parse-remote.sh |   48 +-----------------------------------------------
 2 files changed, 12 insertions(+), 47 deletions(-)


The changes in this series are also available in the git repository at:
  git.pengutronix.de:/git/ukl/git.git ls-remote-in-get-remote-url

basing on commit 7ed863a85a6ce2c4ac4476848310b8f917ab41f9:

  Git 1.7.4 (2011-01-30 19:02:37 -0800)

Uwe Kleine-König (2):
      get_remote_url(): use the same data source as ls-remote to get remote urls
      git-request-pull: open-code the only invocation of get_remote_url

(Note: this pull request was generated using the new git-request-pull
and using a remote with a slash \o/)

 builtin/ls-remote.c |   11 +++++++++++
 git-parse-remote.sh |   50 --------------------------------------------------
 git-request-pull.sh |    3 +--
 3 files changed, 12 insertions(+), 52 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 97eed40..1a1ff87 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -33,6 +33,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int i;
 	const char *dest = NULL;
 	unsigned flags = 0;
+	int get_url = 0;
 	int quiet = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
@@ -69,6 +70,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 				quiet = 1;
 				continue;
 			}
+			if (!strcmp("--get-url", arg)) {
+				get_url = 1;
+				continue;
+			}
 			usage(ls_remote_usage);
 		}
 		dest = arg;
@@ -94,6 +99,12 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	}
 	if (!remote->url_nr)
 		die("remote %s has no configured URL", dest);
+
+	if (get_url) {
+		printf("%s\n", *remote->url);
+		return 0;
+	}
+
 	transport = transport_get(remote, NULL);
 	if (uploadpack != NULL)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 1cc2ba6..0ab1192 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -4,54 +4,8 @@
 # this would fail in that case and would issue an error message.
 GIT_DIR=$(git rev-parse -q --git-dir) || :;
 
-get_data_source () {
-	case "$1" in
-	*/*)
-		echo ''
-		;;
-	.)
-		echo self
-		;;
-	*)
-		if test "$(git config --get "remote.$1.url")"
-		then
-			echo config
-		elif test -f "$GIT_DIR/remotes/$1"
-		then
-			echo remotes
-		elif test -f "$GIT_DIR/branches/$1"
-		then
-			echo branches
-		else
-			echo ''
-		fi ;;
-	esac
-}
-
 get_remote_url () {
-	data_source=$(get_data_source "$1")
-	case "$data_source" in
-	'')
-		echo "$1"
-		;;
-	self)
-		echo "$1"
-		;;
-	config)
-		git config --get "remote.$1.url"
-		;;
-	remotes)
-		sed -ne '/^URL: */{
-			s///p
-			q
-		}' "$GIT_DIR/remotes/$1"
-		;;
-	branches)
-		sed -e 's/#.*//' "$GIT_DIR/branches/$1"
-		;;
-	*)
-		die "internal error: get-remote-url $1" ;;
-	esac
+	git ls-remote --get-url "$1"
 }
 
 get_default_remote () {
-- 
1.7.2.3

[PATCH 2/2] git-request-pull: open-code the only invocation of get_remote_url

[Uwe Kleine-König]

So sh:get_remote_url can go now and git-request-pull
doesn't need to source git-parse-remote. anymore.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 git-parse-remote.sh |    4 ----
 git-request-pull.sh |    3 +--
 2 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 0ab1192..e7013f7 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -4,10 +4,6 @@
 # this would fail in that case and would issue an error message.
 GIT_DIR=$(git rev-parse -q --git-dir) || :;
 
-get_remote_url () {
-	git ls-remote --get-url "$1"
-}
-
 get_default_remote () {
 	curr_branch=$(git symbolic-ref -q HEAD | sed -e 's|^refs/heads/||')
 	origin=$(git config --get "branch.$curr_branch.remote")
diff --git a/git-request-pull.sh b/git-request-pull.sh
index 6fdea39..fc080cc 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -15,7 +15,6 @@ p    show patch text as well
 '
 
 . git-sh-setup
-. git-parse-remote
 
 GIT_PAGER=
 export GIT_PAGER
@@ -55,7 +54,7 @@ branch=$(git ls-remote "$url" \
 		p
 		q
 	}")
-url=$(get_remote_url "$url")
+url=$(git ls-remote --get-url "$url")
 if [ -z "$branch" ]; then
 	echo "warn: No branch of $url is at:" >&2
 	git log --max-count=1 --pretty='tformat:warn:   %h: %s' $headrev >&2
-- 
1.7.2.3