ls-files --exclude broken?

ls-files --exclude broken?

[Jay Soffian]

Am I missing something really obvious here?

kore:~/Repos/git (master)$ git ls-files | wc -l
    2009
kore:~/Repos/git (master)$ git ls-files -x \* | wc -l
    2009
kore:~/Repos/git (master)$ git ls-files -i -x \* | wc -l
    2009
kore:~/Repos/git (master)$ git version
git version 1.7.3.rc1.5.g95127

j.

Re: ls-files --exclude broken?

[Nguyen Thai Ngoc Duy]

On Thu, Sep 16, 2010 at 8:40 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> Am I missing something really obvious here?
>
> kore:~/Repos/git (master)$ git ls-files | wc -l
>    2009
> kore:~/Repos/git (master)$ git ls-files -x \* | wc -l
>    2009
> kore:~/Repos/git (master)$ git ls-files -i -x \* | wc -l
>    2009
> kore:~/Repos/git (master)$ git version
> git version 1.7.3.rc1.5.g95127

excluded() is not called as far as I can tell, which explains why it
did not exclude anything for you. That exclude code was touched by
Jeff in commits b5227d8 (ls-files: excludes should not impact tracked
files) and 500348a (ls-files: unbreak "ls-files -i").

Something like this works for me, but I haven't finished my morning
tea yet, so..

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index bb4f612..ab87843 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -216,9 +216,14 @@ static void show_files(struct dir_struct *dir)
 		for (i = 0; i < active_nr; i++) {
 			struct cache_entry *ce = active_cache[i];
 			int dtype = ce_to_dtype(ce);
-			if (dir->flags & DIR_SHOW_IGNORED &&
-			    !excluded(dir, ce->name, &dtype))
-				continue;
+			if (dir->flags & DIR_SHOW_IGNORED) {
+				if (!excluded(dir, ce->name, &dtype))
+					continue;
+			}
+			else {
+				if (excluded(dir, ce->name, &dtype))
+					continue;
+			}
 			if (show_unmerged && !ce_stage(ce))
 				continue;
 			if (ce->ce_flags & CE_UPDATE)
-- 
Duy

Re: ls-files --exclude broken?

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> Am I missing something really obvious here?
>
> kore:~/Repos/git (master)$ git ls-files | wc -l
>     2009

You asked it to show the cached paths (default).  Your project currently
tracks 2009 paths in the index.

> kore:~/Repos/git (master)$ git ls-files -x \* | wc -l
>     2009

You told that '*' is the exclude pattern for carrying out some operation,
but you didn't tell what operation you want.  You are shown the cached
paths (default).

> kore:~/Repos/git (master)$ git ls-files -i -x \* | wc -l
>     2009

You told that '*' is the exclude pattern, you want only paths that match
the exclude pattern, and chose to show files in the index (which again is
the default) by not saying -o.

I've never found -i useful myself (actually I don't find many options the
command has very useful anymore), but the above is how I read the ls-files
manual page.

Re: ls-files --exclude broken?

[Jay Soffian]

On Wed, Sep 15, 2010 at 7:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Am I missing something really obvious here?
>>
>> kore:~/Repos/git (master)$ git ls-files | wc -l
>>     2009
>
> You asked it to show the cached paths (default).  Your project currently
> tracks 2009 paths in the index.

Correct.

>> kore:~/Repos/git (master)$ git ls-files -x \* | wc -l
>>     2009
>
> You told that '*' is the exclude pattern for carrying out some operation,
> but you didn't tell what operation you want.  You are shown the cached
> paths (default).

I want cached paths, minus the exclude pattern. Using -c -x \* gives
the same result.

There is no indication in the man page that -x doesn't apply to -c.

>> kore:~/Repos/git (master)$ git ls-files -i -x \* | wc -l
>>     2009
>
> You told that '*' is the exclude pattern, you want only paths that match
> the exclude pattern, and chose to show files in the index (which again is
> the default) by not saying -o.
>
> I've never found -i useful myself (actually I don't find many options the
> command has very useful anymore), but the above is how I read the ls-files
> manual page.

I don't care about -i myself, and maybe I should have been clearer.
AFAICT, [-c] -x is broken:

