threads / bug / 26279

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

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

## tl;dr

8 messages between Jan 14, 2011 and Mar 1, 2011.

replies: 7people: 3as markdown or json

Uwe Kleine-König· Jan 14, 2011, 09:06 UTC · lore
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/  |
Stefan Naewe· Jan 14, 2011, 11:36 UTC · re: Uwe Kleine-König · lore

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

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
Junio C Hamano· Jan 14, 2011, 19:55 UTC · re: Stefan Naewe · lore

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

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.

Show 16 quoted lines
> 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 ''
 		;;
 	.)
Uwe Kleine-König· Feb 28, 2011, 09:36 UTC · re: Uwe Kleine-König · lore

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

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
Junio C Hamano· Feb 28, 2011, 23:38 UTC · re: Uwe Kleine-König · lore

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

Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:
Show 7 quoted lines
> 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.

Uwe Kleine-König· Mar 1, 2011, 08:41 UTC · re: Junio C Hamano · lore

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

On Mon, Feb 28, 2011 at 03:38:26PM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> 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/  |
Uwe Kleine-König· Mar 1, 2011, 09:21 UTC · re: Uwe Kleine-König · lore

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

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
Uwe Kleine-König· Mar 1, 2011, 09:21 UTC · re: Uwe Kleine-König · lore

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

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

← back to recent threads