Unexpected (bug-like) behavior in `git ls-remote` matching.

Unexpected (bug-like) behavior in `git ls-remote` matching.

[William Blevins]

Setup as follows.

example repo: git@github.com:owner/repo.git
example branches:
* ABC-1
* feature/ABC-1
* XBC-1
* EABC-1

These are all things that work as "expected".
```
$ git ls-remote --heads git@github.com:owner/repo.git ABC-
$ git ls-remote --heads git@github.com:owner/repo.git BC-1
$ git ls-remote --heads git@github.com:owner/repo.git XBC-1
<ref>    refs/head/XBC-1
$ git ls-remote --heads git@github.com:owner/repo.git *BC-1
<ref>    refs/head/ABC-1
<ref>    refs/head/EABC-1
<ref>    refs/head/XBC-1
<ref>    refs/head/feature/ABC-1
$ git ls-remote --heads git@github.com:owner/repo.git "[^/]ABC-1"
<ref>    refs/head/EABC-1
```

What is totally unexpected.... is the most simple search for ABC-1...
```
$ git ls-remote --heads git@github.com:owner/repo.git ABC-1
<ref>    refs/head/ABC-1
<ref>    refs/head/feature/ABC-1
```

It appears that the matching behavior trims at the last slash... which
isn't correct.

V/R,
William

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[Junio C Hamano]

William Blevins <wblevins001@gmail.com> writes:

> What is totally unexpected.... is the most simple search for ABC-1...
> ```
> $ git ls-remote --heads git@github.com:owner/repo.git ABC-1
> <ref>    refs/head/ABC-1
> <ref>    refs/head/feature/ABC-1
> ```

Sorry, but I cannot see what is surprising about the above.  If you
have these branches locally, you should also see these refs in the
output of "git show-ref ABC-1".  Refname hierarchies work just like
pathnames with directories, and without glob in the pattern, tail
matching that honors path component boundary is very much the norm
in the oldest part of Git, i.e. ABC-1 matches refs/heads/ABC-1 but
not refs/heads/XABC-1.

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[William Blevins]

This still feels "weird" to me. Other pattern matching tools like grep
and sed don't have exceptions to their behavior like this.

Can you reference the unit tests that verifies this specific behavior?

On Wed, Feb 8, 2023 at 2:20 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> William Blevins <wblevins001@gmail.com> writes:
>
> > What is totally unexpected.... is the most simple search for ABC-1...
> > ```
> > $ git ls-remote --heads git@github.com:owner/repo.git ABC-1
> > <ref>    refs/head/ABC-1
> > <ref>    refs/head/feature/ABC-1
> > ```
>
> Sorry, but I cannot see what is surprising about the above.  If you
> have these branches locally, you should also see these refs in the
> output of "git show-ref ABC-1".  Refname hierarchies work just like
> pathnames with directories, and without glob in the pattern, tail
> matching that honors path component boundary is very much the norm
> in the oldest part of Git, i.e. ABC-1 matches refs/heads/ABC-1 but
> not refs/heads/XABC-1.

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 07 2023, William Blevins wrote:

> Setup as follows.
>
> example repo: git@github.com:owner/repo.git
> example branches:
> * ABC-1
> * feature/ABC-1
> * XBC-1
> * EABC-1
>
> These are all things that work as "expected".
> ```
> $ git ls-remote --heads git@github.com:owner/repo.git ABC-
> $ git ls-remote --heads git@github.com:owner/repo.git BC-1
> $ git ls-remote --heads git@github.com:owner/repo.git XBC-1
> <ref>    refs/head/XBC-1
> $ git ls-remote --heads git@github.com:owner/repo.git *BC-1
> <ref>    refs/head/ABC-1
> <ref>    refs/head/EABC-1
> <ref>    refs/head/XBC-1
> <ref>    refs/head/feature/ABC-1
> $ git ls-remote --heads git@github.com:owner/repo.git "[^/]ABC-1"
> <ref>    refs/head/EABC-1
> ```
>
> What is totally unexpected.... is the most simple search for ABC-1...
> ```
> $ git ls-remote --heads git@github.com:owner/repo.git ABC-1
> <ref>    refs/head/ABC-1
> <ref>    refs/head/feature/ABC-1
> ```

Aside from what Junio mentioned, I don't see why you'd think that the
result for "ABC-1" and "BC-1" is correct if you'd like your "ABC-1"
query to also find "EABC-1", but maybe I'm missing something obvious.

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[Philip Oakley]