$ git ls-files -c -x \* |wc -l
    2009

       -c, --cached
           Show cached files in the output (default)

       -x <pattern>, --exclude=<pattern>
           Skips files matching pattern. Note that pattern is a shell
wildcard pattern.

Oh, geez, here it is lower down in the man page:

  git ls-files can use a list of "exclude patterns" when traversing
the directory tree and finding files to show when the flags --others
or --ignored are specified. gitignore(5) specifies the format of
exclude patterns.

Bah, what use is that? Minimally ls-files shouldn't accept -x unless
given either --ignored or --others if that's all it applies to. I
guess this is my itch to scratch.

j.

Re: ls-files --exclude broken?

[Nguyen Thai Ngoc Duy]

On Thu, Sep 16, 2010 at 9:19 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> On Wed, Sep 15, 2010 at 7:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Am I missing something really obvious here?
>>>
>>> kore:~/Repos/git (master)$ git ls-files | wc -l
>>>     2009
>>
>> You asked it to show the cached paths (default).  Your project currently
>> tracks 2009 paths in the index.
>
> Correct.
>
>>> kore:~/Repos/git (master)$ git ls-files -x \* | wc -l
>>>     2009
>>
>> You told that '*' is the exclude pattern for carrying out some operation,
>> but you didn't tell what operation you want.  You are shown the cached
>> paths (default).
>
> I want cached paths, minus the exclude pattern. Using -c -x \* gives
> the same result.
>
> There is no indication in the man page that -x doesn't apply to -c.
>
>>> kore:~/Repos/git (master)$ git ls-files -i -x \* | wc -l
>>>     2009
>>
>> You told that '*' is the exclude pattern, you want only paths that match
>> the exclude pattern, and chose to show files in the index (which again is
>> the default) by not saying -o.
>>
>> I've never found -i useful myself (actually I don't find many options the
>> command has very useful anymore), but the above is how I read the ls-files
>> manual page.
>
> I don't care about -i myself, and maybe I should have been clearer.
> AFAICT, [-c] -x is broken:
>
> $ git ls-files -c -x \* |wc -l
>    2009
>
>       -c, --cached
>           Show cached files in the output (default)
>
>       -x <pattern>, --exclude=<pattern>
>           Skips files matching pattern. Note that pattern is a shell
> wildcard pattern.
>
> Oh, geez, here it is lower down in the man page:
>
>  git ls-files can use a list of "exclude patterns" when traversing
> the directory tree and finding files to show when the flags --others
> or --ignored are specified. gitignore(5) specifies the format of
> exclude patterns.
>
> Bah, what use is that? Minimally ls-files shouldn't accept -x unless
> given either --ignored or --others if that's all it applies to. I
> guess this is my itch to scratch.

I sense another use of negative pathspecs here..
-- 
Duy

Re: ls-files --exclude broken?

[Jay Soffian]

On Wed, Sep 15, 2010 at 7:29 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> I sense another use of negative pathspecs here..

Yeah, I'm really not sure I agree with the reasoning of b5227d8
(ls-files: excludes should not impact tracked files, 2009-10-12).

I thought about submitting a patch to revert b5227d8, but with an
additional option to guard the behavior
(--exclude-cached-files-too-pretty-please).

But I guess I don't see the harm in allowing excludes to apply to
cached files when it is explicitly requested on the command-line. Here
I think ls-files is slightly different than the other areas of git
that don't apply excludes to cached files as it's about, well, listing
files, and it's clear (I think) what the user wants.

But I'll go dig in the mailing list archives now to see if I can find
what motivated Jeff's change beyond what he gave in the commit
message.

j.

Re: ls-files --exclude broken?

[Jay Soffian]

On Wed, Sep 15, 2010 at 7:41 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> But I'll go dig in the mailing list archives now to see if I can find
> what motivated Jeff's change beyond what he gave in the commit
> message.

http://thread.gmane.org/gmane.comp.version-control.git/129889/focus=129976

Hmm.

j.

Re: ls-files --exclude broken?

[Jeff King]

On Wed, Sep 15, 2010 at 07:41:50PM -0400, Jay Soffian wrote:

> On Wed, Sep 15, 2010 at 7:29 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> > I sense another use of negative pathspecs here..
> 
> Yeah, I'm really not sure I agree with the reasoning of b5227d8
> (ls-files: excludes should not impact tracked files, 2009-10-12).
> 
> I thought about submitting a patch to revert b5227d8, but with an
> additional option to guard the behavior
> (--exclude-cached-files-too-pretty-please).
> 
> But I guess I don't see the harm in allowing excludes to apply to
> cached files when it is explicitly requested on the command-line. Here
> I think ls-files is slightly different than the other areas of git
> that don't apply excludes to cached files as it's about, well, listing
> files, and it's clear (I think) what the user wants.

It's not just the command line. It's also what's in .gitignore files. If
you disable just half of that, then you get the awful behavior that some
excludes apply to index files, and some don't.

Yes, it's confusing that "-i" is not actually an inversion for index
files. See

  http://article.gmane.org/gmane.comp.version-control.git/131719

It would help if I understood exactly what you're trying to accomplish.

-Peff

Re: ls-files --exclude broken?

[Jay Soffian]

On Wed, Sep 15, 2010 at 8:16 PM, Jeff King <peff@peff.net> wrote:
> It's not just the command line. It's also what's in .gitignore files. If
> you disable just half of that, then you get the awful behavior that some
> excludes apply to index files, and some don't.

Files matched by the standard excludes are not likely to be in the
index in the first place. So in that sense, arguably -x is special.

But nonetheless, I agree with you, and since the user must
specifically ask ls-files for the various exclusions, I think it makes
sense to apply those even to cached files.

> It would help if I understood exactly what you're trying to accomplish.

I'm building a project file for my editor and I want to exclude
certain files that make no sense for it to care about even though they
are part of the repo. So I tried:

$ git ls-files -x png -x jpg ...

and was confused by that not working. I've worked around this by just
filtering the ls-files output through grep, but, ick.

j.

Re: ls-files --exclude broken?

[Jeff King]

On Wed, Sep 15, 2010 at 08:33:29PM -0400, Jay Soffian wrote:

> On Wed, Sep 15, 2010 at 8:16 PM, Jeff King <peff@peff.net> wrote:
> > It's not just the command line. It's also what's in .gitignore files. If
> > you disable just half of that, then you get the awful behavior that some
> > excludes apply to index files, and some don't.
> 
> Files matched by the standard excludes are not likely to be in the
> index in the first place. So in that sense, arguably -x is special.
> 
> But nonetheless, I agree with you, and since the user must
> specifically ask ls-files for the various exclusions, I think it makes
> sense to apply those even to cached files.

But then if somebody asks for both indexed and untracked files together,
the behavior is quite confusing. There is no way to say
"--exclude-standard just for the untracked files", and having exclusions
impact cached files is unlike any other part of git. So the behavior of
something like "git ls-files -s -o --exclude-standard" would be broken.

I have no idea if people are actually doing that.

So I think the best fix would be to leave the default behavior as-is,
and add the --exclude-cached-files-too-pretty-please you mentioned.

> I'm building a project file for my editor and I want to exclude
> certain files that make no sense for it to care about even though they
> are part of the repo. So I tried:
> 
> $ git ls-files -x png -x jpg ...
> 
> and was confused by that not working. I've worked around this by just
> filtering the ls-files output through grep, but, ick.

I certainly have sympathy for you being confused by the behavior of
ls-files. It undoubtedly is a mess.

But as for having to use grep, I would feel worse for you if you were
actually trying to exclude patterns from your .gitignore and you needed
to _convert_ them into a grep pattern. But it doesn't seem like:

  git ls-files | grep -v '\.png$'

is really much more work, and it's way more flexible.

At one point I proposed a "git check-ignore" similar to "git
check-attr", but I never got around to coding it. If you really liked
the gitignore-style patterns for some reason, it would be a nice
flexible way of doing what you want.

-Peff

Re: ls-files --exclude broken?

[Elijah Newren]

On Wed, Sep 15, 2010 at 5:19 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> I want cached paths, minus the exclude pattern. Using -c -x \* gives
> the same result.
>
> There is no indication in the man page that -x doesn't apply to -c.

