[PATCH] Restore ls-remote reference pattern matching

[PATCH] Restore ls-remote reference pattern matching

[Daniel Barkalow]

I entirely missed that "git ls-remote <repo> <ref-pattern>..." is
supposed to work. This restores it.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
How's this? I vaguely tested it, and it doesn't break existing tests, and 
it matches my guess at how the old code worked, at least maybe.

 builtin-ls-remote.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
index 56f3f88..f8669ce 100644
--- a/builtin-ls-remote.c
+++ b/builtin-ls-remote.c
@@ -17,6 +17,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	const char **refpathspec = NULL;
 
 	setup_git_directory_gently(&nongit);
 
@@ -50,9 +51,12 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	if (!dest || i != argc - 1)
+	if (!dest)
 		usage(ls_remote_usage);
 
+	if (argc > i + 1)
+		refpathspec = get_pathspec("*", argv + i);
+
 	remote = nongit ? NULL : remote_get(dest);
 	if (remote && !remote->url_nr)
 		die("remote %s has no configured URL", dest);
@@ -66,7 +70,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		return 1;
 
 	while (ref) {
-		if (check_ref_type(ref, flags))
+		if (check_ref_type(ref, flags) && 
+		    (!refpathspec || 
+		     pathspec_match(refpathspec, NULL, ref->name, 0)))
 			printf("%s	%s\n", sha1_to_hex(ref->old_sha1), ref->name);
 		ref = ref->next;
 	}
-- 
1.5.3.6.886.gb204

Re: [PATCH] Restore ls-remote reference pattern matching

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> How's this? I vaguely tested it, and it doesn't break existing tests, and 
> it matches my guess at how the old code worked, at least maybe.

Well, contrib/examples/git-ls-remote.sh is your friend and you do not
have to "guess".

It did, for each ref $path it got from peek-remote, this:

		for pat
		do
			case "/$path" in
			*/$pat )
				match=yes
				break ;;
			esac
		done

I do not think pathspec_match() matches the string in a way compatible
with the above loop, and calling get_pathspec(prefix, argv) with
anything but a real path is a misuse of the interface.

I think if you do fnmatch(3) that would be compatible with the shell
loop.

Re: [PATCH] Restore ls-remote reference pattern matching

[Daniel Barkalow]

On Sat, 8 Dec 2007, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > How's this? I vaguely tested it, and it doesn't break existing tests, and 
> > it matches my guess at how the old code worked, at least maybe.
> 
> Well, contrib/examples/git-ls-remote.sh is your friend and you do not
> have to "guess".
> 
> It did, for each ref $path it got from peek-remote, this:
> 
> 		for pat
> 		do
> 			case "/$path" in
> 			*/$pat )
> 				match=yes
> 				break ;;
> 			esac
> 		done
> 
> I do not think pathspec_match() matches the string in a way compatible
> with the above loop, and calling get_pathspec(prefix, argv) with
> anything but a real path is a misuse of the interface.

I'd found the same code ("git log -p -- git-ls-remote.sh" also reveals it, 
and I couldn't remember it's contrib/examples that things end up in), but 
I don't really follow that shell syntax.

> I think if you do fnmatch(3) that would be compatible with the shell
> loop.

Maybe:
--- cut here ---
I entirely missed that "git ls-remote <repo> <ref-pattern>..." is
supposed to work. This restores it.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
This matches git-name-rev --refs=<ref-pattern>, anyway, which is the 
closest example I could find. If this isn't the desired behavior, it's 
probably easier to just edit this instead of trying to explain the right 
thing to me.

 builtin-ls-remote.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
index 56f3f88..d936c28 100644
--- a/builtin-ls-remote.c
+++ b/builtin-ls-remote.c
@@ -17,6 +17,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	const char **refpatterns = NULL;
 
 	setup_git_directory_gently(&nongit);
 
@@ -50,9 +51,12 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	if (!dest || i != argc - 1)
+	if (!dest)
 		usage(ls_remote_usage);
 
+	if (argc > i + 1)
+		refpatterns = argv + i;
+
 	remote = nongit ? NULL : remote_get(dest);
 	if (remote && !remote->url_nr)
 		die("remote %s has no configured URL", dest);
@@ -66,8 +70,18 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		return 1;
 
 	while (ref) {
-		if (check_ref_type(ref, flags))
-			printf("%s	%s\n", sha1_to_hex(ref->old_sha1), ref->name);
+		if (check_ref_type(ref, flags)) {
+			int match = 0;
+			if (refpatterns) {
+				for (i = 0; refpatterns[i]; i++) {
+					if (!fnmatch(refpatterns[i], ref->name, 0))
+						match = 1;
+				}
+			} else
+				match = 1;
+			if (match)
+				printf("%s	%s\n", sha1_to_hex(ref->old_sha1), ref->name);
+		}
 		ref = ref->next;
 	}
 	return 0;
-- 
1.5.3.6.886.gb204

Re: [PATCH] Restore ls-remote reference pattern matching

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> I'd found the same code ("git log -p -- git-ls-remote.sh" also reveals it, 
> and I couldn't remember it's contrib/examples that things end up in), but 
> I don't really follow that shell syntax.