On 08/02/2023 13:49, William Blevins wrote:
> This still feels "weird" to me. Other pattern matching tools like grep
> and sed don't have exceptions to their behavior like this.
>
> Can you reference the unit tests that verifies this specific behavior?

As someone who also trips up over the *nix command line, Is this a bash
(glob) expansion issue?
So there is a two step 'expansion', first by bash, and then by git's
pattern match. Even plain text could be "expanded" if there's a suitable
expansion

a quick search suggested
https://superuser.com/questions/861077/is-it-possible-to-print-out-the-shell-expansion
which has some tipe I wasn't aware of.

Philip

>
> On Wed, Feb 8, 2023 at 2:20 AM Junio C Hamano <gitster@pobox.com> wrote:
>> William Blevins <wblevins001@gmail.com> writes:
>>
>>> What is totally unexpected.... is the most simple search for ABC-1...
>>> ```
>>> $ git ls-remote --heads git@github.com:owner/repo.git ABC-1
>>> <ref>    refs/head/ABC-1
>>> <ref>    refs/head/feature/ABC-1
>>> ```
>> Sorry, but I cannot see what is surprising about the above.  If you
>> have these branches locally, you should also see these refs in the
>> output of "git show-ref ABC-1".  Refname hierarchies work just like
>> pathnames with directories, and without glob in the pattern, tail
>> matching that honors path component boundary is very much the norm
>> in the oldest part of Git, i.e. ABC-1 matches refs/heads/ABC-1 but
>> not refs/heads/XABC-1.

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[Jeff King]

On Wed, Feb 08, 2023 at 08:49:57AM -0500, William Blevins wrote:

> This still feels "weird" to me. Other pattern matching tools like grep
> and sed don't have exceptions to their behavior like this.
> 
> Can you reference the unit tests that verifies this specific behavior?

See commit 631f0f8c4b (ls-remote: do not send ref prefixes for patterns,
2018-10-31), which also adds a test. However, it was just handling
existing behavior (which in fact confused another developer and caused a
bug which the commit was fixing!).

I think the tail-matching behavior is not what we would probably choose
today, but that is how it has behaved since 2005, and we are not going
to break backwards compatibility in a plumbing tool like ls-remote.

There's some discussion in this thread about adding a new option to do
prefix-matching, but I don't think any code was ever written:

  https://lore.kernel.org/git/m2k12g7v5u.fsf@gmail.com/

Likewise, something more elaborate like full-path globbing or even
regex matching would be possible, but would need to be activated by an
option.

-Peff

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[Jeff King]

On Wed, Feb 08, 2023 at 11:30:04AM -0500, Jeff King wrote:

> See commit 631f0f8c4b (ls-remote: do not send ref prefixes for patterns,
> 2018-10-31), which also adds a test. However, it was just handling
> existing behavior (which in fact confused another developer and caused a
> bug which the commit was fixing!).
> 
> I think the tail-matching behavior is not what we would probably choose
> today, but that is how it has behaved since 2005, and we are not going
> to break backwards compatibility in a plumbing tool like ls-remote.
> 
> There's some discussion in this thread about adding a new option to do
> prefix-matching, but I don't think any code was ever written:
> 
>   https://lore.kernel.org/git/m2k12g7v5u.fsf@gmail.com/
> 
> Likewise, something more elaborate like full-path globbing or even
> regex matching would be possible, but would need to be activated by an
> option.

Oh, and I forgot to mention: the documentation for ls-remote is quite
weak here, and simply says "matching" without defining it. So the most
obvious improvement is fixing that documentation to describe the current
rules (which AFAIK is basically matching the pattern as a glob, but with
an implicit "/" anchor, but somebody should double check the code before
writing a documentation patch).

-Peff

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think the tail-matching behavior is not what we would probably choose
> today, but that is how it has behaved since 2005, and we are not going
> to break backwards compatibility in a plumbing tool like ls-remote.

The tail-matching behaviour that was Linus's preference (his
show-ref behaves the same way) has one interesting feature---looking
for all 'master' branches in different hierarchies is trivial.

In today's world, it would be tremendously benefitial if "ls-remote"
can trim not just the communication between the repositories but
also the enumeration of the ref namespace at the source repository
using the pattern, and tail-matching would not contribute to it at
all (unless if your server side adds special index).  If we used
prefix matching, our refs API can take advantage of it in reducing
the cost to enumerate refs, and on-the-wire protocol has ref-prefix
capability to help it.

