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

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

William Blevins <wblevins001@gmail.com> writes:

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.

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:

On 08/02/2023 13:49, William Blevins wrote:

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 08, 2023 at 08:49:57AM -0500, William Blevins 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.

-Peff

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

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

Jeff King <peff@peff.net> writes:

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.

Jeff King <peff@peff.net> writes:

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.

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.

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

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

Jeff King <peff@peff.net> writes:

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".

True, too.

Thanks.

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

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

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(-)

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(-)

Jeff King <peff@peff.net> writes:

Micronit.

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

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?

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

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).

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(-)

On Tue, Feb 07 2023, William Blevins wrote:

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.