Sorry, I should have been more explicit.

At this point in the scripted version:

	for pat
	do
		case "/$path" in
		*/$pat )
			match=yes
			break ;;
		esac
	done

 - $path is what we read from peek-remote (or the equivalent from curl
   for http), e.g. "refs/heads/master", or "HEAD".

 - we iterate over remainder of arguments to the script, assigning each
   to $pat;

 - we check if the pattern "*/$pat" matches "/$path".  "case" matching
   rule is an entire string match with globbing, so it is asking if
   /$path ends with /$pat.  IOW, $pat="master" matches:

	$path=refs/heads/master
        $path=refs/remotes/origin/master

   but it does not match $path="refs/heads/omaster".

---

 builtin-ls-remote.c |   44 ++++++++++++++++++++++++++++++++++++++------
 1 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
index 56f3f88..bae7c48 100644
--- a/builtin-ls-remote.c
+++ b/builtin-ls-remote.c
@@ -6,6 +6,35 @@
 static const char ls_remote_usage[] =
 "git-ls-remote [--upload-pack=<git-upload-pack>] [<host>:]<directory>";
 
+/*
+ * pattern is a list of tail-part of accepted refnames.  Is there one
+ * among then that is a suffix of the path?  Directory boundary must
+ * be honored when doing this match.  IOW, patterns "master" and
+ * "sa/master" both match path "refs/hold/sa/master".  On the other
+ * hand, path "refs/hold/foosa/master" is matched by "master" but not
+ * by "sa/master".
+ */
+
+static int tail_match(const char **pattern, const char *path)
+{
+	int pathlen;
+	const char *p;
+
+	if (!*pattern)
+		return 1; /* no restriction */
+
+	for (pathlen = strlen(path); (p = *pattern); pattern++) {
+		int pfxlen = pathlen - strlen(p);
+		if (pfxlen < 0)
+			continue; /* pattern is longer, will never match */
+		if (strcmp(path + pfxlen, p))
+			continue; /* no tail match */
+		if (!pfxlen || path[pfxlen - 1] == '/')
+			return 1; /* fully match at directory boundary */
+	}
+	return 0;
+}
+
 int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -13,6 +42,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int nongit = 0;
 	unsigned flags = 0;
 	const char *uploadpack = NULL;
+	const char **pattern = NULL;
 
 	struct remote *remote;
 	struct transport *transport;
@@ -50,9 +80,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		break;
 	}
 
-	if (!dest || i != argc - 1)
+	if (!dest)
 		usage(ls_remote_usage);
-
+	pattern = argv + i + 1;
 	remote = nongit ? NULL : remote_get(dest);
 	if (remote && !remote->url_nr)
 		die("remote %s has no configured URL", dest);
@@ -65,10 +95,12 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (!ref)
 		return 1;
 
-	while (ref) {
-		if (check_ref_type(ref, flags))
-			printf("%s	%s\n", sha1_to_hex(ref->old_sha1), ref->name);
-		ref = ref->next;
+	for ( ; ref; ref = ref->next) {
+		if (!check_ref_type(ref, flags))
+			continue;
+		if (!tail_match(pattern, ref->name))
+			continue;
+		printf("%s	%s\n", sha1_to_hex(ref->old_sha1), ref->name);
 	}
 	return 0;
 }

Re: [PATCH] Restore ls-remote reference pattern matching

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

On Sat, 08 Dec 2007 22:51:05 -0800 Junio C Hamano wrote:

[...]
> 	for pat
> 	do
> 		case "/$path" in
> 		*/$pat )
> 			match=yes
> 			break ;;
> 		esac
> 	done
[...]
> +/*
> + * pattern is a list of tail-part of accepted refnames.  Is there one
> + * among then that is a suffix of the path?  Directory boundary must
> + * be honored when doing this match.  IOW, patterns "master" and
> + * "sa/master" both match path "refs/hold/sa/master".  On the other
> + * hand, path "refs/hold/foosa/master" is matched by "master" but not
> + * by "sa/master".
> + */
> +
> +static int tail_match(const char **pattern, const char *path)
> +{
> +	int pathlen;
> +	const char *p;
> +
> +	if (!*pattern)
> +		return 1; /* no restriction */
> +
> +	for (pathlen = strlen(path); (p = *pattern); pattern++) {
> +		int pfxlen = pathlen - strlen(p);
> +		if (pfxlen < 0)
> +			continue; /* pattern is longer, will never match */
> +		if (strcmp(path + pfxlen, p))
> +			continue; /* no tail match */
> +		if (!pfxlen || path[pfxlen - 1] == '/')
> +			return 1; /* fully match at directory boundary */
> +	}
> +	return 0;
> +}

This still does not match the behavior of the old shell implementation
completely - because $pat was not quoted, shell pattern characters in
$pat worked, and things like "git ls-remote . 'refs/heads/something--*'"
were possible (and used in some of my scripts), so a full fnmatch()
call is still needed.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Restore ls-remote reference pattern matching

[Eyvind Bernhardsen]