> Likewise, something more elaborate like full-path globbing or even
> regex matching would be possible, but would need to be activated by an
> option.

True.  We should be able to do a bit better than just tail-matching
with an option.

I would not recommend sending over regex as protocol capability the
same way as ref-prefix works, unless we adopt something that can
match linear-time like re2 and use it everywhere, as you can send a
pattern that is deliberately made inefficient to inconvenience the
other side.

Thanks.

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Oh, and I forgot to mention: the documentation for ls-remote is quite
> weak here, and simply says "matching" without defining it. So the most
> obvious improvement is fixing that documentation to describe the current
> rules (which AFAIK is basically matching the pattern as a glob, but with
> an implicit "/" anchor, but somebody should double check the code before
> writing a documentation patch).

Very much.  

On the pattern side we add */ in front of the given pattern (so,
'master' becomes "*/master", and 'refs/heads/master' becomes
'*/refs/heads/master), and run wildmatch() against refs prefixed
with '/' (so 'refs/heads/master' becomes '/refs/heads/master' and
matches '*/master', and it also matches '*/refs/heads/master').

THanks.

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[Jeff King]

On Wed, Feb 08, 2023 at 09:40:06AM -0800, Junio C Hamano wrote:

> > Likewise, something more elaborate like full-path globbing or even
> > regex matching would be possible, but would need to be activated by an
> > option.
> 
> True.  We should be able to do a bit better than just tail-matching
> with an option.
> 
> I would not recommend sending over regex as protocol capability the
> same way as ref-prefix works, unless we adopt something that can
> match linear-time like re2 and use it everywhere, as you can send a
> pattern that is deliberately made inefficient to inconvenience the
> other side.

Yeah, I should not even have mentioned regex. It was really meant as "if
you really wanted to go wild, you could do something as crazy as
regexes". But I agree that we would never want to pass regexes over the
wire. If we want to make things more efficient, prefix-matching is the
way. If we want to make things more ergonomic for the client-side user,
then we should stick to globbing (which we already do, but we could do
things like full-string globs rather than strict tail-matching).

But I would hold off on all of that until somebody has a concrete case
that shows why their preferred matching scheme is useful.

-Peff

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I would not recommend sending over regex as protocol capability the
>> same way as ref-prefix works, unless we adopt something that can
>> match linear-time like re2 and use it everywhere, as you can send a
>> pattern that is deliberately made inefficient to inconvenience the
>> other side.
>
> Yeah, I should not even have mentioned regex. It was really meant as "if
> you really wanted to go wild, you could do something as crazy as
> regexes".

The cautionary comment wasn't meant for you (you know I know you
better than that by now) but was primarily to deter those who are
reading from sidelines from going wild for "low hanging fruit".

Unlike normal desktop features, a feature that can easily be abused
for DoS cannot be initially built in a way that is inefficient, with
a hope that we will iterate and improve over time, until which time
we ship it labeled as "experimental".

> But I would hold off on all of that until somebody has a concrete case
> that shows why their preferred matching scheme is useful.

True, too.

Thanks.

Re: Unexpected (bug-like) behavior in `git ls-remote` matching.

[Jeff King]

On Thu, Feb 09, 2023 at 11:43:02AM -0800, Junio C Hamano wrote:

> > But I would hold off on all of that until somebody has a concrete case
> > that shows why their preferred matching scheme is useful.
> 
> True, too.

Let's not forget to fix the documentation, though. Here's a brief
series.

  [1/2]: doc/ls-remote: cosmetic cleanups for examples
  [2/2]: doc/ls-remote: clarify pattern format

 Documentation/git-ls-remote.txt | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

-Peff

[PATCH 1/2] doc/ls-remote: cosmetic cleanups for examples

[Jeff King]

There are effectively three example commands and their output, but
they're smushed together with no extra whitespace. Let's add some blank
lines to make them more readable.

Likewise, the first example uses "./." to refer to the path of the
current repository, which is somewhat distracting. That may have been
necessary back in 2005 when it was added, but we can just say "." these
days.

Signed-off-by: Jeff King <peff@peff.net>
---
This isn't strictly necessary here, but seems worth doing. I had
intended it as preparation for adding another example in patch 2, but I
ended up thinking that the text change was sufficient.

 Documentation/git-ls-remote.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 492e573856..f17567945f 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -95,15 +95,17 @@ EXAMPLES
 --------
 
 ----