That caught me too.

<snip>
> Bah, what use is that? Minimally ls-files shouldn't accept -x unless
> given either --ignored or --others if that's all it applies to. I
> guess this is my itch to scratch.

I ran into the same thing earlier this week and came up with a patch
similar to Nguyen's (though I just restored it closer to what Johannes
originally had there), before I discovered that this was an
intentional change by Jeff.  (And then I tabled it to get back to work
on fixing up merge-recursive some more)

I can't say I really understand the changes from b5227d8 and 500348,
though; I found (and find) it confusing as well.  I did notice that
restoring the old behavior didn't work very well with items below the
current directory, so it'd take a bit more work than just restoring
the original behavior.


Elijah

Re: ls-files --exclude broken?

[Kevin Ballard]

On Sep 15, 2010, at 4:12 PM, Junio C Hamano wrote:

> Jay Soffian <jaysoffian@gmail.com> writes:
> 
>> Am I missing something really obvious here?
>> 
>> kore:~/Repos/git (master)$ git ls-files | wc -l
>>    2009
> 
> You asked it to show the cached paths (default).  Your project currently
> tracks 2009 paths in the index.
> 
>> kore:~/Repos/git (master)$ git ls-files -x \* | wc -l
>>    2009
> 
> You told that '*' is the exclude pattern for carrying out some operation,
> but you didn't tell what operation you want.  You are shown the cached
> paths (default).
> 
>> kore:~/Repos/git (master)$ git ls-files -i -x \* | wc -l
>>    2009
> 
> You told that '*' is the exclude pattern, you want only paths that match
> the exclude pattern, and chose to show files in the index (which again is
> the default) by not saying -o.
> 
> I've never found -i useful myself (actually I don't find many options the
> command has very useful anymore), but the above is how I read the ls-files
> manual page.

My impression is that -x is pretty much only useful when you're using the -o flag, as anything that's already tracked isn't supposed to be affected by exclude patterns. The argument can be made that ls-files should indeed apply the exclude pattern to other requested display modes, and in fact a reading of the -i switch seems to imply that it should. Running `git ls-files -x '*.m' -c -i` displays all files ending in .m in the index. It would seem logical that the similar `git ls-files -x '*.m' -c` should display all files that don't end in .m in the index, but it doesn't appear to apply the exclude pattern in this case. As for -i being useful, I've never actually used it myself (though obviously it can be used as a way to grep through the index via the exclude pattern), but I can imagine a 
 scenario where I want to clean up all untracked files and end up running something like `git ls-files -z -o -i --exclude-standard | xargs -0 rm`.

-Kevin Ballard

Re: ls-files --exclude broken?

[Daniel Johnson]