On 9. des.. 2007, at 14.26, Sergey Vlasov wrote:
> This still does not match the behavior of the old shell implementation
> completely - because $pat was not quoted, shell pattern characters in
> $pat worked, and things like "git ls-remote . 'refs/heads/something-- 
> *'"
> were possible (and used in some of my scripts), so a full fnmatch()
> call is still needed.


The example in the ls-remote manpage ("git ls-remote --tags <remote> v 
\*") also fails because of this.

Eyvind Bernhardsen

Re: [PATCH] Restore ls-remote reference pattern matching

[Junio C Hamano]

Sergey Vlasov <vsu@altlinux.ru> writes:

> This still does not match the behavior of the old shell implementation
> completely - because $pat was not quoted, shell pattern characters in
> $pat worked, and things like "git ls-remote . 'refs/heads/something--*'"
> were possible (and used in some of my scripts), so a full fnmatch()
> call is still needed.

Ah, true.  I wanted to cheat but that was a mistake.

Re: [PATCH] Restore ls-remote reference pattern matching

[Junio C Hamano]

Sergey Vlasov <vsu@altlinux.ru> writes:

> This still does not match the behavior of the old shell implementation
> completely - because $pat was not quoted, shell pattern characters in
> $pat worked, and things like "git ls-remote . 'refs/heads/something--*'"
> were possible (and used in some of my scripts), so a full fnmatch()
> call is still needed.

Sigh...

-- >8 --
Subject: [PATCH] Re-fix ls-remote

An earlier attempt in 2ea7fe0 (ls-remote: resurrect pattern limit support) forgot
that the user string can also be a glob.  This should finally fix it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-ls-remote.c |   39 +++++++++++++++++++++------------------
 1 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
index e5d670a..c2caeea 100644
--- a/builtin-ls-remote.c
+++ b/builtin-ls-remote.c
@@ -7,30 +7,22 @@ static const char ls_remote_usage[] =
 "git-ls-remote [--upload-pack=<git-upload-pack>] [<host>:]<directory>";
 
 /*
- * pattern is a list of tail-part of accepted refnames.  Is there one
- * among them that is a suffix of the path?  Directory boundary must
- * be honored when checking this match.  IOW, patterns "master" and
- * "sa/master" both match path "refs/hold/sa/master".  On the other
- * hand, path "refs/hold/foosa/master" is matched by "master" but not
- * by "sa/master".
+ * Is there one among the list of patterns that match the tail part
+ * of the path?
  */
-
 static int tail_match(const char **pattern, const char *path)
 {
-	int pathlen;
 	const char *p;
+	char pathbuf[PATH_MAX];
 
-	if (!*pattern)
+	if (!pattern)
 		return 1; /* no restriction */
 
-	for (pathlen = strlen(path); (p = *pattern); pattern++) {
-		int pfxlen = pathlen - strlen(p);
-		if (pfxlen < 0)
-			continue; /* pattern is longer, will never match */
-		if (strcmp(path + pfxlen, p))
-			continue; /* no tail match */
-		if (!pfxlen || path[pfxlen - 1] == '/')
-			return 1; /* fully match at directory boundary */
+	if (snprintf(pathbuf, sizeof(pathbuf), "/%s", path) > sizeof(pathbuf))
+		return error("insanely long ref %.*s...", 20, path);
+	while ((p = *(pattern++)) != NULL) {
+		if (!fnmatch(p, pathbuf, 0))
+			return 1;
 	}
 	return 0;
 }
@@ -77,12 +69,23 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 			usage(ls_remote_usage);
 		}
 		dest = arg;
+		i++;
 		break;
 	}
 
 	if (!dest)
 		usage(ls_remote_usage);
-	pattern = argv + i + 1;
+
+	if (argv[i]) {
+		int j;
+		pattern = xcalloc(sizeof(const char *), argc - i + 1);
+		for (j = i; j < argc; j++) {
+			int len = strlen(argv[j]);
+			char *p = xmalloc(len + 3);
+			sprintf(p, "*/%s", argv[j]);
+			pattern[j - i] = p;
+		}
+	}
 	remote = nongit ? NULL : remote_get(dest);
 	if (remote && !remote->url_nr)
 		die("remote %s has no configured URL", dest);
-- 
1.5.3.7-1142-gbb4e

Re: [PATCH] Restore ls-remote reference pattern matching

[Eyvind Bernhardsen]

On 9. des. 2007, at 21.26, Junio C Hamano wrote:

> Sergey Vlasov <vsu@altlinux.ru> writes:
>
>> This still does not match the behavior of the old shell  
>> implementation
>> completely - because $pat was not quoted, shell pattern characters in
>> $pat worked, and things like "git ls-remote . 'refs/heads/ 
>> something--*'"
>> were possible (and used in some of my scripts), so a full fnmatch()
>> call is still needed.
>
> Sigh...

This patch makes ls-remote work as expected for me.
-- 
Eyvind

Re: [PATCH] Restore ls-remote reference pattern matching

[Junio C Hamano]

Eyvind Bernhardsen <eyvind-git@orakel.ntnu.no> writes:

> This patch makes ls-remote work as expected for me.

Thanks, the patch is queued for master.