-$ git ls-remote --tags ./.
+$ git ls-remote --tags .
 d6602ec5194c87b0fc87103ca4d67251c76f233a	refs/tags/v0.99
 f25a265a342aed6041ab0cc484224d9ca54b6f41	refs/tags/v0.99.1
 7ceca275d047c90c0c7d5afb13ab97efdf51bd6e	refs/tags/v0.99.3
 c5db5456ae3b0873fc659c19fafdde22313cc441	refs/tags/v0.99.2
 0918385dbd9656cab0d1d81ba7453d49bbc16250	refs/tags/junio-gpg-pub
+
 $ git ls-remote http://www.kernel.org/pub/scm/git/git.git master seen rc
 5fe978a5381f1fbad26a80e682ddd2a401966740	refs/heads/master
 c781a84b5204fb294c9ccc79f8b3baceeb32c061	refs/heads/seen
+
 $ git remote add korg http://www.kernel.org/pub/scm/git/git.git
 $ git ls-remote --tags korg v\*
 d6602ec5194c87b0fc87103ca4d67251c76f233a	refs/tags/v0.99
-- 
2.39.1.795.g4b3688ded9

[PATCH 2/2] doc/ls-remote: clarify pattern format

[Jeff King]

We document that you can specify "refs" to ls-remote, but we don't
explain any further than that they are "matched" as patterns. Since this
can be interpreted in a lot of ways, let's clarify that they are
tail-matched globs.

Likewise, let's use the word "patterns" to refer to them consistently,
rather than "refs", and mention more explicitly that only one pattern
needs to be matched (though there is also an example already that shows
this in action).

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-ls-remote.txt | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index f17567945f..2a941292a4 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
 	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
-	      [--symref] [<repository> [<refs>...]]
+	      [--symref] [<repository> [<patterns>...]]
 
 DESCRIPTION
 -----------
@@ -85,11 +85,14 @@ OPTIONS
 	either a URL or the name of a remote (see the GIT URLS and
 	REMOTES sections of linkgit:git-fetch[1]).
 
-<refs>...::
+<patterns>...::
 	When unspecified, all references, after filtering done
-	with --heads and --tags, are shown.  When <refs>... are
-	specified, only references matching the given patterns
-	are displayed.
+	with --heads and --tags, are shown.  When <patterns>... are
+	specified, only references matching one or more of the given
+	patterns are displayed. Each pattern is interpreted as a glob
+	(see `glob` in linkgit:gitglossary[7]) which is matched against
+	the "tail" of a ref, starting from a slash separator (so `bar`
+	matches `refs/heads/bar` but not `refs/heads/foobar`).
 
 EXAMPLES
 --------
-- 
2.39.1.795.g4b3688ded9

Re: [PATCH 2/2] doc/ls-remote: clarify pattern format

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We document that you can specify "refs" to ls-remote, but we don't
> explain any further than that they are "matched" as patterns. Since this
> can be interpreted in a lot of ways, let's clarify that they are
> tail-matched globs.
>
> Likewise, let's use the word "patterns" to refer to them consistently,
> rather than "refs", and mention more explicitly that only one pattern
> needs to be matched (though there is also an example already that shows
> this in action).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/git-ls-remote.txt | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index f17567945f..2a941292a4 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -11,7 +11,7 @@ SYNOPSIS
>  [verse]
>  'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
>  	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
> -	      [--symref] [<repository> [<refs>...]]
> +	      [--symref] [<repository> [<patterns>...]]

Micronit.

builtin/ls-remote.c::ls_remote_usage[] needs a matching update.

>  
>  DESCRIPTION
>  -----------
> @@ -85,11 +85,14 @@ OPTIONS
>  	either a URL or the name of a remote (see the GIT URLS and
>  	REMOTES sections of linkgit:git-fetch[1]).
>  
> -<refs>...::
> +<patterns>...::
>  	When unspecified, all references, after filtering done
> -	with --heads and --tags, are shown.  When <refs>... are
> -	specified, only references matching the given patterns
> -	are displayed.
> +	with --heads and --tags, are shown.  When <patterns>... are
> +	specified, only references matching one or more of the given
> +	patterns are displayed. Each pattern is interpreted as a glob
> +	(see `glob` in linkgit:gitglossary[7]) which is matched against
> +	the "tail" of a ref, starting from a slash separator (so `bar`
> +	matches `refs/heads/bar` but not `refs/heads/foobar`).