[-- Attachment #1: Type: Text/Plain, Size: 1670 bytes --]

On Wednesday 15 September 2010 18:40:20 Jay Soffian wrote:
> Am I missing something really obvious here?
> kore:~/Repos/git (master)$ git ls-files | wc -l
>     2009
> kore:~/Repos/git (master)$ git ls-files -x \* | wc -l
>     2009
> kore:~/Repos/git (master)$ git ls-files -i -x \* | wc -l
>     2009
> kore:~/Repos/git (master)$ git version
> git version 1.7.3.rc1.5.g95127

For fun, I bisected this. This behavior changed in the following commit. 
Before this, it worked as you expected. The other replies seem to explain this 
more.

b5227d80aee5173bfda6aa43a890d03110b0df26 is the first bad commit
commit b5227d80aee5173bfda6aa43a890d03110b0df26
Author: Jeff King <peff@peff.net>
Date:   Mon Oct 12 01:11:57 2009 -0400

    ls-files: excludes should not impact tracked files
    
    In all parts of git, .gitignore and other exclude files
    impact only how we treat untracked files; they should have
    no effect on files listed in the index.
    
    This behavior was originally implemented very early on in
    9ff768e, but only for --exclude-from. Later, commit 63d285c
    accidentally caused us to trigger the behavior for
    --exclude-per-directory.
    
    This patch totally ignores excludes for files found in the
    index. This means we are reversing the original intent of
    9ff768e, while at the same time fixing the accidental
    behavior of 63d285c. This is a good thing, though, as the
    way that 9ff768e behaved does not really make sense with the
    way exclusions are used in modern git.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

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

[PATCH] git-ls-files.txt: clarify -x/--exclude option

[Jay Soffian]

Since b5227d8, -x/--exclude does not apply to cached files.
This is easy to miss unless you read the discussion in the
EXCLUDE PATTERNS section. Clarify that the option applies
to untracked files and direct the reader to EXCLUDE PATTERNS.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 Documentation/git-ls-files.txt |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Well, at least we can clarify the man page to reduce likelihood
of future confusion.

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 15aee2f..f52b06a 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -79,8 +79,9 @@ OPTIONS
 
 -x <pattern>::
 --exclude=<pattern>::
-	Skips files matching pattern.
-	Note that pattern is a shell wildcard pattern.
+	Skips untracked files matching pattern.
+	Note that pattern is a shell wildcard pattern. See EXCLUDE PATTERNS
+	below for more information.
 
 -X <file>::
 --exclude-from=<file>::
-- 
1.7.3.rc1.5.g95127

Re: [PATCH] git-ls-files.txt: clarify -x/--exclude option

[Jeff King]

On Wed, Sep 15, 2010 at 07:56:48PM -0400, Jay Soffian wrote:

> Since b5227d8, -x/--exclude does not apply to cached files.
> This is easy to miss unless you read the discussion in the
> EXCLUDE PATTERNS section. Clarify that the option applies
> to untracked files and direct the reader to EXCLUDE PATTERNS.

Yeah, I think this is a good idea.

Acked-by: Jeff King <peff@peff.net>

-Peff

Re: [PATCH] git-ls-files.txt: clarify -x/--exclude option

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 15aee2f..f52b06a 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -79,8 +79,9 @@ OPTIONS
>  
>  -x <pattern>::
>  --exclude=<pattern>::
> -	Skips files matching pattern.
> -	Note that pattern is a shell wildcard pattern.
> +	Skips untracked files matching pattern.
> +	Note that pattern is a shell wildcard pattern. See EXCLUDE PATTERNS
> +	below for more information.

Would be helpful; will queue directly on top of 500348a.

Thanks

By the way, I think the language in that file needs a bit of tweaking for
readability.  Perhaps this on top of your patch...

-- >8 --
Subject: [PATCH] ls-files documentation: reword for consistency

Similar to descriptions of other options, state what -x does in imperative
mood.  Start sentences for -X and --exclude-per-directory options in
capital letters.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-ls-files.txt |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index c05e7a5..44b6480 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -79,16 +79,16 @@ OPTIONS
 
 -x <pattern>::
 --exclude=<pattern>::
-	Skips untracked files matching pattern.
+	Skip untracked files matching pattern.
 	Note that pattern is a shell wildcard pattern. See EXCLUDE PATTERNS
 	below for more information.
 
 -X <file>::
 --exclude-from=<file>::
-	exclude patterns are read from <file>; 1 per line.
+	Read exclude patterns from <file>; 1 per line.
 
 --exclude-per-directory=<file>::
-	read additional exclude patterns that apply only to the
+	Read additional exclude patterns that apply only to the
 	directory and its subdirectories in <file>.
 
 --exclude-standard::
@@ -178,7 +178,7 @@ These exclude patterns come from these places, in order:
      file containing a list of patterns.  Patterns are ordered
      in the same order they appear in the file.
 
-  3. command line flag --exclude-per-directory=<name> specifies
+  3. The command line flag --exclude-per-directory=<name> specifies
      a name of the file in each directory 'git-ls-files'
      examines, normally `.gitignore`.  Files in deeper
      directories take precedence.  Patterns are ordered in the
-- 
1.7.3.rc2.221.gbf93f

Re: [PATCH] git-ls-files.txt: clarify -x/--exclude option

[Jeff King]

On Wed, Sep 15, 2010 at 05:41:52PM -0700, Junio C Hamano wrote:

> By the way, I think the language in that file needs a bit of tweaking for
> readability.  Perhaps this on top of your patch...

Also an improvement, IMHO.

-Peff