git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [bug] git-ls-files sometimes does not list files with pathspec magic ":(exclude)"
@ 2021-01-13 17:41 Thomas Haller
  2021-01-14  2:07 ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Thomas Haller @ 2021-01-13 17:41 UTC (permalink / raw)
  To: git

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

Hi,


It seems git-ls-files has an issue together with certain ":(exclude)"
tags.

For example, on NetworkManager's git tree (top-level directory) I run

  git ls-files -- src/platform/ ":(exclude)shared/n-acd"

which wrongly lists not files. It seems to be some
relation between the exclude and the search path because a different
path/exclude does not exhibit the problem.

I also tested current git-master (72c4083ddf91b489b7b7b812df67ee8842177d98)
which has the same issue.

Here is a reproducer (in a container run on Fedora 33/x86_64):

  podman run -ti alpine:latest \
    sh -c '
      apk add git &&
      git clone https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git &&
      cd NetworkManager &&
      git checkout -B tmp cd754680a6a0e35b286d4157269053ccc3996a32 &&
      echo ">>>>>>now BAD1..." &&
      git ls-files -- src/platform/ ":(exclude)shared/n-acd"
      echo ">>>>>>now GOOD1..." &&
      git ls-files -- src/platform/ ":(exclude)shared/c-list"
      echo ">>>>>>now GOOD2..." &&
      git ls-files -- src/platform ":(exclude)shared/n-acd"
      echo ">>>>>>now GOOD3..." &&
      git ls-files -- src/vpn/ ":(exclude)shared/n-acd"
   '

Note that only the first call in the reproducer has the unexpected
result.


best,
Thomas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [bug] git-ls-files sometimes does not list files with pathspec magic ":(exclude)"
  2021-01-13 17:41 [bug] git-ls-files sometimes does not list files with pathspec magic ":(exclude)" Thomas Haller
@ 2021-01-14  2:07 ` Junio C Hamano
  2021-01-14 20:21   ` Jeff King
  0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2021-01-14  2:07 UTC (permalink / raw)
  To: Thomas Haller; +Cc: git

Thomas Haller <thaller@redhat.com> writes:

>       git ls-files -- src/platform/ ":(exclude)shared/n-acd"

This does look interesting.

$ git ls-files -- src/platform/ ":(exclude)shared/n-acd"
$ git ls-files -- src/platform/ ":(exclude)??????/?????"
$ git ls-files -- src/platform/ ":(exclude)??????/????"
$ git ls-files -- src/platform/ ":(exclude)??????/???"
$ git ls-files -- src/platform/ ":(exclude)??????/??"
$ git ls-files -- src/platform/ ":(exclude)??????/?"

None of the above gives any output.  And the '/' seems to be a red
herring.  None of the below (where the '/' in the exclude pathspec
is replaced with a single '?') gives any output, either.

$ git ls-files -- src/platform/ ":(exclude)????????????"
$ git ls-files -- src/platform/ ":(exclude)???????????"
$ git ls-files -- src/platform/ ":(exclude)??????????"
$ git ls-files -- src/platform/ ":(exclude)?????????"
$ git ls-files -- src/platform/ ":(exclude)????????"

But if we add one more "?" to the longuest ones, i.e.

$ git ls-files -- src/platform/ ":(exclude)?????????????"
$ git ls-files -- src/platform/ ":(exclude)shared/n-acd?"

we start seeing output.

What is curious is that the longest problematic negative pathspec,
"shared/n-acd" or "????????????", have the same length as
"src/platform" without the trailing slash.

    "shared/n-acd"
    "src/platform/"
    "????????????"

The rule IIUC is that a path must match one of the positive pathspec
and none of the negative pathspec, but it looks as if there is some
bogus optimization based on string length.

An experiment.  These ought to do the same as the first exacmple:

$ git ls-files -- src/platform/ ":(exclude)shared/n-ac[d]"
$ git ls-files -- src/platform/ ":(exclude)shared/n-[acd][acd][acd]"

but probably because of the character class [d], it seems to
defeat/bypass the broken "optimization" and gives what we expect
back.

I'll have to go back to the desk where I have a development
environment (not an end user enviornment) to dig deeper, but this is
intriguing.  I'll look more later unless somebody else beats me to
it.

Thanks for a report.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [bug] git-ls-files sometimes does not list files with pathspec magic ":(exclude)"
  2021-01-14  2:07 ` Junio C Hamano
@ 2021-01-14 20:21   ` Jeff King
  0 siblings, 0 replies; 3+ messages in thread
From: Jeff King @ 2021-01-14 20:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Thomas Haller, git

On Wed, Jan 13, 2021 at 06:07:56PM -0800, Junio C Hamano wrote:

> The rule IIUC is that a path must match one of the positive pathspec
> and none of the negative pathspec, but it looks as if there is some
> bogus optimization based on string length.

I dug a little on this earlier this morning, but didn't get far enough
to have any confidence that I wasn't barking totally up the wrong tree.
But I found the way the "prefix" is passed match_pathspec to be
confusing. This seems to make the problem go away:

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index c8eae899b8..93796404bd 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -232,7 +232,7 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,
 	    is_submodule_active(repo, ce->name)) {
 		show_submodule(repo, dir, ce->name);
 	} else if (match_pathspec(repo->index, &pathspec, fullname, strlen(fullname),
-				  max_prefix_len, ps_matched,
+				  0, ps_matched,
 				  S_ISDIR(ce->ce_mode) ||
 				  S_ISGITLINK(ce->ce_mode))) {
 		tag = get_tag(ce, tag);

which I guess is likewise disabling the same optimization you're talking
about. But that probably means the bug is in match_pathspec().

(I probably won't look further at this for a while, so please continue
your prodding; I just wanted to drop what little knowledge I came up
with).

-Peff

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-01-14 20:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-13 17:41 [bug] git-ls-files sometimes does not list files with pathspec magic ":(exclude)" Thomas Haller
2021-01-14  2:07 ` Junio C Hamano
2021-01-14 20:21   ` Jeff King

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git