Good.  Is it too obvious that the pattern `refs/heads/bar` matches
the ref `refs/heads/bar`, even though it becomes fuzzy what
"starting from a slash separator" means in such a scenario?

[PATCH v2 2/2] doc/ls-remote: clarify pattern format

[Jeff King]

On Fri, Feb 10, 2023 at 06:54:03PM -0800, Junio C Hamano wrote:

> >  'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
> >  	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
> > -	      [--symref] [<repository> [<refs>...]]
> > +	      [--symref] [<repository> [<patterns>...]]
> 
> Micronit.
> 
> builtin/ls-remote.c::ls_remote_usage[] needs a matching update.

Good catch. It is more than a micronit, as it causes t0450 to fail (I
did not even think to run the tests, since it was just a doc change).

> > -<refs>...::
> > +<patterns>...::
> >  	When unspecified, all references, after filtering done
> > -	with --heads and --tags, are shown.  When <refs>... are
> > -	specified, only references matching the given patterns
> > -	are displayed.
> > +	with --heads and --tags, are shown.  When <patterns>... are
> > +	specified, only references matching one or more of the given
> > +	patterns are displayed. Each pattern is interpreted as a glob
> > +	(see `glob` in linkgit:gitglossary[7]) which is matched against
> > +	the "tail" of a ref, starting from a slash separator (so `bar`
> > +	matches `refs/heads/bar` but not `refs/heads/foobar`).
> 
> Good.  Is it too obvious that the pattern `refs/heads/bar` matches
> the ref `refs/heads/bar`, even though it becomes fuzzy what
> "starting from a slash separator" means in such a scenario?

Ah, thank you for bringing that up. I actually meant to call attention
to that case, as when I tried "git ls-remote . refs/heads/master", it
did not match anything, which seemed to me like a bug. But in fact it is
because I don't have a master branch in my repo (I only keep my feature
branches, plus an integration branch, and always refer to yours as
origin/master), and my experiment was buggy. :)

I do think it's worth mentioning (and thankfully there is no bug to
fix).

Here's a re-roll with both changes.

-- >8 --
Subject: [PATCH] doc/ls-remote: clarify pattern format

We document that you can specify "refs" to ls-remote, but we don't
explain any further than that they are "matched" as patterns. Since this
can be interpreted in a lot of ways, let's clarify that they are
tail-matched globs.

Likewise, let's use the word "patterns" to refer to them consistently,
rather than "refs" (both here and in the quick "-h" help), and mention
more explicitly that only one pattern needs to be matched (though there
is also an example already that shows this in action).

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-ls-remote.txt | 15 ++++++++++-----
 builtin/ls-remote.c             |  2 +-
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index f17567945f..ff3da547dd 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
 	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
-	      [--symref] [<repository> [<refs>...]]
+	      [--symref] [<repository> [<patterns>...]]
 
 DESCRIPTION
 -----------
@@ -85,11 +85,16 @@ OPTIONS
 	either a URL or the name of a remote (see the GIT URLS and
 	REMOTES sections of linkgit:git-fetch[1]).
 
-<refs>...::
+<patterns>...::
 	When unspecified, all references, after filtering done
-	with --heads and --tags, are shown.  When <refs>... are
-	specified, only references matching the given patterns
-	are displayed.
+	with --heads and --tags, are shown.  When <patterns>... are
+	specified, only references matching one or more of the given
+	patterns are displayed. Each pattern is interpreted as a glob
+	(see `glob` in linkgit:gitglossary[7]) which is matched against
+	the "tail" of a ref, starting either from the start of the ref
+	(so a full name like `refs/heads/foo` matches) or from a slash
+	separator (so `bar` matches `refs/heads/bar` but not
+	`refs/heads/foobar`).
 
 EXAMPLES
 --------
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 5d5ac03871..6516177348 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -8,7 +8,7 @@
 static const char * const ls_remote_usage[] = {
 	N_("git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]\n"
 	   "              [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]\n"
-	   "              [--symref] [<repository> [<refs>...]]"),
+	   "              [--symref] [<repository> [<patterns>...]]"),
 	NULL
 };
 
-- 
2.39.1.795.g4b3688ded9